Update EventQueue API to use chrono times #13975
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@adbridge, thank you for your changes. |
pan-
reviewed
Nov 27, 2020
events/include/events/EventQueue.h
Outdated
| * @see EventQueue::dispatch | ||
| * | ||
| * Executes events indefinitely unless the dispatch loop is forcibly broken. | ||
| * See break_dispatch() |
There was a problem hiding this comment.
You can use @see to reference another function. Doxygen should generate a link to it and depending on it's configuration it can list references in break_dispatch doc.
There was a problem hiding this comment.
Typo error removing the '@' by the looks of it!
events/include/events/EventQueue.h
Outdated
| * @param ms Time to wait for events in milliseconds, a negative | ||
| * value will dispatch events indefinitely | ||
| * (default to -1) | ||
| * @param wait Time to wait for events in milliseconds, expressed as a |
There was a problem hiding this comment.
I always found that description confusing; the parameter expresses the duration of the dispatch process. It has nothing to do with the time to wait for events. For example, if I request a dispatch for 10ms and an event is ready at 2ms into the process. The function won't exit after processing the event, it will continue the dispatch process for the remaining 8ms.
There was a problem hiding this comment.
I see what you mean, but it also isn't a time limit on the dispatch. If there are multiple events scheduled for the time as it stood on entry, it will run them all. It won't run the first few and then decide "oh, those first few events took longer than 10ms, let's stop dispatching.
So it is specifically determining how long it will wait, not dispatch, but yes, it's relative to the entry time, not a "gap" timeout after last event.
These sort of multi-op things with timeouts are always confusing. Compare condition variables, where there is a timeout on the wait, but no timeout on the mutex acquire. Time could be well past the deadline before you regain control, whether or not the wait timed out...
There was a problem hiding this comment.
That's part of the original comment by the author, are we saying we disagree with what was originally written ?
There was a problem hiding this comment.
I think the original comment is acceptable, and "wait" is right. @pan- didn't like it, interpreting it as a one-off wait for first event. I view it as correctly describing when we'll give up at any point we find ourselves waiting.
pan-
reviewed
Nov 27, 2020
| * In this case, the dispatch function does not wait and is IRQ safe. | ||
| * | ||
| */ | ||
| void dispatch_once(); |
There was a problem hiding this comment.
I lost my comment for that item.
@kjbracey-arm The implementation doesn't do exactly this. It may not break immediately out of the loop if the next tick hasn't been reached. Do you think we should patch the implementation ?
The change is trivial, we just have to return if ms is equal to 0.
mbed-os/events/source/equeue.c
Lines 525 to 530 in 68612cb
There was a problem hiding this comment.
I'm not seeing what you're seeing here. If ms == 0, then it computes tickdiff(timeout, tick), ie entry_tick + 0 - tick, and that's got to be <= 0, so it exits immediately. Right?
BTW the name here feels perilously close to dispatch_one(), but I guess it's okay...
We must mark the function has deprecated and remove it in a future major release. |
kjbracey
reviewed
Nov 30, 2020
events/include/events/EventQueue.h
Outdated
| * @param ms Time to wait for events in milliseconds, a negative | ||
| * value will dispatch events indefinitely | ||
| * (default to -1) | ||
| * @param wait Time to wait for events in milliseconds, expressed as a |
There was a problem hiding this comment.
I see what you mean, but it also isn't a time limit on the dispatch. If there are multiple events scheduled for the time as it stood on entry, it will run them all. It won't run the first few and then decide "oh, those first few events took longer than 10ms, let's stop dispatching.
So it is specifically determining how long it will wait, not dispatch, but yes, it's relative to the entry time, not a "gap" timeout after last event.
These sort of multi-op things with timeouts are always confusing. Compare condition variables, where there is a timeout on the wait, but no timeout on the mutex acquire. Time could be well past the deadline before you regain control, whether or not the wait timed out...
| * In this case, the dispatch function does not wait and is IRQ safe. | ||
| * | ||
| */ | ||
| void dispatch_once(); |
There was a problem hiding this comment.
I'm not seeing what you're seeing here. If ms == 0, then it computes tickdiff(timeout, tick), ie entry_tick + 0 - tick, and that's got to be <= 0, so it exits immediately. Right?
BTW the name here feels perilously close to dispatch_one(), but I guess it's okay...
events/include/events/EventQueue.h
Outdated
|
|
||
| /** Dispatch currently queued events only and then terminate | ||
| * | ||
| * In this case, the dispatch function does not wait and is IRQ safe. |
There was a problem hiding this comment.
I think we can't quite say it's IRQ safe. It wouldn't be in an RTOS build, as it still uses mutexes for its state locks. It's close to it, and in certain specialised non-RTOS runtime environments this might be important, but for mainline Mbed OS API I think I'd like to not try to specify that.
evedon
previously requested changes
Jan 8, 2021
events/include/events/EventQueue.h
Outdated
| */ | ||
| void dispatch(int ms = -1); | ||
| void dispatch(duration wait); |
There was a problem hiding this comment.
You need to leave the existing API with a deprecation warning. This PR then becomes a Feature update rather than Breaking change.
|
This pull request has automatically been marked as stale because it has had no recent activity. @adbridge, please carry out any necessary work to get the changes merged. Thank you for your contributions. |
|
Going to consider the changes here along side the fix needed for type inconsistencies across the event/eventqueue/equeue APIs (triggered by #14176) |
|
Updated Greentea test results: |
|
@evedon please re-review |
evedon
approved these changes
Feb 5, 2021
Difficult to be precise as I saw a variety of effects depending on what values were provided. These ranged from just hanging without actually dispatching the event to core dumping the chip. I can certainly add that. |
|
This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged. |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.8.0
Release-pending
and removed
release-type: feature
Release-pending
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Main dispatch function is updated to take a Chrono duration instead of an integer milliseconds parameter.
To allow for the instances of blocking and non wait versions, which previously were actioned by passing either -1 or 0 as the millisecond delay respectively, two other functions are now available.
dispatch_once() - new
dispatch_forever() - internally modified from the original version but no change in functionality
Note without this fix EventQueue dispatch function is actually broken. Events may not be dispatched at all or in the worst case the chip will crash.
Impact of changes
This is an API change, affecting the following areas:
E.g. '100ms'
Migration actions required
The following migrations actions are required:
as simple as adding 'ms' to any passed literal value. E.g. '100' becomes '100ms'. Variables passed in will need to be explicitly
converted.
Documentation
EventQueue docs may need to be updated . Code snippet examples will certainly need to be.
NB. This also has an impact on the Events snippet example as that uses the EventQueue.dispatch() function.
Pull request type
Test results
Reviewers
@kjbracey-arm @donatieng @pan-