Go Down

Topic: Sticky - Blink Two LEDs, independent, no delay (Read 7055 times) previous topic - next topic

Msquare

Code: [Select]
/* "Multiple independent delay" - Example 1 - Two blinking LEDs */

const int LED1 = 8 ;
const int LED2 = 10 ;
const int QuickBlink = 555 ;
const int SlowBlink = 1234 ;

unsigned long QuickTimer , SlowTimer ;
int LED1State = LOW ;
int LED2State = LOW ;

void setup() {
  pinMode(LED1,OUTPUT) ;
  pinMode(LED2,OUTPUT) ;
}

void loop() {
  if ( millis() - QuickTimer > QuickBlink ) {
    if (LED1State==LOW) LED1State = HIGH ; else LED1State = LOW ;
    digitalWrite(LED1, LED1State ) ;
    QuickTimer = millis() ;
  }
  if ( millis() - SlowTimer > SlowBlink ) {
    if (LED2State==LOW) LED2State = HIGH ; else LED2State = LOW ;
    digitalWrite(LED2, LED2State ) ;
    SlowTimer = millis() ;
  }
}


This is (almost) without comments. That is very intentional. But feel free to disagree - constructivly ;)

In a possible footnote comment; you can verify unintentional behaviour if you accidentally make the timers int rather than unsigned long (leaving out unsigned takes 23 days to show).

Nick Gammon

Rats! I was about to do a "blink two LEDs" example ... but you beat me to it!

If I may constructively suggest, this amended version:

Code: [Select]
/* "Multiple independent delay" - Example 1 - Two blinking LEDs */

const byte LED1 = 8 ;    // first LED
const byte LED2 = 10 ;   // second LED

const unsigned long QuickBlink = 555 ;   // time in mS to blink first LED
const unsigned long SlowBlink = 1234 ;   // time in mS to blink second LED

void setup()
  {
  pinMode (LED1, OUTPUT) ;
  pinMode (LED2, OUTPUT) ;
  }  // end of setup

void loop() {

static unsigned long QuickTimer,   // when we last blinked LED1
                     SlowTimer;    // when we last blinked LED2
 
  // get current elapsed time
  unsigned long now = millis ();
 
  if ( now - QuickTimer > QuickBlink )
    {
    digitalWrite (LED1, !digitalRead (LED1) );  // toggle LED
    QuickTimer = now;
    }  // end if QuickTimer time up
 
  if ( millis() - SlowTimer > SlowBlink )
    {
    digitalWrite (LED2, !digitalRead (LED2) ); // toggle LED
    SlowTimer = now;
    }  // end if SlowTimer time up
 
 
  // do other stuff here ...
 
}  // end of loop


Changes:


  • Comments (eg. time is in mS)

  • Blink times as unsigned long in case you want more than 32.767 seconds

  • Toggling the LEDs done in one line rather than use another variable

  • Moved the variables inside the loop function to avoid pollution of static variable space

  • Only called millis () once to save time, and to make it slightly clearer

  • Added a comment to make it clear you can do other stuff as well as blinking LEDs

Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

Coding Badly


">=" not ">"

Given the precision of the blink times...
Code: [Select]
QuickTimer += QuickBlink;
SlowTimer += SlowBlink;

...is a better way to honor that precision.

