delayMicroseconds() is broken in AVR core versions that do LTO...

[WestfW]

Recent versions of the Arduino IDE/Core for AVR do Link Time Optimization, which means that calls to delayMicroseconds() where the argument is constant are likely to be optimized to the point where they become significantly inaccurate for small numbers...

The simplest solution is probably to define delayMicroseconds() with a "never inline" attribute, but it would probably be a better idea to use libc and/or compiler built-ins instead.

test sketch:

void setup() {
}

void loop() {
  byte x;
  x = PORTB;
  delayMicroseconds(1);
  x = PORTB;
  delayMicroseconds(2);
  x = PORTB;
  delayMicroseconds(3);
  x = PORTB;
  delayMicroseconds(100);
  x = PORTB;
  delayMicroseconds(2334);
  x = PORTB;
  delayMicroseconds(x);
  x = PORTB;
}

[WestfW]

Analysis:

;;; Somewhere in main()
	
 21c:	c3 e0       	ldi	r28, 0x03	;;; r28:r29 gets constant for 2us
 21e:	d0 e0       	ldi	r29, 0x00
 220:	07 e0       	ldi	r16, 0x07	;;; R16:R17 gets constant for 3us  
 222:	10 e0       	ldi	r17, 0x00	;
 224:	3b e8       	ldi	r19, 0x8B	;;; R14:R15 gets constant for 100us
 226:	e3 2e       	mov	r14, r19
 228:	ff 24       	eor	r15, r15
 22a:	f3 94       	inc	r15
 22c:	43 e7       	ldi	r20, 0x73 ;;; R12:R13 gets constant for 2334us
 22e:	c4 2e       	mov	r12, r20
 230:	44 e2       	ldi	r20, 0x24
 232:	d4 2e       	mov	r13, r20
	
;;; :

