-
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
[BREAKING] wifi: remove pseudo-modes for shutdown, expose ::[resumeFrom]shutdown()
#7956
Conversation
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. edit |
::[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 bool shutdown(uint32_t us) { return shutdown(nullptr, us); } |
And I'd also suggest to rename |
btw, are we still operating under the assumption mode(WIFI_OFF) will turn off the modem in some patch before v3? 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 |
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 ?
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). |
... |
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).
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); |
@mcspr please go ahead ! |
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
|
@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! |
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.
The API is cleaner, thanks !
(side note: the one-thing-to-learn-everyday was strnlen()
for me)
coming from #7902 discussion:
#7902 (comment)
make
shutdown()
andresumeFromShutdown()
publicremoves extra code from the
mode()
related to shutdown stateinclude force sleep and shutdown method descriptions in the docs