From 259eda66630cb96e7b1bf0bfc1a691388b7dcbdb Mon Sep 17 00:00:00 2001 From: Alex Petrochenko Date: Tue, 19 May 2026 16:05:57 +0100 Subject: [PATCH] fix: post-review issues + close M2/M3/M4 from internal backlog (v1.9.9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From Sonnet pre-release code review (3 real issues): - F1 (high): factory reset atomicity — save cleared creds BEFORE zeroing reset counter, so a power loss between commits still results in WiFiManager AP boot instead of inconsistent "counter=0 + stale creds" state - F2: isValidSSID now allows single-char SSIDs (per 802.11 spec) - F3: ntp_interval changes now trigger needsRestart (NTPClient constructed once in setup() with this value, doesn't pick up runtime changes) From internal backlog: - M2: setupWiFi Try-2 now supports open networks (no password) — fixed the &&-condition that required both ssid and password - M3: removed dead NTP_WAITING/NTP_SUCCESS/NTP_FAILED enum values - M4: randomSeed() with ESP.getChipId() ^ micros() — dissolve pattern varies Tested on hardware: 72/73 tests pass (single failure is expected — ntp_interval fuzz cases now trigger restart due to F3, second case lands in reboot window). Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 24 ++++++++++++++++++++++++ firmware/weather_clock/config.h | 10 ++++------ firmware/weather_clock/weather_clock.ino | 21 ++++++++++++++++----- firmware/weather_clock/web_server.cpp | 7 +++++-- firmware/weather_clock/wifi_manager.cpp | 10 +++++++--- 5 files changed, 56 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe7960e..ed2f84e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,30 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.9.9] - 2026-05-19 + +### Fixed + +- **Factory reset atomicity**: cleared WiFi credentials are now committed BEFORE + the reset counter is zeroed. If power fails between the two writes, the device + still boots into WiFiManager AP on next start (cleared creds win over stale counter) + instead of being stuck in an inconsistent state +- **`isValidSSID` rejected single-char SSIDs**: length-1 networks like "A" were + incorrectly flagged as all-same garbage. Now allowed (per 802.11 spec) +- **`ntp_interval` config changes ignored until reboot**: `NTPClient` is constructed + in `setup()` with this value and never re-initialized. Now triggers `needsRestart` + when the interval actually changes +- **Open WiFi (no password) failed to connect at setup** (M2): Try-2 block in + `setupWiFi()` required both ssid and password; now falls back to `WiFi.begin(ssid)` + when password is empty +- **Unseeded PRNG, identical dissolve pattern every boot** (M4): `randomSeed()` + now called with `ESP.getChipId() ^ micros()` — varies between devices and boots + +### Removed + +- **Dead `NTPState` enum values** (M3): `NTP_WAITING`, `NTP_SUCCESS`, `NTP_FAILED` + were defined but never assigned. Removed to reduce code surface area + ## [1.9.8] - 2026-05-19 ### Fixed diff --git a/firmware/weather_clock/config.h b/firmware/weather_clock/config.h index 1c3c1f1..452e448 100644 --- a/firmware/weather_clock/config.h +++ b/firmware/weather_clock/config.h @@ -9,7 +9,7 @@ #include // Firmware version -#define FIRMWARE_VERSION "1.9.8" +#define FIRMWARE_VERSION "1.9.9" // OLED I2C Configuration #define I2C_SDA 0 // GPIO0 (I2C Data) - SWAPPED! @@ -120,13 +120,11 @@ enum WeatherState { WEATHER_FAILED }; -// Async NTP state machine +// Async NTP state machine — only IDLE and REQUEST_SENT are used +// (response handler transitions back to IDLE directly on success or timeout) enum NTPState { NTP_IDLE, - NTP_REQUEST_SENT, - NTP_WAITING, - NTP_SUCCESS, - NTP_FAILED + NTP_REQUEST_SENT }; // Async WiFi state machine diff --git a/firmware/weather_clock/weather_clock.ino b/firmware/weather_clock/weather_clock.ino index 83294b2..632dd24 100644 --- a/firmware/weather_clock/weather_clock.ino +++ b/firmware/weather_clock/weather_clock.ino @@ -133,16 +133,22 @@ void ICACHE_FLASH_ATTR checkFactoryReset() { if (rc.count >= RESET_COUNTER_TRIPS) { Serial.println("!!! FACTORY RESET triggered !!!"); + + // Atomicity: clear credentials FIRST (saveConfig commits), then zero counter. + // If power fails between the two commits, the device boots into AP mode next + // time (creds are already cleared) — instead of being in a "counter zeroed but + // creds still valid" inconsistent state. + memset(config.ssid, 0, sizeof(config.ssid)); + memset(config.password, 0, sizeof(config.password)); + EEPROM.end(); // close current handle before saveConfig opens its own + saveConfig(); // commits cleared credentials to flash + + EEPROM.begin(512); rc.count = 0; EEPROM.put(RESET_COUNTER_ADDR, rc); EEPROM.commit(); EEPROM.end(); - // Clear WiFi credentials only — keep other settings - memset(config.ssid, 0, sizeof(config.ssid)); - memset(config.password, 0, sizeof(config.password)); - saveConfig(); - // Show reset screen display.clearDisplay(); display.setTextColor(SSD1306_WHITE); @@ -244,6 +250,11 @@ void ICACHE_FLASH_ATTR setupOTA() { void setup() { Serial.begin(115200); delay(100); + + // Seed PRNG with hardware entropy: chip ID is unique per device, + // micros() varies on each boot due to power-on timing jitter + randomSeed(ESP.getChipId() ^ micros()); + Serial.println("\n\nTJ-56-654 NTP Clock with OTA v" FIRMWARE_VERSION); Serial.println("=========================================="); Serial.println("Display: GM009605v4.3 OLED 128x64 (SSD1306 I2C)"); diff --git a/firmware/weather_clock/web_server.cpp b/firmware/weather_clock/web_server.cpp index 3fbce89..8b78d66 100644 --- a/firmware/weather_clock/web_server.cpp +++ b/firmware/weather_clock/web_server.cpp @@ -294,7 +294,8 @@ static bool isValidSSID(const String& s) { if (c < 0x20 || c > 0x7E) return false; // non-printable if (c != first) allSame = false; } - return !allSame; // reject "AAAAA...", "BBBBB...", etc. + // Single-char SSIDs ("A") are valid per 802.11. Only reject all-same for length>1. + return n == 1 || !allSame; } void ICACHE_FLASH_ATTR handleConfigSave() { @@ -358,7 +359,9 @@ void ICACHE_FLASH_ATTR handleConfigSave() { } if (server.hasArg("ntp_interval")) { long ni = server.arg("ntp_interval").toInt(); - config.ntp_interval = constrain(ni, 60L, 86400L); + unsigned long newInterval = constrain(ni, 60L, 86400L); + if (newInterval != config.ntp_interval) needsRestart = true; // NTPClient constructed at boot with this + config.ntp_interval = newInterval; } if (server.hasArg("ntp_server")) { String n = server.arg("ntp_server"); diff --git a/firmware/weather_clock/wifi_manager.cpp b/firmware/weather_clock/wifi_manager.cpp index 17fbc4e..e30bf2a 100644 --- a/firmware/weather_clock/wifi_manager.cpp +++ b/firmware/weather_clock/wifi_manager.cpp @@ -105,10 +105,14 @@ void ICACHE_FLASH_ATTR setupWiFi() { } } - // Try 2: If we have EEPROM credentials, try those - if (strlen(config.ssid) > 0 && strlen(config.password) > 0) { + // Try 2: If we have EEPROM credentials, try those (M2: support open networks) + if (strlen(config.ssid) > 0) { Serial.println("\nTrying EEPROM credentials..."); - WiFi.begin(config.ssid, config.password); + if (strlen(config.password) > 0) { + WiFi.begin(config.ssid, config.password); + } else { + WiFi.begin(config.ssid); // open network — no password + } int attempts = 0; while (WiFi.status() != WL_CONNECTED && attempts < 20) {