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

Remove unnecessary delay #1794

Merged
merged 4 commits into from
Jul 15, 2024
Merged

Conversation

SergioGasquez
Copy link
Member

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

In ESP-IDF (using S3), the equivalent method is https://github.com/espressif/esp-idf/blob/5ca9f2a49aaabbfaf726da1cc3597c0edb3c4d37/components/esp_hw_support/port/esp32s3/rtc_sleep.c#L174 which is then called in https://github.com/espressif/esp-idf/blob/5ca9f2a49aaabbfaf726da1cc3597c0edb3c4d37/components/esp_hw_support/sleep_modes.c#L926.

There is no delay in the ESP-IDF source code, so I think we could delete it.

Testing

Not tested yet

@SergioGasquez SergioGasquez linked an issue Jul 15, 2024 that may be closed by this pull request
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.

LGTM - I think originally that delay was to make it likely that the UART is fully flushed before sleeping - but the user can do that (which the examples show)

Copy link
Member

@MabezDev MabezDev 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 looking into this, LGTM!

@MabezDev MabezDev added this pull request to the merge queue Jul 15, 2024
@SergioGasquez SergioGasquez removed this pull request from the merge queue due to a manual request Jul 15, 2024
Copy link
Contributor

@playfulFence playfulFence left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
Just a small one, it appears in the updated examples.

image

@SergioGasquez
Copy link
Member Author

LGTM, thanks! Just a small one, it appears in the updated examples.

image

Good catch! Fixed them

Copy link
Contributor

@playfulFence playfulFence left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@jessebraham jessebraham enabled auto-merge July 15, 2024 13:03
@jessebraham jessebraham added this pull request to the merge queue Jul 15, 2024
Merged via the queue into esp-rs:main with commit 7ea471c Jul 15, 2024
31 checks passed
@SergioGasquez SergioGasquez deleted the fix/remove-delay branch July 15, 2024 13:59
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.

Possibly redundant delay in Rtc::sleep
5 participants