Problem with Dual Safety Switches Initiating Timed Relay

Question

I have been working on an Arduino project that has me kind of stumped. What it needs to achieve is that two momentary safety switches need to be pushed and held within a specified time frame as defined by "PushTimeWindow" then continue to be held as a relay is engaged for the period of time as defined by "RelayOnTime" and at any point if either or both of the momentary switches are released the relay needs to turn off immediately. The number of successful pressed is then displayed on an LCD screen. I have most of that portion of the code working OK so far. It seems to do what I want it to do even if the code could be much more efficient.

I need to figure out a way to ensure the buttons are released first before another successful two hand press can be registered and another cycle initiated and they can't simply be held as the cycles continue. I have been trying to figure out how to place "state change" logic into the code but can't figure out where or how to include it properly. I created a variable called "LastButtonSafeChecked" and one called "ButtonSafeChecked" to act as a comparison but I can't get it to work properly. Where can I include that logic and is a state change the correct way to go about it in this case? Any suggestions on improving the code and implementing the missing logic are greatly appreciated as I think it is outside the scope of my coding knowledge. I included a lot of serial print lines in order to try and debug what events were happening. Here is the code:

/*
Designed to detect two button presses within specific time gap
Then turn on a relay for a specified time
Then turn it off and look for another press   
  */
// include the library code:
#include <LiquidCrystal.h>

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

// this constant won't change:
 const int  ButtonPin1 = 2;   // the pin that pushbutton 1 is attached to
 const int  ButtonPin2 = 3;   // the pin that pushbutton 2 is attached to
 const int  RelayPin = 5;     // the pin that the Relay is attached to

// Variables will change:
int ButtonPushCounter = 0;    // counter for the number of button presses
int ButtonState1 = 0;         // current state of the button1
int ButtonState2 = 0;         // current state of button2
int LastRelayState = 0;     // previous state of relay
int ButtonSafeOn1 = 0;        // button1 time check passed
int ButtonSafeOn2 = 0;        // button2 time check passed
int ButtonSafeChecked = 0;     // Button Check passed flag
int LastButtonSafeChecked = 0;  // Last State of button check
int RelayOn = 0;            // toggles relay on off state
unsigned long RelayMillis = 0;  //on time check for relay
unsigned long Button1Millis = 0;  //on time check for Button1
unsigned long Button2Millis = 0;  //on time check for Button2
const long RelayOnTime = 1000; //Length of time for relay
const long PushTimeWindow = 1000; //Length of time to allow two pushes

void setup() {
  // set up the LCD's number of columns and rows:
  lcd.begin(16, 2);
  // Print a message to the LCD.
  lcd.print("Press Count");
   // initialize the button pins as a inputs:
   pinMode(ButtonPin1, INPUT);
   pinMode(ButtonPin2, INPUT);
   // initialize the Relay Pin as an output:
   pinMode(RelayPin, OUTPUT);
   // initialize serial communication:
   digitalWrite(RelayPin, LOW);
   Serial.begin(9600);
}


void loop() {
unsigned long currentMillis = millis();
   // read the pushbutton input pins:
   ButtonState1 = digitalRead(ButtonPin1);
   ButtonState2 = digitalRead(ButtonPin2);
//check if Button1 is HIGH
    if (ButtonState1 == HIGH) {
        Button1Millis = currentMillis;
        Serial.println("Button1"); 
     }
//check if Button2 is HIGH
    if (ButtonState2 == HIGH) {
        Button2Millis = currentMillis;
        Serial.println("Button2");
      }     
//let program know Button1 is within window  
   if (ButtonState1 == HIGH) {
      if (currentMillis - Button1Millis < PushTimeWindow){
        ButtonSafeOn1 = 1;
    } else {
        ButtonSafeOn1 = 0;
    }
   }
//let program know Button2 is within window
   if (ButtonState2 == HIGH) {
      if (currentMillis - Button2Millis < PushTimeWindow){
        ButtonSafeOn2 = 1;
    } else {
        ButtonSafeOn1 = 0;
    }
   }
//Double Check both buttons are still pushed
   if (ButtonState1 == HIGH && ButtonState2 == HIGH) {
//Check to see if both button safetys add up and Change RelayOn to 1
    if (ButtonSafeOn1 + ButtonSafeOn2 == 2){
      ButtonSafeChecked = 1;
      RelayOn = 1;
      Serial.println("Relay On");
      } else {RelayOn = 0;
      ButtonSafeChecked = 0;
     }
    }
//Turn off relay if too much time elapsed
    if (currentMillis - RelayMillis < RelayOnTime) {
      RelayOn = 0;
      ButtonSafeChecked = 0;     
    }
//Turn on relay pin if RelayOn is 1
  if (RelayOn == 1) {
       digitalWrite(RelayPin, HIGH);
       Serial.println("Relay Pin should go High");
    } else {digitalWrite(RelayPin, LOW);
    }
//update the RelayMillis if it is still on
  if (RelayOn == 1){
      RelayMillis = currentMillis;
    }
//increment counter to diplay press count 
    if (LastRelayState != RelayOn){
      ButtonPushCounter++;
      Serial.println("State of Relay");
      Serial.print(RelayOn);
      Serial.println("  ");
      Serial.println("State of Last Relay State");
      Serial.print(LastRelayState);
    }
     // Delay a little bit to avoid bouncing
     delay(75);
     // set the cursor to column 0, line 1
  // (note: line 1 is the second row, since counting begins with 0):
  lcd.setCursor(0, 1);
  // print the number of seconds since reset:
  lcd.print((ButtonPushCounter/2));   
  //Save Current Relay State for Next Loop   
   LastRelayState = RelayOn;
  //Save Current Button Safety check for comparison for state change
   LastButtonSafeChecked =  ButtonSafeChecked;
}

update 10/13/17 Here is the current code that includes the Bounce2 library as suggested by user Jot as well as the state machine he created to handle the task. Could I have implemented Bounce2 better?

// 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,
  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

// 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 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;
    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 <= PushTimeWindow)
      {
        // 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);

}

Answer 1

The Arduino has a loop function, that runs over and over again.
The Arduino has a millis function for the time.
Those two combine beautifully with a state machine.

Once I made a function that returns the input value as BOTH_OFF, BOTH_ON, or SOMETHINE_ELSE, then the state machine automatically unfolds itself.

I hope you see how clean the code is. Every state has its own code. You don't have to think for hours how to make a spaghetti of if-statements. Although the sketch is now larger, there is no unnecessary code.
If you want to change something, you only have to find the state that you want to change. Adding a new state is also easy.

I have added some explanation in the sketch.

// A state machine is used, only to make it more clear what is going on.
//
// When a button is pressed, the input turns HIGH.
// Are there an external pulldown resistors ?
//
// 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.
enum
{
  WAIT_FOR_BOTH_OFF,
  WAIT_FOR_BOTH_ON,
  SAFETY_WINDOW,
  RELAY_ON,
} state;       // an 'enum' is automatically an integer <- it is not 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;


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

const unsigned long RelayOnTime = 1000;    // Length of time for relay
const unsigned long PushTimeWindow = 1000; // Length of time to allow two pushes

// 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;


void setup()
{
  Serial.begin(9600);
  Serial.println("Hello");

  pinMode(ButtonPin1, INPUT);
  pinMode(ButtonPin2, INPUT);

  pinMode(LED_BUILTIN, OUTPUT);  // LED_BUILTIN for testing

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


void loop()
{
  unsigned long currentMillis = millis();

  // Collect all input data.
  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 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;
    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 >= PushTimeWindow)
      {
        // 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(LED_BUILTIN, HIGH);  // LED_BUILTIN for testing
        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( LED_BUILTIN, LOW);  // LED_BUILTIN for testing
        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");
        digitalWrite(LED_BUILTIN, LOW);  // LED_BUILTIN for testing
        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 buttons and create a input state according to the 'enum' values.
  int ButtonState1 = digitalRead(ButtonPin1);
  int ButtonState2 = digitalRead(ButtonPin2);

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

  return(returnInputState);
}

A few extra notes about a state machine or "finite state machine" or "FSM":

In the SAFETY_WINDOW state, I use "if .. else if .." instead of two seperate "if" statements. When the state is changed due to the buttons, then I don't want to check the timeout anymore.

In the SAFETY_WINDOW state, the relay is turned on, because there I prepare for the next state. If you think that is confusing, you can add a state in between, for example called INITIATE_RELAY_ON, and turn the relay on in that state and then continue to the next state.
That is also nice about a state machine. You can add or remove states and move code around for you convenience.

In the SAFETY_WINDOW state I check the buttons first and then the timeout. I could also check the timeout first. It does not matter.
This is typical for a state machine. When delay is replaced by millis and the Arduino loop and the state machine are executed many times per second, then it does not matter which condition is checked first.

A sketch can have many state machines. Sometimes a library contains a state machine. Such libraries often have a 'update' function, that is supposed to be called every time the Arduino loop runs.

Correction nov 4, 2017
Oops, an 'enum' is not automatically an integer. An 'enum' is its own type. Its type is more like the type of a union or struct.

Answer 2

From what I understood, this is what you're trying to do:

• Turn a relay on as long as both buttons are pushed for 'pushWindowTime' milliseconds;
• Keep the relay on for 'relayOnTime' milliseconds, or until at least one of the buttons are pushed.

I believe the difficulty you're facing is mostly because time is a parameter in your program, and in order to achieve that you're using 'millis()', which returns the number of milliseconds elapsed since the program starts. But rather than working with that, it may be easier in this case to use a delay associated to a loop, which is what I did.

int ButtonPin1 = 0;
int ButtonPin2 = 0;
int RelayPin = 0;

int ButtonState1 = 0;
int ButtonState2 = 0;

bool highState = false;

int pushTimeWindow = 10000;
int relayOnTime = 10000;

void setup() {
  //IOs definition
  pinMode(ButtonPin1, INPUT);
  pinMode(ButtonPin2, INPUT);
  pinMode(RelayPin, OUTPUT);

  digitalWrite(RelayPin, LOW);
}

void loop() {  

  //Loop for reading each button's state in a given period of time 'pushTimeWindow'.
  for(int i = 0; i < pushTimeWindow; i++) {

    //Reads each button's state at every iteraction
    ButtonState1 = digitalRead(ButtonPin1);
    ButtonState2 = digitalRead(ButtonPin2);

    //Flags a boolean if both buttons are pushed.
    if((ButtonState1 == HIGH)&&(ButtonState2 == HIGH))
      highState = true;

    //If a button is released, breaks the loop.
    else {
      highState = false;
      break;
    }
    //Delays the loop one second.
    delay(1000);
  }

  if(highState) {

    //Loop to activate the relay as long as both buttons are pushed, during the time limit 'relayOnTime'.
    for(int i = 0; i < relayOnTime; i++) {

      ButtonState1 = digitalRead(ButtonPin1);
      ButtonState2 = digitalRead(ButtonPin2);

      //Activates the relay if both buttons are pushed.
      if((ButtonState1 == HIGH)&&(ButtonState2 == HIGH))
        digitalWrite(RelayPin, HIGH);

      //Shuts down the relay if a button is released and breaks the loop.
      else {
        digitalWrite(RelayPin, LOW);
        break;
      }

  //Delays the loop one second.
  delay(1000);
}

  }

  //Makes sure that every time loop() starts again, the relay is shut down.
  highState = false;

}

Hope this answers your question. Let us know what you find.

Answer 3

Here is the code:

your code and the code pieces posted so far suffer from two problems: 1) too specific to the application; and 2) too complicated.

here is a different approach to consider:

1) write a piece of code that reads the state of a button, with or without debouncing;

2) write a piece of code that detects no press, single press, double press, and long press (or even more if you want), using 1) above.

3) write a piece of code that detects the same but for two buttons, using 2) above.

...

you can consider the use of enum to make the code more robust.

with this approach, your code can be reused for future projects, and will be easier to maintain / understand as it is very intuitive and modular.