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 mono delay #843

Merged
merged 36 commits into from
Dec 1, 2023
Merged

Fix mono delay #843

merged 36 commits into from
Dec 1, 2023

Conversation

Finomnis
Copy link
Contributor

@Finomnis Finomnis commented Nov 27, 2023

Fixes #844.

Solution

  • Compensate for timer uncertainty by always waiting for one extra timer tick.
  • Replace all .micros() with .micros_at_least() and all .millis() with .millis_at_least().

Open questions / Problems

  • This is technically a breaking change for rtic-time, and indirectly therefore also for rtic-monotonics and rtic itself. This would require all of those to be updated to a new major version.
    • Answer: It's a soundness issue, so it is fine to release it as a patch.
  • The previous versions technically did not comply with embedded-hal's definitions of DelayX. So should we yank those?
    • Answer: Yes. Let's yank previous versions.

@burrbull
Copy link
Contributor

@Finomnis
Copy link
Contributor Author

Finomnis commented Nov 27, 2023

https://docs.rs/fugit/latest/fugit/trait.ExtU64Ceil.html

That doesn't fix the problem.

  • 1ms tick rate, clock is at 0.99ms
  • "900.micros_at_least()" -> 1ms delay scheduled
  • actual wait time 0.01ms, although at_least() was used

To fulfill the requirement, both is actually needed.

With our fix, but without at_least():

  • 1ms tick rate, clock is at 0.99ms
  • "1200.micros()" - rounds to 1ms delay
  • still only 0.01ms waited, although even with rounding, everyone would expect at least 1ms

@Finomnis Finomnis marked this pull request as draft November 27, 2023 09:43
@Finomnis Finomnis marked this pull request as ready for review November 27, 2023 10:07
@Finomnis Finomnis marked this pull request as draft November 27, 2023 10:49
@Finomnis
Copy link
Contributor Author

Delays for blocking eh is still broken.

  • Implement both delay_us and delay_ms and add one timer tick to the wait.

@burrbull
Copy link
Contributor

Add more tests, please. Those cases which this PR fixes.

@Finomnis
Copy link
Contributor Author

Add more tests, please. Those cases which this PR fixes.

I can add some to rtic-time. I can't really add some to the monotonic impls, as this would require an external time measurement separate from the microcontroller.

Would that be good enough then?

@burrbull
Copy link
Contributor

Yes, please.

@korken89 korken89 marked this pull request as ready for review November 28, 2023 06:42
@Finomnis
Copy link
Contributor Author

Finomnis commented Nov 28, 2023

@korken89 still working on more tests. Need to rewrite the test to do a sub-tick test

@korken89
Copy link
Collaborator

Alright!

@korken89 korken89 added the skip-changelog Sometimes changes are not significant enough for a changelog entry label Nov 29, 2023
@Finomnis
Copy link
Contributor Author

Finomnis commented Nov 29, 2023

@burrbull I wasn't able to add tests to the blocking functions; those are hard to test reliably. But I now test all async functions. Is that enough for now?

EDIT: I added some tests for the blocking functions and they do work, but they are unreliable. So I disabled them by default.

Copy link
Collaborator

@korken89 korken89 left a comment

Choose a reason for hiding this comment

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

Thanks for the effort!

@korken89 korken89 added this pull request to the merge queue Dec 1, 2023
Merged via the queue into rtic-rs:master with commit 612a47e Dec 1, 2023
49 checks passed
@Finomnis Finomnis deleted the fix_mono_delay branch December 1, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Sometimes changes are not significant enough for a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monotonic delays are too short
3 participants