Go Down

Topic: For loop best practice question (Read 2373 times)previous topic - next topic

command_z

Dec 18, 2012, 08:31 pm
Is this (top example) the best way to write more efficient code for the longer code (bottom example)?

Code: [Select]
`  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: [Select]
`  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);   `

udoklein

#1
Dec 18, 2012, 08:53 pmLast Edit: Dec 18, 2012, 09:16 pm by Udo Klein Reason: 1
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: [Select]
`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.

command_z

#2
Dec 18, 2012, 09:13 pm
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

Docedison

#3
Dec 18, 2012, 09:40 pm
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
--> 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

guix

#4
Dec 18, 2012, 10:18 pm
You can change this:
Code: [Select]
`if (i % 2 == 0){  toggle = HIGH;} else {  toggle = LOW; }    digitalWrite(led, toggle);delay(del);`

to this:
Code: [Select]
`toggle = !toggle;digitalWrite(led, toggle);delay(del);`

lloyddean

#5
Dec 19, 2012, 12:55 amLast Edit: Dec 19, 2012, 04:02 am by lloyddean Reason: 1
Faster, more correct, easier to read and modify.

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

Code: [Select]
`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);}`

nickgammon

#6
Dec 19, 2012, 02:01 am
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.
Please post technical questions on the forum, not by personal message. Thanks!

dhenry

#7
Dec 19, 2012, 02:24 am
Quote
I want something that is more efficient

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

lloyddean

#8
Dec 19, 2012, 02:25 am
Nick Gammon's comment was aimed at my example just in case that wasn't clear to you!

lloyddean

#9
Dec 19, 2012, 02:34 amLast Edit: Dec 19, 2012, 02:50 am by lloyddean Reason: 1

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.

nickgammon

#10
Dec 19, 2012, 03:08 am

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

I was actually thinking of:

Code: [Select]
`   digitalWrite(led, !(i&1));  `

With all due respect to Udo Klein.

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

Code: [Select]
`   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.
Please post technical questions on the forum, not by personal message. Thanks!