Fixups to timer_api test #24080
Merged
Fixups to timer_api test #24080
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
|
@dcpleung reported this on the microchip board. I haven't tested it there, and (oddly) it doesn't seem to happen on my nRF board which should have roughly the same properties. He should validate before merge. |
zephyrbot
added
area: Tests
andyross
force-pushed
the
busywait-vs-tick-fix
branch
from
621e976
to
8837b55
Compare
|
(FWIW: I just remembered that nRF are 1:1 with ticks:cycles, so they aren't subject to this problem. We should check, if the microchip config has a non-divisible ratio we should fix that because it can cause problems like this in app code.) |
|
While Nordic has ticks and cycles the same, the busywait uses a different clock. Further, on it an integral number of ticks is rarely an exact number of milliseconds. So this may also be a factor in #23106. |
pabigot
approved these changes
Apr 4, 2020
|
just ran this on a few boards: |
ioannisg
approved these changes
Apr 6, 2020
andyross
force-pushed
the
busywait-vs-tick-fix
branch
from
8837b55
to
9a7f902
Compare
andyross
changed the title
|
Add a patch to fix the abs_timeout test's conversion goof. |
added 2 commits
April 8, 2020 13:19
This test sets a timer, busy waits for half the duration, and then checks the remaining time is correct. And it correctly does all its math in tick precision and aligns to a timer interrupt to eliminate aliasing due to the tick stride. But it's waiting using k_busy_wait(), not a timer: "half the duration" in MICROSECONDS (for k_busy_wait()) is not necessarily representable as an integer number of TICKS on all platforms. Because k_busy_wait() always rounds up, we need one extra tick of buffer on those platforms. Signed-off-by: Andy Ross <[email protected]>
The test of the absolute timeout feature was a simple whitebox test that inspected the generated ticks field of different constructors for identity. But it wasn't simple enough, because it was doing a ticks->ms->ticks conversion (at compile time, sigh) on the input data, which is obviously lossy on platforms where ticks are shorter than milliseconds by non-integral factors. Fix to do the conversion in just one direction. Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
busywait-vs-tick-fix
branch
from
9a7f902
to
c64e7fe
Compare
|
Rebase to clear out that weird doc failure. |
|
@andyross , I see your change to add one tick to the |
|
Fixes #24072 |
andrewboie
approved these changes
Apr 14, 2020
|
@nashif does this still cause regressions, or is it now an improvement on the master branch with your testing rig? |
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.
This test sets a timer, busy waits for half the duration, and then
checks the remaining time is correct. And it correctly does all its
math in tick precision and aligns to a timer interrupt to eliminate
aliasing due to the tick stride.
But it's waiting using k_busy_wait(), not a timer: "half the duration"
in MICROSECONDS (for k_busy_wait()) is not necessarily representable
as an integer number of TICKS on all platforms. Because k_busy_wait()
always rounds up, we need one extra tick of buffer on those platforms.
Fixes #24001
Signed-off-by: Andy Ross [email protected]