Bug in delay_us function (timer640.c)?

[JadeKnight]

In timer640.c:
Current version:

Code: [Select]

void delay_us(unsigned short time_us)
{
unsigned short delay_loops;
register unsigned short i;

delay_loops = (time_us+3)/5*CYCLES_PER_US; // +3 for rounding up (dirty)

// one loop takes 5 cpu cycles
for (i=0; i < delay_loops; i++) {};
}
The delay_loops calculation appears to be incorrect. If you pass in 1us, you get a delay_loops of 0 (1+3=4, 4/5=0, 0*16=0). For the Axon you really want something close to 16 cycles, or 3 loops (provided the 5 cycle per loop comment holds true).

Proposed new version:

Code: [Select]

void delay_us(unsigned short time_us)
{
unsigned short delay_loops;
register unsigned short i;

delay_loops = ((time_us * CYCLES_PER_US)+3) / 5; // +3 for rounding up (dirty)

// one loop takes 5 cpu cycles
for (i=0; i < delay_loops; i++) {};
}
With this version, if you pass in 1us, you get a delay_loops of 3 (1*16=16, 16+3=19, 19/5=3), which equates to 15 cycles (closest possible to the 16 cycles per us on the Axon).

Is this correct, or am I missing something?

[Admin]

I took the delay_us code directly from AVRlib.

I never measured the accuracy . . . do you have an oscope to try both?

[Webbot]

In timer640.c:
Current version:

Code: [Select]

void delay_us(unsigned short time_us)
{
unsigned short delay_loops;
register unsigned short i;

delay_loops = (time_us+3)/5*CYCLES_PER_US; // +3 for rounding up (dirty)

// one loop takes 5 cpu cycles
for (i=0; i < delay_loops; i++) {};
}
The delay_loops calculation appears to be incorrect. If you pass in 1us, you get a delay_loops of 0 (1+3=4, 4/5=0, 0*16=0). For the Axon you really want something close to 16 cycles, or 3 loops (provided the 5 cycle per loop comment holds true).

Proposed new version:

Code: [Select]

void delay_us(unsigned short time_us)
{
unsigned short delay_loops;
register unsigned short i;

delay_loops = ((time_us * CYCLES_PER_US)+3) / 5; // +3 for rounding up (dirty)

// one loop takes 5 cpu cycles
for (i=0; i < delay_loops; i++) {};
}
With this version, if you pass in 1us, you get a delay_loops of 3 (1*16=16, 16+3=19, 19/5=3), which equates to 15 cycles (closest possible to the 16 cycles per us on the Axon).

Is this correct, or am I missing something?

Your original formula for the existing code fails to take into account how long it takes to perform:

Code: [Select]

delay_loops = (time_us+3)/5*CYCLES_PER_US;
Plus the overhead of it being in a subroutine ie pushing/popping various registers at the start/end of the code

Edit:- Oh, and if you have other interrupts going on for Uarts, Timers etc then the time taken to service those interrupts will also get added on.

Just a thought

[JadeKnight]

I don't have an oscope. I'm just going by the code and the comments.

Not counting the valid timing concerns Webbot mentioned, these are the results of the two different functions.

input  delay_loops   Cycles (5*delay_loops)   Theoretical us (16Mhz = cycles/16)
us      old / new      old / new               old / new
1    =    0 / 3          0 / 15                  0 / 0.9375
2    =   16 / 7         80 / 35                  5 / 2.1875
3    =   16 / 10        80 / 50                  5 / 3.125
4    =   16 / 13        80 / 65                  5 / 4.0625
5    =   16 / 16        80 / 80                  5 / 5
6    =   16 / 19        80 / 95                  5 / 5.9375
7    =   32 / 23       160 / 115                10 / 7.1875

The current code only seems to be valid for rounded multiples of 5us. The new code would have an overflow problem for values > 4095 (at 16MHz), but at that point it would be better to call delay_ms for any multiples of 1000us.

[Admin]

I updated the file based on the new delay_us() suggestion. I didn't test it however as I think my AVR Programmer just fried . . .

It'll be included in the next update.