From f349d1d4161b770d468223aaadc8d3dd27bc5934 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Mon, 5 Oct 2026 10:44:50 -0700 Subject: [PATCH] Preserve healthy OTA access points and drain stopping uploaders safely --- src/helpers/ESP32Board.cpp | 60 +++++++++++++++++++++----------- src/helpers/ESP32Board.h | 1 + test/fixtures/wifi_ota_start.cpp | 26 +++++++++++--- test/test_wifi_ota_lifecycle.py | 29 ++++++--------- test/test_wifi_ota_start.py | 27 ++++++++++++++ 5 files changed, 101 insertions(+), 42 deletions(-) diff --git a/src/helpers/ESP32Board.cpp b/src/helpers/ESP32Board.cpp index 1d85846e9..bfcd75035 100644 --- a/src/helpers/ESP32Board.cpp +++ b/src/helpers/ESP32Board.cpp @@ -67,8 +67,6 @@ bool ESP32Board::isUserGpioAvailable(uint8_t pin) const { #include #include -static bool lightweight_ota_started_ap; - // Full Companion retains its established uploader URL on port 8080. // Infrastructure uses port 80; CLI startup hands WebConfig's WiFi to OTA. #if defined(COMPANION_RADIO_FULL) @@ -214,6 +212,7 @@ class LightweightOTAServer { } } while (line[0]); + if (!running) return; if (get_page) sendPage(client); else if (get_log) sendLog(client); else if (post_update) receiveUpdate(client, content_length); @@ -256,11 +255,17 @@ public: return true; } - void end() { + bool isRunning() const { return running; } + + bool end() { running = false; server.stop(); for (unsigned i = 0; task != nullptr && i < 100; i++) delay(10); + // A slow client can still be returning from a socket write. Keep its + // board/session alive until the task exits; a retry can finish cleanup. + if (task != nullptr) return false; board = nullptr; + return true; } }; @@ -268,6 +273,10 @@ static LightweightOTAServer lightweight_ota_server; bool ESP32Board::startOTAUpdate(const char* id, char reply[], bool force_ap) { (void)id; + if (ota_server != nullptr && !lightweight_ota_server.isRunning()) { + strcpy(reply, "ERR: OTA stopping; retry stop ota first"); + return false; + } #if defined(COMPANION_RADIO_FULL) const esp_partition_t* running = esp_ota_get_running_partition(); const esp_partition_t* next = esp_ota_get_next_update_partition(nullptr); @@ -283,17 +292,19 @@ bool ESP32Board::startOTAUpdate(const char* id, char reply[], bool force_ap) { inhibit_sleep = true; IPAddress ip; - const bool use_ap = force_ap || WiFi.status() != WL_CONNECTED; + const bool use_ap = force_ap || WiFi.status() != WL_CONNECTED + || static_cast(WiFi.localIP()) == 0; if (!use_ap) { ip = WiFi.localIP(); } else { - if (!lightweight_ota_started_ap) { + if (!ota_started_ap || !(WiFi.getMode() & WIFI_AP) + || static_cast(WiFi.softAPIP()) == 0) { const IPAddress ap_ip(192, 168, 4, 1); const IPAddress ap_mask(255, 255, 255, 0); - lightweight_ota_started_ap = WiFi.softAPConfig(ap_ip, ap_ip, ap_mask) + ota_started_ap = WiFi.softAPConfig(ap_ip, ap_ip, ap_mask) && WiFi.softAP("MeshCore-OTA", nullptr); } - if (!lightweight_ota_started_ap) { + if (!ota_started_ap) { inhibit_sleep = ota_server != nullptr; strcpy(reply, "ERR: OTA WiFi failed"); return false; @@ -303,8 +314,8 @@ bool ESP32Board::startOTAUpdate(const char* id, char reply[], bool force_ap) { if (ota_server == nullptr) { if (!lightweight_ota_server.begin(this)) { - if (lightweight_ota_started_ap) WiFi.softAPdisconnect(true); - lightweight_ota_started_ap = false; + if (ota_started_ap) WiFi.softAPdisconnect(true); + ota_started_ap = false; inhibit_sleep = false; strcpy(reply, "ERR: OTA server failed"); return false; @@ -329,10 +340,13 @@ bool ESP32Board::stopOTAUpdate(char reply[]) { return true; } - lightweight_ota_server.end(); + if (!lightweight_ota_server.end()) { + strcpy(reply, "ERR: OTA stopping; retry stop ota"); + return false; + } ota_server = nullptr; - if (lightweight_ota_started_ap) WiFi.softAPdisconnect(true); - lightweight_ota_started_ap = false; + if (ota_started_ap) WiFi.softAPdisconnect(true); + ota_started_ap = false; inhibit_sleep = false; strcpy(reply, "OK - OTA stopped"); MESH_DEBUG_PRINTLN("stopOTAUpdate: %s", reply); @@ -361,14 +375,19 @@ bool ESP32Board::startOTAUpdate(const char* id, char reply[], bool force_ap) { // reachable even when the joined network applies client isolation and the // station IP can't be reached. IPAddress ip; - const bool use_ap = force_ap || WiFi.status() != WL_CONNECTED; + const bool use_ap = force_ap || WiFi.status() != WL_CONNECTED + || static_cast(WiFi.localIP()) == 0; if (!use_ap) { ip = WiFi.localIP(); } else { - const IPAddress ap_ip(192, 168, 4, 1); - const IPAddress ap_mask(255, 255, 255, 0); - if (!WiFi.softAPConfig(ap_ip, ap_ip, ap_mask) - || !WiFi.softAP("MeshCore-OTA", NULL)) { + if (!ota_started_ap || !(WiFi.getMode() & WIFI_AP) + || static_cast(WiFi.softAPIP()) == 0) { + const IPAddress ap_ip(192, 168, 4, 1); + const IPAddress ap_mask(255, 255, 255, 0); + ota_started_ap = WiFi.softAPConfig(ap_ip, ap_ip, ap_mask) + && WiFi.softAP("MeshCore-OTA", NULL); + } + if (!ota_started_ap) { inhibit_sleep = ota_server != nullptr; strcpy(reply, "ERR: OTA WiFi failed"); return false; @@ -385,9 +404,9 @@ bool ESP32Board::startOTAUpdate(const char* id, char reply[], bool force_ap) { if (ota_server != nullptr) return true; static char id_buf[60]; - sprintf(id_buf, "%s (%s)", id, getManufacturerName()); + snprintf(id_buf, sizeof(id_buf), "%s (%s)", id, getManufacturerName()); static char home_buf[90]; - sprintf(home_buf, "

Hi! I am a MeshCore Repeater. ID: %s

", id); + snprintf(home_buf, sizeof(home_buf), "

Hi! I am a MeshCore Repeater. ID: %s

", id); ota_server = new AsyncWebServer(80); @@ -414,7 +433,8 @@ bool ESP32Board::stopOTAUpdate(char reply[]) { ota_server->end(); delete ota_server; ota_server = nullptr; - WiFi.softAPdisconnect(true); + if (ota_started_ap) WiFi.softAPdisconnect(true); + ota_started_ap = false; inhibit_sleep = false; strcpy(reply, "OK - OTA stopped"); diff --git a/src/helpers/ESP32Board.h b/src/helpers/ESP32Board.h index 05275fd7b..bd40afe9f 100644 --- a/src/helpers/ESP32Board.h +++ b/src/helpers/ESP32Board.h @@ -39,6 +39,7 @@ class ESP32Board : public mesh::MainBoard { protected: uint8_t startup_reason; bool inhibit_sleep = false; + bool ota_started_ap = false; #if MESH_ESP32_USB_CONSOLE_COOPERATIVE mesh::UsbHostSleepPolicy usb_host_sleep_policy; #endif diff --git a/test/fixtures/wifi_ota_start.cpp b/test/fixtures/wifi_ota_start.cpp index 6efa78cad..a72e38fb7 100644 --- a/test/fixtures/wifi_ota_start.cpp +++ b/test/fixtures/wifi_ota_start.cpp @@ -10,6 +10,7 @@ #define MESH_DEBUG_PRINTLN(...) ((void)0) constexpr int WL_CONNECTED = 3; constexpr int HTTP_GET = 0; +constexpr int WIFI_AP = 2; struct IPAddress { std::string value; IPAddress() = default; @@ -18,11 +19,13 @@ struct IPAddress { + std::to_string(c) + "." + std::to_string(d); } std::string toString() const { return value; } + explicit operator uint32_t() const { return value.empty() || value == "0.0.0.0" ? 0 : 1; } }; struct WiFiFake { bool connected = true, ap = false, allow_ap = true; unsigned starts = 0; int status() const { return connected ? WL_CONNECTED : 0; } + int getMode() const { return ap ? WIFI_AP : 0; } IPAddress localIP() const { return IPAddress(10, 20, 30, 40); } IPAddress softAPIP() const { return ap ? IPAddress(192, 168, 4, 1) : IPAddress(0, 0, 0, 0); } bool softAPConfig(IPAddress ip, IPAddress gateway, IPAddress mask) { @@ -54,6 +57,7 @@ struct Elegant { static int SPIFFS; struct ESP32Board { bool inhibit_sleep = false; + bool ota_started_ap = false; #ifdef LIGHTWEIGHT_WIFI_OTA void* ota_server = nullptr; #else @@ -64,11 +68,11 @@ struct ESP32Board { bool stopOTAUpdate(char*); }; struct LightweightServer { - bool allow = true; - bool begin(ESP32Board*) { ++server_starts; return allow; } - void end() {} + bool allow = true, running = false, allow_stop = true; + bool begin(ESP32Board*) { ++server_starts; running = allow; return allow; } + bool isRunning() const { return running; } + bool end() { running = false; return allow_stop; } } lightweight_ota_server; -static bool lightweight_ota_started_ap; #ifdef COMPANION_RADIO_FULL static constexpr uint16_t LIGHTWEIGHT_OTA_PORT = 8080; #else @@ -114,6 +118,20 @@ int main() { // In particular, an already-running LAN server must honor a later `ap`. start(true, "192.168.4.1", "Join WiFi MeshCore-OTA"); assert(WiFi.ap && WiFi.connected && server_starts == 1); + const auto ap_starts = WiFi.starts; + start(true, "192.168.4.1", "Join WiFi MeshCore-OTA"); + assert(WiFi.starts == ap_starts); // Healthy repeats retain associated clients. + WiFi.ap = false; // An external driver stop invalidates cached AP ownership. + start(true, "192.168.4.1", "Join WiFi MeshCore-OTA"); + assert(WiFi.starts == ap_starts + 1); +#ifdef LIGHTWEIGHT_WIFI_OTA + lightweight_ota_server.allow_stop = false; + assert(!board.stopOTAUpdate(reply.text)); + assert(strstr(reply.text, "OTA stopping") && board.inhibit_sleep && board.ota_server); + assert(!board.startOTAUpdate("test node", reply.text, true)); + assert(strstr(reply.text, "retry stop ota first")); + lightweight_ota_server.allow_stop = true; +#endif assert(board.stopOTAUpdate(reply.text)); assert(!board.inhibit_sleep && !board.ota_server && !WiFi.ap && WiFi.connected); diff --git a/test/test_wifi_ota_lifecycle.py b/test/test_wifi_ota_lifecycle.py index 6d19864a3..154d0cccf 100644 --- a/test/test_wifi_ota_lifecycle.py +++ b/test/test_wifi_ota_lifecycle.py @@ -1,9 +1,7 @@ """Execute production WiFi ownership, ESP-NOW cleanup and ElegantOTA startup. -Desired-behavior assertions are marked as known failures until firmware is fixed. Set -MESHCORE_REQUIRE_WIFI_OTA_FIX=1 to make those regressions ordinary failures. -Compilation, every fixture execution and observation parsing happen in -setUpClass, outside expectedFailure, so infrastructure failures cannot pass. +All lifecycle regressions are required to pass. Compilation, every fixture +execution and observation parsing happen in setUpClass. OTA startup coverage deliberately selects ElegantOTA, not LightweightOTA. """ from pathlib import Path @@ -39,12 +37,14 @@ STUBS = r''' #define WITH_ESPNOW_BRIDGE 1 #define MESH_DEBUG_PRINTLN(...) ((void)0) constexpr int WL_CONNECTED = 3, WL_DISCONNECTED = 6, HTTP_GET = 0; +constexpr int WIFI_AP = 2; struct IPAddress { uint32_t value = 0; IPAddress() = default; IPAddress(uint8_t a, uint8_t b, uint8_t c, uint8_t d) : value((uint32_t(a) << 24) | (uint32_t(b) << 16) | (uint32_t(c) << 8) | d) {} + explicit operator uint32_t() const { return value; } std::string toString() const { char text[16]; std::snprintf(text, sizeof(text), "%u.%u.%u.%u", (value >> 24) & 255, @@ -62,6 +62,7 @@ struct MockWiFi { bool initialized_cache = false, started_cache = false; IPAddress station_ip, ap_ip, configured_ap_ip; int status() const { return station_status; } + int getMode() const { return sdk_ap_active ? WIFI_AP : 0; } IPAddress localIP() const { return station_ip; } IPAddress softAPIP() const { return ap_ip; } bool softAPConfig(IPAddress ip, IPAddress, IPAddress) { @@ -108,6 +109,7 @@ struct MockSPIFFS {} SPIFFS; class ESP32Board { public: bool inhibit_sleep = false; + bool ota_started_ap = false; AsyncWebServer* ota_server = nullptr; ~ESP32Board() { delete ota_server; } const char* getManufacturerName() const { return "test board"; } @@ -289,9 +291,8 @@ class WifiOtaLifecycleTests(unittest.TestCase): def test_espnow_alone_is_not_infrastructure_wifi(self): self.assertEqual(self.observations["owners_espnow"]["enabled"], 4) - @unittest.expectedFailure def test_http_ota_alone_owns_wifi(self): - """Known bug: enabled() omits board HTTP OTA ownership in both AP and STA modes.""" + """HTTP OTA owns WiFi in both AP and STA modes.""" self.assertEqual(tuple(self.observations[scenario]["enabled"] & 1 for scenario in ("owners_httpota", "owners_httpota_sta")), (1, 1)) @@ -321,9 +322,8 @@ class WifiOtaLifecycleTests(unittest.TestCase): def test_espnow_without_infrastructure_allows_driver_cleanup(self): self.assert_cleanup("cleanup_espnow", 1) - @unittest.expectedFailure def test_http_ota_owner_protects_wifi_from_bridge_cleanup(self): - """Known bug: raw cleanup stops/deinits the AP or STA owned only by HTTP OTA.""" + """Bridge cleanup retains the AP or STA owned only by HTTP OTA.""" self.assertEqual(tuple((self.observations[scenario]["stop_calls"], self.observations[scenario]["deinit_calls"], self.observations[scenario]["endpoint_usable"], @@ -334,7 +334,8 @@ class WifiOtaLifecycleTests(unittest.TestCase): def assert_healthy_ota(self, scenario, url, ap_starts): observation = self.observations[scenario] self.assertTrue(observation["first_started"] and observation["started"]) - self.assertEqual(observation["reply"], f"Started: http://{url}/update") + network = "Join WiFi MeshCore-OTA" if url == "192.168.4.1" else "Use same WiFi/LAN" + self.assertEqual(observation["reply"], f"Started: http://{url}/update - WiFi on; {network}") self.assertEqual(observation["server_count"], 1) self.assertEqual(observation["server_begin_calls"], 1) self.assertEqual(observation["ap_start_calls"], ap_starts) @@ -351,9 +352,8 @@ class WifiOtaLifecycleTests(unittest.TestCase): def test_healthy_station_repeat_start_is_idempotent(self): self.assert_healthy_ota("ota_sta_repeat", "10.0.0.7", 0) - @unittest.expectedFailure def test_stale_server_cannot_claim_success_at_zero_ap_ip(self): - """Known bug: non-null ElegantOTA server bypasses dead-AP validation/recovery.""" + """A non-null ElegantOTA server still validates/recovers a dead AP.""" observation = self.observations["ota_stale_ap"] if observation["started"]: self.assertTrue(observation["endpoint_usable"]) @@ -371,12 +371,5 @@ class WifiOtaLifecycleTests(unittest.TestCase): self.assertFalse(observation["ota_running"] or observation["sleep_inhibited"]) -if os.environ.get("MESHCORE_REQUIRE_WIFI_OTA_FIX") == "1": - # A strict run must fail on current firmware, and passes only after fixes. - for test in vars(WifiOtaLifecycleTests).values(): - if getattr(test, "__unittest_expecting_failure__", False): - test.__unittest_expecting_failure__ = False - - if __name__ == "__main__": unittest.main() diff --git a/test/test_wifi_ota_start.py b/test/test_wifi_ota_start.py index bb327217e..82d4c1194 100644 --- a/test/test_wifi_ota_start.py +++ b/test/test_wifi_ota_start.py @@ -216,6 +216,33 @@ int main() { ''' self.compile_and_run(fixture.replace("@COMMAND@", command), "-DWITH_WEBCONFIG=1") + def test_lightweight_stop_keeps_slow_client_state_until_task_exits(self): + source = (ROOT / "src/helpers/ESP32Board.cpp").read_text() + implementation = source[source.index("class LightweightOTAServer {"):] + method = extract_braced(implementation, "bool end()") + fixture = r''' +#include +#include +static unsigned delays = 0; +static void delay(unsigned value) { assert(value == 10); ++delays; } +struct Server { void stop() {} }; +struct Updater { + bool running = true; + Server server; + void* task = reinterpret_cast(1); + void* board = reinterpret_cast(2); + @END@ +}; +int main() { + Updater updater; + assert(!updater.end()); + assert(!updater.running && updater.task && updater.board && delays == 100); + updater.task = nullptr; // Slow network write has now returned and task exited. + assert(updater.end() && !updater.board && delays == 100); +} +''' + self.compile_and_run(fixture.replace("@END@", method)) + if __name__ == "__main__": unittest.main()