Code: [Select]
void loop() {
static unsigned long QuickTimer,   // when we last blinked LED1
                     SlowTimer;    // when we last blinked LED2

...Uninitialized automatic variables is a common mistake.  I suggest making QuickTimer and SlowTimer "normal" global variables to avoid the issue.  Otherwise, the example needs an explanation for the "static".

PaulS

Quote
...is a better way to honor that precision.

Except that addition leads to rollover issues.

Coding Badly


Naw.  It works.  In the one extreme case (no overrun), these two are functionally equivalent...

Code: [Select]
QuickTimer += QuickBlink;
Code: [Select]
QuickTimer = now;

Nick Gammon

Amended version:

Code: [Select]
/* "Multiple independent delay" - Example 1 - Two blinking LEDs */

const byte LED1 = 8 ;    // first LED pin number
const byte LED2 = 10 ;   // second LED pin number

const unsigned long QuickBlinkInterval = 555;   // time interval in mS to blink first LED
const unsigned long SlowBlinkInterval = 1234;   // time interval in mS to blink second LED

void setup()
  {
  pinMode (LED1, OUTPUT);
  pinMode (LED2, OUTPUT);
  }  // end of setup

void loop() {

// static variables persist between iterations
static unsigned long QuickTimer = 0,   // when we last blinked LED1
                     SlowTimer = 0;    // when we last blinked LED2
 
  // get current elapsed time
  unsigned long now = millis ();
 
  // check if time to toggle LED 1
  if ( (now - QuickTimer) >= QuickBlinkInterval)
    {
    digitalWrite (LED1, !digitalRead (LED1));  // toggle LED
    QuickTimer = now;
    }  // end if QuickTimer time up
 
  // check if time to toggle LED 2
  if ( (now - SlowTimer) >= SlowBlinkInterval )
    {
    digitalWrite (LED2, !digitalRead (LED2)); // toggle LED
    SlowTimer = now;
    }  // end if SlowTimer time up
 
 
  // do other stuff here ...
 
}  // end of loop


This explicitly initializes the timer variables, with a comment about why they are static.
Fixed the >= issue.

You shouldn't get too much creep because we are using a single call to millis (). In any case I prefer to keep something like a tutorial simple, rather than more complex to save the occasional millisecond creep. We are blinking LEDs here after all, not sending pulses to a time-critical device.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

retrolefty

Quote
You shouldn't get too much creep because we are using a single call to millis (). In any case I prefer to keep something like a tutorial simple, rather than more complex to save the occasional millisecond creep. We are blinking LEDs here after all, not sending pulses to a time-critical device.


Nice job Nick;

Looks good to me. We aren't after a system quality library function, but rather a piece of educational code to show and explain the concept. This seems to me to me that criteria. Probably needs an introduction sentence or two in the posting before this code placed into a code window.

Coding Badly


I agree.  Nice job Nick.

Quote
You shouldn't get too much creep because we are using a single call to millis ().


That is one source.  Another source is a result of loop taking more than the resolution of millis to execute (about 1 millisecond).  This is a very common case but the amount of creep will be zero to a very small amount (loop execution time / delay time).  The most extreme case is when loop takes more than the fastest blink rate (555 milliseconds in the example).

Quote
We are blinking LEDs here after all, not sending pulses to a time-critical device.


I disagree for two reasons: 1. There are people who use blink-without-delay to perform (somewhat) time critical tasks.  2. The goal is provide a concrete general purpose example.  A bit of code that the user can rely upon no matter what the circumstances.

Quote
In any case I prefer to keep something like a tutorial simple, rather than more complex to save the occasional millisecond creep.


I agree.  I cannot recall a creeping blink-without-delay ever being an issue.

Is there a reason not to include both versions?

Nick Gammon

After careful consideration I think you are completely correct. :)

Amended version:

Code: [Select]
/* "Multiple independent delay" - Example 1 - Two blinking LEDs */

const byte LED1 = 8 ;    // first LED pin number
const byte LED2 = 10 ;   // second LED pin number

const unsigned long QuickBlinkInterval = 555;   // time interval in mS to blink first LED
const unsigned long SlowBlinkInterval = 1234;   // time interval in mS to blink second LED

void setup()
  {
  pinMode (LED1, OUTPUT);
  pinMode (LED2, OUTPUT);
  }  // end of setup

void loop() {

// static variables persist between iterations
static unsigned long QuickTimer = 0,   // when we last blinked LED1
                     SlowTimer = 0;    // when we last blinked LED2
 
  // get current elapsed time
  unsigned long now = millis ();
 
  // check if time to toggle LED 1
  if ( (now - QuickTimer) > QuickBlinkInterval)
    {
    digitalWrite (LED1, !digitalRead (LED1));  // toggle LED
    QuickTimer += QuickBlinkInterval;  // increment to next expected fire time
    }  // end if QuickTimer time up
 
  // check if time to toggle LED 2
  if ( (now - SlowTimer) > SlowBlinkInterval )
    {
    digitalWrite (LED2, !digitalRead (LED2)); // toggle LED
    SlowTimer += SlowBlinkInterval;  // increment to next expected fire time
    }  // end if SlowTimer time up
 
 
  // do other stuff here ...
 
}  // end of loop


This isn't much (any) more complicated than the earlier, slightly creepy, version.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

Msquare

I am not wholeheartedly in agreement with the alternate suggestion. Instead of just writing negative comments, or just saying "mine is better" (well, that may be my :P opinion), I will explain my deliberations to achieve the goal:

  • A short, simple and uncomplicated example, cut-n-paste ready to run.  It is to aid people who look at the (in)famous BlinkWithoutDelay who can not make the mental leap to generalize this to two or more LEDs.

(Note to self: start another thread with a rewritten BlinkWithoutDelay that is correct and a clearer comment on what "NoDelay" means to the other code in loop)

In particular, I did not want to introduce a number of other "tricks of the trade" which would detract from getting the main point.

So here is a line-by-line argument for my choices, where I now have included the good points from above discussion.

Code: [Select]
/* "Multiple independent delay" - Two blinking LEDs

Working code example that blinks two LEDs, each with its own independent period.
This extends the BlinkWithoutDelay example of a single LED, showing how you
can implement multiple independent timing paths in the main loop.

Written by several forum members. In public domain.  Oct 2011.
*/
Yeah ... I amended it to this, inspired by the official examples. (No mention of using a resistor, which is the anode and so on. This is a programming exercise.)

Code: [Select]

// Which pins are connected to which LED
const int GreenLED = 8 ;
const int RedLED = 10 ;
This is to indirectly teach that meanigfull variable names are usefull. If I added a comment "//Pin8 connect to greenLED" it defeats half the reason for variable name. int choosen beacuse that is what the documentation says the pinMode, digitalWrite expect.

Code: [Select]

// Time periods of blinks. Time variables and constants should be unsigned long, always
const unsigned long QuickBlink = 555UL ;
const unsigned long SlowBlink = 1234UL ;
Make it clear that millis/micros are unsigned long and all variable and constants should be of that form. As such the constants should be suffixed UL. The lesser sophisticated reader may not understand why, but will follow the example, and thus avoid the classic "why isnt 100,000 working". (No indvidual line comment for each variable for the same reason as before)

Code: [Select]

// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer , SlowTimer ;

// Variables to store what to change the LED to.
int GreenLEDState = LOW ;
int RedLEDState = LOW ;
I am undecided whether to initialize the timer variables or not.

Code: [Select]
void setup() {
  pinMode(GreenLED,OUTPUT) ;
  pinMode(RedLED,OUTPUT) ;
}
Intentionally avoided adding comments that "here starts the setup. Setup is only called once. ..." "this is the closing brace that ends setup ..."

Code: [Select]
void loop() {

// Handling the blink of one LED.
// First, check if it is time to change state
  if ( millis() - QuickTimer > QuickBlink ) {
Comments added, inspired by Nick's example and the official examples.

Code: [Select]
    // It is time to change state. Calculate next state.
    if (GreenLEDState==LOW) GreenLEDState = HIGH ; else GreenLEDState = LOW ;
This item is most carefully choosen. The "new" reader knows and understands the if-then-else construct. I sometimes write lines like this using the trenary "X?y:z" operator, but that is not part of the subset of C that Arduino uses. (And it generates the same binary stuff anyhow). I was considering whether to use multiple lines (makes the example long-ish) and if to relgiously use braces. I slightly favour braces, even though here they would be redundant, but then it is less ambigous, less likely to break code when modifying. In the end keeping the "toggle" line as short as possible won the day.

I really like the digitalWrite(p, !digitalRead(p) ) alternative. I will use it in my code. But it is NOT used here because it "misdirects" in this context. The unsophisticated user gets sidetracked, thinks it is part of the "simultaneous magic", and it will not let him/her generalize to do PWM (analogWrite(p, analogRead(p)+1) ??) or other intelligent items. Lastly on a pure note, it "violates" type, we are relying on that LOW is !HIGH by the quirks of C's boolean representation.

Thus a boring if-then-else, and the comment shows it could be any calculation of the next state. (And I will "submit" an example with PWM, too, later, another day)

Code: [Select]
    // Write new state
    digitalWrite(GreenLED, GreenLEDState ) ;
    // Note the current millis(), so we change state again "QuickTimer" milliseconds later
    QuickTimer = millis() ;
  }
Comments added, inspired by Nick's example and the official examples. Codewise there is not much choice here, as I intentionally wanted to show the similarity to the BlinkWithoutDelay. (I'll comment on the "creep" and the "save millis() in a variable" shown in Nick's versions later.)

The whole code example without interruptions in the next post, where I have included an expanded version of the trailing comment for "here goes more code". Good point that.

Msquare

Code: [Select]
/* "Multiple independent delay" - Two blinking LEDs

Working code example that blinks two LEDs, each with its own independent period.
This extends the BlinkWithoutDelay example of a single LED, whoowing how you
can implement multiple indpendnt timing paths in the main loop.

Written by several forum members. In public domain.  Oct 2011.
*/

// Which pins are connected to which LED
const int GreenLED = 8 ;
const int RedLED = 10 ;

// Time periods of blinks. Time variable and constants should be unsigned long, always
const unsigned long QuickBlink = 555UL ;
const unsigned long SlowBlink = 1234UL ;

// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer , SlowTimer ;

// Variable to store what to change the LED to.
int GreenLEDState = LOW ;
int RedLEDState = LOW ;

void setup() {
  pinMode(GreenLED,OUTPUT) ;
  pinMode(RedLED,OUTPUT) ;
}

void loop() {

// Handling the blink of one LED.
// First, check if it is time to change state
  if ( millis() - QuickTimer > QuickBlink ) {
    // It is time to change state. Calculate next state.
    if (GreenLEDState==LOW) GreenLEDState = HIGH ; else GreenLEDState = LOW ;
    // Write new state
    digitalWrite(GreenLED, GreenLEDState ) ;
    // Note the current millis(), so we change state again "QuickTimer" milliseconds later
    QuickTimer = millis() ;
  }

// The other LED is controlled the same way. Repeat for more LEDs
  if ( millis() - SlowTimer > SlowBlink ) {
    if (RedLEDState==LOW) RedLEDState = HIGH ; else RedLEDState = LOW ;
    digitalWrite(RedLED, RedLEDState ) ;
    SlowTimer = millis() ;
  }

/* Other code that needs to execute goes here.
   It will be called many thousand times per second because the above code
   does not wait for a LED single blink to finish. */

}

PaulS

Quote
In the end keeping the "toggle" line as short as possible won the day.

I still prefer that this take 4 physical lines. To me,
Code: [Select]
    if (GreenLEDState==LOW)
      GreenLEDState = HIGH;
    else
      GreenLEDState = LOW;

is far easier to read and understand than
Code: [Select]
    if (GreenLEDState==LOW) GreenLEDState = HIGH ; else GreenLEDState = LOW ;
I have trouble picking out the individual statements in that mess.

Msquare

Interesting with various ways to "increment" the Timer variable, also known as creep ellimination. I originally omitted that for the same reasons; it is a sideline to the main point: having two indpendent timers using millis(), the extension of DelayWithoutBlink.

Thinking about it for a while, I am changing my mind here. I magnanimously ;) concur to change
QuickTimer = millis() ;
to
QuickTimer += QuickInterval ;
It seems "cleaner" and seems closer to the intent. It really should be in the WithoutDelayBlink example, too. (The observant reader will notice I have also decided to change the constant name. And I like to have names a bit shorter)

I tried single ifthenelse and the 4line version. The loop code is still short enough, ie. fits on one screen. So thats changed, too.

I have not used the "now" variable. Matter of clutter. (You may notice I have not used the static local timer variables - same reason)

And I have a couple of typos in the comments. <sigh> OK, whole code is posted (again). The code has been tested so the syntax is OK and it works.
Code: [Select]
/* "Multiple independent delay" - Two blinking LEDs

Working code example that blinks two LEDs, each with its own independent period.
This extends the BlinkWithoutDelay example of a single LED, showing how you
can implement multiple independent timing paths in the main loop.

Written by several forum members. In public domain.  Oct 2011.
*/

// Which pins are connected to which LED
const int GreenLED = 8 ;
const int RedLED = 10 ;

// Time periods of blinks. Time variable and constants should be unsigned long, always
const unsigned long QuickInterval = 555UL ;
const unsigned long SlowInterval = 1234UL ;

// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer , SlowTimer ;

// Variable to know what to change the LED to.
int GreenLEDState = LOW ;
int RedLEDState = LOW ;

void setup() {
  pinMode(GreenLED,OUTPUT) ;
  pinMode(RedLED,OUTPUT) ;
}

void loop() {

// Handling the blink of one LED.
// First, check if it is time to change state
  if ( millis() - QuickTimer > QuickInterval ) {
    // It is time to change state. Calculate next state.
    if (GreenLEDState==LOW)
      GreenLEDState = HIGH;
    else
      GreenLEDState = LOW;
    // Write new state
    digitalWrite(GreenLED, GreenLEDState ) ;
    // Adjust Timer to fire again "QuickInterval" later
    QuickTimer += QuickInterval ;
  }

// The other LED is controlled the same way. Repeat for more LEDs
  if ( millis() - SlowTimer > SlowInterval ) {
    if (RedLEDState==LOW)
      RedLEDState = HIGH;
    else
      RedLEDState = LOW;
    digitalWrite(RedLED, RedLEDState ) ;
    SlowTimer += SlowInterval ;
  }

/* Other code that needs to execute goes here.
   It will be called many thousand times per second because the above code
   does not wait for the LED blink interval to finish. */

}

And so on .... I have spent a few hours on this now, and it is a fascinating challenge, but there is family, a hobby, work and sleep... so I just leave it here for the while being. I'll be back.

(I was planning to write more "dual timer" examples (Playground?), the LED being the simplest, the other would combine the Servo sweep with a LED PWM, and possibly button debounce and LED blink)

PaulS

Quote
I was planning to write more "dual timer" examples (Playground?), the LED being the simplest, the other would combine the Servo sweep with a LED PWM

Hey, there's a poster over in Programming now that wants to do this. Can you hurry it up?  :)

Coding Badly

#14
Oct 10, 2011, 09:47 pm Last Edit: Oct 10, 2011, 09:49 pm by Coding Badly Reason: 1
Quote
QuickTimer = millis() ;
to
QuickTimer += QuickInterval ;
It seems "cleaner" and seems closer to the intent.


There is a different intent for each.  For the first, the intent is to never run more frequently than the interval.  I want this code to run every 555 ms but I don't want there to ever be less than 555 ms between runs.  This version seems the most appropriate for blinking an LED.

For the second, the intent is to execute so many times in a given amount of time.  I want this code to run once per 555 ms no matter what else happens in the application.  This version is required for some control applications (pulse triggered pump where the goal is a certain flow rate).


What is the typical intent?  What do most new users want to accomplish?


Does it matter?  For the majority of applications, will either version work?  Does one version seem more appropriate for a beginner (we need a few beginners to answer)?


Regarding the addition-version: To be robust, the if needs to be changed to a while.  If even a single overrun occurs, the if version may never catch up (fire continuously).

 while ( millis() - QuickTimer >= QuickInterval ) {
   // stuff goes here
   QuickTimer += QuickInterval ;
 }


Go Up