Skip to content

Followup for adding support for deep and light sleep #555

Open
fa993 wants to merge 31 commits intoesp-rs:masterfrom
fa993:master
Open

Followup for adding support for deep and light sleep #555
fa993 wants to merge 31 commits intoesp-rs:masterfrom
fa993:master

Conversation

@fa993
Copy link

@fa993 fa993 commented Nov 8, 2025

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 fmt command to ensure that all changed code is formatted correctly.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

Please provide a clear and concise description of your changes, including the motivation behind these changes. The context is crucial for the reviewers.

Testing

Describe how you tested your changes.

@fa993
Copy link
Author

fa993 commented Nov 8, 2025

Changes made:

  • PinDriver no longer has a generic Pin param
  • The type of esp_sleep_enable_uart_wakeup is now an i32 (is this expected)?
  • the input type of gpio_wakeup_enable is i32, but all the pin functions return PinId type, which is u8
  • Removed RTCWakeupPinTrait and RTCWakeupPin struct, the functionality is directly implemented from - PinDriver. (Since the struct itself was just a wrapper).
  • Created a chain function on the RTCWakeupPins trait, so that we can start the chain call with pin_1.chain(pin_2) instead of EmptyRtcWakeupPins::chain(pin_1).chain(pin_2).
  • Changed examples which initialise pin driver to initialise it with state of Pull::Down

Changes not done:

  • Completely Removing RTCWakeupPins trait for unsupported cfg directives (bigger change, will do it after initial changes are cleared off).
  • GPIO subsystem modifications, once RTC approach is cleared off, will apply the same to it.

Changes that I would like to do:

  • Completely eliminate the EmptyRtcWakeupPins struct.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 9, 2025

If you want the CI to succeed, you might want to put

#![allow(unknown_lints)]
#![allow(unexpected_cfgs)]

at the top of your example.

And then look at the remaining Clippy errors.

But it would be good to fix the CI before requesting a review. I'll be able to review mid/late next week (on travel ATM).

@fa993
Copy link
Author

fa993 commented Nov 9, 2025

Yeah.. no I was wondering why the CI was failing myself, (the feature flags used weren't anything new and not present in the codebase), I'll fix the remaining errors and make sure CI is passing before the next review.

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

@fa993 I can merge that, but please fix all cfgs.

Lesser priority, but also look at those extra modules - what's the point?

pub use crate::rmt_legacy::*;
}

#[cfg(not(feature = "riscv-ulp-hal"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feature does not exist anymore, remove.

Copy link
Author

Choose a reason for hiding this comment

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

Is there any other equivalent feature or should this be enabled for all modules? What would the feature be replaced by?

#![allow(unknown_lints)]
#![allow(unexpected_cfgs)]

#[cfg(any(esp32, esp32s2, esp32s3, esp32c2, esp32c3))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful review of all your cfgs is necessary.

Copy link
Collaborator

@ivmarkov ivmarkov Feb 7, 2026

Choose a reason for hiding this comment

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

Also and in general - your examples have way too many cfgs. Look at mine as a comparison. Please simplify. This is difficult to support and reason about.

@fa993
Copy link
Author

fa993 commented Feb 14, 2026

@ivmarkov

  1. I went through your PR and replaced all the earlier erroneous cfgs with yours, they were mostly 1-1 replacements.
  2. Additionally, I have removed the outer source module, which was as you pointed.. not particularly useful, the other modules for every kind of wakeup add some sort of separation since all of them expose the same configure function, we could also make them configure_ and solve that, your call.
  3. I have also converted the sleep structs to having a private unit field.

I have addressed the cfg review comments, I have a question on one of the review comments around the non-existing feature,

Let me know if anything else needs to be done. Additionally, thank you for your patience.

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.

3 participants

Comments