using multiple buttons with millis() - having a slight problem

Hi there,

First of all ... thank you. Reading this forum has been game changing for a new arduino user like myself. I thank you all.

So I am learning the millis() thing having recently graduated from delay(). I'm hoping to build a simple irrigation system wherein 2 (with the idea to expand) momentary push buttons activate a relay (solenoid valve) and display and indicator LED while on.

The code I am using is copied below. Each button works independently, but if I press one, wait 3 seconds and press the second, the second time dictates when they both turn off.

unsigned long elapsedTime;
unsigned long onTime;

void setup()
{
 onTime = millis();
 onTime = elapsedTime; 
 pinMode(13, OUTPUT);  //Initialize pin 12 as status LED 
 pinMode(2, INPUT);    // Our button pin 
 pinMode(8, OUTPUT);
 pinMode(3, INPUT);
}

void loop()
{
 if (digitalRead(2) == HIGH)   // Switch is closed to start LED timer
        {
         digitalWrite(13, HIGH);  // LED comes On  
         onTime = millis();  
         }
     if(onTime > 0 && millis() - onTime > 5000) 
           {      
           digitalWrite(13, LOW);  // LED goes off
            onTime = 5000;
           }

 if (digitalRead(3) == HIGH)   // Switch is closed to start LED timer
        {
         digitalWrite(8, HIGH);  // LED comes On  
         onTime = millis();  
         }
     if(onTime > 0 && millis() - onTime > 5000) 
           {      
           digitalWrite(8, LOW);  // LED goes off
            onTime = 5000;
           }
}

Can you give me a rough example?

Thank you very much for your time and knowledge.

NEVERMIND!!!!
Thank you for your help! I played with the code and got it working!

Thank you !!!!!!!!!

Hi,

Just to share... This is the code that I've been able to make work. I am sure that there are problems and issues here... but just to share and hopefully help others.

unsigned long elapsedTime;
unsigned long onTime1;
unsigned long onTime2;

void setup()
{
 Serial.begin(9600);
 onTime1 = millis();
 onTime2 = millis();
 onTime1 = elapsedTime;
 onTime2 = elapsedTime;
 pinMode(13, OUTPUT);  //Initialize pin 12 as status LED 
 pinMode(2, INPUT);    // Our button pin 
 pinMode(8, OUTPUT);
 pinMode(3, INPUT);
}

void loop()
{
 if (digitalRead(2) == HIGH)   // Switch is closed to start LED timer
        {
         digitalWrite(13, HIGH);  // LED comes On  
         onTime1 = millis();  
         }
     if(onTime1 > 0 && millis() - onTime1 > 5000) // This is the time for button 2
           {      
           digitalWrite(13, LOW);  // LED goes off
            onTime1 = 5000;
           }

 if (digitalRead(3) == HIGH)   // Switch is closed to start LED timer
        {
         digitalWrite(8, HIGH);  // LED comes On  
         onTime2 = millis();  
         }
     if(onTime2 > 0 && millis() - onTime2 > 1000) // This is the time for the delay on button 3
           {      
           digitalWrite(8, LOW);  // LED goes off
            onTime2 = 5000;
           }
}

You have read the wrong examples :confused:

I always use an extra variable to turn it on. In this example I call that variable "enabled": https://gist.github.com/Koepel/01a6088f321eef5ec62f0b470c97a01e.

The millis() function returns a unsigned long variable for the time in milliseconds. It can therefor not be used as a flag as well by testing if it is not zero.

Your text layout of the code can be improved. Can you try to have the indents and spaces nicer ? If the text is a big mess, then the result of your coding might be a big mess as well. The best code is code that shows what is going on with a single glance at it.

You have some issues with your code, please look at this:

unsigned long elapsedTime; //This value is not used, remove it
unsigned long onTime1; //Initialize to zero / 0
unsigned long onTime2; //Initialize to zero / 0

void setup()
{
 Serial.begin(9600);

 //Why assign onTime1 + onTime2 twice for no reason?
 onTime1 = millis(); //If onTime1 is initialized to 0 above, remove this
 onTime2 = millis(); //If onTime2 is initialized to 0 above, remove this
 onTime1 = elapsedTime; //Remove this, not used
 onTime2 = elapsedTime; //Remove this, not used

 //Usually you would use INPUT_PULLUP for buttons and test for LOW instead of HIGH
 pinMode(13, OUTPUT);  //Initialize pin 12 as status LED
 pinMode(2, INPUT);    // Our button pin
 pinMode(8, OUTPUT);
 pinMode(3, INPUT);
}

void loop()
{
 if (digitalRead(2) == HIGH)   // Switch is closed to start LED timer
 {
    digitalWrite(13, HIGH);  // LED comes On 
    onTime1 = millis(); 
 }
 //Add parantheses  for readability
 if((onTime1 > 0) && (millis() - onTime1 > 5000)) // This is the time for button 2
 {
   digitalWrite(13, LOW);  // LED goes off
   onTime1 = 5000; //Should be set to zero/0 to indicate LED-off
 }

 //Below as above
 if (digitalRead(3) == HIGH)   // Switch is closed to start LED timer
        {
         digitalWrite(8, HIGH);  // LED comes On 
         onTime2 = millis(); 
         }
     if(onTime2 > 0 && millis() - onTime2 > 1000) // This is the time for the delay on button 3
           {     
           digitalWrite(8, LOW);  // LED goes off
            onTime2 = 5000;
           }
}

Thank you for your help and suggestions. I will work on addressing your criticisms.

Thanks again.

Now that you have graduated to millis(), the next thing to garduate to is using classes.

PaulMurrayCbr:
Now that you have graduated to millis(), the next thing to garduate to is using classes.

Why? The functionality of the sketch does not require coding of any classes. Arrays could make the code less repetitivive, classes are overkill for such a simple sketch.

Danois90:
Why? The functionality of the sketch does not require coding of any classes. Arrays could make the code less repetitivive, classes are overkill for such a simple sketch.

I beg to differ.
Classes would offer encapsulation of the different "onTime"s etc., and make it much easier to read.

Of course, it's not necessary, but if the OP wants to learn, it's a great opportunity.

Pieter

Danois90:
Why? The functionality of the sketch does not require coding of any classes. Arrays could make the code less repetitivive, classes are overkill for such a simple sketch.

Well, functionality never requires classes. Arrays are fine, but it means all your variables need array indexes, you have to pass those indexes around. You need a set of button pins, a set of led pins, a set of timer variables, variables for state, a set of solenoid pins, etc. A class is a great way to encapsulate that.

Sure, classes aren't necessary. But for the amount of functionality the OP wants, I don't think it's overkill.

Doing it first with arrays and then discovering how much neater classes are in accomplishing the same job is a great way to proceed with learning the language.

step 1 - use separate arrays of all the variables, with functions taking an array index
step 2 - use an array of struct, with functions taking an array index
step 3 - make a constructor for initialization of pin assignments
step 4 - move functionality into member functions, get rid of the indexes

There is no need to build a bridge in order to pass a creek once :wink:

Danois90:
There is no need to build a bridge in order to pass a creek once :wink:

+1

I like the idea of classes when one needs to deal with several instances. The example that comes to mind is a simulation of a bee-farm. I would make a class for bee and for hive but not for farm.

...R

Hi,
Thank you for all your help. I found another example and expanded that to two buttons and it seems to be working pretty well. I still don't fully understand the entire program, but I am enjoying the process of tweaking and seeing the results. I am going to post the code below just for the sake of it, if you feel so inclined I am open to feedback. Thank you.

//CONSTANTS

const int BUTTON_PIN2 = 2; // Button2
const int BUTTON_PIN3 = 3; // Button3
const int relay1 = 7;
const int relay2 = 6;
const int greenLED2 = 8;
const int redLED2 = 9;
const int greenLED3 = 10;
const int redLED3 = 11;

//VARIABLES

//int buttonPushCounter = 0;   // counter for the number of button presses
int buttonState1 = 0;         // current state of the button
int buttonState2 = 0;         // current state of the button
int lastButtonState = 0;     // previous state of the button
bool relayOn1 = false;
bool relayOn2 = false;

//MILLIS
unsigned long previousMillis = 0;
unsigned long previousMillis2 = 0;
const unsigned long interval = 3000;
const unsigned long redLedInterval = 1000;
//const unsigned long redLedInterval2 = 5000;


void setup()
{
  pinMode(BUTTON_PIN2, INPUT);
  digitalWrite(BUTTON_PIN2, HIGH); // pull-up
  pinMode(BUTTON_PIN3, INPUT);
  digitalWrite(BUTTON_PIN3, HIGH); // pull-up
  Serial.begin(9600);

  pinMode(greenLED2, OUTPUT);
  pinMode(redLED2, OUTPUT);
  pinMode(greenLED3, OUTPUT);
  pinMode(redLED3, OUTPUT);

  //SWITCH HANDLERS
  pinMode(relay1, OUTPUT);
  digitalWrite(relay1, HIGH);
  pinMode(relay2, OUTPUT);
  digitalWrite(relay2, HIGH);

}


void loop(){
  
    // read the pushbutton input pin:
    buttonState1 = digitalRead(BUTTON_PIN2);
    buttonState2 = digitalRead(BUTTON_PIN3);
    unsigned long currentMillis1 = millis();
    unsigned long currentMillis2 = millis();

    // if button is pressed, turn relay on (if it wasn't already on), and reset the timer
    if( buttonState1==HIGH ) // no need to check for previous state, in this specific case
    {
        previousMillis = currentMillis1;
        digitalWrite(relay1, LOW);
        digitalWrite(greenLED2, HIGH);
        digitalWrite(redLED2, LOW);
        relayOn1 = true;
    }

    // if relay is currently on...
    if( relayOn1 )
    {
        // turn red led on, if close to turning off the relay
        if (currentMillis1 - previousMillis >= interval-redLedInterval )
            digitalWrite(redLED2, (millis()/100)%2);//blink red led; 300ms on; 300ms off     
        // if enough time has elapsed, turn of the relay
        if (currentMillis1 - previousMillis >= interval) 
        {
            // .. turn off relay
            digitalWrite(relay1, HIGH);
            digitalWrite(greenLED2, LOW);
            digitalWrite(redLED2, LOW);
            relayOn1 = false;
        }
    Serial.write("Garden Bed 1 is being Watered");    
    Serial.println(relayOn1);
    } 
    if( buttonState2==HIGH ) // no need to check for previous state, in this specific case
    {
        previousMillis2 = currentMillis2;
        digitalWrite(relay2, LOW);
        digitalWrite(greenLED3, HIGH);
        digitalWrite(redLED3, LOW);
        relayOn2 = true;
    }

    // if relay is currently on...
    if( relayOn2 )
    {
        // turn red led on, if close to turning off the relay
        if (currentMillis2 - previousMillis2 >= interval-redLedInterval )
            digitalWrite(redLED3, (millis()/100)%2);//blink red led; 300ms on; 300ms off     
        // if enough time has elapsed, turn of the relay
        if (currentMillis2 - previousMillis2 >= interval) 
          
        {
            // .. turn off relay
            digitalWrite(relay2, HIGH);
            digitalWrite(greenLED3, LOW);
            digitalWrite(redLED3, LOW);
            relayOn2 = false;
        }
    
    Serial.write("Garden Bed 2 is being Watered");    
    Serial.println(relayOn2);  
    } 
}

Yeah, I've played with different intervals. When this is eventually installed I may have different intervals set for different garden beds, but I set it the same here for ease of testing. Thanks for the feedback.

Ahhhh, I see. No i didn't understand, but i see what you mean now. Thank you.