Pages: 1 [2] 3 4 ... 6   Go Down
Author Topic: Sticky - Blink Two LEDs, independent, no delay  (Read 5270 times)
0 Members and 1 Guest are viewing this topic.
Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

...
Code:
// 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:
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.
Logged

Copenhagen, Denmark
Offline Offline
Edison Member
*
Karma: 25
Posts: 1143
Have you testrun your INO file today?
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

What can I say...<bows head>  smiley-fat
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
 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:
 if ( (millis() - QuickTimer) > QuickInterval ) {

Quote
Code:
// 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:
// Variable holding the timer value so far. One for each "Timer"
unsigned long QuickTimer = 0,
              SlowTimer = 0 ;

Or perhaps more generally:

Code:
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:
/* "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. */

}
« Last Edit: October 10, 2011, 03:57:40 pm by Nick Gammon » Logged

Seattle, WA USA
Online Online
Brattain Member
*****
Karma: 549
Posts: 46125
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Left Coast, CA (USA)
Offline Offline
Brattain Member
*****
Karma: 331
Posts: 16523
Measurement changes behavior
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Lefty
Logged

Seattle, WA USA
Online Online
Brattain Member
*****
Karma: 549
Posts: 46125
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Copenhagen, Denmark
Offline Offline
Edison Member
*
Karma: 25
Posts: 1143
Have you testrun your INO file today?
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
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.
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'll go with "QuickTimer = millis()" because there is no lingering doubt about the wrap-around, and because it is arguably simpler to understand.
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12283
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
unsigned long LED1timer = 0,
              LED2timer = 0 ;

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

Code:
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:
// 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!
« Last Edit: October 14, 2011, 02:42:19 am by Coding Badly » Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
...
    // 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:
    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.
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12283
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
The glue?

 smiley-lol  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:
    // 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.
Logged

Copenhagen, Denmark
Offline Offline
Edison Member
*
Karma: 25
Posts: 1143
Have you testrun your INO file today?
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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 smiley

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?
Logged

Pages: 1 [2] 3 4 ... 6   Go Up
Jump to: