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

Added ESP32 compatible methods for setting/getting sleep mode #7901

Merged
merged 4 commits into from
Mar 15, 2021

Conversation

drzony
Copy link
Contributor

@drzony drzony commented Mar 1, 2021

As discussed with @d-a-v

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.

Thanks

}
bool getSleep()
{
return getSleepMode() == WIFI_LIGHT_SLEEP;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSleepMode() != WIFI_NONE_SLEEP
(so LIGHT and MODEM are treated as sleep mode enabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but see my comment above. To maintain ESP32 compatibility I did it the same way that it's on ESP32:
https://github.com/espressif/arduino-esp32/blob/3fe7c2e8cdcc34af0f5d3c55f8b4cb79ef5318cf/libraries/WiFi/src/WiFiGeneric.cpp#L669

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other option would be to drop the bool version

Copy link
Collaborator

@d-a-v d-a-v Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the comment there is:

// @return true if modem sleep is enabled
bool WiFiGenericClass::getSleep()

The naming is confusing, because "light sleep" is also an enabled sleep mode while "none sleep" has sleep mode disabled.
What about replicating the comment in esp8266 .h file, insisting on that it reflects the esp32 API ?

One other option would be to drop the bool version

That would defeat the goal of this PR. We just need to not add it in esp8266 documentation.
esp32 libraries will compile, esp8266 .h readers will see the comment, esp8266 api is unchanged.

@d-a-v d-a-v self-requested a review March 1, 2021 13:49
Comment on lines 54 to 56
WIFI_PS_NONE = WIFI_NONE_SLEEP,
WIFI_PS_MIN_MODEM = WIFI_LIGHT_SLEEP,
WIFI_PS_MAX_MODEM = WIFI_MODEM_SLEEP,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right though? MODEM is default, hence it should be MIN? LIGHT is 'sleepier' mode afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment:
#7901 (comment)

Copy link
Collaborator

@mcspr mcspr Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs on ESP8266 it seems that WIFI_PS_MIN_MODEM == WIFI_LIGHT_SLEEP and WIFI_PS_NONE == WIFI_NONE_SLEEP

https://www.espressif.com/sites/default/files/documentation/2c-esp8266_non_os_sdk_api_reference_en.pdf ?
(still applies to the 2.2.x branch)

3.5.54. wifi_set_sleep_type

  • Default mode: Modem-sleep.
  • In order to lower the power comsumption, ESP8266 changes the TCP timer
    tick from 250ms to 3s in light-sleep mode, which leads to increased timeout for
    TCP timer. Therefore, the modem-sleep or deep-sleep mode should be used
    where there is a requirement for the accurancy of the TCP timer.

Plus:
https://github.com/espressif/arduino-esp32/blob/15bae92a72c102331e7eaf7dcb439e8ad6920184/tools/sdk/include/esp32/esp_wifi_types.h#L192-L196

Meaning, NONE - power-save features are disabled, MIN - minimum amount of power-saving features, but some are enabled, MAX - maximum power-save mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I don't understand the number 3 in @d-a-v comment. Why WIFI_MODEM_SLEEP is on enabled and WIFI_LIGHT_SLEEP is on disabled?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷
Following esp32 api, 1st choice is used there. ::setSleep(enum) is used to select the MAX sleep mode
(ref. espressif/arduino-esp32#3896 and espressif/arduino-esp32#3900)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong, sorry! I swapped MODEM and LIGHT sleep modes.
MODEM is indeed the default and is between NONE and LIGHT.

@d-a-v d-a-v added the alpha included in alpha release label Mar 3, 2021
@earlephilhower earlephilhower merged commit 48e1ccb into esp8266:master Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants