[SOLVED] "Hourglass" without delay - understanding if, while, & for loops

I’m trying to put together a (seemingly) simple sketch that has 8 LEDs. I want the sketch to turn them all on, and then turn them off one by one every 6667 milliseconds (1 minute / 9), then reset every minute and do it all again.

I can do it without issue using the delay function, but I want to eventually cycle through different light sequences based on a button press, so I don’t want a delay there to cause the button press to be missed.

This is my attempt to try to do my little psuedo hourglass without delay, but I end up getting all LEDs always on.

int timer = 6667;

void setup() {
  // use a for loop to initialize each pin as an output:
  for (int thisPin = 2; thisPin < 10; thisPin++)  {
    pinMode(thisPin, OUTPUT);      
  }
}

void loop() {
 long previousMillis = 1;
 unsigned long currentMillis = millis();
 for (int thisPin = 2; thisPin < 10; thisPin++) {
   digitalWrite(thisPin, HIGH);   
 }
 while (previousMillis % 60000 != 0){
   for (int thisPin = 2; thisPin < 11; thisPin++) { 
     if (currentMillis - previousMillis > timer) {
       previousMillis = currentMillis;
       if (thisPin != 10) {
        digitalWrite(thisPin, LOW);
       }
     }
   }
 }
}

I’m just learning all of this, and have tried several different things, what am I doing wrong?

Does it work any better if you replace this:

 while (previousMillis % 60000 != 0){

with this:

 while (previousMillis % 60000UL != 0){

-br

No, all the LEDs still stay on.

I should note that I did move this:

long previousMillis = 1;

outside of the loop, up above the setup. I realized that it's resetting the variable every time it goes through the loop. It didn't solve my problem though.

What does adding "UL" do?

You are not guaranteed to see every millisecond via millis(); sometimes it skips one, which would botch your % 60000 scheme, wouldn't it?

It would be much more reliable to use the difference in time, instead of time % 60000, to trigger the turn-off-leds section. You can see an example of this in the Blink Without Delay sketch.

Adding UL makes the constant Unsigned Long so the arithmetic is not demoted to 16-bit as is default in C. A frequent gotcha.

-br

Edit: sorry, posted too fast. Why not move the part that turns the lights on to setup(), and make a nice outer timer at 60000 ms that runs the inner lights-off timer.

In a later iteration, I did a Serial.print(previousMillis % 60000) and did notice that it wouldn't hit 0. I've tried changing != 0 to > 25 and starting previousMillis = 30, but that doesn't work either.

The basic approach of comparing current and previous values from millis() is the right way to do it, and addresses your valid concerns about handling button presses while the timer is in progress. However, the code is a bit wonky.

When you say this:

I want the sketch to turn them all on, and then turn them off one by one every 6667 milliseconds (1 minute / 9), then reset every minute and do it all again

I guess you want the sketch to count down from seven to zero, wait one minute and then start the countdown again.

I'd solve it like this:

Define an array of integers holding your LED pin numbers. Define an integer constant recording the number of items in the array. Write a function which takes an integer argument (which will be in the range 0 .. 7) which turns the appropriate LEDs on and off. (I'd implement that using a FOR loop.)

Define a global integer which holds the current countdown value i.e. 0 .. 7. Define a global (unsigned long) integer which holds the value of millis() when the countdown value was last changed.

Write a function which uses your millis() comparison logic to decrement the countdown value at regular intervals, and to restart the countdown. This is just a simple extension to the code in 'blink without delay' to test whether the countdown value is zero or non-zero, test whether the corresponding interval has elapsed, and either decrement the countdown or set it back to the initial value.

You main loop then just needs to call:

  • The function to perform the countdown
  • The function to display the LED state for current countdown value

I would use a switch case statement.

int state =0;
unsigned long startTime = millis();
void setup{
turn on all your LEDs
}

void loop(){

if (millis() -startTime >=6667){
 state ++;
 startTime =millis();
 switch (state){ 

case1:
 turn off first LED;
 break;

case2:
 turn off second LED;
 break;

case3:
 turn off third LED;
 break;

...etc

case9:
 turn on all LEDs;
 state = 0;
 break;

default:
 whatever you want to happen if there's an error;
 break;
}
}
read your button presses here.
}

I got it working. Actually, a friend not on these forums got it working for me (I did have to do one little tweak). Here’s the code that works:

void setup() {
  for (int thisPin = 2; thisPin < 10; thisPin++)  {     // Set up 8 pins to control the LEDs, using pins 2-9 
    pinMode(thisPin, OUTPUT);
  }
}

void loop() {
  Hourglass();  
}

void Hourglass() {
  unsigned long currentMillis = millis();
  unsigned long offTimer = 6667;
  int pinOff = 2 ;
  unsigned long nextOffTime = millis() + offTimer ;    // set the time that we need to turn off the next time (now + 6.667 sec)
  boolean done = false;
  
  for (int thisPin = 2; thisPin < 10; thisPin++) {
    digitalWrite(thisPin, HIGH);   
  }

  while(!done) {
    unsigned long currentMillis = millis() ;

    if(currentMillis > nextOffTime) {    // never do == here, since it might NEVER equal it.  As long as at least that much time has expired, we’re golden
      if(pinOff == 10) {                 // if we're at the last pin, say "we're done"
        done = true ;
      }
      digitalWrite(pinOff++, LOW);        // It's time to turn off the next Pin. turn it off, and increment the pin number
      nextOffTime = currentMillis + offTimer ;    // determine the time to turn off the next pin which is NOW + offTimer
    }
  }
}
  while(!done) {
    unsigned long currentMillis = millis() ;

    if(currentMillis > nextOffTime) {    // never do == here, since it might NEVER equal it.  As long as at least that much time has expired, we’re golden
      if(pinOff == 10) {                 // if we're at the last pin, say "we're done"
        done = true ;
      }
      digitalWrite(pinOff++, LOW);        // It's time to turn off the next Pin. turn it off, and increment the pin number
      nextOffTime = currentMillis + offTimer ;    // determine the time to turn off the next pin which is NOW + offTimer
    }
  }

How is this blocking function an improvement over delay()? Try again. This time, write a NON-BLOCKING function. Hint: There will be no while loops in the code.

I'm still learning this stuff, but that makes sense.

Back to the drawing board.

I tried again, using all if statements, but something is still wrong. Here is my code (semi-annotated):

boolean done = false;
unsigned long currentMillis = millis();
unsigned long offTimer = 500;
unsigned long nextOffTime = millis() + offTimer ;    // set the time that we need to turn off the next time (now + offTimer)
int pinOff = 2;

void setup() {
  for (int thisPin = 2; thisPin < 10; thisPin++)  {     // Set up 8 pins to control the LEDs, using pins 2-9 
    pinMode(thisPin, OUTPUT);
    Serial.begin(9600);
  }
}

void loop() {
  Hourglass();  
}

void Hourglass() {
  Serial.print("Done: ");
  Serial.print(done);
  Serial.print(" | Current Milliseconds: ");
  Serial.print(currentMillis);
  Serial.print(" | Next Pin: ");
  Serial.print(pinOff);
  Serial.print(" | Next Off: ");
  Serial.println(nextOffTime);
  currentMillis = millis();
  
  if(done = false) {
    if(currentMillis > nextOffTime) {    // never do == here, since it might NEVER equal it.  As long as at least that much time has expired, we’re golden
      if(pinOff = 10) {                 // if we're at the last pin, say "we're done"
        done = true;
      }
      else  {
        digitalWrite(pinOff, LOW);        // It's time to turn off the next Pin. turn it off, and increment the pin number
        pinOff = pinOff +1;
        nextOffTime = currentMillis + offTimer;    // determine the time to turn off the next pin which is NOW + offTimer
      }
    }
  }
  else  {
    for (int thisPin = 2; thisPin < 10; thisPin++) {
    digitalWrite(thisPin, HIGH);   
    }
    done = false;
    // nextOffTime = millis() + offTimer ;
  }
  
}

Here is the Serial Readout, it’s not recognizing if(currentMillis > nextOffTime):

Done: 0 | Current Milliseconds: 1 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 67 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 136 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 205 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 275 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 345 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 414 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 484 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 553 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 623 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 693 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 762 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 833 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 903 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 972 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1042 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1113 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1183 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1254 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1325 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1395 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1466 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1537 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1607 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1678 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1748 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1819 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1890 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 1960 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2032 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2103 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2173 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2244 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2315 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2385 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2456 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2527 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2597 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2668 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2739 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2809 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2880 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 2951 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3021 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3092 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3164 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3234 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3305 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3376 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3446 | Next Pin: 2 | Next Off: 500
Done: 0 | Current Milliseconds: 3517 | Next Pin: 2 | Next Off: 500
  if(done = false)
if(pinOff = 10) {

= and == are two different operators (assignment versus equality).

nextOffTime = currentMillis + offTimer

Addition can cause issues with overflow, you're much better off using subtraction:

if (millis() - lastTickTime > interval)
{
  lastTickTime = millis();
  // turn off an LED
  // set the next LED to turn off

  // If you're done turning off leds
    // turn on all LEDs
}

NOW it’s solved!!

I’m still learning the difference between = and ==

boolean done = true;                                 // done means all of the lights have been turned off. The lights haven't been turned on yet, so we start with true to get them on
unsigned long currentMillis = millis();              // start the timer
unsigned long offTimer = 6667;                       // set the interval between each light being turned off
unsigned long nextOffTime = millis() + offTimer ;    // set the time that we need to turn off the next time (now + offTimer)
int pinOff = 2;                                      // set the first pin that will be turned off

void setup() {
  for (int thisPin = 2; thisPin < 10; thisPin++)  {     // Set up 8 pins to control the LEDs, using pins 2-9 
    pinMode(thisPin, OUTPUT);
  }
}

void loop() {
  Hourglass();  
}

void Hourglass() {
  currentMillis = millis();              // update the timer
  
  if(done == false) {
    if(currentMillis > nextOffTime) {    // never do == here, since it might NEVER equal it.  As long as at least that much time has expired, we’re golden
      if(pinOff == 10) {                 // if we're at the last pin...
        pinOff = 2;                      // reset the pin to 2
        done = true;                     // change the logic to done to start over
      }
      else  {
        digitalWrite(pinOff++, LOW);        // It's time to turn off the next Pin. Turn it off, and increment the pin number
        nextOffTime = currentMillis + offTimer;    // determine the time to turn off the next pin which is NOW + offTimer
      }
    }
  }
  else  {
    for (int thisPin = 2; thisPin < 10; thisPin++) {    // "done" is true, so we have to turn all the LEDs back on for the next cycle.
    digitalWrite(thisPin, HIGH);
    }
    done = false;                                      // we also have to change "done" back to false so it will start turning them off again.
    nextOffTime = millis() + offTimer ;                // and we have to set the time when the first LED will be turned off.
  }
  
}

Arrch,

Just noticed your post. Thanks for catching that for me.

Canyonero: NOW it's solved!!

I think you would have a much simpler solution with less code duplication if you used the approach outlined in Reply #6.

Note that your method of comparing times does not deal with timer overflow correctly. It would be OK as long as you don't plan to leave this running for weeks on end, but it's not a good habit to get into.

PeterH:

Canyonero:
NOW it’s solved!!

I think you would have a much simpler solution with less code duplication if you used the approach outlined in Reply #6.

Note that your method of comparing times does not deal with timer overflow correctly. It would be OK as long as you don’t plan to leave this running for weeks on end, but it’s not a good habit to get into.

I get the timer overflow issue, that subtraction is better than addition to avoid the problem. As such, I’ve corrected the code, which is below. As for the approach in Reply #6, I don’t quite understand how it would be done in practice. If you have time to write it, I’d love to see what you mean.

boolean done = true;                                 // done means all of the lights have been turned off. The lights haven't been turned on yet, so we start with true to get them on
unsigned long currentTime = millis();              // start the timer
unsigned long nextTime = 6667;                       // set the interval between each light being turned off
unsigned long previousTime = millis();    // set the time that we need to turn off the next time (now + nextTime)
int pinOff = 2;                                      // set the first pin that will be turned off

void setup() {
  for (int thisPin = 2; thisPin < 10; thisPin++)  {     // Set up 8 pins to control the LEDs, using pins 2-9 
    pinMode(thisPin, OUTPUT);
  }
}

void loop() {
  Hourglass();  
}

void Hourglass() {
  currentTime = millis();              // update the timer
  offTimer = 6667;
  
  if(done == false) {
    if(currentTime - previousTime >= nextTime) {    // never do == here, since it might NEVER equal it.  As long as at least that much time has expired, we’re golden
      if(pinOff == 10) {                 // if we're at the last pin...
        pinOff = 2;                      // reset the pin to 2
        done = true;                     // change the logic to done to start over
      }
      else  {
        digitalWrite(pinOff++, LOW);        // It's time to turn off the next Pin. Turn it off, and increment the pin number
        previousTime = currentTime;    // determine the time to turn off the next pin which is NOW + nextTime
      }
    }
  }
  else  {
    for (int thisPin = 2; thisPin < 10; thisPin++) {    // "done" is true, so we have to turn all the LEDs back on for the next cycle.
    digitalWrite(thisPin, HIGH);
    }
    done = false;                                      // we also have to change "done" back to false so it will start turning them off again.
    previousTime = currentTime;                // and we have to set the time when the first LED will be turned off.
  }
  
}

Hey, I solved it too. The biggest problem I see with the OP’s code is that it expects all the led pins to be sequential and he makes absolute reference to them. With an array the physical sequence of the pins doesn’t matter. As you’ll see in my sketch.

const byte ledPin[] = {
  9, 7, 4, 5, 6, 8, 10, 11};
const byte ledCount = sizeof(ledPin) / sizeof(ledPin[0]);

void setup(){
  for (byte n = 0 ; n < ledCount; n++) pinMode(ledPin[n], OUTPUT);
}

void loop(){
  static byte pointer = ledCount;
  static unsigned long lastChangeTime;
  const unsigned long interval = 6667;
  unsigned long currentTime = millis();
  if (pointer == ledCount) 
  {
    for (byte n = 0; n < ledCount; n++) 
    {
      digitalWrite(ledPin[n], HIGH);
    }
  }
  if (currentTime - lastChangeTime > interval)
  {
    lastChangeTime = currentTime;
    if (pointer < ledCount)
    { 
      digitalWrite(ledPin[pointer], LOW);
    }
    if (++pointer > ledCount) 
    {
      pointer = 0;
    }
  }
}

Canyonero: As for the approach in Reply #6, I don't quite understand how it would be done in practice. If you have time to write it, I'd love to see what you mean.

I went into quite a lot of detail, and each sentence translates pretty directly into code. Which part of the description don't you understand?

Jimmy60:
Hey, I solved it too. The biggest problem I see with the OP’s code is that it expects all the led pins to be sequential and he makes absolute reference to them. With an array the physical sequence of the pins doesn’t matter. As you’ll see in my sketch.

const byte ledPin[] = {

9, 7, 4, 5, 6, 8, 10, 11};
const byte ledCount = sizeof(ledPin) / sizeof(ledPin[0]);

void setup(){
  for (byte n = 0 ; n < ledCount; n++) pinMode(ledPin[n], OUTPUT);
}

void loop(){
  static byte pointer = ledCount;
  static unsigned long lastChangeTime;
  const unsigned long interval = 6667;
  unsigned long currentTime = millis();
  if (pointer == ledCount)
  {
    for (byte n = 0; n < ledCount; n++)
    {
      digitalWrite(ledPin[n], HIGH);
    }
  }
  if (currentTime - lastChangeTime > interval)
  {
    lastChangeTime = currentTime;
    if (pointer < ledCount)
    {
      digitalWrite(ledPin[pointer], LOW);
    }
    if (++pointer > ledCount)
    {
      pointer = 0;
    }
  }
}

I just got my Arduino about a week ago, with almost no programming experience before then. There is a bunch of stuff in there that I’m going to have to read up on, such as byte, sizeof, and the in ledPin (the array, I take it? I haven"t learned arrays yet.).

The only issue I have after seeing it run is that the last pin (11) turns off and then back on so fast it appears to stay lit. My sketch turns it off for the entire interval time before turning all of them back on.