Problem combining code

Using Arduino Programming Questions
1

Hi all,

VEEEEERY Noob experience level over here; sorry in advance if my code is a trainwreck!

So....

Here's what I'm trying to do.

I've got a motor and I've got an electro mechanical valve (basically just a coil that when energized pulls a piston that allows water to flow out).

The motor has 2 positions, UP and DOWN.

Here's what I'm asking of the Arduino:

  1. I want to press a momentary button to start the program
  2. The motor will then lower to its DOWN position and shut off via a closed circuit reading from a roller switch (basically a LOW signal)
  3. A few seconds later I would like the valve to fire open and then close a few seconds after.
  4. Then, I would like the motor from step 2 to raise back to the UP positions (this can be a function of time and does not interface with the roller switch).

So, how I have it rigged up now is the motor will start when I hit the button and then it will shut off with the input from the roller switch (this is working). The valve is based on milliseconds timed from when the button is pressed (this is working). I then am trying to raise the motor based on a similar set of code as the millisecond code as the valve (this is NOT working).

Here is the code (the ino file is also attached):

***The following code has been edited 12.7.2016

const byte startButton = 2; 
const byte rollerSwitch = 3;
const byte brewMotor = 13;      
const byte brewValve = 12; 

int currButtonState = LOW;      // the current state of the output pin 2
int readButtonState;           // the current reading from the input pin 2
int readRollerState;           // the current reading from the input pin 3
int prevButtonState = HIGH;    // the previous reading from the input pin 2
int prevRollerState = HIGH;    // the previous reading from the input pin 3

long time = 0;         // the last time the output pin was toggled
long debounce = 200;   // the debounce time, increase if the output flickers

unsigned long buttonPushedMillis; // when button was released
unsigned long brewValveTurnedOnAt; // when brewvalve was turned on
unsigned long brewValveTurnOnDelay = 5000; // wait to turn on brewvalve
unsigned long brewValveTurnOffDelay = 3000; // turn off brewvalve after this time
bool brewValveReady = false; // flag for when button is let go
bool brewValveState = false; // for brewvalve is on or not.

unsigned long brewMotorTurnedOnAt; // when brewmotor was turned on
unsigned long brewMotorTurnOnDelay = 10000; // wait to turn on brewmotor
unsigned long brewMotorTurnOffDelay = 3000; // turn off brewmotor after this time
bool brewMotorReady = false; // flag for when button is let go
bool brewMotorState = false; // for brewmotor is on or not.


void setup()
{
  pinMode(startButton, INPUT_PULLUP);
  pinMode(brewMotor, OUTPUT);
  pinMode(rollerSwitch, INPUT_PULLUP);
  pinMode(brewValve, OUTPUT);
  digitalWrite(brewMotor, LOW);
}


void loop()
{
  readButtonState = digitalRead(startButton);
  
   if (readButtonState == LOW && prevButtonState == LOW && millis() - time > debounce) {
    if (currButtonState == HIGH)
      currButtonState = LOW;
    else
      currButtonState = HIGH;

    time = millis();    
  }

  digitalWrite(brewMotor, currButtonState);

  prevButtonState = readButtonState;

  readRollerState = digitalRead(rollerSwitch);
  
   if (readRollerState == LOW && prevRollerState == HIGH && millis() - time > debounce) {
    if (currButtonState == LOW)
      currButtonState = HIGH;
    else
      currButtonState = LOW;

    time = millis();    
  }

  digitalWrite(brewMotor, currButtonState);

  prevRollerState = readRollerState;

  

  unsigned long currentMillis = millis(); 
 
 if (digitalRead(startButton) == LOW) {
  buttonPushedMillis = currentMillis;
  brewValveReady = true;
 }
  
 if (brewValveReady) {
   if ((unsigned long)(currentMillis - buttonPushedMillis) >= brewValveTurnOnDelay) {
     digitalWrite(brewValve, HIGH);
     brewValveState = true;
     brewValveTurnedOnAt = currentMillis;
     brewValveReady = false;
   }
 }
  
 if (brewValveState) {
   if ((unsigned long)(currentMillis - brewValveTurnedOnAt) >= brewValveTurnOffDelay) {
     brewValveState = false;
     digitalWrite(brewValve, LOW);
   }
 }

 digitalWrite(brewMotor, LOW);

 if (digitalRead(startButton) == LOW) {
  buttonPushedMillis = currentMillis;
  brewMotorReady = true;
 }
  
 if (brewMotorReady) {
   if ((unsigned long)(currentMillis - buttonPushedMillis) >= brewMotorTurnOnDelay) {
     digitalWrite(brewMotor, HIGH);
     brewMotorState = true;
     brewMotorTurnedOnAt = currentMillis;
     brewMotorReady = false;
   }
 }
  
 if (brewMotorState) {
   if ((unsigned long)(currentMillis - brewMotorTurnedOnAt) >= brewMotorTurnOffDelay) {
     brewMotorState = false;
     digitalWrite(brewMotor, LOW);
   }
 }
}

Any suggestions? Should I lay it out differently?

BUAT_VERSION_1.1.ino (3.17 KB)

2 
 if (state == LOW)
      state = LOW;
    else
      state = LOW;

?

3

Please modify your post and use the code button </> so your code looks like this and is easy to copy to a text editor. See How to use the Forum Your code is too long for me to study quickly without copying to a text editor.

...R

4 
reading = digitalRead(button);
 
   if (reading == HIGH && previous == LOW && millis() - time > debounce) {
    if (state == HIGH)
      state = LOW;
    else
      state = HIGH;


    time = millis();   
  }


  digitalWrite(brewmotor, state);


  previous = reading;

So, previous contains the state of the switch connected to the pin in button, for next time through loop.

reading3 = digitalRead(roller);
 
   if (reading3 == LOW && previous == HIGH && millis() - time > debounce) {

So, why are you comparing the current reading of the pin in roller to the current reading of the pin in button?

 if (digitalRead(button) == LOW) {
  buttonPushedMillis = currentMillis;
  brewvalveReady = true;
 }

You've already determine that the switch connected to the pin in button (that really IS a poor name for variable containing a pin number) did, or did not, change state. Why are you reading the pin again? Why are you now concerned about the state the switch IS in?

Start by giving all the variables MEANINGFUL names. buttonPin, currButtonState, and prevButtonState look like they go together, and are certainly more meaningful that button, reading, and previous. But, even then, the term button does NOT imply what the switch is used for, which is really what should be in the name.

5

Hi all,

Thank you for all the great feedback. As I said, total noob and I just frankensteined this code together from bits of several tutorials I found online. I've hopefully corrected the variable naming issues PaulS pointed out and also put the code into a more forum friendly viewing style. I'm going to chew back through and see if I can figure this thing out.

Thanks again for your help and keep the feedback coming.

Best,
Stueve

6

Phew...

Been cranking on this thing all day. No resolution.

Here is my rough "if" statement:

  1. If the startButton is switched LOW turn brewMotor pinout HIGH(on)
    and turn brewValve HIGH(on) for "X" number of seconds
  2. If rollerSwitch is switched LOW turn brewMotor pinout LOW(off)
  3. If rollerSwitch is LOW && brewValve pinout is HIGH
    then switch brewMotor High(on) for "X" number of seconds.

Does this make sense?

I basically want to be able to push a button and have the brewMotor turn on, then shut off with an input from the rollerSwitch, then the brewVavle to turn on for a period of time, and then for the brewMotor to turn on for a period of time.

7

What you have described is called a state machine. Instead of using lots of if statements, I find it easier to follow / code in a switch/case construction (or even using function pointers but I think that will be a bit more complicated.

You have a few states (based on the description in the opening post)

1) wait for button press
2) move motor down
3) wait for roller activation
4) delay
5) open valve
6) delay
7) close valve
8) move motor up

The below can act as a framework; it probably does not fully do what you want it to do

void sm()
{

  // remember the state; initial value equals 1
  static byte currentState = 1;

  byte buttonState = digitalRead(startButton);
  byte rollerState = digitalRead(rollerSwitch);

  switch (currentState)
  {
    // wait for button press
    case 1:
      if (buttonState == LOW)
      {
        currentState++;   // go to next state
      }
      break;
    //move motor down
    case 2:
      digitalWrite(brewMotor, HIGH);
      currentState++;     // go to next state
      break;
    // wait for roller to be activated
    case 3:
      if (rollerState == LOW)
      {
        currentState++;   // go to next state
      }
      break;
    // wait a bit
    case 4:
      if (doWait(5000, false))
      {
        currentState++;   // go to next state
      }
      break;
    // open valve
    case 5:
      digitalWrite(brewValve, HIGH);
      currentState++;     // go to next state
      break;
    // keep valve open for 10 seconds
    case 6:
      if (doWait(10000, false))
      {
        currentState++;   // go to next state
      }
      break;
    // close valve
    case 7:
      digitalWrite(brewValve, LOW);
      currentState++;     // go to next state
      break;
    // move motor up
    case 8:
      digitalWrite(brewMotor, LOW);
      currentState++;     // go to next state
      break;
    case 9:
      // nothing really to do
      // go back to first state
      currentState = 1;
    default:
      // unexpected state
      // handle the error, e.g. switch everything 'off'
      ...
      ...
      // hang forever
      for(;;);
      break;
  }
}

Every time a step (state) is completed, the code will increment the currentState variable and on the next call to sm, that case will be executed.

This is a simple approach; state machines can be far more complex in which case simply incrementing will not work. If you want to interrupt the process (e.g. when the button is released), it requires a slightly different approach. You probably want to name the states (e.g. use a #define ST_WAITBUTTONPRESS 1 and #define MOVEMOTORDOWN 2

switch(currentState)
{
  case WAITBUTTONPRESS:
      if (buttonState == LOW)
      {
        currentState = MOVEMOTORDOWN;
      }
      break;
    case 2:
      digitalWrite(brewMotor, HIGH);
      currentState = WAITROLLERACTIVATION;
      break;
    ...
    ...

You can reorganize (e.g. close valve (step 7) and motor up (step 8) can be one step). You can also expand on it.

I did not really look at your code to understand what is all exactly going on in there.

The delays are implemented using a non-blocking delay

/*
  non-blocking delay with option to force it to finish immediately
  input:
    duration of delay
    flag to force delay to finish immediatedly
  returns:
    true if delay is finished, else false
*/
bool doWait(unsigned long delayTime, bool fForceStop)
{
  // force end of delay immediately
  if (fForceStop == true)
  {
    // reset start time for next time that we need a delay
    startTime = 0;
    // indicate that delay is finished
    return true;
  }

  // start time of delay
  static unsigned long startTime = 0;
  if (startTime == 0)
  {
    startTime = currentTime;
    // indicate that delay is not finished yet
    return false;
  }

  if (currentTime - startTime >= delayTime)
  {
    // reset start time for next time that we need a delay
    startTime = 0;
    // indicate that delay is finished
    return true;
  }

  // indicate that delay is not finished yet
  return false;
}

The code returns false while a delay is in progress, else true; the calling function (sm in this case) can check the return value to determine if the delay is finished or not.

It also allows to use the second parameter (fForceStop) to immediately reset the 'timing' and terminate the delay; you can use this if you need to interrupt the delay when e.g. the startButton is released.

I think it's a lot easier to follow than all the if statements :wink:

8

Hi All,

Thanks for the feedback everyone. I'm still cranking on this problem and I could really use some help! Several people have suggested switch statement code and I've done a bunch of searches and countless youtube videos in an attempt to better understand it and I'm still stuck.

I've attached the code that I've got so far. Any suggestions or points towards other materials where I can better educate myself would be AWESOME!!!!

Thanks in Advance,
Stueve

  const byte startButton = 2;
  const byte rollerSwitch = 3;
  const byte brewMotor = 13;      
  const byte brewValve = 12;

  int readButtonState;           // the current reading from the input pin 2
  int readRollerState;           // the current reading from the input pin 3
  

void setup()  {

  Serial.begin(9600);
  
  pinMode(3, INPUT_PULLUP);
  pinMode(2, INPUT_PULLUP);
  pinMode(13, OUTPUT);
  pinMode(12, OUTPUT);
  digitalWrite(brewMotor, LOW);
}


 void loop()  {

  int readButtonState = digitalRead(startButton);
  Serial.print ("buttonState:");
  Serial.println (readButtonState);

  int readRollerState = digitalRead(rollerSwitch);
  Serial.print ("rollerState:");
  Serial.println (readRollerState);
  
  switch (readButtonState)
  {
  
    case 1: //move brewMotor down
      if (readButtonState == LOW)
      {
        digitalWrite(brewMotor, HIGH);
      }
      break;
    
    case 2: //stop brewMotor
      if (readRollerState == LOW)
      {
        digitalWrite(brewMotor, LOW);
      }
      break;
    
    case 3: //open brewValve
      if (readRollerState == LOW && readButtonState == LOW)
      {
        delay(1000);
        digitalWrite(brewValve, HIGH);
        delay(5000);
      }
      break;
    
    case 4: // move brewMotor up
      if (readRollerState == LOW && readButtonState == LOW)
      {
        delay(10000);
        digitalWrite(brewMotor, HIGH);
        delay(2000);
      }
      break;
  }
  }
9 
  int readButtonState;           // the current reading from the input pin 2


 void loop()  {

  int readButtonState = digitalRead(startButton);

Why do you have two variables with the same name?

  switch (readButtonState)

What values can be in readButtonState?

    case 2: //stop brewMotor
      if (readRollerState == LOW)
      {
        digitalWrite(brewMotor, LOW);
      }
      break;
   
    case 3: //open brewValve

No, not those.

    case 4: // move brewMotor up
      if (readRollerState == LOW && readButtonState == LOW)

How can readButtonState equal LOW when you think it is 4?

I gave up at this point.

10

You're missing the variable currentState; that's the variable that you must use in the switch statement. Look at the framework that I presented.