Misc. timing test fixes #19596
Closed
Misc. timing test fixes #19596
Conversation
This file contains 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
zephyrbot
added
area: Tests
andrewboie
approved these changes
Oct 3, 2019
pabigot
reviewed
Oct 3, 2019
pizi-nordic
reviewed
Oct 4, 2019
added 3 commits
October 4, 2019 09:50
These tests had what appeared to be a hard-coded one tick slop added to the timeout for a fifo to handle the addition of a full tick (vs. proper rounding) in k_sleep() (which is a separate bug). But the timeout side has been tightened with the recent change and doesn't wait as long as an equivalent sleep anymore (which is a feature), so the sides don't match. And it's wrong anyway, as there are two sleeps being accounted for (the fifo timeout and the sleep in the other thread doing the put), both of which can misalign. Do this with the proper tick conversion API, and use two ticks, not one. Signed-off-by: Andy Ross <[email protected]>
In the "POLL" implementation of k_queue_get(), if it ever happened that the elapsed time exactly equalled the time we were supposed to wait, the k_poll() call would be called (correctly) with a timeout of zero, and return synchronously. But the "done" flag would not be set because of an off by one error, so we would have to spin for the next tick needlessly. Even worse: on native_posix, nothing in this loop returns control to the main loop so in fact there we spin forever! (Which is how this got discovered.) Signed-off-by: Andy Ross <[email protected]>
This tests hard codes a 100 Hz tick rate, but it sets a k_timer for a DURATION value of 5ms, which cannot be done with a precision, then checks it against an unrelated k_busy_wait() time of exactly 10ms, which is right at the boundary of what is possible. Playing with timer stuff can push this over the edge easily. Correct DURATION to reflect "one tick", and spin for that value, plus exactly one tick of slop before testing to see whether the timer ISR ran. Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
timer-test-fixes
branch
from
5ad7918 to
2ab9855
Compare
|
Removed the mis-merged fix that had already been rediscovered and merged via @vanti |
nashif
approved these changes
Oct 4, 2019
|
@andyross should this be revived? |
|
The queue one should be cleaned up and merged for sure; it's a rare but real hard spin condition. The other two are less obviously needed. I think they're probably coded wrong, but the final version of the timeout patch matched the per-cycle behavior of the original code a little better than the first version did, so they aren't needed. |
github-actions
bot
added
has-conflicts
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.
More evacuees from #17155 that can stand on their own. These aren't related to the timeout work per se, except insofar as the bugs are sensitivities to timing. See commit notes for details.
Note that two of the patches were written above the new conversion API, so they require #19591 to be merged before they will build in CI.