-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Extending on the new WiFi-less startup and new power saving APIs in ESP class #7979
base: master
Are you sure you want to change the base?
Conversation
5732573
to
ab8a20e
Compare
1f08089
to
68875bf
Compare
I've been doing some testing for Dirk for a week or two, and have re-verified the correct operation of this last commit 637f5d6. The timers are properly re-attached after Forced Light Sleep, which I never could get to. Thanks, Dirk! The three examples function as expected, and all power saving modes hit the right current levels. No surprises, no warts that I've seen. For people using servos I'd stop them before Forced Light Sleep, else they may slam the stops when PWM halts. You can restart them afterwards. In order to maintain WiFi across Forced Light Sleep you must sleep the modem cleanly before ESP.forcedLightSleepBegin(). I tried it the dirty way and WiFi wouldn't recover after Sleep. I need to revisit #6989 and clean that demo program up with the new API calls. When merged this closes #7055 I didn't review all of the code, merely tested the functionality very thoroughly. |
Utterly side comment, not required for this PR: I think I just figured out how the NodeMCU folks implemented something like 'wake from Forced Light Sleep with a CHANGE level interrupt', not just the HI LEVEL and LO LEVEL specified in the API Reference. If you read the pin level of the interrupt pin just before going to Forced Light Sleep and set the interrupt to the opposite level, it accomplishes the same thing. Going in with a HIGH on the pin, set the interrupt to LOW and vice versa. Then any change during Forced Light Sleep wakes it, and the user doesn't care what state the pin had been in immediately prior to Sleep; any change wakes it up. Since the interrupt attachment is done outside of this API wrapper, the end user would have to do this in their code. Due to the delay() needed to put it to Sleep, there's a small but nonzero chance that the pin might change after you read it, and it wouldn't come out of Sleep at all without a Reset as the Sleep logic is all mangled. |
e8cab42
to
5a73c73
Compare
2a34cbd
to
336c1c9
Compare
…ck-pressed button.
…nter when it is no longer needed.
… user_rf_pre_init Since WiFi is disabled at boot by default, and the above change to ESP.deepSleep..., (sample) sketches for special RF modes on resume from deep sleep may need fixing or documentation update. That's not done by this commit alone.
…ibs where feasible.
Reinstated |
void EspClass::forcedModemSleepOff() | ||
{ | ||
const sleep_type_t sleepType = wifi_fpm_get_sleep_type(); | ||
if (sleepType != NONE_SLEEP_T) { | ||
if (sleepType == MODEM_SLEEP_T) wifi_fpm_do_wakeup(); | ||
wifi_fpm_close(); | ||
} | ||
wifi_fpm_set_sleep_type(saved_sleep_type); | ||
saved_sleep_type = NONE_SLEEP_T; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine with begin() as a single operation?
If begin() called once and then off(), sort-of works (if fpm was not enabled externally)
If begin() called once and then again, sleep mode gets overwritten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any calls to the SDK or modifying sleep modes by sidelining this API is undefined behavior. Simple as that.
The saved_sleep_type
, if that is what you are focused on, is rather a convenience than a full-blown requirement for this to be useful. I don't see any real malfunction if its used the way you described.
cores/esp8266/Esp.cpp
Outdated
#ifdef HAVE_ESP_SUSPEND | ||
esp_delay(10); | ||
#else | ||
delay(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we waiting for certain condition to happen? Should it check something instead of assuming 10ms delay does something?
also not necessary ifdef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your opinion may differ, even for a reason, but I implement to the public specification and as my comment // SDK turns on forced modem sleep in idle task
explains, there shall be a delay for the idle task to take over.
Thanks for picking up the quite dated HAVE_ESP_SUSPEND, it's been so long, I don't even recall where and when it was ever defined.
wifi_fpm_set_sleep_type(MODEM_SLEEP_T); | ||
wifi_fpm_open(); | ||
saved_wakeupCb = nullptr; | ||
if (wakeupCb) wifi_fpm_set_wakeup_cb(wakeupCb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fpm_wakeup_cb_func will be called after system wakes up only if the force sleep
time out (wifi_fpm_do_sleep and the parameter is not 0xFFFFFFF).
fpm_wakeup_cb_func will not be called if wake-up is caused by
wifi_fpm_do_wakeup from MODEM_SLEEP_T type force sleep.
Also, perhaps, not necessary to expose to user? Isn't return signal enough of a result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no hurt in exposing it as an option, let's keep it this way, please.
#endif | ||
{ | ||
esp8266::InterruptLock lock; | ||
saved_timer_list = timer_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-//- as modem - if we hide timer list, it should not be possible for user to overwrite this accidentally. Single operation instead of separate begin() -> end()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, it worked/works, other ways, it didn't work/does work, i.e. doesn't enter the power saving modes properly. It was all carefully measured by the other guy at the time. Whatever happened to him :-(
Single operation instead of separate begin() -> end()?
I don't understand that sentence, I am sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, it worked/works, other ways, it didn't work/does work, i.e. doesn't enter the power saving modes properly. It was all carefully measured by the other guy at the time. Whatever happened to him :-(
Sadly, yeah, might be difficult to test at this point from our baseline of knowledge :/
As I understand it, we have issue where SDK processes timer list. So, that is what we try to avoid in code. If there is an ets_post to a task, does it still work? Are timers waking up early, or SDK never sleeps / idles to begin with?
I don't understand that sentence, I am sorry.
Something like InterruptLock. Imagine FpmThingy class that enables sleep on begin or construction, and disables on destruction. It may or may not be applicable to our internal usage, but seems like a better API for user side of things. Plus, it would not be necessary to expose too much methods to control it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcspr I have added an RAII class and modified libraries\esp8266\examples\ForcedLightSleep\ForcedLightSleep.ino
. Please review that and let me know if that matches to what you had in mind. Thanks for the suggestion!
#endif | ||
{ | ||
esp8266::InterruptLock lock; | ||
saved_timer_list = timer_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating the q from above - what is the difference between GPIO-activated sleep and timed one here? Is CPU really sleeping or we just simulate that it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG, you are asking questions that I cannot remember the answer to, except that I am 100% certain that we literally did hours of repetitive tests to show that power consumption dropped to the expected levels, and it didn't or the MCU became unreliable or the tables got corrupted if it wasn't done in exactly this - documented by Espressif - way. We really should not change a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but now we have a half-understood feature that maybe works?
Reading our issues simply repeats the above - we copy nodemcu and test how it did. I'd have to read that yet again if you don't have link or an explanation :/
(nodemcu/nodemcu-firmware#1231 (comment) and below)
edit: link oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we are not talking past each other. The sleep isn't actived by the GPIO, but cancelled. Same for the timer. I'm sure that's what you meant. As far as it got measure two years ago, yes we were sure it did as advertised - sleeping to save power, resuming either by GPIO trigger - push of a button - or timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcspr Is this now stalled again due to to unrelated workload, or have my changes and explanations been unconvincing so far? I would be totally relieved if this finally got merged, I am sure it will help people a lot, at least it might allow more specific reports about power-saving mode issues?
Implementing in ESP class the sleep modes that are pertinent with and without WiFi:
delay()
delay()
NONE_SLEEP_T
)For sketches without WiFi, additional RF powersaving mode support on power up and deep-sleep resume. Could avoid power use spikes and delays during startup.
WiFi default is "off" now, don't force sleep in
setup()
of examples that do use WiFi.Examples for the new modes.
Fixes #7055 - thanks to @Tech-TX for pointing this out.