If with Millis() not counting as expected with greater than equation [SOLVED]

Hello, please bare with me. I’m new to both Arduino and the C++ language so it might be something easy. I’ve played with my code in every possible way I can imagine, including putting my math into Serial.println’s as to find out what’s happening. But I still don’t get it.

Below is my code, what I’m doing is using button1 to trigger relay1 for 8 seconds using the Millis() command. As you can see I’ve used Serial as much as I can and below my code is what I’m seeing through serial. It appears to sort of work when the Millis is under 8000 but as soon as it higher the math I’m using doesn’t seem to be working the way I’ve read everywhere else.

What am I missing? I’m sorry if I’m repeating a common issue but I’ve had a look around and can’t seem to find it. Thank you in advance.

Then while I’ve got your expertise I’m going to make this a sub program and am wondering how do I make all the 1’s a variable i.e. relay(1, 8000) then place the 1 after the int’s - relay and button. Or 2’s in the case of relay(2, 10).

Thank you

int relay1 = 41; //Setting of relay pin
int button1 = 31; //Setting of button pin
unsigned long timerMillis1; //Using an unsigned long to store a Millis entry

void setup() {
Serial.begin(9600); //Start the serial monitoring
digitalWrite(button1, HIGH); //Tell the Arduino to hold the button pin high
digitalWrite(relay1, HIGH); //Push relay pin high to keep normally open position
pinMode(relay1,OUTPUT); //State that the relay pin is an output
}

void loop() {
if (digitalRead(relay1) == HIGH) { //Is the relay closed waiting to open
if (digitalRead(button1) == LOW) { //Is the button depressed
unsigned long timerMillis1 = millis(); //Freeze a Millis to compair for timeout
digitalWrite(relay1, LOW); //Open the relay
Serial.print("Button pressed for relay 1, timerMillis is: ");
Serial.println(“timerMillis1”); //Print the Millis to serial
}
} else { //If relay is open see if it needs to closed
Serial.println(millis() - timerMillis1); //Write to serial the count up
if (millis() - timerMillis1 > 8000) { //Check to see if the time has lapsed
Serial.println(“Timer Finished”); //Advise that the timer is complete
digitalWrite(relay1, HIGH); //Return relay ro open
}
} //close if statement relay high
}

Button pressed for relay 1, timerMillis is: timerMillis1
4374
4374
.......truncated for forum post.......
7991
7997
Timer Finished
Button pressed for relay 1, timerMillis is: timerMillis1
10904
Timer Finished
Button pressed for relay 1, timerMillis is: timerMillis1
10980
Timer Finished
  if (digitalRead(relay1) == HIGH) {           //Is the relay closed waiting to open

Why are you reading from an output pin? Can't you remember that you've activated the relay?

        unsigned long timerMillis1 = millis(); //Freeze a Millis to compair for timeout

What "now" means is NOT timerMillis1. Use a name that means something!

And, do NOT use the same name as the equally poorly named global variable.

  digitalWrite(button1, HIGH);  //Tell the Arduino to hold the button pin high

It doesn't appear that you have called pinMode for this pin. Arduino pins default to inputs, so what you appear to be doing is activating the pin's internal pullup resistors. This code works, but it would be clearer and more readable if it were written as:

pinMode(button1, INPUT_PULLUP);

Alternatively, a comment such as, "Activate internal pullup resistors on the button's input pin," would be better. I point this out, because at first I thought your code had a mistake, until I really thought it through. Readable code will help you get better help.

  digitalWrite(relay1, HIGH);   //Push relay pin high to keep normally open position
  pinMode(relay1,OUTPUT);       //State that the relay pin is an output

These lines are out of order. The digital pin defaults to INPUT mode, so your first line is not setting it to HIGH, but is activating the internal pullup resistor, since that's what happens when you write HIGH to a digital pin that is in INPUT mode. Then you set the pin to OUTPUT mode. It may happen that this works (the same logic that activates the pullup resistor also causes the pin to be set to HIGH when you switch it to OUTPUT mode), but it would always be better to set the pinMode() before doing a digitalWrite() to the pin, just to be sure that everything is as you expect it to be.

        Serial.println("timerMillis1");        //Print the Millis to serial

When printing a variable, do not enclose it in quotes. Otherwise, you will just print the variable's name. Correct code would be:

Serial.println(timerMillis1);

I think that your actual problem may be related to the duplicate declaration of timerMillis1. You declare it both globally and inside the if{} structure. Once the if{} structure ends, the local copy of the variable goes away, and your stored millis() value is lost. The timerMillis1 variable that you're referencing in the else{} structure is the global one, which has never been properly initialized. Frankly, I'm not sure how this code is working at all.

Try replacing this line:

        unsigned long timerMillis1 = millis(); //Freeze a Millis to compair for timeout

With this:

        timerMillis1 = millis(); //Freeze a Millis to compair for timeout

By including "unsigned long", you are telling the compiler that you are declaring a new variable in the local scope. This isn't what you intend to be doing. And if it was what you intend to be doing, using the same name for a global and a local variable is bad practice, for reasons just like this.

joshuabardwell, I'm so grateful with how much information your providing and thank you for your time for a rookie. I'm glad I put the comment lines in now, I nearly didn't.

Serial.println("timerMillis1");        //Print the Millis to serial

How did those get in there, grrr, I had that working but was trying to reduce my lines for the forum. Turns out you can't have two things in a println then when I broke it down I must have added the "'s

  pinMode(button1, INPUT_PULLUP);
  pinMode(relay1,OUTPUT);       //State that the relay pin is an output
  digitalWrite(relay1, HIGH);   //Push relay pin high to keep normally open position

Yep, no worries I'll make those changestimerMillis1 = millis(); //Freeze a Millis to compair for timeout Thank you for that, the unsigned long only just make sense to me, but I'm not sure it this will fix my math issue. I'll find out tonight. Thank you again for your help.

PaulS, ummm, thank you for your input.

sndguru: ...but I'm not sure it this will fix my math issue.

Good one sndguru, tell the expert that he doesn't know anything!!

joshuabardwell, your a legend! that's got it working perfectly now. Thank you so much, I've learnt more than you know from this post.

Finshed code for those of you playing at home.

int relay1 = 41; //Setting of relay pin int button1 = 31; //Setting of button pin int delay1 = 8000; //The delay required for relay1 to be open for int count1 = 0; unsigned long timerMillis1; //Using an unsigned long to store a Millis entry

void setup() { Serial.begin(9600); //Start the serial monitoring pinMode(button1, INPUT_PULLUP); //Tell the Arduino to hold the button pin high pinMode(relay1,OUTPUT); //State that the relay pin is an output digitalWrite(relay1, HIGH); //Push relay pin high to keep normally open position }

void loop() { if (digitalRead(relay1) == HIGH) { //Is the relay closed waiting to open if (digitalRead(button1) == LOW) { //Is the button depressed timerMillis1 = millis(); //Freeze a Millis to compair for timeout digitalWrite(relay1, LOW); //Open the relay Serial.print("relay 1 button pressed, delay is: "); // Serial.print(delay1 / 1000); // --Print the Millis in seconds to serial Serial.println(" second(s)"); // } } else { //If relay is open see if it needs to closed if (millis() - timerMillis1 > delay1) { //Check to see if the time has lapsed Serial.println("relay 1 timer finished"); //Advise that the timer is complete digitalWrite(relay1, HIGH); //Return relay ro open count1 = 0; } else { if (millis() - timerMillis1 > count1 + 1000) { // Has a second lapsed during the counting count1 = millis() - timerMillis1; // Increase the count for the next time Serial.print("relay1 at "); // Serial.print(count1 / 1000); // --Print counter in seconds to serial Serial.println(" second(s)"); // } } } //close if statement relay high }

button pressed for relay 1, delay is: 8 second(s)
relay1 at 1 second(s)
relay1 at 2 second(s)
relay1 at 3 second(s)
relay1 at 4 second(s)
relay1 at 5 second(s)
relay1 at 6 second(s)
relay1 at 7 second(s)
relay 1 timer finished