Go Down

Topic: found bug in delayMicroSeconds when called with 0; (Read 2461 times) previous topic - next topic

robtillaart

Nov 18, 2012, 11:18 am Last Edit: Nov 18, 2012, 11:26 am by robtillaart Reason: 1
The delayMicroseconds function fails when called with a value of 0.
Although this is not done normally, it can happen when it is called with a variable which is the result of some expression/calculation.

The bug can be traced back to the if (--us ==0) statement, which causes an overflow when the param us is 0. The delay is then far longer (about 16384 micros) .

BUG seen in IDE 0.22 + 1.0.0 + 1.0.2

Find test program and patch below : NOTE the patch does not include the 1.0.2 20Mhz addition as I cannot test it .

The patched version of the delayMicroseconds() will return asap when called with 0 . This still takes about 1.2 uSec but that is way better than 16K uSec :)

Q: can anybody confirm this bug in a 20Mhz duino?

Program to show bug
Code: [Select]
//
//    FILE: delayMicrosecondsBUG.pde
//  AUTHOR: Rob Tillaart
//    DATE: 2012-11-18
//
// PUPROSE: test delayMicroseconds()
//

void setup()
{
 Serial.begin(9600);
 Serial.println("start...");
 
 unsigned long m = micros();
 for (uint8_t i=0; i<100; i++)
 {
   delayMicroseconds(0);
 }
 Serial.println(micros()- m);

 m = micros();
 for (uint8_t i=0; i<100; i++)
 {
   delayMicroseconds(1);
 }
 Serial.println(micros()- m);

 m = micros();
 for (uint8_t i=0; i<100; i++)
 {
   delayMicroseconds(2);
 }
 Serial.println(micros()- m);
 
 m = micros();
 delayMicroseconds(20);
 Serial.println(micros()- m);
 m = micros();
 delayMicroseconds(200);
 Serial.println(micros()- m);
 m = micros();
 delayMicroseconds(2000);
 Serial.println(micros()- m);
}

void loop()
{}


====================
Find below a patch for the function (for 0.22 - 1.0.0 version):

Code: [Select]
void delayMicroseconds(unsigned int us)
{
// calling avrlib's delay_us() function with low values (e.g. 1 or
// 2 microseconds) gives delays longer than desired.
//delay_us(us);

#if 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 yields a delay of approximately 1 1/8 us.
//  PATCH
// if (--us == 0)
// return;
if (us < 2) return;
us--;

// the following loop takes a quarter of a microsecond (4 cycles)
// per iteration, so execute it four times for each microsecond of
// delay requested.
us <<= 2;

// account for the time taken in the preceeding commands.
us -= 2;
#else
// for the 8 MHz internal clock on the ATmega168

// for a one- or two-microsecond delay, simply return.  the overhead of
// the function calls takes more than two microseconds.  can't just
// subtract two, since us is unsigned; we'd overflow.
// PATCHED LINES
// if (--us == 0)
// return;
// if (--us == 0)
// return;
if (us < 3) return;
us -= 2;

// the following loop takes half of a microsecond (4 cycles)
// per iteration, so execute it twice for each microsecond of
// delay requested.
us <<= 1;
   
// partially compensate for the time taken by the preceeding commands.
// we can't subtract any more than this or we'd overflow w/ small delays.
us--;
#endif

// busy wait
__asm__ __volatile__ (
"1: sbiw %0,1" "\n\t" // 2 cycles
"brne 1b" : "=w" (us) : "0" (us) // 2 cycles
);
}


TODO: post on the issue list yet ...
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart


Reported as #1121 on  - https://github.com/arduino/Arduino/issues?state=open -
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

a more generic patch might work for the 20Mhz too    (values smaller than 2 are exceeded by function call overhead)

Code: [Select]

void delayMicroseconds(unsigned int us)
{
  if (us <2) return;
...
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

retrolefty



Reported as #1121 on  - https://github.com/arduino/Arduino/issues?state=open -


It will be interesting to see if the arduino developers will consider this a bug or a feature as they already note the minimum value on can use will accurate results. I the reference for delayMicroseconds:

Quote

Caveats and Known Issues
This function works very accurately in the range 3 microseconds and up. We cannot assure that delayMicroseconds will perform precisely for smaller delay-times.



Lefty

robtillaart

Hi Lefty,

Rationale for that statement is the fact that a function call takes approx 1 usec. Checking the param takes some clock cycles too. So before one can start to count down, too much time may have passed already (for small values). The root cause is that one is near the cpu speed.

But mapping 0 usec on 16K usec is a serious Bug - with capital B ;)

For the Due which is 84(?) Mhz I expect a re-implementation for this function. However no experience yet with the Due.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

retrolefty

#5
Nov 18, 2012, 06:14 pm Last Edit: Nov 18, 2012, 06:30 pm by retrolefty Reason: 1

Hi Lefty,

Rationale for that statement is the fact that a function call takes approx 1 usec. Checking the param takes some clock cycles too. So before one can start to count down, too much time may have passed already (for small values). The root cause is that one is near the cpu speed.

But mapping 0 usec on 16K usec is a serious Bug - with capital B ;)

For the Due which is 84(?) Mhz I expect a re-implementation for this function. However no experience yet with the Due.


I believe there is also a minimum 4 usec step size resolution for the delayMicroseconds function as implemented in the arduio AVR platform? So it's not capable of 1 usec resolution timing delays as one might assume otherwise even at values greater then 3?

Lefty

robtillaart

The delayMicroseconds() implementation ends with
Code: [Select]

// busy wait
__asm__ __volatile__ (
"1: sbiw %0,1" "\n\t" // 2 cycles
"brne 1b" : "=w" (us) : "0" (us) // 2 cycles

which is a tight loop decrementing the var us. So I think the timing is quite precise for values > 2.  This loop takes 4 cycles so it is executed 4 times per usec (16Mhz assumed).

The stepsize of 4 usec is the precision of the micros() function so if you want to measure time smaller than 4 usec you need a HW timer solution. Can't find the link now.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

retrolefty


The delayMicroseconds() implementation ends with
Code: [Select]

// busy wait
__asm__ __volatile__ (
"1: sbiw %0,1" "\n\t" // 2 cycles
"brne 1b" : "=w" (us) : "0" (us) // 2 cycles

which is a tight loop decrementing the var us. So I think the timing is quite precise for values > 2.  This loop takes 4 cycles so it is executed 4 times per usec (16Mhz assumed).

The stepsize of 4 usec is the precision of the micros() function so if you want to measure time smaller than 4 usec you need a HW timer solution. Can't find the link now.


Thanks for that.
Yes, I was mixing up micros() with delayMicroseconds(). Put then again I'm old, so I'm allowed a certain amount of 'brain farts' per fortnight, as I continue to chase kids off my lawn.  ;)

Lefty

Coding Badly

Find test program and patch below : NOTE the patch does not include the 1.0.2 20Mhz addition as I cannot test it .


The what?

robtillaart

#9
Nov 20, 2012, 06:59 pm Last Edit: Nov 20, 2012, 07:44 pm by robtillaart Reason: 1
@CB
Quote
The what?

The 1.0.2 version of wiring.c has additional code to implement this delayMicroseconds() function correctly(?) for a 20Mhz arduino.
As I do not have such a thing I cannot test it.

Clear now?
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Coding Badly


robtillaart

did some rework and fine tuning of the delayMicroseconds code, see  - https://github.com/arduino/Arduino/issues/1121 -

Resulted in more accurate timing for values up to 100uSeconds (above not tested)

Code: [Select]

/* Delay for the given number of microseconds.  Assumes a 8 or 16 MHz clock. */
void delayMicroseconds(unsigned int us)
{
    // calling avrlib's delay_us() function with low values (e.g. 1 or
    // 2 microseconds) gives delays longer than desired.
    // delay_us(us);

#if F_CPU == 20000000L
    // for the 20 MHz clock on rare Arduino boards

    // handle 0 and 1
    if (us == 0) return;
    __asm__ __volatile__ (
        "nop" "\n\t"
        "nop" "\n\t"
        "nop" "\n\t"
        "nop");
    if (us == 1) return;

    // calc # iterations
    us = (us<<2) + us; // x5 us

    // account for the time taken in the preceeding commands.
    us -= 7;

#elif F_CPU == 16000000L
    // for the 16 MHz clock on most Arduino boards

    // handle 0 and 1
    if (us == 0) return;
    if (us == 1) return;

    // calc # iterations
    us <<= 2;
    // account for the time taken in the preceeding commands.
    us -= 5;

#elif F_CPU == 8000000L
    // for the 8 MHz internal clock on the ATmega168

    // handle 0, 1;
    if (us < 2) return;
    // handle 2;
    if (us == 2) return;

    // calc # iterations
    us <<= 1;

    // account for the time taken in the preceeding commands.
    us -= 5;
    // quarter uSecond adjust
    __asm__ __volatile__ (
        "nop\n\t"
    );

#elif F_CPU == 1000000L

// TODO 1 MHz
#error "delayMicroseconds: 1 MHz not supported yet"

# else

#error "delayMicroseconds: not supported clockspeed"

#endif

    // busy wait
    __asm__ __volatile__ (
        "1: sbiw %0,1" "\n\t" // 2 cycles
        "brne 1b" : "=w" (us) : "0" (us) // 2 cycles
    );
}


16MHz now gives the following output:

delayMicroseconds(0) => 0.81920 ==> inf
delayMicroseconds(1) => 1.01880 ==> 0.01880
delayMicroseconds(2) => 2.02280 ==> 0.01140
delayMicroseconds(3) => 3.02880 ==> 0.00960
delayMicroseconds(4) => 4.03520 ==> 0.00880
delayMicroseconds(5) => 5.04080 ==> 0.00816
delayMicroseconds(10) => 10.07080 ==> 0.00708
delayMicroseconds(20) => 20.13200 ==> 0.00660
delayMicroseconds(50) => 50.31200 ==> 0.00624
delayMicroseconds(100) => 100.61360 ==> 0.00614

Last column is the relative error (inf due to divide by zero) and it is decreasing which is good. The value for DMS(0) is as good as it gets.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Go Up