void loop() {
  byte x;
  x = PORTB;
 23c:	85 b1       	in	r24, 0x05	; 5
  delayMicroseconds(1);
;;; That was NOT a microsecond!
  x = PORTB;
 23e:	85 b1       	in	r24, 0x05	; 5
  delayMicroseconds(2);
;;; Note that all of the carefully cycle-counted math instructions from
;;; delayMicroseconds() are missing, instead we're using pre-calculated
;;; constants that are even set up outside of loop()
 240:	ce 01       	movw	r24, r28
 242:	01 97       	sbiw	r24, 0x01	; 1
 244:	f1 f7       	brne	.-4      	; 0x242 <main+0xe2>
  x = PORTB;
 246:	85 b1       	in	r24, 0x05	; 5
  delayMicroseconds(3);
 248:	c8 01       	movw	r24, r16
 24a:	01 97       	sbiw	r24, 0x01	; 1
 24c:	f1 f7       	brne	.-4      	; 0x24a <main+0xea>
  x = PORTB;
 24e:	85 b1       	in	r24, 0x05	; 5
  delayMicroseconds(100);
 250:	c7 01       	movw	r24, r14
 252:	01 97       	sbiw	r24, 0x01	; 1
 254:	f1 f7       	brne	.-4      	; 0x252 <main+0xf2>
  x = PORTB;
 256:	85 b1       	in	r24, 0x05	; 5
  delayMicroseconds(2334);
 258:	c6 01       	movw	r24, r12
 25a:	01 97       	sbiw	r24, 0x01	; 1
 25c:	f1 f7       	brne	.-4      	; 0x25a <main+0xfa>
  x = PORTB;
 25e:	85 b1       	in	r24, 0x05	; 5
;;; Using a variable argument, we finally see the instructions being included,
;;;  but note that the call/ret cycles (about 0.5us!) are still missing.
  delayMicroseconds(x);
 260:	90 e0       	ldi	r25, 0x00	; 0
#elif F_CPU >= 16000000L
	// for the 16 MHz clock on most Arduino boards

	// for a one-microsecond delay, simply return.  the overhead
	// of the function call takes 14 (16) cycles, which is 1us
	if (us <= 1) return; //  = 3 cycles, (4 when true)
 262:	82 30       	cpi	r24, 0x02	; 2
 264:	91 05       	cpc	r25, r1
 266:	38 f0       	brcs	.+14     	; 0x276 <main+0x116>

	// the following loop takes 1/4 of a microsecond (4 cycles)
	// per iteration, so execute it four times for each microsecond of
	// delay requested.
	us <<= 2; // x4 us, = 4 cycles
 268:	88 0f       	add	r24, r24
 26a:	99 1f       	adc	r25, r25
 26c:	88 0f       	add	r24, r24
 26e:	99 1f       	adc	r25, r25

	// account for the time taken in the preceeding commands.
	// we just burned 19 (21) cycles above, remove 5, (5*4=20)
	// us is at least 8 so we can substract 5
	us -= 5; // = 2 cycles,
 270:	05 97       	sbiw	r24, 0x05	; 5
	

#endif

	// busy wait
	__asm__ __volatile__ (
 272:	01 97       	sbiw	r24, 0x01	; 1
 274:	f1 f7       	brne	.-4      	; 0x272 <main+0x112>
  x = PORTB;
 276:	85 b1       	in	r24, 0x05	; 5
 278:	a1 14       	cp	r10, r1
 27a:	b1 04       	cpc	r11, r1
 27c:	f9 f2       	breq	.-66     	; 0x23c <main+0xdc>
 27e:	0e 94 00 00 	call	0	; 0x0 <__vectors>
 282:	dc cf       	rjmp	.-72     	; 0x23c <main+0xdc>

[PaulStoffregen]

As a quick sanity check, I ran this code just now on an Arduino Uno. I used Arduino 1.8.5 on Linux 64 bit for testing. The boards manager says I have version 1.6.20 of the Arduino AVR Boards.

void setup() {
  DDRB = 255;
}

void loop() {
  volatile byte x=10;

  PORTB = 255;
  delayMicroseconds(2);
  PORTB = 0;
  delayMicroseconds(3);
  PORTB = 255;
  delayMicroseconds(x);
  PORTB = 0;
  delayMicroseconds(20);
  PORTB= 255;
  delayMicroseconds(10);
  PORTB = 0;
  delay(6);  // set scope for 5ms trigger holdoff
}

Here is the waveform I see on pin 8.

The two 10us and 20us regions look ok, maybe slightly shorter than that should be, but pretty close to 1 and 2 divisions on the scope, and of course the was taken with the timebase at 10 us/div.

Here's a zoom in to the first part:

This one is done at 1 us/div, and again the first pulse is pretty close to 1 microsecond, and the second is pretty close to 2 us.

Seems things are working, at least in this quick test on real hardware.

[WestfW]

the first pulse is pretty close to 1 microsecond

But it was supposed to be 2 microseconds, according to the code you quoted...
(Yeah, I guess "broken" is harsh, for what amounts to "1 us off.")

[PaulStoffregen]

They look like approx 0.8 and 1.6 us. I'd go with ~20% off for the short pulses, rather than "broken".

[WestfW]

So we were getting pretty incorrect delays even when non-constant arguments were used.
See the thread here: http://forum.arduino.cc/index.php?topic=526381.0
Changing the prototype in Arduino.h seems to be quite effective:
void __attribute__((noinline)) delayMicroseconds(unsigned int us);

[WestfW]

No, the "noinlne" still apparently allows calls with constant arguments to get their own "constprop" not-inline but still too short functions to be generated.

void __attribute__((noinline)) __attribute__((optimize("no-ipa-cp"))) delayMicroseconds(unsigned int us);

works better, but is becoming pretty obscure and gcc-specific. Maybe it'd be easier to code the whole mess in assembly language...

[PaulStoffregen]

Maybe it'd be easier to code the whole mess in assembly language...

That's what I did many years ago for delayMicroseconds on AVR-based Teensy.

You're welcome to use this code for Arduino's boards, if you like...

static inline void delayMicroseconds(uint16_t) __attribute__((always_inline, unused));
static inline void delayMicroseconds(uint16_t usec)
{
    if (__builtin_constant_p(usec)) {
            #if F_CPU == 16000000L
            uint16_t tmp = usec * 4;
            #elif F_CPU == 8000000L
            uint16_t tmp = usec * 2;
            #elif F_CPU == 4000000L
            uint16_t tmp = usec;
            #elif F_CPU == 2000000L
            uint16_t tmp = usec / 2;
            if (usec == 1) {
                    asm volatile("rjmp L%=\nL%=:\n" ::);
            }
            #elif F_CPU == 1000000L
            uint16_t tmp = usec / 4;
            if (usec == 1) {
                    asm volatile("nop\n");
            } else if (usec == 2) {
                    asm volatile("rjmp L%=\nL%=:\n" ::);
            } else if (usec == 3) {
                    asm volatile("rjmp L%=\nL%=:\n" ::);
                    asm volatile("nop\n");
            }
            #else
            #error "Clock must be 16, 8, 4, 2 or 1 MHz"
            #endif
            if (tmp > 0) {
                    if (tmp < 256) {
                            uint8_t tmp2 = tmp;
                            asm volatile(
                            "L_%=_loop:"                            // 1 to load
                                    "subi   %0, 1"          "\n\t"  // 1
                                    "nop"                   "\n\t"  // 1
                                    "brne   L_%=_loop"      "\n\t"  // 2 (1 on last)
                                    : "=d" (tmp2)
                                    : "0" (tmp2)
                            );
                    } else {
                            asm volatile(
                            "L_%=_loop:"                            // 2 to load
                                    "sbiw   %A0, 1"         "\n\t"  // 2
                                    "brne   L_%=_loop"      "\n\t"  // 2 (1 on last)
                                    : "=w" (tmp)
                                    : "0" (tmp)
                            );
                    }
            }
    } else {
            asm volatile(
            #if F_CPU == 16000000L
                    "sbiw   %A0, 2"                 "\n\t"  // 2
                    "brcs   L_%=_end"               "\n\t"  // 1
                    "breq   L_%=_end"               "\n\t"  // 1
                    "lsl    %A0"                    "\n\t"  // 1
                    "rol    %B0"                    "\n\t"  // 1
                    "lsl    %A0"                    "\n\t"  // 1
                    "rol    %B0"                    "\n\t"  // 1  overhead: (8)/4 = 2us
            #elif F_CPU == 8000000L
                    "sbiw   %A0, 3"                 "\n\t"  // 2
                    "brcs   L_%=_end"               "\n\t"  // 1
                    "breq   L_%=_end"               "\n\t"  // 1
                    "lsl    %A0"                    "\n\t"  // 1
                    "rol    %B0"                    "\n\t"  // 1  overhead: (6)/2 = 3 us
            #elif F_CPU == 4000000L
                    "sbiw   %A0, 4"                 "\n\t"  // 2
                    "brcs   L_%=_end"               "\n\t"  // 1
                    "breq   L_%=_end"               "\n\t"  // 1  overhead: (4) = 4 us
            #elif F_CPU == 2000000L
                    "sbiw   %A0, 12"                "\n\t"  // 2
                    "brcs   L_%=_end"               "\n\t"  // 1
                    "breq   L_%=_end"               "\n\t"  // 1
                    "lsr    %B0"                    "\n\t"  // 1
                    "ror    %A0"                    "\n\t"  // 1  overhead: (6)*2 = 12 us
            #elif F_CPU == 1000000L
                    "sbiw   %A0, 32"                "\n\t"  // 2
                    "brcs   L_%=_end"               "\n\t"  // 1
                    "breq   L_%=_end"               "\n\t"  // 1
                    "lsr    %B0"                    "\n\t"  // 1
                    "ror    %A0"                    "\n\t"  // 1
                    "lsr    %B0"                    "\n\t"  // 1
                    "ror    %A0"                    "\n\t"  // 1  overhead: (8)*4 = 32 us
            #endif
            "L_%=_loop:"
                    "sbiw   %A0, 1"                 "\n\t"  // 2
                    "brne   L_%=_loop"              "\n\t"  // 2
            "L_%=_end:"
                    : "=w" (usec)
                    : "0" (usec)
            );
    }
}

[cmaglie]

You're welcome to use this code for Arduino's boards, if you like...

@PaulStoffregen thank you, impressive implementation!

@WestfW what do you think? Looks very good at a first sight.

[matthijskooijman]

Looks good at first glance. Some questions (probably for @PaulStoffregen):

• The exceptions for usec = 1/2/3 for 1Mhz (and similar for 2Mhz), couldn't these be generalized to if (usec % 4 == 1) etc.? That should remove rounding errors even when usec is bigger than 4.
• Why the distinction between < 256 and >= 256 in the constant case? Is this just to reduce the error from the initial load from 2 to 1 cycle? Or also to reduce register pressure? In either case, wouldn't it make sense to add some additional instructions to round the initialization up to 4 cycles and reduce the loop count by one? That probably requires special casing tmp == 1, since the general case would then always have 4 cycles init plus one 4-cycle loop.
• For 16Mhz, there is a shortcut for usec == 1 and usec == 2. AFAICS the delay will be only 5 cycles for usec == 2. Did you try getting that right too? Might be hard to get right, though. A similar thing happens for 8Mhz and usec == 3. And of course at lower speeds, small delays will be too long, but that's not really fixable.
• The old code supported 20Mhz and 24Mhz as well, so we might need to add that again (at least 20Mhz, 24Mhz is overclocking and probably not used so much).
• On big values of usec and F_CPU > 1Mhz, usec will be multiplied, so it might overflow. This is also the case with the current code, and the docs already say not to use a value of more than 16383us (technically, 20Mhz and 24Mhz need a lower limit, but those are corner cases anyway).

Summarizing: This code is great, certainly better than what is present now (only 20Mhz and perhaps 24Mhz support needs to be added). There seem to be opportunities for further improving the accuracy in some corner cases, but nothing that should block this.

[PaulStoffregen]

Yes, all good points. Who knows, maybe you guys will improve things and maybe I'll end up merging your improvements back into my copy? :-)

But the honest reality is I'm very focused on ARM these days and not really developing much for AVR anymore. These are all good questions. Years ago I would have poured quite a bit of time into a conversation about the finer points. Today AVR is very much "legacy" to me. I do not plan to ever make another AVR-based product. So I'm pretty much just sharing the code with you, without much interest to get involved in changes.

[matthijskooijman]

@PaulStoffregen, makes total sense. Still, I'm drawn by the challenge of making things better, so I'll probably spend a bit of time on this.

I had some spare time in the train this week, so I started on a bit of code to measure the error in the implementation. This code uses timer1 running at full CPU speed to measure the delay. The fun part of this is that you can test different CPU speeds by changing F_CPU, without actually having to run at that particular CPU speed (since it's just checking if the number of cycles is correct, keeping F_CPU into account). The code tests constant and variable delays from 0 to 100, and then from 101 to 1000 in steps of 7 (so 101, 108, 115, etc.) to save time and space.

I did a test with the original delayMicroseconds() and a test with Paul's version above (I tested on a Mega, since the code was too big for a Uno). I put the code, and results on gist.

For the original: https://gist.github.com/matthijskooijman/1d59323f07dcca08976a4f0fb1477766

>> Summary
24Mhz: 1 ok, 459 errors, totalling 4808 cycles
20Mhz: 0 ok, 460 errors, totalling 1058 cycles
16Mhz: 1 ok, 459 errors, totalling 4820 cycles
8Mhz: 228 ok, 232 errors, totalling 3451 cycles
1Mhz: 2 ok, 458 errors, totalling 6244 cycles

Interestingly, the original implementation is mostly correct for variable arguments at 8Mhz, but failed for pretty much all other cases, with varying error amounts.

For Paul's implementation: https://gist.github.com/matthijskooijman/75ec512e86578fa5375ed33ecfebb233

>> Summary
16Mhz: 58 ok, 402 errors, totalling 5680 cycles
8Mhz: 105 ok, 355 errors, totalling 4230 cycles
4Mhz: 126 ok, 334 errors, totalling 2830 cycles
2Mhz: 31 ok, 429 errors, totalling 4560 cycles
1Mhz: 37 ok, 423 errors, totalling 5802 cycles

These results actually surprise me, there's still quite some big deviations in Paul's implementation. Some of them correspond to what I noted earlier, some of them.

I already looked at the constant 9 delay at 16Mhz, which has a 1-cycle error, which seem to be caused by the compiler making a weird register allocation (causing a ldi; mov instead of directly ldi into the right register). Removing the "tmp2" output from asm statement seems to fix this, but I'm not sure if there is a reason for marking this an output...

I'l look into improving the code later :-)

[matthijskooijman]

I significantly improved Paul's implementation, it is now near-perfect in most cases. I tracked my changes in git, see https://github.com/matthijskooijman/Arduino-delay-test and below for a discussion of the changes. I also added 24Mhz and 20Mhz implementations.

For constant delays:

• As I suggested above, I added compensation for rounding errors at 1Mhz and 2Mhz, which fixes the delay when us is not divisible by 2 or 4.
• It turns out the compiler was not always loading the constant loop count with just an ldi instruction, sometimes it would also generate additional (pointless) mov instructions. I moved the ldi instruction into the asm statement, passing the value to the assembler as an integer constant.
• As suggested above, I fixed an off-by-one cycle error for >= 256 loop delays.

With these changes, all constant delays (including 0us) are now perfect.

For non-constant delays:

• The overhead calculation counted cycles, but then multiplied them as if they were loop counts, resulting in an overhead in us that was a factor 4 higher than in reality. This resulted in a delays that were too short by a fixed amount (1.5us at 16Mhz, up to over 15us at 1Mhz). I fixed this by properly calculating the overhead, and adding some additional delays and corrections where the overhead was not a multiple of the loop count or a whole number of us.
• For some delays, at 1Mhz and 2Mhz, the loop count would be calculated as 0 due to division. Since the loop starts with decrementing the counter, this resulted in an underflow, and the loop executing 65536 times instead of 0 times. This is fixed by explicitly checking for a zero loop count after the division (instead of before).
• For 1Mhz and 2Mhz, there would be rounding errors (just like like with the constant delays): The actual delay would be rounded to multiples of 2us or 4us. For 2Mhz, I fixed this by checking the bit shifted out by the division, but for 1Mhz this turned out to still leave quite some errors in the lower delay amounts. I spent quite some time puzzling and came up with a dozen of different approaches, with increasing complexity (and corresponding flash size) and increasing accuracy (see below).

With these changes, most variable delays are now perfect. At 24Mhz, 20Mhz, 16Mhz and 8Mhz, only a variable 0 passed has a small error, at 4Mhz only a variable 0 or 1 has some error. At 2Mhz, values up to 6 have some errors, the rest is perfect.

For 1Mhz, it depends a bit on the code used. The simplest approach, without compensation for the rounding, delays 5us when passed a value up to 9, delays 12us when you pass 10-13, then increases the delays in blocks of 4 (so passing 14-17 results in 14us delay, 18-21 in 18us, etc.). The most complex approach delays 8us when passed a value up to 9, and is correct for all values of 10 and up. I'm not quite sure what would the tradeoff here, so I put up a few different options in this gist: https://gist.github.com/matthijskooijman/27706e2593da873e6b6707753f91ec28

Since I added some testcases, since my previous code, here's a summary of the different implementations' performance. Current Arduino code:

24Mhz: 1 ok, 745 errors, totalling 6964 cycles
20Mhz: 0 ok, 746 errors, totalling 1702 cycles
16Mhz: 1 ok, 745 errors, totalling 6973 cycles
8Mhz: 421 ok, 325 errors, totalling 4848 cycles
1Mhz: 2 ok, 744 errors, totalling 9397 cycle

Paul's code:

16Mhz: 61 ok, 685 errors, totalling 10403 cycles
8Mhz: 105 ok, 641 errors, totalling 7797 cycles
4Mhz: 129 ok, 617 errors, totalling 5236 cycles
2Mhz: 32 ok, 714 errors, totalling 8317 cycles
1Mhz: 61 ok, 685 errors, totalling 10813 cycles

My improvements (with most complex 1Mhz implementation):

24Mhz: 745 ok, 1 errors, totalling 5 cycles
20Mhz: 745 ok, 1 errors, totalling 5 cycles
16Mhz: 745 ok, 1 errors, totalling 5 cycles
8Mhz: 745 ok, 1 errors, totalling 5 cycles
4Mhz: 744 ok, 2 errors, totalling 7 cycles
2Mhz: 739 ok, 7 errors, totalling 17 cycles
1Mhz: 737 ok, 9 errors, totalling 37 cycles

The remaining errors are:

>> Constants @24Mhz
>> Variables @24Mhz
delayMicroseconds(0): ERR: 5 cycles == 0.21us
>> Constants @20Mhz
>> Variables @20Mhz
delayMicroseconds(0): ERR: 5 cycles == 0.25us
>> Constants @16Mhz
>> Variables @16Mhz
delayMicroseconds(0): ERR: 5 cycles == 0.31us
>> Constants @8Mhz
>> Variables @8Mhz
delayMicroseconds(0): ERR: 5 cycles == 0.63us
>> Constants @4Mhz
>> Variables @4Mhz
delayMicroseconds(0): ERR: 5 cycles == 1.25us
delayMicroseconds(1): ERR: 2 cycles == 0.50us
>> Constants @2Mhz
>> Variables @2Mhz
delayMicroseconds(0): ERR: 5 cycles == 2.50us
delayMicroseconds(1): ERR: 3 cycles == 1.50us
delayMicroseconds(2): ERR: 1 cycles == 0.50us
delayMicroseconds(3): ERR: -1 cycles == -0.50us
delayMicroseconds(4): ERR: -3 cycles == -1.50us
delayMicroseconds(5): ERR: 2 cycles == 1.00us
delayMicroseconds(6): ERR: 2 cycles == 1.00us
>> Constants @1Mhz
>> Variables @1Mhz
delayMicroseconds(0): ERR: 8 cycles == 8.00us
delayMicroseconds(1): ERR: 7 cycles == 7.00us
delayMicroseconds(2): ERR: 6 cycles == 6.00us
delayMicroseconds(3): ERR: 5 cycles == 5.00us
delayMicroseconds(4): ERR: 4 cycles == 4.00us
delayMicroseconds(5): ERR: 3 cycles == 3.00us
delayMicroseconds(6): ERR: 2 cycles == 2.00us
delayMicroseconds(7): ERR: 1 cycles == 1.00us
delayMicroseconds(9): ERR: -1 cycles == -1.00us

[WestfW]

If "we" are willing to use __builtin_constant_p() in Arduino.h for this, with entirely separate actual code from the non-constant case, why don't we just use the avr-libc _delay_us() ? (Just for the constant case...)

[matthijskooijman]

@WestfW excellent point. I just tried it (see https://github.com/matthijskooijman/Arduino-delay-test) and that also generates perfect delays for all tested values, and is a lot simpler. It even generates slightly shorter (2% for my testcase), because it uses 3-cycle loops instead of 4-cycle loops when possible.

[PaulStoffregen]

Wow, impressive work here!