1

I have been working on getting a state machine to work correctly but am having trouble with adding in the last bit of logic. What it needs to do is several things:

  • Read two button inputs.
  • Make sure they are pressed and held within a certain amount of time of each other as defined by "PushTimeWindow" and if not wait til both are released.
  • Wait for a certain period as defined by "PushSafetyWindow".
  • Turn on a relay for an amount of time as specified by "RelayOnTime".
  • After the relay is off wait for both buttons to be released before they can be read again.
  • If at any time one or both released turn off the relay and wait for both to be released to be pushed again.
  • Count the number of successful relay periods and display them

Almost everything is doing what I had hoped it would do with the exception that I can't get the program to only allow the two switches to be pushed within a certain time frame as defined by the PushTimeWindow. Either button one needs to be pushed and held and then the second button pressed within the specified time. This window of time is intended to only allow the relay to be turned on if both buttons have been pushed within the set time. I have two states that I am trying to use to handle it but it seems to ignore them. Cases ONE_PUSHED and PUSHED_WINDOW are intended to handle this and seem to be ignored by the program. These two were added to handle that additional logic but otherwise the rest of the program works great and handles the other functions fine. I can't figure out what I am doing wrong with that logic. This is an added function to a previous question answered by user Jot here. I suspect that the two new cases might be redundant and true no matter what and that is why they seem to be ignored, or is it that they are never called on to be tested as true or not?

The sketch uses the Bounce2 library to debounce the noisy switches and each button has pull down resistors and otherwise works well.

Here is the sketch (Updated 10/17/17):

// A state machine is used, only to make it more clear what is going on.
//
// When a button is pressed, the input turns HIGH.
//
// Normally only one 'enum' is used for the state, that controls the code.
// This sketch has also a 'enum' for the buttons, which is called the input state.
// define the different states for this sketch.
// An 'enum' is like a number of #define with increasing numbers.

// include the library code:
#include <Bounce2.h>
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(7, 8, 9, 10, 11, 12);

enum
{
  WAIT_FOR_BOTH_OFF,
  WAIT_FOR_BOTH_ON,
  ONE_PUSHED,
  PUSHED_WINDOW,
  SAFETY_WINDOW,
  RELAY_ON,
} state;       // an 'enum' is automatically an integer.

// define the different input states.
enum
{
  BOTH_OFF,        // no buttons are pressed.
  BOTH_ON,         // both buttons are pressed.
  SOMETHING_ELSE,  // probably one of the buttons is pressed.
};


const int ButtonPin1 = 2;
const int ButtonPin2 = 3;
const int RelayPin = 5;

// The previousMillis is used twice for two different things.
// That is okay in this state machine.
unsigned long previousMillis;

const unsigned long RelayOnTime = 1750;    // Length of time for relay
const unsigned long PushTimeWindow = 300; // Length of time to allow two pushes
const unsigned long PushSafetyWindow = 50; // Safety Delay after push

// Counters for the number of failed relay triggers and successful cycles.
// They can probably be normal integers as well.
unsigned long failed = 0;
unsigned long success = 0;

Bounce debouncer1 = Bounce();
Bounce debouncer2 = Bounce();

void setup()
{
  lcd.begin(16, 2);
  lcd.print("Press Count");
  Serial.begin(9600);
  Serial.println("Unit Online");

// Set up first button
  pinMode(ButtonPin1, INPUT);
  debouncer1.attach(ButtonPin1);
  debouncer1.interval(5); //Debounce time

// Set up second button  
  pinMode(ButtonPin2, INPUT);
  debouncer2.attach(ButtonPin2);
  debouncer2.interval(5); //Debounce time

  pinMode(RelayPin, OUTPUT);  // OUTPUT to Relay

  Serial.println("Going to WAIT_FOR_BOTH_OFF state");
  state = WAIT_FOR_BOTH_OFF;
}


void loop()
{

  unsigned long currentMillis = millis();

// Update the bounce instances
  debouncer1.update();
  debouncer2.update();

  int inputState = getInputState();

  // Process the data in a state machine.
  // The output part is also in the state machine.
  switch(state)
  {
    case WAIT_FOR_BOTH_OFF:
      // Both buttons must be released to be able to continue.
      if(inputState == BOTH_OFF)
      {
        // Prepare to go to the next state.
        Serial.println("Going to WAIT_FOR_BOTH_ON state");
        state = WAIT_FOR_BOTH_ON;
      }
      break;
   case ONE_PUSHED:
      //One button is pushed
      if(inputState == SOMETHING_ELSE)
      {
        //One button was pushed
        Serial.println("One button Pushed");
        previousMillis = currentMillis;
        state = PUSHED_WINDOW;
      }
      break; 
    case WAIT_FOR_BOTH_ON:
      // This is the state when everything is idle and no buttons are pressed.
      // Normally both buttons are released,
      // but when both are pressed, then go to the next state.
      if(inputState == BOTH_ON)
      {
        // Prepare to go to the next state.
        Serial.println("Going to SAFETY_WINDOW state");
        previousMillis = currentMillis;
        state = SAFETY_WINDOW;
      }
      break;

    //Check to see if the other button is pushed, if not wait for both off
    case PUSHED_WINDOW:
      if(currentMillis - previousMillis >= PushTimeWindow)
      {
        Serial.println("Going to WAIT_FOR_BOTH_ON state 2");
        state = WAIT_FOR_BOTH_OFF;
        previousMillis = currentMillis;
      }
      //If window to push has expired go to WAIT_FOR_BOTH_OFF state
      else if(inputState == BOTH_ON)
      {
        Serial.println("Going to SAFETY_WINDOW 2");
        state = SAFETY_WINDOW;
      }
    case SAFETY_WINDOW:
      if(inputState != BOTH_ON)
      {
        // During the safety time window, a button was released.
        // Return to idle.
        Serial.println("Going to WAIT_FOR_BOTH_OFF state");
        state = WAIT_FOR_BOTH_OFF;
      }
      else if(currentMillis - previousMillis >= PushSafetyWindow)
      {
        // The buttons are still pressed.
        // The time window has reached the end.
        // Prepare to go to the next state.
        Serial.println("Going to RELAY_ON state");
        previousMillis = currentMillis;
        digitalWrite(RelayPin, HIGH);  // Turn ON RelayPin
        state = RELAY_ON;
      }
      break;
    case RELAY_ON:
      if(inputState != BOTH_ON)
      {
        // One of the buttons was released.
        // Stop the relay and return to idle.
        // Although the relay was triggered, it was not succesful.
        failed++;
        Serial.print("Failed=");
        Serial.println(failed);
        Serial.println("Going to WAIT_FOR_BOTH_OFF state");
        digitalWrite( RelayPin, LOW);  // Turn Off RelayPin
        state = WAIT_FOR_BOTH_OFF;
      }
      else if(currentMillis - previousMillis >= RelayOnTime)
      {
        // Everyting was okay.
        // The buttons are still pressed.
        // It was complete and succesful.
        // It is time to turn off the relay and return to idle.
        success++;
        Serial.print("Success=");
        Serial.println(success);
        Serial.println("Going to WAIT_FOR_BOTH_OFF state");
        lcd.setCursor(0,1);
        lcd.print(success);
        digitalWrite(RelayPin, LOW);  // Turn Off RelayPin
        state = WAIT_FOR_BOTH_OFF;
      }
      break;
    default:
      Serial.println("Error, unknown state");
      break;
  }
}


