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

[BREAKING] wifi: remove pseudo-modes for shutdown, expose ::[resumeFrom]shutdown() #7956

Merged
merged 10 commits into from
May 15, 2021

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Apr 2, 2021

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
@d-a-v
Copy link
Collaborator

d-a-v commented Apr 3, 2021

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.

@d-a-v d-a-v changed the title wifi: remove pseudo-modes for shutdown [BREAKING] wifi: remove pseudo-modes for shutdown, expose ::[resumeFrom]shutdown() Apr 3, 2021
@d-a-v d-a-v added this to the 3.0.0 milestone Apr 3, 2021
doc/esp8266wifi/generic-class.rst Outdated Show resolved Hide resolved
@d-a-v
Copy link
Collaborator

d-a-v commented Apr 4, 2021

bool shutdown(WiFiState* stateSave) { return shutdown(0, stateSave); }

Do you think this would simplify the clarity of the API and the examples ?

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 4, 2021

Maybe rotate the arguments?

bool shutdown(WiFiState* state = nullptr, uint32 time = 0);

Since the default arguments are already there

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 5, 2021

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); }

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 5, 2021

And I'd also suggest to rename ::shutdown() to ::shutdown_us() in both case.

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 5, 2021

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

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 5, 2021

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).

@dok-net
Copy link
Contributor

dok-net commented Apr 5, 2021

... 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.

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 7, 2021

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);

@d-a-v d-a-v self-requested a review April 7, 2021 23:31
@d-a-v
Copy link
Collaborator

d-a-v commented Apr 9, 2021

@mcspr please go ahead !

@dok-net
Copy link
Contributor

dok-net commented Apr 15, 2021

@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.

@dok-net
Copy link
Contributor

dok-net commented Apr 16, 2021

@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!

Copy link
Collaborator

@d-a-v d-a-v left a 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)

@d-a-v d-a-v merged commit b0ece8c into esp8266:master May 15, 2021
@mcspr mcspr deleted the wifi-mode-as-opmode branch January 7, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants