Go Down

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

Nick Gammon


...
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.


Thanks for the comments. Before I address the style considerations I want to comment on the above.

From wiring.h:

Code: [Select]

typedef uint8_t byte;

...

void pinMode(uint8_t, uint8_t);
void digitalWrite(uint8_t, uint8_t);
int digitalRead(uint8_t);


The documentation for pinMode doesn't specifically say it takes an int, it (rather unhelpfully) implies it with example code.

It actually takes a byte.
http://www.gammon.com.au/electronics

Msquare


Nick Gammon


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


Whilst I appreciate the intention here, I think it is confusing for beginner code (like the toggling of the LED perhaps). Treating "while" very similarly to "if" is likely to get beginners thinking that "while" means "if".

Still, you are right about it.

Perhaps stick to the creeping version, or just add a note that for extremely short intervals, and a loop that does lots of other things, you may have this problem. For the posted code, of course you won't.
http://www.gammon.com.au/electronics

Nick Gammon

#18
Oct 10, 2011, 10:56 pm Last Edit: Oct 10, 2011, 10:57 pm by Nick Gammon Reason: 1

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.


It *is* a nice challenge isn't it? It reminds me of the time developers spend getting 'starting zones' working right for MMO games. Get the basics right and new people will stay with you.

Code: [Select]
GreenLED  ... QuickInterval ... QuickTimer ... GreenLEDState
RedLED ... SlowInterval ... RedLEDState ... SlowTimer


As a final suggestion, I think the code as it currently stands is hard to amend for, say, a third timer. Also there is a bit of an implication that you need to use green and red LEDs, and that one interval has to be faster than the other.

I suggest renaming (as below) LED1 and LED2. That way, you can add a third LED by copy/pasting, and just searching for LED1 and making it LED3, as appropriate. Having a consistent string to search for will avoid minor errors like changing Green to Blue but not Slow to Slower or whatever.

Quote
Code: [Select]
 if ( millis() - QuickTimer > QuickInterval ) {


I think I prefer the brackets I had. Leaving them out, whilst correct, just makes you pause and wonder about operator precedence. At least I do, when people complain that their timing examples "don't work".

So:

Code: [Select]
 if ( (millis() - QuickTimer) > QuickInterval ) {

Quote
Code: [Select]
// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer , SlowTimer ;


I think I prefer to train people to initialize variables (and yes, I know it isn't necessary right here).

Code: [Select]
// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer = 0,
             SlowTimer = 0 ;


Or perhaps more generally:

Code: [Select]
void setup() {
 pinMode(LED1,OUTPUT) ;
 pinMode(LED2,OUTPUT) ;
 QuickTimer = SlowTimer = millis ();
}


Then this code could be put elsewhere. In other words, assuming your timers start at zero basically assumes your blinking sketch does nothing else (eg. open files, web pages).

And for example, if the timers were Unix time (which they aren't) then starting them at zero would be much different from starting them at "the current time".

Amended version:

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 byte LED1 = 8 ;
const byte LED2 = 10 ;

// Time periods of blinks in milliseconds (1000 to a second).
// Time variable and constants should be unsigned long, always
const unsigned long LED1interval = 555UL ;
const unsigned long LED2interval = 1234UL ;

// Variable holding the timer value so far. One for each "Timer"
unsigned long LED1timer = 0,
             LED2timer = 0 ;

// Variable to know what to change the LED to.
int LED1State = LOW ;
int LED2State = LOW ;

void setup() {
 pinMode(LED1,OUTPUT) ;
 pinMode(LED2,OUTPUT) ;
 LED1timer = LED2timer = millis();
}

void loop() {

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

// The other LED is controlled the same way. Repeat for more LEDs
 if ( (millis() - LED2timer) > LED2interval ) {
   if (LED2State==LOW)
     LED2State = HIGH;
   else
     LED2State = LOW;
   digitalWrite(LED2, LED2State ) ;
   LED2timer += LED2interval ;
 }

/* 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. */

}
http://www.gammon.com.au/electronics

PaulS

Quote
Does one version seem more appropriate for a beginner (we need a few beginners to answer)?

I'd suggest that some "experts" opinion be considered, too. To me, the QuickTimer = millis(); version is easier to understand and adapt. I have to think a while about why the QuickTimer += QuickInterval is necessary in some circumstances.

For a beginner, simpler is better.

Nick Gammon

I'm inclined to agree with Paul here. I still can't quite convince myself that the addition will always work. I did a few examples and it did, but we keep banging on about how "only subtraction works".

And if we use the code:

Code: [Select]
LED1timer = mills () ;

... then the problem of turning the "if" into a "while" goes away, because it can't ever get the problem of "not catching up". You get the creep problem, sure, but this exercise is supposed to be teaching multiple timers, really.
http://www.gammon.com.au/electronics

retrolefty

All this construction discussion on the topic makes me wonder how useful our past advice to beginners, 'just check out the blink-without-delay example' was. As if the applying millis() to multiple timers was too trivial to expound on.  :D

Lefty

PaulS

Quote
makes me wonder how useful our past advice to beginners, 'just check out the blink-without-delay example' was.

I guess it depends on whether you want to give a person a fish, or teach them to fish. Personally, I learn more by experimenting, and learning how to do something myself than by being given code.

Nick Gammon


I guess it depends on whether you want to give a person a fish, or teach them to fish.


But as you said elsewhere:


... until the fish are all gone.


Just teasing, Paul.

I agree the discussion shows that the "loop without delay" concept is perhaps less trivial than it seems, and this example code would be helpful to people who are not familiar with asynchronous processing of events.
http://www.gammon.com.au/electronics

Msquare


Code: [Select]
GreenLED  ... QuickInterval ... QuickTimer ... GreenLEDState
RedLED ... SlowInterval ... RedLEDState ... SlowTimer


As a final suggestion, I think the code as it currently stands is hard to amend for, say, a third timer. Also there is a bit of an implication that you need to use green and red LEDs, and that one interval has to be faster than the other.

I suggest renaming (as below) LED1 and LED2. That way, you can add a third LED by copy/pasting, and just searching for LED1 and making it LED3, as appropriate. Having a consistent string to search for will avoid minor errors like changing Green to Blue but not Slow to Slower or whatever.


I have to disagree. I think we can assume a minimum of intelligence in our audience and extrapolating from Red or Green to - ... ehr... wait... hmmm... what else could it be ... ah YES! - Yellow is reasonable. I see to many programs with motor1, motor2 and then in the question they talk about the X axis motor, where I ask why not call the variable Xmotor then.

Mindless cut-n-paste-plus-edit is something I do not want to encourage. The minor error should be minor enough for them to find.




Shall we have a show-of-hands to decide on QuickTimer += QuickInterval or QuickTimer = millis() ? I am undecided.

Nick Gammon

I'll go with "QuickTimer = millis()" because there is no lingering doubt about the wrap-around, and because it is arguably simpler to understand.
http://www.gammon.com.au/electronics

Coding Badly

#26
Oct 14, 2011, 09:02 am Last Edit: Oct 14, 2011, 09:42 am by Coding Badly Reason: 1
Quote
Shall we have a show-of-hands to decide on QuickTimer += QuickInterval or QuickTimer = millis() ? I am undecided.


As the one with "sticky power" I'm making the decision ... QuickTimer = millis();


Numbers (LED1, LED2 [Nick's style]) or names (GreenLED, RedLED [Msquare's style])?  We're going with names.  Far too many users post code with cryptic variable names.  I really do not want to encourage that behaviour.


Code: [Select]
unsigned long LED1timer = 0,
             LED2timer = 0 ;


Adds another concept: multiple declarations.  I'd prefer the explicit version (below).  Comments?

Code: [Select]
unsigned long LED1timer = 0;
unsigned long LED2timer = 0;



I like the comments.  Does a "this code survives millis wrap" comment need to be added?


I prefer more whitespace.  Comments?

Code: [Select]
// Handling the blink of one LED.
// First, check if it is time to change state

 if ( (millis() - LED1timer) >= LED1interval )
 {
   // It is time to change state. Calculate next state.
   if ( LED1State == LOW )
   {
     LED1State = HIGH;
   }
   else
   {
     LED1State = LOW;
   }
   // Write new state
   digitalWrite( LED1, LED1State ) ;

   // Adjust Timer to fire again "LED1interval" later
   LED1timer += LED1interval ;
 }



Did I miss anything?


Edit:  Argh!  Nick, you have me leaving out the equals!

Nick Gammon


As the one with "sticky power" I'm making the decision ...


The glue?

Quote
We're going with names.  Far too many users post code with cryptic variable names.  I really do not want to encourage that behaviour.


As you wish. Numbers are perhaps more cryptic, but easier to expand to 3 or 4 LEDs.

Quote
Adds another concept: multiple declarations.  I'd prefer the explicit version (below).  Comments?

Code: [Select]
unsigned long LED1timer = 0;
unsigned long LED2timer = 0;




I'm OK with that.

Quote
I like the comments.  Does a "this code survives millis wrap" comment need to be added?


Yes. It removes lingering doubt.

Quote
I prefer more whitespace.  Comments?

Code: [Select]

...
    // It is time to change state. Calculate next state.
    if ( LED1State == LOW )
    {
      LED1State = HIGH;
    }
    else
    {
      LED1State = LOW;
    }
    // Write new state
    digitalWrite( LED1, LED1State ) ;

...


Ach. You added brackets. My earlier line:

Code: [Select]
    digitalWrite (LED1, !digitalRead (LED1));  // toggle LED


... has now become 11 lines. Is that really an improvement? I suggest minimizing it. We don't want to get to the stage where toggling two LEDs takes more space than the concept we are trying to demonstrate.
http://www.gammon.com.au/electronics

Coding Badly

Quote
The glue?


XD  According to Grumpy Mike, that would be "solder".

Quote
Does a "this code survives millis wrap" comment need to be added?  Yes. It removes lingering doubt.


"This code works correctly even though millis wraps around to zero after approximately 49 days"?

Quote
... has now become 11 lines. Is that really an improvement?


Hmm.  I wasn't paying attention.  When did we get to an if instead of logical-not?  Ah, there it is.  The original post.   :smiley-red:

This is roughly what is in the current blink-without-delay...

Code: [Select]
    // It is time to change state. Calculate next state.
    if ( LED1State == LOW )
      LED1State = HIGH;
    else
      LED1State = LOW;
    // Write new state
    digitalWrite( LED1, LED1State ) ;


I cannot recall a single user complaint about it.  So, we'll go with that.

Msquare


As the one with "sticky power" I'm making the decision ...

Pragmatic.

Quote
...(GreenLED, RedLED [Msquare's style])?  We're going with names

Not going to disagree with that :)

Quote
I prefer more whitespace.  Comments?

Too much of a good thing. I would say the essential code (basically the loop() up to the long final comment) should easily fit on one page (standard forum browser)

Quote
Edit:  Argh!  Nick, you have me leaving out the equals!

Confused. Was not the idea to have QuickTimer = millis() ?

Dont forget that the timer test should be a ">=" not just a ">"

Lets put this baby on the forum (it can be tweaked). There are still 16 more to go!

Who gets the honors?

Go Up