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