Make ticker computation use shift-by-0 #14532
Conversation
Runtime code that analysed clock frequency to determine numerator and denominator for conversion to standard 1MHz failed to handle the case of either being 1 correctly. Although it would spot other values that could be performed as shifts, it failed to spot that 1 is "shift by 0", so would end up doing runtime multiply and/or divide by 1. The runtime divide by 1 could be slow on a Cortex-M0 device, increasing interrupt latency. UART character loss on STM32F0 devices has been traced to this incorrect code. Correct the `exact_log2` routine so that `exact_log2(1)` returns 0 to fix this. Original code had a single special no-multiply-or-divide case for hardware clock frequency being exactly 1MHz, as USTICKER is on STM32F0 - this code lacks that but has a more general special case that covers all shift-convertible frequencies like 500kHz or 8MHz, which should be similar speed as shifts are cheap.
ciarmcom
added
the
release-type: patch
|
@kjbracey-arm, thank you for your changes. |
|
Nice find ! Shall this add a test case for this bugfix? |
Well, @jeromecoutant was reporting a failure in an existing test on one platform - see #12897, but that was rather indirect, We already have a bunch of stuff that checks that timers run at the right speed - that we get the correct answer, but those only run with custom-tickers true. So the gaps are "are runtime optimisations kicking in correctly" and "does compile-time-optimised code activate or work at all?". The runtime optimisations can be checked with a single test that creates a bunch of custom tickers with different frequencies and checks the Going beyond that, testing compile-time stuff is tougher. You'd need particular custom targets with particular frequencies to fully exercise it. At the minute the compile-time optimisations are not exercised by tests at all, except that we cross-check targets to ensure they have their defines set correctly (if they provide them - they're not currently required to). A start would be to have at least some Then to check if compile-time optimisation is kicking in as expected, I don't know - you'd want to check that certain members of |
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
CI restarted |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
removed
release-type: patch
Summary of changes
Runtime code that analysed clock frequency to determine numerator and denominator for conversion to standard 1MHz failed to handle the case of either being 1 correctly.
Although it would spot other values that could be performed as shifts, it failed to spot that 1 is "shift by 0", so would end up doing runtime multiply and/or divide by 1. The runtime divide by 1 could be slow on a Cortex-M0 device, increasing interrupt latency.
UART character loss on STM32F0 devices has been traced to this incorrect code.
Correct the
exact_log2routine so thatexact_log2(1)returns 0 to fix this.Original code had a single special no-multiply-or-divide case for hardware clock frequency being exactly 1MHz, as USTICKER is on STM32F0 - this code lacks that but has a more general special case that covers all shift-convertible frequencies like 500kHz or 8MHz, which should be similar speed as shifts are cheap.
Impact of changes
Fixes #14489.
Documentation
None
Pull request type
Test results
Reviewers
@SibertEnovates @jeromecoutant