From dfdcc25b50de07b40312431c6582ddbfb1dabe2e Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 9 Sep 2026 14:44:14 -0700 Subject: [PATCH] fix(wifi): one power-save mapping for CLI, startup and reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `set wifi.powersave min` stored 0 and applied WIFI_PS_MIN_MODEM, but the bridge's post-reconnect mapping read the same 0 as WIFI_PS_NONE. A node explicitly configured for minimum modem sleep therefore ran with power save off after its first reconnect, while `get wifi.powersave` still reported min. The stored default is already 1 (`none`), so nothing here changes the product default — it stops an operator's explicit choice from being reinterpreted. Move the value/name/mode table into WifiPowerSavePolicy (pure, host-tested) and use it from the CLI setter and getter, the web config snapshot and the bridge. The bridge now also applies the mode when it finds the STA already associated at start — that path has no connect transition to carry the setting, so a bridge restart used to leave whatever mode was set before. --- src/helpers/CommonCLI_Observer.cpp | 30 ++------ src/helpers/WifiPowerSavePolicy.h | 73 +++++++++++++++++++ src/helpers/bridges/MQTTBridge.cpp | 31 +++++--- src/helpers/bridges/MQTTBridge.h | 1 + src/helpers/esp32/WebConfigServer.cpp | 4 +- .../test_wifi_power_save_policy.cpp | 58 +++++++++++++++ 6 files changed, 163 insertions(+), 34 deletions(-) create mode 100644 src/helpers/WifiPowerSavePolicy.h create mode 100644 test/test_wifi_power_save_policy/test_wifi_power_save_policy.cpp diff --git a/src/helpers/CommonCLI_Observer.cpp b/src/helpers/CommonCLI_Observer.cpp index 9b2b2d30..dbb6e021 100644 --- a/src/helpers/CommonCLI_Observer.cpp +++ b/src/helpers/CommonCLI_Observer.cpp @@ -15,6 +15,7 @@ #include "TxtDataHelpers.h" #include "AlertReporter.h" // for alertReporterBannedChannelMatch[Hex]() #include "MQTTObserverValidation.h" // pure input validators (host-testable) +#include "WifiPowerSavePolicy.h" // one powersave value->mode/name mapping #include #include #ifdef ESP_PLATFORM @@ -438,41 +439,28 @@ bool CommonCLI::handleObserverSetCmd(uint32_t sender_timestamp, const char* conf if (!persistObserverPrefs(reply)) return true; strcpy(reply, "OK"); } else if (memcmp(config, "wifi.powersave ", 15) == 0) { - const char* value = &config[15]; uint8_t ps_value; - bool valid = false; - if (memcmp(value, "min", 3) == 0 && (value[3] == 0 || value[3] == ' ')) { - ps_value = 0; - valid = true; - } else if (memcmp(value, "none", 4) == 0 && (value[4] == 0 || value[4] == ' ')) { - ps_value = 1; - valid = true; - } else if (memcmp(value, "max", 3) == 0 && (value[3] == 0 || value[3] == ' ')) { - ps_value = 2; - valid = true; - } - if (!valid) { + if (!WifiPowerSavePolicy::parseName(&config[15], &ps_value)) { strcpy(reply, "Error: must be none, min, or max"); } else { _mqtt_prefs.wifi_power_save = ps_value; if (!persistObserverPrefs(reply)) return true; + const char* ps_name = WifiPowerSavePolicy::nameFor(ps_value); #ifdef ESP_PLATFORM if (WiFi.status() == WL_CONNECTED) { - wifi_ps_type_t ps_mode = (ps_value == 1) ? WIFI_PS_NONE : - (ps_value == 2) ? WIFI_PS_MAX_MODEM : WIFI_PS_MIN_MODEM; - esp_err_t ps_result = esp_wifi_set_ps(ps_mode); + // Same mapping the bridge applies on every association, so this cannot + // drift back apart (see WifiPowerSavePolicy). + esp_err_t ps_result = + esp_wifi_set_ps((wifi_ps_type_t)WifiPowerSavePolicy::modeFor(ps_value)); if (ps_result == ESP_OK) { - const char* ps_name = (ps_value == 1) ? "none" : (ps_value == 2) ? "max" : "min"; sprintf(reply, "OK - power save set to %s", ps_name); } else { sprintf(reply, "OK - saved, but failed to apply: %d", ps_result); } } else { - const char* ps_name = (ps_value == 1) ? "none" : (ps_value == 2) ? "max" : "min"; sprintf(reply, "OK - saved as %s (will apply on next WiFi connection)", ps_name); } #else - const char* ps_name = (ps_value == 1) ? "none" : (ps_value == 2) ? "max" : "min"; sprintf(reply, "OK - saved as %s", ps_name); #endif } @@ -1090,9 +1078,7 @@ bool CommonCLI::handleObserverGetCmd(uint32_t sender_timestamp, const char* conf #endif } } else if (memcmp(config, "wifi.powersave", 14) == 0) { - uint8_t ps = _mqtt_prefs.wifi_power_save; - const char* ps_name = (ps == 1) ? "none" : (ps == 2) ? "max" : "min"; - sprintf(reply, "> %s", ps_name); + sprintf(reply, "> %s", WifiPowerSavePolicy::nameFor(_mqtt_prefs.wifi_power_save)); } else if (memcmp(config, "timezone.offset", 15) == 0) { // Must precede the "timezone" (8-byte) check below — that prefix-matches // "timezone.offset" too, so the more-specific key has to come first or diff --git a/src/helpers/WifiPowerSavePolicy.h b/src/helpers/WifiPowerSavePolicy.h new file mode 100644 index 00000000..bdebd57e --- /dev/null +++ b/src/helpers/WifiPowerSavePolicy.h @@ -0,0 +1,73 @@ +#pragma once + +#include +#include + +// The single mapping between the stored `wifi.powersave` preference, its CLI +// name, and the IDF power-save mode. +// +// Pure lookup so host tests can hold startup, reconnect, CLI and web config to +// one table: they used to map the same stored value differently, so a node set +// to `min` silently ran with power save off after its first reconnect while +// `get wifi.powersave` still said min. +// +// Stored values are fleet state — never renumber them. The product default is +// `none`, which is a *default* (MQTTDefaults.h), not a reinterpretation of an +// operator's explicit `min`. +namespace WifiPowerSavePolicy { + +enum StoredValue : uint8_t { + kMin = 0, // WIFI_PS_MIN_MODEM + kNone = 1, // WIFI_PS_NONE (default) + kMax = 2, // WIFI_PS_MAX_MODEM +}; + +// Mirrors wifi_ps_type_t. MQTTBridge.cpp static_asserts these against the SDK. +enum Mode : uint8_t { + kModeNone = 0, + kModeMinModem = 1, + kModeMaxModem = 2, +}; + +// Anything outside the known range reads as the default rather than as the +// lowest-numbered mode, so a corrupt byte cannot silently enable modem sleep. +static inline Mode modeFor(uint8_t stored) { + switch (stored) { + case kMin: return kModeMinModem; + case kMax: return kModeMaxModem; + case kNone: return kModeNone; + default: return kModeNone; + } +} + +static inline const char* nameFor(uint8_t stored) { + switch (stored) { + case kMin: return "min"; + case kMax: return "max"; + case kNone: return "none"; + default: return "none"; + } +} + +// Parses a CLI argument, which may be followed by trailing text (the observer +// setters take the rest of the command line). Returns false and leaves *out +// untouched for anything else. +static inline bool parseName(const char* value, uint8_t* out) { + if (value == nullptr || out == nullptr) return false; + static const struct { const char* name; uint8_t stored; } kNames[] = { + { "min", kMin }, + { "none", kNone }, + { "max", kMax }, + }; + for (unsigned i = 0; i < sizeof(kNames) / sizeof(kNames[0]); i++) { + const size_t len = strlen(kNames[i].name); + if (strncmp(value, kNames[i].name, len) == 0 && + (value[len] == '\0' || value[len] == ' ')) { + *out = kNames[i].stored; + return true; + } + } + return false; +} + +} // namespace WifiPowerSavePolicy diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 39c8f05b..a45dd612 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -1,4 +1,5 @@ #include "MQTTBridge.h" +#include "../WifiPowerSavePolicy.h" #include "../MQTTConnectionPolicy.h" #include "../MQTTMessageBuilder.h" #include "../MQTTPacketQueuePolicy.h" @@ -427,6 +428,19 @@ int MQTTBridge::getMaxActiveSlots() { #endif } +// One mapping for startup, reconnect and CLI (see WifiPowerSavePolicy). The +// stored default is `none`; `min` means MIN_MODEM here exactly as the CLI says +// it does. +void MQTTBridge::applyWifiPowerSave() { + #ifdef ESP_PLATFORM + static_assert((int)WifiPowerSavePolicy::kModeNone == (int)WIFI_PS_NONE, "wifi_ps_type_t drift"); + static_assert((int)WifiPowerSavePolicy::kModeMinModem == (int)WIFI_PS_MIN_MODEM, "wifi_ps_type_t drift"); + static_assert((int)WifiPowerSavePolicy::kModeMaxModem == (int)WIFI_PS_MAX_MODEM, "wifi_ps_type_t drift"); + if (!_obs) return; + esp_wifi_set_ps((wifi_ps_type_t)WifiPowerSavePolicy::modeFor(_obs->wifi_power_save)); + #endif +} + uint8_t MQTTBridge::getLastWifiDisconnectReason() { return s_wifi_disconnect_reason; } unsigned long MQTTBridge::getLastWifiDisconnectTime() { return s_wifi_disconnect_time; } @@ -2804,6 +2818,12 @@ bool MQTTBridge::handleWiFiConnection(unsigned long now) { _wifi_status_initialized = true; setWifiOutage(AlertFaultPolicy::applyWifiStatus( (uint32_t)now, current_wifi_status == WL_CONNECTED, wifiOutage(), false)); + #ifdef ESP_PLATFORM + // Already associated at bridge start (end()/begin() leaves STA up): there is + // no connect transition below to carry the setting, so apply it here or the + // node runs on whatever the previous mode was. + if (current_wifi_status == WL_CONNECTED) applyWifiPowerSave(); + #endif } if (now - _last_wifi_check <= 10000) { // Events own the snapshot between 10 s polls. If STA is associated again @@ -2827,16 +2847,7 @@ bool MQTTBridge::handleWiFiConnection(unsigned long now) { s_wifi_connected_at = now; _wifi_reconnect_backoff_attempt = 0; #ifdef ESP_PLATFORM - wifi_ps_type_t ps_mode; - uint8_t ps_pref = _obs->wifi_power_save; - if (ps_pref == 1) { - ps_mode = WIFI_PS_NONE; - } else if (ps_pref == 2) { - ps_mode = WIFI_PS_MAX_MODEM; - } else { - ps_mode = WIFI_PS_NONE; // default: no power save; eliminates DTIM wake latency on mains-powered bridges - } - esp_wifi_set_ps(ps_mode); + applyWifiPowerSave(); #ifdef MQTT_WIFI_TX_POWER WiFi.setTxPower(MQTT_WIFI_TX_POWER); #else diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index e2d27253..b4ee7b99 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -517,6 +517,7 @@ private: void getClientVersion(char* buffer, size_t buffer_size) const; void logMemoryStatus(); void refreshOriginFromPrefs(); + void applyWifiPowerSave(); // one mapping, applied on every association // begin()/end()-scoped PSRAM buffers. Each allocation is independent so a // transient heap shortage degrades to the existing stack fallback instead // of making the bridge unusable. diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index 34bdca4b..d62a07f1 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include "WebConfigHtml.h" @@ -722,8 +723,7 @@ void WebConfigServer::handleConfigGet(AsyncWebServerRequest* req) { JsonObject wifi = doc.createNestedObject("wifi"); wifi["ssid"] = (const char*)_obs->wifi_ssid; wifi["pwd"] = _obs->wifi_password[0] ? SECRET_SENTINEL : ""; - wifi["powersave"] = _obs->wifi_power_save == 0 ? "min" - : _obs->wifi_power_save == 2 ? "max" : "none"; + wifi["powersave"] = WifiPowerSavePolicy::nameFor(_obs->wifi_power_save); JsonObject mqtt = doc.createNestedObject("mqtt"); mqtt["origin"] = (const char*)_obs->mqtt_origin; diff --git a/test/test_wifi_power_save_policy/test_wifi_power_save_policy.cpp b/test/test_wifi_power_save_policy/test_wifi_power_save_policy.cpp new file mode 100644 index 00000000..66803aa5 --- /dev/null +++ b/test/test_wifi_power_save_policy/test_wifi_power_save_policy.cpp @@ -0,0 +1,58 @@ +#include "helpers/WifiPowerSavePolicy.h" + +#include + +using namespace WifiPowerSavePolicy; + +// F11: the CLI applied stored 0 as MIN_MODEM while the reconnect path applied it +// as NONE, so a node configured for `min` changed behaviour after every +// reconnect and `get wifi.powersave` still reported min. One table, one answer. +TEST(WifiPowerSavePolicy, StoredValuesMapToOneModeEach) { + EXPECT_EQ(kModeMinModem, modeFor(kMin)); + EXPECT_EQ(kModeNone, modeFor(kNone)); + EXPECT_EQ(kModeMaxModem, modeFor(kMax)); +} + +TEST(WifiPowerSavePolicy, NamesRoundTripWithStoredValues) { + for (uint8_t stored = 0; stored <= 2; stored++) { + uint8_t parsed = 0xFF; + ASSERT_TRUE(parseName(nameFor(stored), &parsed)) << "stored " << (int)stored; + EXPECT_EQ(stored, parsed); + EXPECT_EQ(modeFor(stored), modeFor(parsed)); + } +} + +// A byte outside the stored range must read as the product default, not as +// whatever mode happens to sit at that index. +TEST(WifiPowerSavePolicy, OutOfRangeStoredValueReadsAsDefault) { + for (int stored = 3; stored <= 255; stored++) { + EXPECT_EQ(kModeNone, modeFor((uint8_t)stored)) << "stored " << stored; + EXPECT_STREQ("none", nameFor((uint8_t)stored)); + } +} + +// The setters take the remainder of the command line, so a trailing argument +// must still parse — and a longer word starting with a valid name must not. +TEST(WifiPowerSavePolicy, ParsesCliArgumentsExactly) { + uint8_t stored = 0xFF; + + EXPECT_TRUE(parseName("min", &stored)); + EXPECT_EQ(kMin, stored); + EXPECT_TRUE(parseName("none extra", &stored)); + EXPECT_EQ(kNone, stored); + EXPECT_TRUE(parseName("max ", &stored)); + EXPECT_EQ(kMax, stored); + + stored = 0xFF; + EXPECT_FALSE(parseName("minimum", &stored)); + EXPECT_FALSE(parseName("", &stored)); + EXPECT_FALSE(parseName("off", &stored)); + EXPECT_FALSE(parseName("MIN", &stored)); + EXPECT_FALSE(parseName(nullptr, &stored)); + EXPECT_EQ(0xFF, stored); // rejected input leaves the caller's value alone +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}