Pages: [1]   Go Down
Author Topic: For loop best practice question  (Read 596 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Jr. Member
**
Karma: 0
Posts: 70
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Is this (top example) the best way to write more efficient code for the longer code (bottom example)?
 


 
Code:
  int led =13;
  int del= 1000;
  boolean toggle;
 
  for (int i=0; i<8; i++){
   
    if (i % 2 == 0){
      toggle = HIGH;
    } else {
      toggle = LOW;
    }
   
   digitalWrite(led, toggle); 
   delay(del);



 
 
Code:
  int led =13;

  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);   
Logged

0
Offline Offline
Faraday Member
**
Karma: 19
Posts: 3420
20 LEDs are enough
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

You need to define "efficient". Do you mean simpler to code or faster to execute? Actually I would opt for a for loop like so:

Code:
const uint8_t led = 13;
const uint16_t del= 1000;
  
for (uint8_t i=0; i<8; i++) {
   digitalWrite(led, !(i&1));  
   delay(del);
}

Notice that I use constants to make it easier for the compiler to generate efficient code. Also I dropped the if and the very expensive %.

If you want to see how to performance push code to the limits have a look into my blog at the counter experiments:

http://blog.blinkenlight.net/experiments/counting/fast-counter/
http://blog.blinkenlight.net/experiments/counting/faster-counter/

But as long as you use "delay" in your code there is absolutely no point in pushing it so far to the limits.
« Last Edit: December 18, 2012, 03:16:14 pm by Udo Klein » Logged

Check out my experiments http://blog.blinkenlight.net

Offline Offline
Jr. Member
**
Karma: 0
Posts: 70
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Oh wow. Thanks for the info.

I want something that is more efficient, but also simpler to code than the example you provided.

Do you think my code would work well for smaller sketches?


Thank you
Logged

Anaheim CA.
Offline Offline
Faraday Member
**
Karma: 44
Posts: 2810
...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Define Smaller...
Quote
Code:
  int led =13;

  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);
  digitalWrite(led, HIGH);   
  delay(1000);               
  digitalWrite(led, LOW);   
  delay(1000);   
This way? or the loop you were offered.
No, your first attempt makes NO effort to be smaller.

Bob
Logged

--> WA7EMS <--
“The solution of every problem is another problem.” -Johann Wolfgang von Goethe
I do answer technical questions PM'd to me with whatever is in my clipboard

France
Offline Offline
God Member
*****
Karma: 29
Posts: 898
Scientia potentia est.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

You can change this:
Code:
if (i % 2 == 0){
  toggle = HIGH;
} else {
  toggle = LOW;
}
   
digitalWrite(led, toggle);
delay(del);

to this:
Code:
toggle = !toggle;
digitalWrite(led, toggle);
delay(del);
Logged

Des Moines, WA - USA
Offline Offline
God Member
*****
Karma: 25
Posts: 779
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Faster, more correct, easier to read and modify.

Oh, and some of that is personal opinion and coding style.

Code:
typedef unsigned long   time_t;

const uint8_t   pinLED          = 13;
const time_t    dtONE_SECOND    = 1000UL;


const time_t    dtDELAY         = dtONE_SECOND;
  
for ( int i = 8; i--;  )
{
    digitalWrite(pinLED, !digitalRead(pinLED));  
    delay(dtDELAY);
}
« Last Edit: December 18, 2012, 10:02:05 pm by lloyddean » Logged

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

Whatever you go with (and I wouldn't use the longer code in the original post) make sure it is clear what you are doing. When you pick up the code a year later, you shouldn't have to stare at it for 10 minutes to work out what it is doing. Comments will probably help here.
Logged

Offline Offline
Edison Member
*
Karma: 116
Posts: 2205
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
I want something that is more efficient

Unrolling a loop is generally faster (aka more time efficient) but bulky (aka less space efficient).
Logged

Des Moines, WA - USA
Offline Offline
God Member
*****
Karma: 25
Posts: 779
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Nick Gammon's comment was aimed at my example just in case that wasn't clear to you!
Logged

Des Moines, WA - USA
Offline Offline
God Member
*****
Karma: 25
Posts: 779
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
I want something that is more efficient

Unrolling a loop is generally faster (aka more time efficient) but bulky (aka less space efficient).

Realizing this is posted on an Arduino forum I would agree with that, but ..., on another (non Atmel) architecture that wouldn't necessarily be true and in fact may be wrong!

EDIT:  Either way the loop will generally be easier to read and follow.  I would likely let the loop stand and see if the compiler can be asked to massage the code as appropriate for target architecture.
« Last Edit: December 18, 2012, 08:50:35 pm by lloyddean » Logged

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

Nick Gammon's comment was aimed at my example just in case that wasn't clear to you!

I was actually thinking of:

Code:
   digitalWrite(led, !(i&1)); 

With all due respect to Udo Klein. smiley



In fact since digitalWrite is supposed to be sent HIGH or LOW, it really should be:

Code:
   digitalWrite (led, (i & 1) ? LOW : HIGH);  // toggle LED each iteration through the loop



As for unrolling the loop, these processors are rather short of program memory, so whether or not that was warranted would depend on whether you wanted to save space or time.

There was a thread not that long ago where someone "unrolled the loop" to speed up sending data out the SPI port as fast as possible. I showed that there was no point, as there was a delay anyway (17 clock cycles) before you could send more data, so the time saved in unrolling the loop did not actually give any benefit (the loop could be executed in under 17 clock cycles).

And as others have pointed out, if you are building in a delay of 1000 mS anyway, trying to save time is rather pointless.
Logged

Pages: [1]   Go Up
Jump to: