[BREAKING] wifi: remove pseudo-modes for shutdown, expose ::[resumeFrom]shutdown() by mcspr · Pull Request #7956 · esp8266/Arduino (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
coming from #7902 discussion:
#7902 (comment)
make shutdown() and resumeFromShutdown() public
removes extra code from the mode() related to shutdown state
include force sleep and shutdown method descriptions in the docs
make shutdown and resumeFromShutdown public removes extra code from the mode handler and include method description in the docs
The pseudo modes are removed, so if it has to be merged it must be soon.
I understand the need to make the methods public.
I don't really mind with the pseudo-modes I added that you removed, they were marked as are experimental.
Most important is the goal: store the sleep&wake functionalities in the core API so they can be improved there for everyone.
edit Title is updated with [BREAKING] because the pseudo modes appeared in v2.6.0
pseudo mode are still marked as experimental
but API has changed forceSleepBegin() => forceSleep(), not exactly sure why, and that is a breaking change.
wifi: remove pseudo-modes for shutdown [BREAKING] wifi: remove pseudo-modes for shutdown, expose ::[resumeFrom]shutdown()
bool shutdown(WiFiState* stateSave) { return shutdown(0, stateSave); }
Do you think this would simplify the clarity of the API and the examples ?
Maybe rotate the arguments?
bool shutdown(WiFiState* state = nullptr, uint32 time = 0);
Since the default arguments are already there
Anything that would make the API clearly readable at the first glance.
The same suggestion would arise anyway, if shutdown(nullptr, 500000) were to be used.
I'd then suggest
bool shutdown(uint32_t us) { return shutdown(nullptr, us); }
And I'd also suggest to rename ::shutdown() to ::shutdown_us() in both case.
btw, are we still operating under the assumption mode(WIFI_OFF) will turn off the modem in some patch before v3?
what is the point of shutdown_us() when there's forceSleepBegin() doing... the same thing?
Considering that, is there a point in default args and pointers then? Should it be
bool mode(WiFiMode_t opmode); bool forceSleepBegin(uint32_t time); bool forceSleepWake(); bool shutdown(WiFiState& state); bool shutdown(WiFiState& state, uint32_t whatForceSleepBeginGets);
?
State is not optional, time is
are we still operating under the assumption mode(WIFI_OFF) will turn off the modem in some patch before v3?
I'm not sure. OFF is not the same state as modem-off and we still want to have the API for this mode right ?
what is the point of shutdown_us() when there's forceSleepBegin() doing... the same no?
shutdown() was private. Now it is exposed, let the API be coherent by unifying names. We can either
- call it
forceSleepBegin()(keep current exposed name) and add optional state parameter - remove default value in the first parameter
and in all case, I think it's good to add the time unit where it is needed, and allow to use the function with only one of the two parameters (time or state).
... forceSleepBegin() - functions ending in Begin() are reminiscent of the asynchronous programming model and I would like to ask such naming to be reserved to that, on any platform, please.
I don't really have a better argument than 'why not' :) There's no use for the modem, so might as well put it to sleep? Plus, WIFI_OFF is not the same as the WIFI_OFF on boot, if the neighboring PR is merged it's sleeping unlike what happens after user does mode(WIFI_AP) and then mode(WIFI_OFF).
(btw also just noticed the wifi_fpm_auto_sleep_set_in_null_mode)
And I still don't really get how the time plays out with the shutdown(). fpm wake up callback is never set, thus how do we know when to restore the state if time is not infinite? Should there be a callback arg then as well? Or is it expected to check for the sleep? (...but there's no get sleep state method?) fpm callback only works with LIGHT_SLEEP_T, nvm
So I guess this is the desired model / naming:
bool mode(WiFiMode_t); bool forceSleep(uint32_t uSec = 0); // 0 aka indefinitely aka 0xFFFFFFF bool forceWake(); bool shutdown(WiFiState& state, uint32_t uSec = 0); bool resumeFromShutdown(WiFiState& state);
prefer to have some specific function, don't simply chain into sleep update examples and docs
@mcspr I've submitted PR #7979 that adds the part of sleep functionality that is not directly associated with WiFi connection setup and restore to the ESP class, among a few other things. I believe our code for that part is mostly identical.
I would like to propose that the forceSleep() and forceSleepWake() in the WiFi class, as being implemented by this PR, make use of that new API. I feel that greater care needs to be taken to align with the language used in Espressif documentation, otherwise it adds to the confusion surrounding the various power efficiency modes. My hunch is that there is no such thing as a "forced sleep" without mentioning modem or light - where modem is actually a lighter sleep than "light" :-)
shutdown and resumeFromShutdown are not only useful for deep sleep, but also for forced light sleep, as opposed to modem sleep modes.
@mcspr For it all to make sense, I've completed the sleep support in PR #7979, ESP class, reviewed this PR and your issue #7975 such that I have not commited any regressions in my work, and would kindly like to ask you to review my PR and consider rebasing your work here on #7979. Any input is highly welcome by me. Thanks!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is cleaner, thanks !
(side note: the one-thing-to-learn-everyday was strnlen() for me)
