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

Fix overflow in timestamp calculation #287

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 14, 2023

This should fix #285 - there's no more multiplication, so there should be no more overflow, either. Still testing the before state though just to make sure.

The original time keeping didn't panic for the same multiplication, because:

  • The global incrementing compiled down to an addition with a constant (2AAA_AAAA I believe)
  • The cycle counter is 32 bits casted to u64, multiplying that with 40_000_000 results in a 64 bit number without overflow, which was then divided by 240_000_000 without problem
  • The results were then the sum of these two values

Why the previous code panicked:

  • The overflow counter and cycle counter were added before the multiplication.
  • The compiler didn't turn * 40_000_000 / 240_000_000 into a division by 6.
  • After a few cycle counter overflows (after the 108th I believe), the multiplication caused an arithmetic overflow.

@bugadani bugadani marked this pull request as draft October 14, 2023 16:32
@bugadani bugadani changed the title (maybe) Fix overflow in timestamp calculation Fix overflow in timestamp calculation Oct 14, 2023
@bugadani bugadani marked this pull request as ready for review October 14, 2023 17:21
@Dav1dde
Copy link

Dav1dde commented Oct 15, 2023

I've been running this for over an hour now and can't reproduce the panic.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't be able to fully test this during this week but given the code and the check by the original issue author this looks good to me

@MabezDev MabezDev merged commit d0a448b into esp-rs:main Oct 18, 2023
7 checks passed
@bugadani bugadani deleted the mult branch October 18, 2023 08:51
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 this pull request may close these issues.

esp32-s3 overflow panic
4 participants