periph/rtt: fix overflow in tick conversion macros #15431
Merged
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
haukepetersen
added
Type: bug
|
|
|
done, |
aabadie
reviewed
Nov 11, 2020
|
good catch, fixed. |
|
here are the results from the HiL Tests |
|
nucleo-f767zi, esp8266-esp-12x seem to have introduced problems when compared to master... If possible please verify. |
|
I will see on Monday which boards I have available at the office and check them out... |
|
I gave this one some testing: nucleo-f767zi: works like a charmesp32-wroom-32 (not esp8266 though): worksfrdm-kw41z (for the fun): worksarduino-mega2560: conversion works (but RTT is not supported anyway)Apply the following patch: diff --git a/boards/common/arduino-atmega/Makefile.features b/boards/common/arduino-atmega/Makefile.features
index 3bc938d8f6..6c234e4bd1 100644
--- a/boards/common/arduino-atmega/Makefile.features
+++ b/boards/common/arduino-atmega/Makefile.features
@@ -2,6 +2,7 @@
FEATURES_PROVIDED += periph_adc
FEATURES_PROVIDED += periph_i2c
FEATURES_PROVIDED += periph_pwm
+FEATURES_PROVIDED += periph_rtt
FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_timer
FEATURES_PROVIDED += periph_uartI could verify that the conversion are also working on 8 bits (RTT doesn't work on arduino-mega2560 but we can already check the conversion): I think we are good with this PR. Please squash! |
haukepetersen
force-pushed
the
fix_periphrtt_tickconversion
branch
from
af03668
to
56ee87e
Compare
|
Squashed - many thanks for the verification @aabadie! @MrKevinWeiss as @aabadie has verified this PR to be working on some of the boards in question, could there any other issue with this PR that causes the HIL to fail? Seems like we should sort this out before merging this fix, right?! |
aabadie
added
CI: ready for build
aabadie
reviewed
Nov 16, 2020
haukepetersen
force-pushed
the
fix_periphrtt_tickconversion
branch
from
56ee87e
to
261d63f
Compare
|
@aabadie fixed. |
haukepetersen
added
CI: ready for build
benpicco
added
CI: ready for build
haukepetersen
added
CI: ready for build
aabadie
added
CI: ready for build
aabadie
approved these changes
Nov 17, 2020
There was a problem hiding this comment.
All good to me. I think the problem raised by @MrKevinWeiss on nucleo-f767zi/esp8266 are false positives (maybe a setup issue ?).
ACK
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.
Contribution description
The tick conversion macros provided by the
periph/rttdriver are subject to overflow for large values. E.g. when doing something likethe resulting tick value will be wrong. Reason is this line:
-> the value of
ticks * 10000000 / RTT_FREQUENCYis larger thenUINT32_MAX, and therefore overflows. And as this macro is used in aRTT_TICKS_TO_MIN() -> RTT_TICKS_TO_SEC() -> RTT_TICKS_TO_MS() -> RTT_TICKS_TO_US()cascade, all resulting values will be flawed.The same goes for the time-to-tick conversion the other way around...
This PR fixes this by omitting the explicit cast to
uint32_tin both directions. Now when assiginguint32_t foo = RTT_TICKS_TO_US()with a (static) value larger thenUINT32_MAXthe compiler will shout at you :-)Also added some basic tests for the tick conversion to the test application.
Testing procedure
Run the included version of the test application on any platform of your choice, it should fail. Run it with the fix included in this PR -> it should run successfully..
Issues/PRs references
none