Skip to content

Fix thread::sleep Duration-handling for ESP-IDF #129232

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

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Conversation

ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented Aug 18, 2024

Addresses the ESP-IDF specific aspect of #129212

A short summary of the problems addressed by this PR:

================================================

  1. Problem 1 - the current implementation of std::thread::sleep does not properly round up the passed Duration

As per the documentation of std::thread::sleep, the implementation should sleep at least for the provided duration, but not less. Since the minimum supported resolution of the usleep syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will not sleep at all.

This is addressed by this PR.

  1. Problem 2 - the implementation of usleep on the ESP-IDF can overflow if the passed number of microseconds is >= u32::MAX - 1_000_000

This is also addressed by this PR.

Extra details for Problem 2:

u32::MAX - 1_000_000 is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within STD. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo1, I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling usleep repeatedly on the ESP-IDF in chunks of u32::MAX - 1_000_000us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the usleep POSIX spec. On the other hand, that might introduce less precision (as we need to call more times usleep in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls usleep on the ESP IDF with bigger durations.

Footnotes

  1. https://github.com/espressif/esp-idf/issues/14390

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 18, 2024
@ivmarkov
Copy link
Contributor Author

r? @workingjubilee

@rustbot rustbot assigned workingjubilee and unassigned tgross35 Aug 18, 2024
@ivmarkov ivmarkov marked this pull request as ready for review August 19, 2024 13:04
@ivmarkov
Copy link
Contributor Author

@workingjubilee Sorry for the ping - this is ready for review now.

@workingjubilee
Copy link
Member

workingjubilee commented Aug 21, 2024

Can the PR note be more descriptive? We spent several minutes in the recent T-libs-api evaluation of the process of something going horribly wrong on discovering various forms of "yeah no one clicks through issue numbers to see the context" are true. While I hope this won't have anything like that be related to it, best not to get in the habit. It is better to copy my issue's entire description than leave it as a number.

@ivmarkov
Copy link
Contributor Author

Can the PR note be more descriptive? We spent several minutes in the recent T-libs-api evaluation of the process of something going horribly wrong on discovering various forms of "yeah no one clicks through issue numbers to see the context" are true. While I hope this won't have anything like that be related to it, best not to get in the habit. It is better to copy my issue's entire description than leave it as a number.

No prob! I've updated the PR description with all the details. Let me know if you want me to correct / provide extra details.

@workingjubilee workingjubilee changed the title Fix for issue #129212 for the ESP-IDF Fix thread::sleep Duration-handling for ESP-IDF Aug 21, 2024
@workingjubilee
Copy link
Member

Very thorough! Thank you.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 21, 2024

📌 Commit 1faccba has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#128727 (bump conflicting_repr_hints lint to be shown in dependencies)
 - rust-lang#129232 (Fix `thread::sleep` Duration-handling for ESP-IDF)
 - rust-lang#129321 (Change neutral element of <fNN as iter::Sum> to neg_zero)
 - rust-lang#129353 (llvm-wrapper: adapt for LLVM 20 API changes)
 - rust-lang#129363 (Force `LC_ALL=C` for all run-make tests)
 - rust-lang#129364 (safe transmute: gracefully bubble-up layout errors)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 94b3953 into rust-lang:master Aug 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
Rollup merge of rust-lang#129232 - ivmarkov:master, r=workingjubilee

Fix `thread::sleep` Duration-handling for ESP-IDF

Addresses the ESP-IDF specific aspect of rust-lang#129212

#### A short summary of the problems addressed by this PR:
================================================

1. **Problem 1** - the current implementation of `std::thread::sleep` does not properly round up the passed `Duration`

As per the documentation of `std::thread::sleep`, the implementation should sleep _at least_ for the provided duration, but not less. Since the minimum supported resolution of the `usleep` syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will _not_ sleep _at all_.

This is addressed by this PR.

2. **Problem 2** - the implementation of `usleep` on the ESP-IDF can overflow if the passed number of microseconds is >= `u32::MAX - 1_000_000`

This is also addressed by this PR.

Extra details for Problem 2:

`u32::MAX - 1_000_000` is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within `STD`. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo[^1], I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling `usleep` repeatedly on the ESP-IDF in chunks of `u32::MAX - 1_000_000`us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the `usleep` POSIX spec. On the other hand, that might introduce less precision (as we need to call more times `usleep` in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls `usleep` on the ESP IDF with bigger durations.

[^1]: espressif/esp-idf#14390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants