tests: drivers: counter: Fix test for nrf51 #24374
Conversation
Fixes test which did not take into account that counter may wrap (nrf51 has 16bit timers) and was setting alarm to wrong value. Error was not seen on platforms with higher top value. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
|
|
||
| k_busy_wait(us/2 + i); | ||
|
|
||
| alarm_cfg.ticks = alarm_cfg.ticks + 2*alarm_cfg.ticks; |
There was a problem hiding this comment.
test was suppose to set some alarm in the future (it is cancelled anyway) and for non wrapping (within test) counters this formula was ok but it was actually wrong and initial intention was that it is 2*ticks (2 ms from now).
There was a problem hiding this comment.
and initial intention was that it is
2*ticks(2 ms from now).
The commit message would be a better place for such additional explanation.
zephyrbot
added
the
area: Tests
|
The test is still failing for me. https://gist.github.com/mbolivar-nordic/8410c20dda61345b67e806d340aa6c0c |
mbolivar-nordic
left a comment
mbolivar-nordic
left a comment
There was a problem hiding this comment.
-1 since this is not a fix in my testing
That test was spurious as it was failing only when alarm expired when counter wrapped. That was revealing a bug in the test - validation that alarm came not earlier than requested. Fixed. |
mbolivar-nordic
left a comment
mbolivar-nordic
left a comment
There was a problem hiding this comment.
Test is passing now so I'm happy, thanks! A couple of non-blocking style things.
There was a problem hiding this comment.
Why static? Just curious.
There was a problem hiding this comment.
puts variable in flash and not on stack.
There was a problem hiding this comment.
Are we really so close to blowing the stack on this simple test case? I'm not objecting to it, it just looked weird.
There was a problem hiding this comment.
Would be good to include processing_limit_us in the output here too, I think.
Test is checking if alarm handler is executed at request time or later. However, validation did not take into account wrapping of the counter. Fixed by taking into account case where counter wraps. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
nordic-krch
force-pushed
the
fix_counter_nrf51
branch
from
ad243f4 to
b767952
Compare
|
i'm getting counter test failures on |
|
@nordic-krch there seems to be a regression in this PR: https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/69627/3/console I have re-run CI but the failure re-appeared so this is not a false positive. |
|
@carlescufi yes, on CI is it consistent but on my setup it is not. This test is passing on my machine and changes do not touch failing test so i think it is also failing on master. |
|
created bug report for failing test: #24635 |
|
@carlescufi can you merge? We accidentally got green build. Root cause is known. #22904 should eventually fix it. |
Since introduction of zephyrproject-rtos#24374 this test fails on STM32 boards. Due to 1Hz frequency of RTC used, the 'diff' could be 0. But then 'counter_us_to_ticks(dev, processing_limit_us)' is also 0. We should allow the equality in the assert. Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
Since introduction of #24374 this test fails on STM32 boards. Due to 1Hz frequency of RTC used, the 'diff' could be 0. But then 'counter_us_to_ticks(dev, processing_limit_us)' is also 0. We should allow the equality in the assert. Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
Since introduction of zephyrproject-rtos#24374 this test fails on STM32 boards. Due to 1Hz frequency of RTC used, the 'diff' could be 0. But then 'counter_us_to_ticks(dev, processing_limit_us)' is also 0. We should allow the equality in the assert. Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
6 participants
Fixes test which did not take into account that counter may
wrap (nrf51 has 16bit timers) and was setting alarm to wrong
value. Error was not seen on platforms with higher top value.
Fixes #24369.
Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no