BUG: chThdSleepMilliseconds() sleeps 1 clock cycle too short

[positron]

Hi,

as I understand, both chThdSleepMilliseconds(x) and ..Microseconds(x) are supposed to sleep AT LEAST the time x.
The actual time should be between x and x+"1 clock cycle" (which might typically be 1ms).

However, what we find when measuring with an oscilloscope is that chThdSleepMilliseconds(1) sleeps between 0 and 1 msec.

I guess, this is a "< vs. <=" or "+1 missing" type of bug in ChibiOS.

(Platform: STM32F407, kernel 2.5.2unstable)

[Giovanni]

Hi,

The behavior is the expected one, the system is tick based so the next tick counts as 1. If you need to work with a guaranteed minimum the you have to add a +1 externally.

chSysSleep(1) is commonly used to align to the next tick boundary, zero is a reserved constant (TIME_IMMEDIATE) and is used as timeout specification by several other primitives.

Giovanni

[positron]

Thanks for your quick reply.

The documentation says: "The specified time is rounded up to a value allowed by the real system tick clock."

To me, this means what I just described above: That the delay can be longer but not shorter.
This is also what just about everybody will expect. An API like that which does delays shorter than specified is prone to introduce bugs on the application side.

However, if this is expected behavior, please clearly state in the documentation that a delay X is at least X - 1 tick long.
Programmers will assume from lots of other systems, that delays are a guaranteed minimum (see usleep(), etc.)

[Giovanni]

The rounding up is done while converting from time to system ticks. So I have a 100Hz system tick and I specify 1mS, 1mS is rounded up to 10mS = 1 system tick.

The correct way to add that +1 would be:

chThdSleep(S2ST(secs) + 1)

This way the rounding is added after the conversion in system ticks, one tick is added instead of a whole second.

Giovanni

[positron]

Thanks, I found out the correct way to do things immediately after my post so I edited the post to remove the second question.

But please clarify the docu!

I introduced:

// Do not use chThdSleepMilliseconds()/Microseconds() because these functions
// will in fact sleep at least 1 clock cycle LESS then specified. To get
// reliable minimum delays, use these:
#define chThdSleepMsec(msec) chThdSleep(MS2ST(msec)+1)
#define chThdSleepUsec(usec) chThdSleep(US2ST(usec)+1)