why does chThdSleep() not allow a zero delay?

[stewo]

Docs say TIME_IMMEDIATE (which is defined as zero) is an invalid parameter to chThdSleep(). Why is this? Most OS allow a zero delay, which then either causes an immediate return or delays for at least one timer tick. I would prefer to see Chibios also allow for zero delay time.

Thanks, Stephan
P.S.: Note also that consequently, docs on chThdSleepMilliseconds() should also explicitly not allow a zero millisecond delay.

[Giovanni]

Hi,

The problem is that the change would require one more "if" in a very "hot" the code path, the API does not have compliance restrictions and I don't see clear advantages in such a change but the performance degradation would be measurable.

I will improve the documentation of the derived functions.

Giovanni

[stewo]

Ok, so what excatly happens when a zero delay is accidentally specified anyway?

Note also that the description on "Reliable timings using Threads" - "A better solution" states:
[http://www.chibios.org/dokuwiki/doku.php?id=chibios:kb:timing]

Note that this solution requires that the do_something() execution time must not exceed the deadline or the thread would stay sleeping into chThdSleepUntil().

I think this is not actually true as chThdSleepUntil() explicitly (and IMHO correctly) checks for positive (non-zero) delays before invoking chThdSleepS(), right? ("Correctly", because it is impossible for the caller to guarantee the deadline has not yet been reached as the calling thread can be interrupted/delayed before chThdSleepUntil() actually runs.)

Thanks, Stephan

[Giovanni]

Parameters are not checked for correctness unless the option CH_DBG_ENABLE_CHECKS is enabled.

Across all the system a parameter error is handled as an assertion and the system is locked, the application is not supposed to pass garbage as parameters so this is something handled at debug time only.

I will look into the article you mentioned, it is quite old now.

Giovanni

[stewo]

Parameters are not checked for correctness unless the option CH_DBG_ENABLE_CHECKS is enabled.

I think you got me wrong. The check for non-zero positive sleep time is always there:

Code: Select all

void chThdSleepUntil(systime_t time) {
  chSysLock();
  if ((time -= chTimeNow()) > 0)
    chThdSleepS(time);
  chSysUnlock();
}

So in contrast to the aforementioned docs it is actually ok to pass a 'time' value to chThdSleepUntil() that lies in the past.

Stephan

[Giovanni]

The past is interpreted as future the system time is a counter that counts up to 0xffffffff and then restarts from zero, so it is OK. It is designed to overflow, it behaves as an infinite counter.

The only forbidden case is passing a sleep value of zero because the insertion in the timers delta list of an element with delta zero is forbidden, this does not affect chThdSleepUntil() of course.

Giovanni

[stewo]

Sorry if this is nit-picking but I first thought systime_t was defined as a signed integer but it is actually unsigned.

So what this means is the aforementioned fact that a thread calling chThdSleepUntil() can possibly pass a 'time' value that is in the past, which is then interpreted as far future (and chThdSleepUntil() then sits there waiting "forever" as described in the docs). The reason behind that is simply that a thread can be interrupted and a context switch might occur right in the moment it calls chThdSleepUntil(). So when execution continues, some unknown time can have elapsed and thus the 'time' deadline might already have passed.

This is why I usually prefer using signed values for time calculations. Note that a simple cast of the calculated sleep time to a signed value of the same size would solve the problem in chThdSleepUntil(). However, since we do not know the actual type/size of systime_t, we can instead inspect the MSBit:

Code: Select all

  time -= chTimeNow();
  if (time > 0 && (time & (1 << (sizeof(sleep)*8-1))) == 0)
    chThdSleepS(time);

Note also that the following would of course not be sufficient as it does not work in the case of a timer wrap:

Code: Select all

  if (time > chTimeNow())  /* bad idea! */
    chThdSleepS(time - chTimeNow());

Just my 2 cents,
Stephan

[Giovanni]

From the point of view of chThdSleepUntil() all times are in the future so negative times make no sense, values lower than "now" are considered as "value + 0x100000000". Many other functions have time parameters that specify intervals, intervals have to be non-negative too.

That "forever" should not be taken literally, it just takes a lot of time for a 32bits counter to wrap.

It is true that the thread calling chThdSleepUntil() could be preempted and put in the ready list for an unspecified amount of time but the whole point of an RTOS is to make this kind of things predictable, that situation should be considered to a design error.

Basically it is responsibility of the designer to specify a time in the future distant enough from "now" not responsibility of the system to workaround somehow errors in design. Also consider that the system cannot determine if a (value < "now") is a missed deadline or just a time far in the future (not with the provided parameters anyway).

Giovanni

[stewo]

That "forever" should not be taken literally, it just takes a lot of time for a 32bits counter to wrap.

Sure, that's why I put it in quotes.

Thanks again, Stephan

[Giovanni]

Hi,

Probably I found a way to allow the constant zero in chThdSleep() without having extra checks but it changes slightly the behavior of the system, currently the the real interval goes from "time - 1" to "time", it would become from "time" to "time + 1", specifying zero as interval would introduce a delay up to 1mS. The time specification would get an "at least" meaning.

TIME_IMMEDIATE would have a different meaning from zero, it requires no interval at all. I have to think about it.

Giovanni