Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integer saturation in Timer::start #342

Closed
apatrushev opened this issue Sep 4, 2023 · 0 comments · Fixed by #356
Closed

Integer saturation in Timer::start #342

apatrushev opened this issue Sep 4, 2023 · 0 comments · Fixed by #356

Comments

@apatrushev
Copy link
Contributor

apatrushev commented Sep 4, 2023

The following code snippets should be equivalent in result, but they are not:

let mut timer = Timer::new(cx.device.TIM2, clocks, &mut rcc.apb1);
timer.enable_interrupt(Event::Update);
timer.start(1.milliseconds());
let mut timer = Timer::new(cx.device.TIM2, clocks, &mut rcc.apb1);
timer.enable_interrupt(Event::Update);
timer.start(1000.microseconds());

This happens due to the integer saturation in following line:

let ticks = clock.integer().saturating_mul(timeout.integer()) * *timeout.scaling_factor();

Yes, the simplest answer would be "don't do that!", but this also happens with legitimate examples like 100.microseconds().

Perhaps one of the following fixes can be applied:

  • ticks can be extended to u64 (more precise result)
  • change the multiplication order as follows (better performance and code size?)
let ticks = ((clock.integer() * *timeout.scaling_factor()) as u32).saturating_mul(timeout.integer());
@apatrushev apatrushev changed the title Integer saturation in Integer saturation in Timer::start Sep 4, 2023
Sh3Rm4n added a commit that referenced this issue Nov 28, 2023
Supersedes #343
Fixes #342

Co-authored-by: Anton Patrushev <apatrushev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant