For loop best practice question

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

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

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:

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:

But as long as you use "delay" in your code there is absolutely no point in pushing it so far to the limits.

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

Define Smaller...

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

You can change this:

if (i % 2 == 0){
  toggle = HIGH;
} else {
  toggle = LOW; 
}
    
digitalWrite(led, toggle);
delay(del);

to this:

toggle = !toggle;
digitalWrite(led, toggle);
delay(del);

Faster, more correct, easier to read and modify.

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

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

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.

I want something that is more efficient

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

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

dhenry:

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.

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

I was actually thinking of:

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

With all due respect to Udo Klein. :slight_smile:


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

   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.