events: Fix zero wait condition in non-rtos semaphore

[geky]

Before, if the semaphore recieved a wait of zero, the semaphore would erronously drop the ticker event to wake up the device, causing the device to go to sleep without any mechanism to wake itself up.

During a dispatch operation, the event queue dispatches all events that have expired, so the call to equeue_sema_wait very rarely has a wait of zero. But this can happen when an event is posted just after a dispatch has occured.

The rtos version handles this correctly:
https://github.com/ARMmbed/mbed-os/blob/master/events/equeue/equeue_mbed.cpp#L105

related issue #3857
cc @pan-, @tbondar, @nvlsianpu

[pan-]

@geky Good catch, I'll test if it solves the issue but there is others in the ticker code.

[pan-]

I'm worried of what can happen with a delay of 0 us.
The timestamp is computed from the current time (here) and there is good chances that it will be in the past (or a far future!) when it will be actually set.

[geky]

Hmm, that's a good point. Do you think we should clamp it to 1 millisecond to be safe?

It does look like timer events in the past are handled correctly (in common code):

• during insertion: https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_ticker_api.c#L70
• during the irq handler: https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_ticker_api.c#L37-L46

[pan-]

It would be reasonable to apply the same behaviour as the one used by RTX semaphore:

when millisec is 0, the function returns instantly.

see http://www.keil.com/pack/doc/CMSIS/RTOS/html/group__CMSIS__RTOS__SemaphoreMgmt.html#gacc15b0fc8ce1167fe43da33042e62098

[0xc0170]

It would be reasonable to apply the same behaviour as the one used by RTX semaphore:

What do you think @geky ?

[geky]

That is the current behaviour of this pr. When ms is 0, the ticker attach should interrupt immediately and set the flag to 1, causing the following while loop (here) to exit immediately.

Do you think the zero case behaviour should be explicit?

-    if (ms > 0) {
+    if (ms == 0) {
+        return false;
+    } else if (ms > 0) {
         timeout.attach_us(callback(equeue_sema_timeout, s), ms*1000);
     }

[pan-]

@geky Yes I think that if ms is 0 then the function should returns instantly. The current version is more like osThreadYield followed by a return when the task is rescheduled.

[geky]

@pan-, should be updated. At the very least this makes the less travelled code path clearer.

[geky]

@pan-, should be updated, let me know if you still have concerns. At the very least this makes the less travelled code path clearer.

[pan-]

Not a blocker for the PR, but it strictly greater than 0 might be clearer.
i also wonder what should happen when ms is less than 0 ?
Is it expected that the semaphore will put the system to sleep without timeout ?

[geky]

Sounds reasonable.

Throughout the equeue library a timeout of -1 is used to represent an infinite wait, so here any negative value would indicate that the semaphore waits forever.

I added a bit of clarification to the function declaration since it's easy to miss that corner case at this level: https://github.com/ARMmbed/mbed-os/pull/3991/files#diff-224d7ae23d9d5591b6a59537012c82d9

[0xc0170]

/morph test

[bridadan]

/morph test

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1866

All builds and test passed!