re-resolve broken k_busy_wait on Nordic platforms #15707
Conversation
|
This isn't quite right; see further discussion in #11626 (comment). In any case I believe the first patch in this PR is valuable and should be merged, for the reasons given in the commit message. I personally consider the objective gross inaccuracies of busywait for short durations in #11626 to be far more worrisome than differences between CPU cycles and the system clock for 1 s delays where If you chose not to unrevert the custom busywait you might want to complete the reversion for nRF91 which still uses the custom one. |
|
@pizi-nordic or @anangl can you add a patch to this branch that completes the reversion for nRF91? |
|
@carlescufi nRF91 was never reverted. With this PR in this state all three targets are treated the same. (At least, by inspection they are: I don't have an nRF91 to test its behavior.) |
|
I've updated the commit message in the second commit (and its copy in the PR description) to correctly reflect the conclusions related to #14186. |
There was a problem hiding this comment.
Would it make sense to move this definition one level up, so we don't get to write it three times?
One can always overwrite it in project...
There was a problem hiding this comment.
Same question applies to this one (now that all nRF SOC have custom busy wait)
ioannisg
left a comment
ioannisg
left a comment
There was a problem hiding this comment.
Seems fine, I left a couple of comments/questions.
|
@ioannisg Your suggestion of refactoring Kconfig makes sense. Assuming @pizi-nordic and @anangl who have approved this in its current form agree in principle I agree that change is appropriate. Before spending more time on rework, though, I'd like clarification from @carlescufi whose last comment could be taken as direction to drop the patch restoring the cycle-based custom busywait to nrf5 and instead complete the reversion of the custom busywait for nRF91 that was missed because nRF91 didn't exist in Zephyr when the commit that was subsequently reverted was originally merged. |
The default system clock on all Nordic devices is based on a 32 KiHz (2^15 Hz) timer. Scheduling ticks requires that deadlines be specified with a timer counter that aligns to a system clock. With the Zephyr default 100 clocks-per-sec configuration this results in 100 ticks every 32700 ticks of the cycle timer. This reveals two problems: * The uptime clock misrepresents elapsed time because it runs 0.208% (68/32768) faster than the best available clock; * Calculation of timer counter compare values often requires an integer division and multiply operation to produce a value that's a multiple of clock-ticks-per-second. Integer division on the Cortex-M1 nRF51 is done in software with a (value-dependent) algorithm with a non-constant runtime that can be significant. This can produce missed Bluetooth deadlines as discussed in upstream zephyrproject-rtos#14577 and others. By changing the default divisor to one that evenly divides the 2^15 clock rate the time interrupts are disabled to manage timers is significantly reduced, as is the error between uptime and real time. Do this at the top level, moving SYS_CLOCK_HW_CYCLES_PER_SEC there as well since the two parameters are related. Note that the central_hr configuration described in upstream zephyrproject-rtos#13610 does not distinguish latency due to timer management from other irq_block/spinlock regions, and the maximum observed latency will still exceed the nominal 10 us allowed maximum. However this does occur much less frequently than changing the timer deadline which can happen multiple times per tick. Signed-off-by: Peter A. Bigot <[email protected]>
This reverts commit bd24b31. While the test case failure described in zephyrproject-rtos#14186 is associated with the cycle-based busy-wait implementation, that test is fragile, and fails less frequently once the incongruence between ticks-per-second and the 32 KiHz RTC clock are resolved. It also assumes that the system clock is more stable than the infrastructure underlying the the busy-wait implementation, which is not necessarily true. The gross inaccuracies in the standard busy-wait on Nordic described in issue zephyrproject-rtos#11626 justify restoring the custom solution. As this applies to all Nordic devices, move the setting to the top-level Kconfig.defconfig. See: zephyrproject-rtos#11626 (comment) Signed-off-by: Peter A. Bigot <[email protected]>
|
@carlescufi Moved all related configs to the top level, including the cycles-per-sec which should be co-located with the override for ticks-per-sec. Revised the commit messages including in the PR description. |
|
@pabigot thank you! |
5 participants
soc: arm: nordic_nrf: change default SYS_CLOCKS_PER_SEC
The default system clock on all Nordic devices is based on a 32 KiHz
(2^15 Hz) timer. Scheduling ticks requires that deadlines be specified
with a timer counter that aligns to a system clock. With the Zephyr
default 100 clocks-per-sec configuration this results in 100 ticks every
32700 ticks of the cycle timer. This reveals two problems:
The uptime clock misrepresents elapsed time because it runs 0.208%
(68/32768) faster than the best available clock;
Calculation of timer counter compare values often requires an integer
division and multiply operation to produce a value that's a multiple
of clock-ticks-per-second.
Integer division on the Cortex-M1 nRF51 is done in software with a
(value-dependent) algorithm with a non-constant runtime that can be
significant. This can produce missed Bluetooth deadlines as discussed
in upstream #14577 and others.
By changing the default divisor to one that evenly divides the 2^15
clock rate the time interrupts are disabled to manage timers is
significantly reduced, as is the error between uptime and real time. Do
this at the top level, moving SYS_CLOCK_HW_CYCLES_PER_SEC there as well
since the two parameters are related.
Note that the central_hr configuration described in upstream #13610 does
not distinguish latency due to timer management from other
irq_block/spinlock regions, and the maximum observed latency will still
exceed the nominal 10 us allowed maximum. However this does occur
much less frequently than changing the timer deadline which can happen
multiple times per tick.
Signed-off-by: Peter A. Bigot [email protected]
:100644 100644 00f3947f39 d0251bbc23 M soc/arm/nordic_nrf/nrf51/Kconfig.defconfig.series
:100644 100644 cff0b9fad9 a91e5efc3f M soc/arm/nordic_nrf/nrf52/Kconfig.defconfig.series
:100644 100644 d88da8aef2 68f636acc0 M soc/arm/nordic_nrf/nrf91/Kconfig.defconfig.series
soc: arm: nordic_nrf: unrevert provide custom busy_wait implementations
This reverts commit bd24b31.
While the test case failure described in #14186 is associated with the
cycle-based busy-wait implementation, that test is fragile, and fails
less frequently once the incongruence between ticks-per-second and the
32 KiHz RTC clock are resolved. It also assumes that the system clock
is more stable than the infrastructure underlying the the busy-wait
implementation, which is not necessarily true.
The gross inaccuracies in the standard busy-wait on Nordic described in
issue #11626 justify restoring the custom solution.
As this applies to all Nordic devices, move the setting to the top-level
Kconfig.defconfig.
See: #11626 (comment)
Signed-off-by: Peter A. Bigot [email protected]
:100644 100644 d0251bbc23 dd6ddbb486 M soc/arm/nordic_nrf/nrf51/Kconfig.defconfig.series
:100644 100644 eb235d1cf6 cb3021a748 M soc/arm/nordic_nrf/nrf51/soc.c
:100644 100644 a91e5efc3f 78a903d659 M soc/arm/nordic_nrf/nrf52/Kconfig.defconfig.series
:100644 100644 44e9edbdf8 eb1f979602 M soc/arm/nordic_nrf/nrf52/soc.c
NB: The reversion did not affect the use of the custom busywait in nrf91.
Closes #11626