// This function returns the input state.
// There is no StateChangeDetection, because a state machine is used.
int getInputState()
{
  int returnInputState;

  // Read the debounce state and create a input state according to the 'enum' values.
  int value1 = debouncer1.read();
  int value2 = debouncer2.read();

  if(value1 == LOW && value2 == LOW)
  {
    returnInputState = BOTH_OFF;
  }
  else if(value1 == HIGH && value2 == HIGH)
  {
    returnInputState = BOTH_ON;
  }
  else
  {
    // Not both on, not both off, so it must be something else.
    returnInputState = SOMETHING_ELSE;
  }

  return(returnInputState);
}

The part of the sketch that doesn't seem to run are these two cases that should be called when one button is pressed:

case ONE_PUSHED:
      //One button is pushed
      if(inputState == SOMETHING_ELSE)
      {
        //One button was pushed
        Serial.println("One button Pushed");
        previousMillis = currentMillis;
        state = PUSHED_WINDOW;
      }
      break;  

The sketch should then run the case PUSHED_WINDOW to check if the inputState goes to BOTH_ON within the window defined in PushTimeWindow. If that time has been exceeded the sketch should then wait until both buttons are released again and then wait for another try.

case PUSHED_WINDOW:
      if(currentMillis - previousMillis >= PushTimeWindow)
      {
        Serial.println("Going to WAIT_FOR_BOTH_ON state 2");
        state = WAIT_FOR_BOTH_OFF;
        previousMillis = currentMillis;
      }
      //If window to push has expired go to WAIT_FOR_BOTH_OFF state
      else if(inputState == BOTH_ON)
      {
        Serial.println("Going to SAFETY_WINDOW 2");
        state = SAFETY_WINDOW;
      }

Unfortunately the sketch never seems to run either of the cases which is where I am having trouble debugging.

The sketch will run on an Arduino Nano with an ATmega328P programmed with Arduino 1.8.5. The two input buttons are Rees mushroom head momentary industrial switches that are software debounced and have smoothing capacitors to hardware debounce as well. Inputs have 1K pull down resistors and the system is powered by a 9V power supply. The output to the relay pin drives the base of a NPN TIP31C transistor that powers the input of a relay with 9V. The relay has a capacitor with a resistor across it on its input to smooth out back spikes that occur when the relay triggers. The hardware all seems to be working fine and as intended.

Edit 10/20/2017 The solution to the question is included in the below code. The problem was with the state machine seeming to skip the window required to press the button. Some of the enum names were also changed as per suggested but if possible will be renamed again if better names come to me to be more clear what they are to do.

Edit 10/23/2017 Tried to name everything as clearly as possible and include documentation as to source and function of sketch.

Here is the final sketch:

// This sketch reads inputs from two buttons to turn on a relay
// It requires that both buttons be pressed within a certain time of each other and held
// A relay pin is then turned on for a specified period of time
// Both buttons must then be released again for another cycle to begin
// A state machine is used, only to make it more clear what is going on.
// Used with an Arduino Nano board on Arduino 1.8.5
// Found on https://arduino.stackexchange.com/questions/45584/state-machine-logic-problem-with-added-states-monitoring-safety-switches/
// Uses the Bounce2 library: https://github.com/thomasfredericks/Bounce2
//
// When a button is pressed, the input turns HIGH.
//
// Normally only one 'enum' is used for the state, that controls the code.
// This sketch has also a 'enum' for the buttons, which is called the input state.
// define the different states for this sketch.
// An 'enum' is like a number of #define with increasing numbers.

// include the library code:
#include <Bounce2.h>
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(7, 8, 9, 10, 11, 12);

enum
{
  WAIT_FOR_BOTH_LOW,
  WAIT_FOR_BUTTON,
  ONE_PUSHED_WAIT_FOR_OTHER_BUTTON,
  SAFETY_WINDOW,
  RELAY_ON,
} state;       // an 'enum' is automatically an integer.

// define the different input states.
enum
{
  BOTH_BUTTONS_LOW,      // no buttons are pressed.
  BOTH_BUTTONS_HIGH,     // both buttons are pressed.
  ONE_BUTTON_HIGH,       // probably one of the buttons is pressed.
};


const int ButtonPin1 = 2;
const int ButtonPin2 = 3;
const int RelayPin = 5;
const int IndicatorPin = 4;

// The 'previousMillis' is exclusively for the state machine portion only.
// It is used for different software timers in different states.
// That is okay, because within the state machine the states are seperated from each other.
// Funtions outside the state machine would require a separate timer.
unsigned long previousMillis;

const unsigned long RelayOnTime = 1750;    // Length of time for relay
const unsigned long TimeFrameForSecondButton = 300; // Length of time to allow two pushes
const unsigned long PushSafetyWindow = 50; // Safety Delay after push

// Counters for the number of failed relay triggers and successful cycles.
// They can probably be normal integers as well.
unsigned long failed = 0;
unsigned long success = 0;

// Instantiate Bounce objects
Bounce debouncer1 = Bounce();
Bounce debouncer2 = Bounce();

void setup()
{
  lcd.begin(16, 2);
  lcd.print("Press Count");
  Serial.begin(9600);
  Serial.println("Unit Online");

// Set up first button
  pinMode(ButtonPin1, INPUT);
  debouncer1.attach(ButtonPin1);
  debouncer1.interval(5); //Debounce time

// Set up second button  
  pinMode(ButtonPin2, INPUT);
  debouncer2.attach(ButtonPin2);
  debouncer2.interval(15); //Debounce time

  pinMode(RelayPin, OUTPUT);  // OUTPUT to Relay
  pinMode(IndicatorPin, OUTPUT); // OUTPUT to Indicator LED
  Serial.println("Going to WAIT_FOR_BOTH_LOW state");
  state = WAIT_FOR_BOTH_LOW;
}


void loop()
{

  unsigned long currentMillis = millis();

// Update the Bounce instances
  debouncer1.update();
  debouncer2.update();

  int inputState = getInputState();

// Process the data in a state machine.
// The output part is also in the state machine.
  switch(state)
  {
    case WAIT_FOR_BOTH_LOW:
      // Both buttons must be released to be able to continue.
      if(inputState == BOTH_BUTTONS_LOW)
      {
        // Prepare to go to the next state.
        Serial.println("Going to WAIT_FOR_BUTTON state");
        state = WAIT_FOR_BUTTON;
      }
      break;
    case WAIT_FOR_BUTTON:
// This is the state when everything is idle and no buttons are pressed.
// Normally both buttons are released.
// Waiting for the first button to be pressed.
// Both buttons could be pressed at the same time !
      if(inputState == ONE_BUTTON_HIGH || inputState == BOTH_BUTTONS_HIGH)
      {
// At least one button is pressed.
// Prepare to go to the next state.
// Start a timer, because there is a timeout.
        Serial.println("Going to ONE_PUSHED_WAIT_FOR_OTHER_BUTTON state");
        previousMillis = currentMillis;
        state = ONE_PUSHED_WAIT_FOR_OTHER_BUTTON;
       }
      break;
    case ONE_PUSHED_WAIT_FOR_OTHER_BUTTON:
      if(inputState == BOTH_BUTTONS_LOW)
      {
// The button was released. That's not what we want.
// Return to the state to wait for both off.
        Serial.println("Going to WAIT_FOR_BOTH_LOW state");
        state = WAIT_FOR_BOTH_LOW;
      }
      else if(currentMillis - previousMillis >= TimeFrameForSecondButton)
      {
// The timeout has ended.
// The second button was not pushed after the first button
// within the timeout. That's too bad.
        Serial.println("Too Long Going to WAIT_FOR_BOTH_LOW state");
        state = WAIT_FOR_BOTH_LOW;
      }
      else if(inputState == BOTH_BUTTONS_HIGH)
      {
// The timeout has not ended and is still running.
// And now both buttons are pressed.
// We can continue.
// Start the timer for the SAFETY_WINDOW state.
        Serial.println("Going to SAFETY_WINDOW state");
        previousMillis = currentMillis;
        state = SAFETY_WINDOW;
      }
      break;
    case SAFETY_WINDOW:
      if(inputState != BOTH_BUTTONS_HIGH)
      {
// During the safety time window, a button was released.
// Return to idle.
        Serial.println("Going to WAIT_FOR_BOTH_LOW state");
        state = WAIT_FOR_BOTH_LOW;
      }
      else if(currentMillis - previousMillis >= PushSafetyWindow)
      {
// The buttons are still pressed.
// The time window has reached the end.
// Prepare to go to the next state.
        Serial.println("Going to RELAY_ON state");
        previousMillis = currentMillis;
        digitalWrite(RelayPin, HIGH);  // Turn ON RelayPin
        digitalWrite(IndicatorPin, HIGH); // Turn ON IndicatorPin
        state = RELAY_ON;
      }
      break;
    case RELAY_ON:
      if(inputState != BOTH_BUTTONS_HIGH)
      {
// One of the buttons was released.
// Stop the relay and return to idle.
// Although the relay was triggered, it was not succesful.
        failed++;
        Serial.print("Failed=");
        Serial.println(failed);
        Serial.println("Going to WAIT_FOR_BOTH_LOW state");
        digitalWrite( RelayPin, LOW);  // Turn Off RelayPin
        digitalWrite(IndicatorPin, LOW); // Turn Off IndicatorPin
        state = WAIT_FOR_BOTH_LOW;
      }
      else if(currentMillis - previousMillis >= RelayOnTime)
      {
        // Everyting was okay.
        // The buttons are still pressed.
        // It was complete and succesful.
        // It is time to turn off the relay and return to idle.
        success++;
        Serial.print("Success=");
        Serial.println(success);
        Serial.println("Going to WAIT_FOR_BOTH_LOW state");
        lcd.setCursor(0,1);
        lcd.print(success);
        digitalWrite(RelayPin, LOW);  // Turn Off RelayPin
        digitalWrite(IndicatorPin, LOW); // Turn Off IndicatorPin
        state = WAIT_FOR_BOTH_LOW;
      }
      break;
    default:
        Serial.println("Error, unknown state");
      break;
    }
}


// This function returns the input state.
// There is no StateChangeDetection, because a state machine is used.
  int getInputState()
{
  int returnInputState;

// Read the debounce state and create a input state according to the 'enum' values.
  int value1 = debouncer1.read();
  int value2 = debouncer2.read();

    if(value1 == LOW && value2 == LOW)
    {
      returnInputState = BOTH_BUTTONS_LOW;
    }
    else if(value1 == HIGH && value2 == HIGH)
    {
      returnInputState = BOTH_BUTTONS_HIGH;
    }
    else
    {
    // Not both on, not both off, so it must be something else.
    returnInputState = ONE_BUTTON_HIGH;
    }

  return(returnInputState);
}
2
  • Do you know why no one is answering ? I can help, but then I will be asking many questions, maybe between 10 and 20 and you will have to put a lot of effort in it. Are you ready for it ?
    – Jot
    Oct 17, 2017 at 19:51
  • Hello, any help is greatly appreciated. I hope it is not because I am that far off from what I was trying to have it do that there weren't any suggestions. I am also open to suggestions if I am posting questions incorrectly. I try to only ask a question when I am really stumped so I may not be posting in a manner that people are expecting to since I don't ask often. I am ready and eager to dive in. I have been trying to wrap my head around the FSM logic and have been reading an article on hackingmajenkoblog and I thought I had the logic correct but something is off.
    – HiWay
    Oct 17, 2017 at 22:56

2 Answers 2

1

Intro

You did not get any answers, because someone has to read the whole sketch and you have not explained which problem you want to solve with the two extra states and you did not fully explain what the extra states do.

I don't see a problem with my original example for the 'SAFETY_WINDOW' state. As soon as one button is released, it bails out.
I can image that the state rushes through 'WAIT_FOR_BOTH_OFF' and 'WAIT_FOR_BOTH_ON' when there is a lot of noise. Slowing it down in the sketch or with Bounce2 is a good improvement.
I could have made a mistake that I don't see, but now it is up to you to investigate where the problem comes from.

Software

You already found Majenko about FSM. Good, I hope you have studied it.
Perhaps you should make a sketch, just for fun, with a simple state machine.

As you can see, the integer 'state' rules it all.
The large switch-case statement contains the seperate pieces of code for each state. The order of those pieces of code does not matter. The integer 'state' defines what will be executed.

You never set the variable 'state' to 'ONE_PUSHED', therefor that piece of code will never be executed. However, please don't add that somewhere in the sketch. Explain why you want to detect if just one button is pushed.

Every piece of code of every state should make sense. I think that you have gone a little too far and the sketch is going haywire and is not going in a direction.

Is the problem that during the time that both buttons needs to be pressed (just before the relay turns on), the sketch thinks that both are pressed, even when one button is released ? Is that the problem ?

When that would be caused by a bug in the sketch, then the bug should be fixed. Trying to make a workaround around a bug is bad. If the workaround contains a bug itself, then fixing a bug with another bug is something you will never get out.

Please add a welcoming message to the display and the serial port. That way you can see if the Arduino was reset.

The software thing to do

There is a problem. Please describe it clearly. Then make the smallest possible test sketch to investigate that problem.
Perhaps a small sketch using the function "getInputState" and display it a number of times per second.

Hardware

The input impedance of an Arduino pin is very high. It could be a few hundred MegaOhms. That means if one pin is connected to something, and the pin next to it is open (not connected), then the open pin might read the same signal as its neighbour pin, just by sensing the nearby voltage through the air.

Can you make a photo of that button ?
Is there a spring inside the button ?
How long are the wires or cables to the Arduino ?
Are there other cables alongside that cable ?
Are there electrical devices nearby that could cause spikes ?
How is the button and the pullup/pulldown resistor connected ? Can you show it in a diagram ? You can even make a drawing and make a photo of that.
Are you sure it is like that, can you verify that ?
Verify it once more with a multimeter. Measure the Arduino pin with the button(s) pressed and released. Is the really going HIGH when the corresponding button is pressed ?
Can you try to connect the buttons to other pins of the Arduino board ? Perhaps an input is blown.
Is the GND of the Arduino connected to a computer, and the computer to earth ground ?
Are the wires to the buttons somehow connected to some kind of ground ?
Or to a metal enclosure or something like that ?
Is there another ground problem ?
Do you use a good and certified power supply for the Arduino ? I hope not a very cheap power supply that is leaking the mains voltage to the output.
I forgot which Arduino board you use. Which Arduino board and which Arduino IDE version do you use ?

Common mistakes

There are a number of common mistakes. I think you made a few of them. If you can recognize them, perhaps you can avoid them.

  • The famous XY-problem: xyproblem.info.
    This happens a lot. Someone is too focussed to fix a problem, and wandering away from the original problem.
  • Trying to get a result right now.
    No experienced software designer does that. Investigate the problem by excluding the rest. Make small tests.
  • Make a good description/explanation.
    If it is an art or a skill, describing something in a logical way is very important. It can be half the work.
  • Ground problems.
    Ground problems is a major problem. When everything is connected okay, the ground could be connected in the wrong place. This is a tough subject.
  • Never trust your tools.
    Once someone knows how to make something work, but there is a bug somewhere that is impossible to find, then perhaps there is no bug. It can be a broken Arduino board, or other bad hardware or a bug in the Arduino software. It happens.

ADDED

I understand now that both buttons should be pushed within a certain time. That is the third millis timer that will be needed. I have a problem with the names that you choose for this new situation. I hope you don't mind that I made my own names. You had most of the conditions and the timeout, but bailing out as soon as the one button was released was missing.
The possibility that both buttons are pressed at the same time should also be accepted.

I did test my previous sketch, but I did not test this code. I had to draw a time diagram on a piece of paper to see the sequence of the states. I suggest you do the same.
The states are still in sequential order, there is no need for a parallel path in the state machine, or wildly jumping to all kind of states. Everything flows in a logic way from one state to the next in a sequential way.

My diagram on my piece of paper tells me that the "WAIT_FOR_BOTH_ON" has to be replaced by two other states. It needs other conditions and it should get a timeout. Let's call the new states "WAIT_FOR_BUTTON" and "ONE_PUSHED_WAIT_FOR_OTHER_BUTTON".

You might give this next constant a better name (it is the time frame in which the second button has to be pushed after the first one is pushed):

const unsigned long TimeFrameForSecondButton = 200;

Waiting for both off is still the same. The next state is changed into "WAIT_FOR_BUTTON".

case WAIT_FOR_BOTH_OFF:
  // Both buttons must be released to be able to continue.
  if(inputState == BOTH_OFF)
  {
    // Prepare to go to the next state.
    Serial.println("Going to WAIT_FOR_BUTTON state");
    state = WAIT_FOR_BUTTON;
  }
  break;

Then wait for a button (just one or perhaps two at the same time).

case WAIT_FOR_BUTTON:
  // This is the state when everything is idle and no buttons are pressed.
  // Normally both buttons are released.
  // Waiting for the first button to be pressed.
  // Both buttons could be pressed at the same time !
  if(inputState == SOMETHING_ELSE || inputState == BOTH_ON)
  {
    // At least one button is pressed.
    // Prepare to go to the next state.
    // Start a timer, because there is a timeout.
    Serial.println("Going to ONE_PUSHED_WAIT_FOR_OTHER_BUTTON state");
    previousMillis = currentMillis;
    state = ONE_PUSHED_WAIT_FOR_OTHER_BUTTON;
  }
  break;

While waiting for the second button, if anything is wrong then return to the state to wait for both off.

case ONE_PUSHED_WAIT_FOR_OTHER_BUTTON:
  if(inputState == BOTH_OFF)
  {
    // The button was released. That's not what we want.
    // Return to the state to wait for both off.
    Serial.println("Going to WAIT_FOR_BOTH_OFF state");
    state = WAIT_FOR_BOTH_OFF;
  }
  else if(currentMillis - previousMillis >= TimeFrameForSecondButton)
  {
    // The timeout has ended.
    // The second button was not pushed after the first button
    // within the timeout. That's too bad.
    Serial.println("Going to WAIT_FOR_BOTH_OFF state");
    state = WAIT_FOR_BOTH_OFF;
  }
  else if(inputState == BOTH_ON)
  {
    // The timeout has not ended and is still running.
    // And now both buttons are pressed.
    // Hurray, we can continue.
    // Start the timer for the SAFETY_WINDOW state.
    Serial.println("Going to SAFETY_WINDOW state");
    previousMillis = currentMillis;
    state = SAFETY_WINDOW;
  }
  break;

Can you merge this in the code ? And remove the things that are not used ?

I'm still unhappy with the names of the variables and states and so. When you look at it a year later, you must know what is going on. Can you give that a thought, it does not matter if the names are very long.

Reaction to 10/20/2017

Awesome!
A medium-complex problem solved in a straightforward logical way without using delay.

A few tips:

The header should explain what it is; the date; which Arduino board; which Arduino IDE version; a link to this stackexange.com page and so on.

Your indents are not always the same. Can you get all the "case" blocks like the others ?

Normally, using three timers with millis() requires three 'previousMillis' variables. The 'previousMillis' is used three times. That needs some extra explanation:

// The 'previousMillis' is exclusively for the state machine only.
// It is used for different software timers in different states.
// That is okay, because the states are seperated from each other.
unsigned long previousMillis;

Now that I think of it, the state machine does not rule the sketch, it processes the buttons.
If you would add a buzzer, and it would need a timeout to turn off, then that timeout must be done outside the state machine. That timeout should also get it's own variable, for example: previousBuzzerMillis.

What about the names for the input states. Is "BOTH_BUTTONS_ON", "BOTH_BUTTONS_OFF, "JUST_ONE_BUTTON" better ? Or "BOTH_BUTTONS_PUSHED", "BOTH_BUTTONS_RELEASED", "ONLY_ONE_BUTTON" ?

I can't think of better names for other variables at the moment. Even the name "SAFETY_WINDOW" seems obvious.

Correction nov 3, 2017
I just learned that 'enum' is not automatically an integer. An 'enum' is its own type.

12
  • Hello, the original example sketch you posted works great and as intended. The issues with bouncing have been cleared up with the combination of the Bounce2 library and smoothing capacitors. What I am trying to do is add the additional logic on top of the existing logic that when one button is pushed the program checks to make sure the second button is pushed within PushTimeWindow and if it is not it waits for both to be released and stay in WAIT_FOR_BOTH_OFF state and for the operator to try again. When that additional criteria is met the sketch should move to SAFETY_WINDOW.
    – HiWay
    Oct 18, 2017 at 14:23
  • I attempted to set the state to ONE_PUSHED when the inputState returned as SOMETHING_ELSE. It detects BOTH_ON and BOTH_OFF just fine and it seems that it should do the same when SOMETHING_ELSE is the inputState. The rest of the sketch works great as is and pushes are detected reliably and the relay operates as intended it is just the extra logic that seems to be getting ignored. I will add more hardware details in the original question to clarify more.
    – HiWay
    Oct 18, 2017 at 14:31
  • I've been going through the explanations about state machines and have set up smaller additional ones for practice. I think that is where I may have a disconnect. I can see that the "state" is what drives the sketch, I think I have a disconnect as to why the state machine ignores the new ONE_PUSHED case when it seems like it is set up like the other two.
    – HiWay
    Oct 18, 2017 at 15:19
  • @HiWay If I did not understand your question, then I'm very sorry. I hope I was not to hars on you. I read you question a few times, but I still don't understand what you want to do. Can you edit your question and explain it once more (assuming I'm really dumb). When I read it now, it seems that "PushTimeWindow" and "PushSafetyWindow" do the same thing. If you can not detect if one button is pressed, then you should make a minimal test sketch to test just that. That is why I asked so many questions about the hardware. That both buttons must be pressed for some time was already in the sketch.
    – Jot
    Oct 20, 2017 at 18:44
  • Hello, no worries. I will try to edit it a bit if I can phrase it differently. The "SAFETY_WINDOW" waits a moment before initiating the relay and works well as is, thank you. What PUSHED_WINDOW is supposed to do is detect the first push and make sure the second push happens within a specified amount of time after the first button is pushed. Failing to meet that window should return the state back to waiting for both buttons to be released again so the operator can try again. If pushed in the proper amount of time the state should then enter the safety window.
    – HiWay
    Oct 20, 2017 at 20:09
0

It looks like you are correct that the ONE_PUSHED and PUSHED_WINDOW branches are being ignored, because you never set the state to ONE_PUSHED.

I believe your just need to update the WAIT_FOR_BOTH_OFF state:

  case WAIT_FOR_BOTH_OFF:
      // Both buttons must be released to be able to continue.
      if(inputState == BOTH_OFF)
      {
        // Prepare to go to the next state.
        Serial.println("Going to ONE_PUSHED state");
        state = ONE_PUSHED;
      }
      break;
1
  • Hello, thanks for taking a look at the sketch. Altering the case in the way suggested seems to prevent the sketch from ever entering the BOTH_ON state. I tried setting the state to ONE_PUSHED when the sketch enters getInputState and returns SOMETHING_ELSE to then enter the ONE_PUSHED state. The two states do appear to be getting ignored.
    – HiWay
    Oct 18, 2017 at 16:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.