New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: Adopt osEventFlags from RTX 5 #4571

Merged
merged 1 commit into from Jun 29, 2017

Conversation

Projects
None yet
7 participants
@geky
Member

geky commented Jun 15, 2017

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
events: Adopt osEventFlags from RTX 5
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.
@tomasfrisbergublox

This comment has been minimized.

tomasfrisbergublox commented Jun 16, 2017

@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-

This comment has been minimized.

Member

pan- commented Jun 16, 2017

@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

This comment has been minimized.

tomasfrisbergublox commented Jun 16, 2017

@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

This comment has been minimized.

Member

geky commented Jun 16, 2017

@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-

This comment has been minimized.

Member

pan- commented Jun 16, 2017

@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 geky referenced this pull request Jun 16, 2017

Merged

RTOS: Fix semaphore #4579

@geky

This comment has been minimized.

Member

geky commented Jun 16, 2017

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-

This comment has been minimized.

Member

pan- commented Jun 16, 2017

@geky Thanks for the precision. LGTM.

@pan-

pan- approved these changes Jun 16, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2017

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

@geky

This comment has been minimized.

Member

geky commented Jun 19, 2017

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-

This comment has been minimized.

Member

pan- commented Jun 20, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2017

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

This comment has been minimized.

Member

geky commented Jun 20, 2017

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-

This comment has been minimized.

Member

pan- commented Jun 20, 2017

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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 27, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2017

retest uvisor

@theotherjimmy theotherjimmy merged commit fecb991 into ARMmbed:master Jun 29, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment