Update events period method to check for invalid values #14087
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
0xc0170
previously requested changes
Jan 5, 2021
Contributor
|
Please also rebase to the latest master to fix Travis |
evedon
previously requested changes
Jan 6, 2021
mergify
bot
dismissed stale reviews from 0xc0170 and evedon
Pull request has been modified.
The period method currently allows any ms value positive, negative and zero. A negative value means dispatch a non periodic event ie just once. 0 is unspecified behaviour. This commit forces the user to use either positive periods or a new constant non_periodic and should any other value be provided will default to non_periodic. A Greentea test case is also provided to check this works as expected.
non_periodic constant has been moved out of the Event class and made static within the events namespace so that it is available both internally within the class and externally. The MBED_ASSERT has been changed to MBED_WARNING. The greentea test has been updated: 1) timings reduced to make the test cases run faster 2) The call handler simplified
Contributor
Author
|
Hopefully rebased for travis fix also. |
Contributor
Author
Contributor
Author
|
Test results: |
Contributor
Author
|
@0xc0170 why does travis keep failing ? I rebased master :( |
evedon
reviewed
Jan 8, 2021
Contributor
evedon
left a comment
evedon
left a comment
There was a problem hiding this comment.
Two minor comments. The rest looks fine.
(Also minor whitespace correction)
Contributor
Author
|
Ci started |
evedon
previously approved these changes
Jan 8, 2021
Contributor
Author
|
@0xc0170 I can't see why astyle isn't happy? Any idea ? |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Contributor
Author
|
@0xc0170 this was already approved and has passed CI (the whitespace push caused reviews to be dismissed). Just needs maintainer approval and merging now.. |
0xc0170
approved these changes
Jan 15, 2021
0xc0170
previously requested changes
Jan 15, 2021
Contributor
0xc0170
left a comment
0xc0170
left a comment
There was a problem hiding this comment.
events/include/events/Event.h 100644 → 100755 - please revert the file attributes change
Contributor
Author
What file attribute changes ? And why would any of those changes need reverting they compile and run perfectly ? They're also fixing things that are currently wrong ? |
This reverts commit c9a3375.
Contributor
|
here, it means that the files were marked as executable. This post can help: https://stackoverflow.com/questions/1257592/how-do-i-remove-files-saying-old-mode-100755-new-mode-100644-from-unstaged-cha?noredirect=1&lq=1 |
0xc0170
approved these changes
Jan 18, 2021
Contributor
|
CI restarted |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mergify
bot
added
release version missing
|
This PR does not contain release version label after merging. |
adbridge
added
release-type: feature
and removed
release version missing
mbedmain
added
release-version: 6.7.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
6 participants
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
The period method currently allows any ms value:
positive, negative and zero.
A negative value means dispatch a non periodic event. ie just once.
0 is unspecified behaviour.
This commit forces the user to use either positive periods or a new constant, non_periodic
and should any other value be provided will default to non_periodic.
A Greentea test case is also provided to check this works as
expected.
I'm classifying this as a feature change rather than major because allowing a zero value was actually a bug.
Impact of changes
If the user provides a 0 value this will now behave differently. Previously it would have run for a period of
time as if this was an indefinite event before eventually (with a timer wrap) have unspecified behaviour.
Now the event period will default instead to non periodic.
To specify a non periodic event a user could previously use any negative value. Whilst this will still work (due a default
being set on invalid values), the user is encouraged to use the new constant, 'non_periodic' to show that this is what was
intended.
Migration actions required
Use periods that are > 0ms or non_periodic constant.
Documentation
Should be picked up automatically in the doxygen
Pull request type
Test results
Reviewers