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

add deepsleep module for battery application #4456

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

elanworld
Copy link

The SleepManager module for WLED is designed to manage power consumption by enabling the ESP32 to enter deep sleep based on idle time or battery voltage levels. This module allows for more efficient battery use by automatically putting the device to sleep and waking it up based on predefined conditions. It is especially useful for battery-powered setups where minimizing power consumption is essential.

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 4, 2025

thank you for contributing!
there are few things implemented in a too simple way though: you should not override any pins, use the pin manager or it will lead to conflicting pin configurations if a pin is used for something else.
There already is a battery usermod that can do all that and more, apart from deep-sleep and there is a deep-sleep usermod that can do the rest with the exception of touch-wake up. Did you try combining those two?

@softhack007 softhack007 added the usermod usermod related label Jan 4, 2025
@elanworld
Copy link
Author

Thank you for the feedback! I didn’t notice an existing deep-sleep module in the repository when I started working on this. My goal was to create a module specifically focused on managing sleep when the battery voltage is low, while adding multiple wake-up methods, including touch wake-up and preset wake-up. Additionally, I included configurable IO pin settings to achieve optimal power efficiency.

I’ll look into integrating these features with the existing deep-sleep module to ensure better compatibility and functionality for users. @DedeHai

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 7, 2025

the PR for the deep-sleep UM was pending for quite some time, I wanted to run some more long-term tests but finally I got it merged just about after you opened this PR.
The battery usermod allows to trigger a preset depending on battery voltage, the deep-sleep usermod also allows delays. so only thing missing is touch wake-up.
Edit:
btw: pullup/pulldown in deep-sleep can not be set the way you do it (and it probably is also still wrong in my UM) reason is: there is a "feature" in the IDF that automatically sets them.

@elanworld
Copy link
Author

elanworld commented Jan 7, 2025

The deep-sleep UM primarily uses a wakeup timer as its trigger source from deep-sleep status. Shouldn't this functionality be distinct from the battery UM?

Regarding pullup/pulldown configuration: it works well for some pins like 12, 13, 14, 15, and 27, but not reliably for others like 25 and 26. Is there any way we can override or influence this "feature" in the IDF?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 7, 2025

The deep-sleep UM primarily uses a wakeup timer as its trigger source from deep-sleep status.

how did you figure that?

Shouldn't this functionality be distinct from the battery UM?

deep sleep has nothing to do with battery UM, so dont know what you mean.

Regarding pullup/pulldown configuration: it works well for some pins like 12, 13, 14, 15, and 27, but not reliably for others like 25 and 26. Is there any way we can override or influence this "feature" in the IDF?

did you test to disable pullup/down successfully?
yes, I found a way around: you can override it by setting it to hold mode.

@elanworld
Copy link
Author

if (presetWake) {
nextWakeupMin = findNextTimerInterval() - 1; // wakeup before next preset
}
esp_sleep_enable_timer_wakeup(nextWakeupMin * 60ULL *
(uint64_t)1e6); // wakeup for preset

how did you figure that?

The battery usermod allows to trigger a preset depending on battery voltage, the deep-sleep usermod also allows delays.

Unlike the battery usermod, the deep-sleep usermod supports preset delays, highlighting their distinct functionalities."

you can override it by setting it to hold mode.

I found that hold mode(rtc_gpio_hold_en) performs reliably on IO25, but it doesn't seem to work consistently on IO26. Do you have any insights into why this happens?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 8, 2025

if (presetWake) { nextWakeupMin = findNextTimerInterval() - 1; // wakeup before next preset } esp_sleep_enable_timer_wakeup(nextWakeupMin * 60ULL * (uint64_t)1e6); // wakeup for preset

I thought you were referring to the existing deep-sleep UM, its a bit confusing both are called that :)
This is actually something I wanted to implement as well so appreciate the addition. However, I do not like at all how you added the code to the existing UM. You changed the whole structure, making a review almost impossible as almost all code is replaced.
Also you broke functionality for C3 and S2. There is debug info in the code: displaying wake up reason should be debug only, the user has no benefit from that.
Configuring GPIOs should be left as it is, the way you implement it is flawed.

Unlike the battery usermod, the deep-sleep usermod supports preset delays, highlighting their distinct functionalities."

I dont know what you mean by that.

I found that hold mode(rtc_gpio_hold_en) performs reliably on IO25, but it doesn't seem to work consistently on IO26. Do you have any insights into why this happens?

I do not unfortunately. you need to dig though the IDF code and the espressif forum as well as the datasheet to see if there is a hardware bug, IDF bug or if that is a feature.

So to sum up:

  • keep the existing code and only change/add whats needed instead of replacing all the code with new functions
  • the additions in the readme should say what it does, instead it just gives hints (I still dont know what you mean by that even after reading the code) - it should also say how it handles preset wake-up: what you mean is scheduler wake up I think. Also I saw no code for GPIO handling for the FETs that are mentioned, so not sure what that line is about.
  • leave GPIO handling to WLED, implement sleep GPIO handling as it was
  • for touch: add a config option to the GPIO, like it is done for pullup/down
  • test on S3, C3 and S2 as well, use ifdefs where needed

@elanworld
Copy link
Author

Add only the touch option and preset wakeup timer

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 12, 2025

  • still has that debug info
  • still does not work on C3 and ESP32

@elanworld
Copy link
Author

displaying wake up reason should be debug only,
still has that debug info

Are you referring to this line:
DEBUG_PRINTF("boot type: %s\n", phase_wakeup_reason());

still does not work on C3 and ESP32

I have successfully run this code on the ESP32. However, I cannot test it on the ESP32-C3 as I don’t have this board.
Could you clarify what exactly is not working on the ESP32?

@elanworld
Copy link
Author

still has that debug info

Thanks for your feedback. I forgot that this function is used in another part of my repository's main branch for the user info UI, and it is useful for other users. I will add this to the WLED repository now.

still does not work on C3 and ESP32

I found this code for esp32 c3
#define SOC_TOUCH_SENSOR_NUM (0) /*! No touch sensors on ESP32-C3 */

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 13, 2025

Thanks for your feedback. I forgot that this function is used in another part of my repository's main branch for the user info UI, and it is useful for other users. I will add this to the WLED repository now.

if you do: add a #define users wanting debug info can use to add it. the wake-up cause is of no use to normal users.

@elanworld
Copy link
Author

done

@DedeHai DedeHai marked this pull request as draft January 13, 2025 08:11
@elanworld elanworld marked this pull request as ready for review January 14, 2025 01:26
@elanworld elanworld requested a review from DedeHai January 14, 2025 01:31
@elanworld elanworld marked this pull request as draft January 14, 2025 01:32
@elanworld
Copy link
Author

What does draft status mean in this repository?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 14, 2025

draft means not finished and not ready to merge

@elanworld
Copy link
Author

what sleps are needed to complete the pull request?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 14, 2025

fix the ones I marked

@elanworld
Copy link
Author

What problems need to be fixed now? Do I need to fix them?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 15, 2025

What problems need to be fixed now? Do I need to fix them?

see inline comments.

@elanworld
Copy link
Author

After pushing the latest commit, I believe I have resolved all the issues you raised. If there's anything you're still not satisfied with, please let me know.

@elanworld elanworld marked this pull request as ready for review January 15, 2025 07:22
@DedeHai
Copy link
Collaborator

DedeHai commented Jan 15, 2025

there is plenty of unresolved comments
https://github.com/Aircoookie/WLED/pull/4456/files

@elanworld
Copy link
Author

image
I just noticed a marked conversation that I solved in this pull request files, but I can't see any comments. Could you provide a link or a screenshot? Thanks!

Or do you mean the conversations? I believe I have resolved all the issues you mentioned. If not, please let me know or show them to me again

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 15, 2025

image

@elanworld
Copy link
Author

image
Why can't I see any comments like the ones in your screenshot?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 15, 2025

I have no idea, maybe you disabled inline comments in github? that is beyond my knowledge I am afraid.

@softhack007
Copy link
Collaborator

softhack007 commented Jan 15, 2025

image

@DedeHai your comments are marked "pending" - it means you may need to finish your review for the comments to be published.


Screenshot_20250115-155324_Samsung Internet

usermods/deep_sleep/usermod_deep_sleep.h Outdated Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Outdated Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Outdated Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Outdated Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Outdated Show resolved Hide resolved
usermods/deep_sleep/usermod_deep_sleep.h Show resolved Hide resolved
@DedeHai
Copy link
Collaborator

DedeHai commented Jan 15, 2025

@softhack007 thanks, that may have been it. interesting, I never had to do that before...

@elanworld
Copy link
Author

Added a commit for ESP32-C3 and ESP32-S2 development boards

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 22, 2025

thanks for the update, I will look at it in more detail soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usermod usermod related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants