WiFi fpm sleep type checks in mode()

mcspr commented 3 years ago

Basic Infos

[x] This issue complies with the issue POLICY doc.
[x] I have read the documentation at readthedocs and the issue is not addressed there.
[x] I have tested that the issue is present in current master branch (aka latest git).
[x] I have searched the issue tracker for a similar issue.
[x] If there is a stack dump, I have decoded it.
[x] I have filled out all fields below.

Problem Description

After https://github.com/esp8266/Arduino/pull/7902

https://github.com/esp8266/Arduino/blob/1cc6960a5516afb8b244e87574a57986101247bb/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp#L429-L433

In case enableWiFiAtBootTime(); is used, wifi_fpm_get_sleep_type() != NONE_SLEEP_T is always true b/c SDK default is MODEM and it seems to be a setting for the wifi_fpm_open(), not the actual active mode check. Should it be set by the opposite __disableWiFiAtBootTime() to NONE since a lot of functions seem to skip WiFi.forceSleepBegin/Wake and directly use SDK API?

MCVE Sketch


#include <Arduino.h>
#include <ESP8266WiFi.h>

void setup() {
    Serial.begin(115200);
#if 0
    enableWifiAtBootTime();
#endif
    Serial.println(wifi_fpm_get_sleep_type());
}

void loop() {
}

Debug Messages

#if 0 & #if 1

dok-net commented 3 years ago

@mcspr I am addressing things like this in PR #7979. Help with review and testing is appreciated. @Tech-TX is extremely helpful with testing, the actual measurable effects of the API additions should be documented, please help with that.

dok-net commented 3 years ago

@mcspr Perhaps it's a misunderstanding as to what NONE really means in NONE_SLEEP_T:

Should it be set by the opposite __disableWiFiAtBootTime() to NONE

I understand that NONE means no sleep mode at all, not no Wifi.

You are right that the code is in violation of Espressif documentation:

This API can only be called when MODEM_SLEEP_T force sleep function is enabled

and should be fixed like so

--- a/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp
+++ b/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp
@@ -426,9 +426,10 @@ bool ESP8266WiFiGenericClass::mode(WiFiMode_t m, WiFiState* state) {
         return true;
     }

-    if (m != WIFI_OFF && wifi_fpm_get_sleep_type() != NONE_SLEEP_T) {
+    const sleep_type_t sleepType = wifi_fpm_get_sleep_type();
+    if (m != WIFI_OFF && sleepType != NONE_SLEEP_T) {
         // wifi starts asleep by default
-        wifi_fpm_do_wakeup();
+        if (sleepType == MODEM_SLEEP_T) wifi_fpm_do_wakeup();
         wifi_fpm_close();
     }

@@ -550,8 +551,9 @@ bool ESP8266WiFiGenericClass::forceSleepBegin(uint32 sleepUs) {
  * @return ok
  */
 bool ESP8266WiFiGenericClass::forceSleepWake() {
-    if (wifi_fpm_get_sleep_type() != NONE_SLEEP_T) {
-        wifi_fpm_do_wakeup();
+    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();
     }

@@ -789,8 +791,9 @@ bool ESP8266WiFiGenericClass::shutdown (uint32 sleepUs, WiFiState* state)

 bool ESP8266WiFiGenericClass::resumeFromShutdown (WiFiState* state)
 {
-    if (wifi_fpm_get_sleep_type() != NONE_SLEEP_T) {
-        wifi_fpm_do_wakeup();
+    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();
     }

dok-net commented 3 years ago

PR #7979 would gladly accept control of the sleep code at that level, and add a cached flag for auto vs. forced sleep modes. Currently without that, it unfortunately has no way of discovering which of auto or forced is in effect when the newly added sleep functions are called, which makes full switching back impossible and always results in auto sleep mode due to

wifi_fpm_close();

when turning off its various new sleep modes.

dok-net commented 3 years ago

Changes suggested above are now incorporated in #7979, plus ESPWiFiGeneric.cpp has been refactored to use the new ESP class-based sleep functions. @mcspr Please consider if you can close this issue?

esp8266 / Arduino