events: Adopt osEventFlags from RTX 5

[geky]

This provides the correct binary semaphore behaviour that was expected by the equeue layer, removes concerns around semaphore overflow, and reduces the number of spurious wakeups which may save a bit of power.

This also fixes some issues we were seeing around the RTX 5 changes to semaphore behaviour.

related issue #4500
cc @pan-, @tomasfrisbergublox, @hesolium

Tests

• /morph test-nightly

[tomasfrisbergublox]

@geky

I have made some initial tests and it looks good. The event flag solution is a good one.

Maybe a minor improvement could be to add a "equeue_sema_wait" after the while loop to dispatch the pending events.
// dispatch events
while (es) {
in the file "equeue.c", function "equeue_dispatch".

This would clear the event flag indicating that there is no more callbacks at this moment. When the "wait for event" code is reached further down in the function, the "equeue_sema_wait" will hang until the deadline is reached or a new event was added.
// wait for events
equeue_sema_wait(&q->eventsema, deadline);

If not, it is likely that the wait call will return immediately and one addition loop is performed. Of course this is not really a problem.

[pan-]

@geky I'm not sure it is 100% correct. From my understanding, osEventFlagsWait will clear the flags specified to wait for. It means that if an event has been post after the computation of the deadline and before equeue_sema_wait (here) then it will be miss.

[tomasfrisbergublox]

@geky @pan-
yes, you are correct.

[deepikabhavnani]

Event flags will be used for TCP/UDP sockets as well in mbed-os. Should we have fix defines for flags (for each module) in mbed-os common file instead of using 1?

[geky]

@pan-, correct me if I'm wrong, but if an event comes in before the equeue_sema_wait, the osEventFlagsSet will set the flag and cause the equeue_sema_wait to return immediately. Clearing the flag is intentional, because any events in the queue will be checked for by equeue_dequeue, so we can wait until either a new signal (osEventFlagsSet) caused by updating the queue, or the timeout to trigger.

@tomasfrisbergublox, unfortunately an additional equeue_sema_wait would need to same logic for correctness, since an incorrect equeue_sema_wait could cause a deadlock and any updates to the queue may add more events to dispatch. So I'm not sure there would be a speed improvement. It's probably safer to just check the queue an extra time if the callbacks change anything.

@deepikabhavnani, One big benefit of the osEventFlags object is that each set of flags is standalone, so we don't need to worry about flag conflicts between modules. An enum may be useful if we have more events, but since the event queue only needs a single event I think 1 is fine.

[pan-]

@geky I'm not sure of the behaviour, is the flag cleared before or after the wait happens ? If it is before and the flag is set then does wait returns immediately or does it actually wait ?

[geky]

The osEventFlagsWait is similar to the osSemaphoreWait, the flags are cleared atomically if the condition is matched in osEventFlagsWait.

Here's where the logic resides in RTX:
https://github.com/ARMmbed/mbed-os/blob/master/rtos/rtx5/TARGET_CORTEX_M/rtx_evflags.c#L95-L110

If the flags are already set when osEventFlagsWait is entered, the function returns immediately:
https://github.com/ARMmbed/mbed-os/blob/master/rtos/rtx5/TARGET_CORTEX_M/rtx_evflags.c#L374-L378

With RTX 5, boolean osEventFlags and binary semaphores should be interchangeable.

[pan-]

@geky Thanks for the precision. LGTM.

[0xc0170]

@geky shall we wait with this one for EventsFlag C++ that is already in the PR queue ?

[geky]

Nah, there's no reason for this layer to be C++. Adding another layer of indirection will just make this layer more instable.

I've also been meaning to move the ticker code over to the C hal now that 64-bit tickers are in.

[pan-]

Nah, there's no reason for this layer to be C++. Adding another layer of indirection will just make this > layer more instable.

I've also been meaning to move the ticker code over to the C hal now that 64-bit tickers are in.

For the HAL it makes sense, the API is not supposed to change and it is managed by the mbed OS team but is it the same for the RTOS ? In many aspects it is a 3rd party component; can we guarantee that RTOS APIs won't change ?

[0xc0170]

Nah, there's no reason for this layer to be C++. Adding another layer of indirection will just make this layer more instable.

I noticed that lot of ppl liked EventsFlag proposal, therefore I considered this as a duplicate (noting this is C++ world, not HAL) plus C++ should simplify usage of events flags, we can see how much boilerplate is needed in this patch. . By moving to C++ API, we would keep it consistent (a reason in another paragraph below).
I would say exactly opposite, making it more stable, readable.

In many aspects it is a 3rd party component; can we guarantee that RTOS APIs won't change ?

We can't. We experienced it with the last update, it is subject to change, however with our C++, we keep backward compatibility.

It is fine as it is here, I had that question just to make it more buletproof for the future and if we all are on the same page that events flag C++ is a proposal we go for.

[0xc0170]

My above comment is just how it could be, but we dont have yet C++, this is an important fix for wifi. Thus we can always refactor this (I would create a tracking issue then to remember if we agree).

Is this ready for integration? Any tests run?

[geky]

My thoughts is we should just move forward as is, the c++ api is currently in flux and might change, and @0xc0170 is right we can move forward for now independently of the c++ semaphore/event flags work.

I'm not opposed to switching back to the c++ api once it's in and stable, although adopting the c++ apis doesn't help much unless we do the same accross the entire codebase: b793a3f

From my point of view this can go in as is. I ran tests locally, should we kick off morph test?

[pan-]

I think we're all on the same page and this pr should go in because the C++ EventFlag abstraction is not ready yet.

Nonetheless the question of API stability remains what guarantees can be provided for the C and the C++ layer after the RTX update ?
If the answer is no guarantee for the C API then it would be better to start fixing use of the C API across the code base because we will have some control over the C++ APi.

On the other hand, if the stability of both API is guaranteed then it should be indicated in the guidelines which API can be use: C++ or C++ and C.

[theotherjimmy]

/morph test

[mbed-bot]

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 651

All builds and test passed!

[0xc0170]

retest uvisor