Which of the following functions is more efficient?

gvi70000 · July 10, 2011, 3:29pm

ISR(TIMER2_OVF_vect) 
{
  if((millis() >= Bk_Light) && bitRead(PIND, 5)) PORTD  &= ~(1<<5);//Turn BackLight Off
  if(MLU_Time && (millis() >= MLU_Time)) MirrorLockUp();//Press Shutter to enable Mirror LockUp if 30s have passed since last Mirror LockUp
}

or

ISR(TIMER2_OVF_vect) 
{
  if(bitRead(PIND, 5))
  {
    if((millis() >= Bk_Light)) PORTD  &= ~(1<<5);//Turn BackLight Off
  }
  if(MLU_Time)
  {
    if(millis() >= MLU_Time) MirrorLockUp();//Press Shutter to enable Mirror LockUp if 30s have passed since last Mirror LockUp
  }
}

or

ISR(TIMER2_OVF_vect) 
{
  if(bitRead(PIND, 5))
  {
    if((millis() >= Bk_Light)) PORTD  &= ~(1<<5);//Turn BackLight Off
  }
  if(MLU_Time) MirrorLockUp();//Press Shutter to enable Mirror LockUp if 30s have passed since last Mirror LockUp
}

void MirrorLockUp(byte AutoActivate = 0)
{
  //for the third case the line below becomes if(ROM_VAL[ROM_MLU].Value && (millis() >= MLU_Time))
  if(ROM_VAL[ROM_MLU].Value)
  {
    PORTB |=  (1<<1);//Shutter 1 On
    delay(50);//Wait ROM_VAL[ROM_BtnDelay].Value
    PORTB &= ~(1<<1);//Shutter 1 Off
    if(AutoActivate)
      MLU_Time = millis() + ROM_VAL[ROM_MLU_WakeUp].Value*1000;//Press Shutter to enable Mirror LockUp if ns have passed since last Mirror LockUp
    else
      MLU_Time = 0;
    delay(ROM_VAL[ROM_MLU_Delay].Value);//Wait for vibrations to end
  }
  else
    MLU_Time = 0;
}

system · July 10, 2011, 3:50pm

Define "efficient".

"MirrorLockup" has "delay" in interrupt context.

gvi70000 · July 10, 2011, 3:59pm

I'm interested do spend less time in the interrupt

system · July 10, 2011, 4:18pm

I'm interested do spend less time in the interrupt

delay(50);//Wait ROM_VAL[ROM_BtnDelay].Value

gvi70000 · July 10, 2011, 4:42pm

the delay is needed for a digital camera to receive a command.
will that cause trouble?

system · July 10, 2011, 6:18pm

gvi70000:
the delay is needed for a digital camera to receive a command.
will that cause trouble?

Does it work? I'm not even sure you CAN effectively call delay() in an ISR, because it relies on the millis interrupt to increment, and that is disabled while you're in the ISR.

In any case, there is negligible efficiency differences between your options, except you discard "millis() >= MLU_Time" in option C.

I'm pretty sure the following forms compile down to the exact same code:

if (A)
  if (B)
    do_something();

if (A && B)
    do_something();

So if you really honestly are looking to save CPU cycles, you typically want to put the thing that is false MOST of the time first, so the conditional cuts out without even having to test the second. Unless the 'A' test is expensive and the 'B' test cheap, in which case do the 'B' test first so you can save the 'A' test cost some of the time.

That said, you have delay's in your logic, so why are you worried about CPU efficiency? Optimize for readability and maintainability so when you come back to the code in 6 months you can easily remember what exactly it does and why.

robtillaart · July 10, 2011, 6:27pm

I'm pretty sure the following forms compile down to the exact same code:
Code:

if (A)
if (B)
do_something();

Code:

if (A && B)
do_something();

depends on the optimizing of boolean expressions; if the compiler supports and uses short circuit boolean evaluations you are right;

but

if (A)
  if (B)
    do_something();

can be faster than

if (B)
  if (A)
    do_something();

if expression B takes longer to evaluate ... e.g A => (x == 0) and B => (digitalRead(10) == HIGH)

If you want to win micros switching testorder might help...

Coding_Badly · July 10, 2011, 6:33pm

if the compiler supports and uses short circuit boolean evaluations you are right

It does and short-circuit evaluation is enabled.

robtillaart · July 10, 2011, 7:04pm

OK, that means you can win some time by changing
if (A && B) ==> if (B && A)

and for the OR of course too
if (A || B) ==> if (B || A)

and if A or B includes function calls can one get sideeffects???
e.g. changing the order in if (i++ == 5 && x == 0) ==> if ( x==0 && i++ == 5)
or
if (f(x) == 5 && x = 0) versus if (x == 0 && f(x)==5)
????

QED test

int x;
void setup()
{
  Serial.begin(115200);
  x = 1;
  if (x == 0 && f(x)==0) x = 3;
  Serial.println(x, DEC);
  for(;;);
}
loop(){}
int f(int z)
{
  x = 4;
  return 0;
}

output: 1 ==> So f(x) get not allways evaluated ...

system · July 10, 2011, 8:50pm

Absolutely. If you want to CAUSE something to happen (e.g. x=4 above), you don't want to hide it in a conditional.

Just for fun, let's take a look see what the compiler did with setup() above:

.global	setup
	.type	setup, @function
setup:
	.stabd	46,0,0
	.stabn	68,0,7,.LM5-.LFBB3
.LM5:
.LFBB3:
/* prologue: function */
/* frame size = 0 */
	.stabn	68,0,9,.LM6-.LFBB3
.LM6:
	ldi r24,lo8(Serial)
	ldi r25,hi8(Serial)
	ldi r20,lo8(115200)
	ldi r21,hi8(115200)
	ldi r22,hlo8(115200)
	ldi r23,hhi8(115200)
	call _ZN14HardwareSerial5beginEl
	.stabn	68,0,10,.LM7-.LFBB3
.LM7:
	ldi r24,lo8(1)
	ldi r25,hi8(1)
	sts (x)+1,r25
	sts x,r24
	.stabn	68,0,12,.LM8-.LFBB3
.LM8:
	ldi r24,lo8(Serial)
	ldi r25,hi8(Serial)
	ldi r22,lo8(1)
	ldi r23,hi8(1)
	ldi r20,lo8(10)
	ldi r21,hi8(10)
	call _ZN5Print7printlnEii
.L6:
	rjmp .L6

Yes indeed, the compiler never even thinks about calling f(), in fact it optimized f() out of the entire program, and simply calls print() with an immediate '1'. Doesn't even bother fetching the value of x.

robtillaart · July 10, 2011, 10:02pm

thanks for the look under the hood ... a lesson to keep in mind when debugging code that could be optimized.

For the assembly noobs: How did you make the assembly listing ?

system · July 11, 2011, 4:15am

robtillaart:
How did you make the assembly listing ?

This tells the compiler to compile down to assembly, but not actually assemble it into an object:

avr-g++ -S sourcefile.cpp -o sourcefile.s

The easy way to do it with the Arduino IDE is to hold down SHIFT while clicking the VERIFY button, and copy the avr-g++ line. So you get this:

avr-g++ -c -g -Os -w -fno-exceptions -ffunction-sections -fdata-sections -mmcu=atmega328p -DF_CPU=16000000L -DARDUINO=22 -I/opt/arduino-0022/hardware/arduino/cores/arduino -I/home/maniac/Source/Arduino/libraries/RF24Network -I/home/maniac/Source/Arduino/libraries/RF24 -I/opt/arduino-0022/libraries/SPI /tmp/build4095425328949125099.tmp/helloworld_tx.cpp -o/tmp/build4095425328949125099.tmp/helloworld_tx.cpp.o

Then you can change the first -c to a -S and the last .o to a .s and then look at the .s file.

It's usually easiest to make a tiny example that illustrates specifically what you're after so you don't have to dig through pages of assembly to find something, kind of like you did above.

gvi70000 · July 11, 2011, 5:05am

Hello,
The MirrorLockUp function works well, till now i didn't think that there is a problem there.
I thought that using delay and serial.print inside the ISR is problematic, but reading your reply's
got me in to a dead end. I realy need that delay there, how can i accomplish that in the "right" way?

Regarding the if's, MLU_Time will be true if the user changes a parameter in the program and in theory will be false most of the time. The Bk_Light i will test it against millis because
also in this case there is an option that will keep pin 5 high all the time.

robtillaart · July 11, 2011, 6:57am

I realy need that delay there, how can i accomplish that in the "right" way?

Option: You could set a timestamped flag in the ISR which triggers the "real work" inside loop().

snippet

void loop()
{
...
if (timestamp >0) 
{
  delay(max(50 - (millis()-timestamp),0)); // delay rest of the waiting time between 0--50 ms
  do your thing
  timestamp = 0;
}

gvi70000 · July 11, 2011, 7:46am

My case is a little bit complicated.
Most of the cpu time is spend on loops in which some analog reads are performed.
Using your suggestion i will have to inset in the loops one more condition (i can miss some triggers from analogread) and by using
simple a simple milliseconds counter i don't think i will need the interrupt.

I might be wrong about this, what do you think?

system · July 11, 2011, 8:19am

Most of the cpu time is spend on loops in which some analog reads are performed.

And most (around 100us) of every analogRead is dead time, waiting for end-of-conversion.

gvi70000 · July 11, 2011, 8:41am

i think i solve this by using fast analog read (17us per read)

system · July 11, 2011, 8:55am

I'm betting a good part of that 17us is dead time.

gvi70000 · July 11, 2011, 11:40am

Is there a fix for this dead time?

for a general view of the project you can look here
http://grozeaion.com/electronics/high-speed-photography/134-wired-remote-control-for-digital-camera.html

i will update the post with the final version of the code as soon as as solve the interrupt problem

system · July 11, 2011, 11:56am

Is there a fix for this dead time?

Use the end-of-conversion interrupt.

Topic		Replies	Views
Time spent in execution of code for shutter tester Programming	39	316	February 7, 2026
Interrupt, usare sia FALLING sia CHANGE + tempi micros Software	32	234	August 31, 2025
Info generica interrupt (mega2560) Software	20	149	August 30, 2025
Yet another millis() (and interrupt) question ! Programming	64	595	October 6, 2025
Interrupt service routine calls General Guidance	100	910	November 6, 2025

Which of the following functions is more efficient?

Related topics