From 5114aed79bb52cd7427fdd0ca1ef481e1de75aef Mon Sep 17 00:00:00 2001 From: Strycher Date: Wed, 9 Sep 2026 01:14:03 -0400 Subject: [PATCH 1/3] feat(#295): WiFi credentials move to NVS; connect becomes a non-blocking state machine; SD import Closes #147. Epic #99 child 1; plan in docs/plans/2026-09-09-wifi-configuration.md (approved 2026-09-09). wifi_store: Preferences namespace 'wifi', five entries in priority order, deduplicated by SSID, oldest slot replaced when full; add/remove/move-up/clear for #296 and #297. One-shot import of /config/wifi.txt at boot; the file is never modified here, the Remove/Keep choice belongs to the screen and the web page. web.cpp: the six placeholder literals are gone. initWiFi() loads the store, imports, starts the first attempt and returns; checkWiFi() drives IDLE/CONNECTING/CONNECTED/WAIT from loop() with an 8 s per-network timeout and the 30 s rest, no delay() loops (the #292 WiFi item). NTP is started on connect and checked on later passes instead of a 5 s getLocalTime() wait. WiFi.persistent(false) so the core keeps no copy of the credentials; setAutoReconnect(false) so this machine is the only actor. Builds: um_feathers3 RAM 22.9% Flash 27.7%; feather_s3 RAM 31.8% Flash 64.9%. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 13 +- docs/plans/2026-09-09-wifi-configuration.md | 51 ++++++ src/src.ino | 2 +- src/web.cpp | 188 +++++++++++-------- src/wifi_store.cpp | 190 ++++++++++++++++++++ src/wifi_store.h | 45 +++++ 6 files changed, 408 insertions(+), 81 deletions(-) create mode 100644 docs/plans/2026-09-09-wifi-configuration.md create mode 100644 src/wifi_store.cpp create mode 100644 src/wifi_store.h diff --git a/CLAUDE.md b/CLAUDE.md index 2c9b382..d3f7fbe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,11 +46,14 @@ consequences that matter day to day: and in Citadel's `external_issue_number` still resolves. Numbers that were pull requests exist as **closed placeholders** pointing at the archive — the PRs themselves could not be recreated. See #245 for the list. -- **`src/web.cpp` holds placeholder credentials** (`REDACTED_SSID_1` and friends; they - were in `src.ino` until E4 band 3e, #274). A build from `main` compiles and **cannot - join WiFi**, which also means no `fieldcompass.local` diagnostics. Until #99 moves - credentials to runtime storage, flashing needs a local uncommitted edit — and nothing - in `.gitignore` protects such an edit from being committed by accident. +- **No WiFi credentials in the source any more** (#295, epic #99). Saved networks live in + NVS on the chip (`src/wifi_store.cpp`, namespace `wifi`, five entries, plain text by + decision). A fresh board joins nothing until it is provisioned: put `/config/wifi.txt` + on the SD card in the format in `wifi_store.h` (the owner types it, never an agent), + boot once, and the entries are imported; the file stays until you choose Remove on + the device or the `/wifi` page. Until then there is no `fieldcompass.local`. The + placeholders that sat here from the #245 rebuild until #295 are gone, so a `main` + build no longer needs a local uncommitted edit to get on the network. Pre-migration commit SHAs quoted in old issues refer to the archive, not here. diff --git a/docs/plans/2026-09-09-wifi-configuration.md b/docs/plans/2026-09-09-wifi-configuration.md new file mode 100644 index 0000000..2cd78d0 --- /dev/null +++ b/docs/plans/2026-09-09-wifi-configuration.md @@ -0,0 +1,51 @@ +# WiFi configuration — plan for epic #99 + +Approved by the owner on 2026-09-09 ("Proceed with the plan"). Children: #295, #296, #297, #298. + +## Diagnosis (main e36a79f) + +- Six credential literals in `src/web.cpp`, placeholders since the #245 rebuild. A build from `main` cannot join WiFi, so no `fieldcompass.local`, and every flash needs a local uncommitted edit that nothing protects from being committed. #147 is the security side of the same thing. +- `initWiFi()` blocks up to 22.5 s at boot (three networks, 7.5 s each). `checkWiFi()` blocks 5 s every 30 s while down, which is always while the credentials are placeholders. #292 measured that stall and lists "the WiFi state machine" as its first item. +- The issue was written in the TFT_eSPI era and plans a hand-built keyboard. The UI is LVGL 9.5 now, and `lv_keyboard`, `lv_textarea`, `lv_list` and `lv_spinner` are all enabled in `include/lv_conf.h`. The keyboard is a widget, not a project. +- The ESP32-S3 has no 5 GHz radio: Espressif specifies the part as "2.4 GHz Wi-Fi (802.11 b/g/n) with 40 MHz of bandwidth support" (espressif.com/en/products/socs/esp32-s3, read 2026-09-09). The second saved network (5 GHz) could never have connected, and #34's "prefer 5 GHz" option is impossible on this chip. + +## Structure + +#99 is a `type:epic`: several PRs, hardware verification at the end. Children, one branch and PR each per the FC per-issue override, in dependency order: + +**1. #295 — Credential store and non-blocking WiFi** (closes #147) +- Store: ESP32 NVS through `Preferences`, namespace `wifi`, up to five entries in priority order, deduplicated by SSID, oldest replaced when full. On-chip flash, not in git, not on a removable card. +- Boot and reconnect become one non-blocking state machine: start `WiFi.begin()` for the first saved network, watch `WiFi.status()` and a timeout from `loop()`, move to the next, no `delay()` loops. This is the #292 item, delivered here because this child rewrites both functions anyway. +- One-shot SD import: if `/config/wifi.txt` is on the card at boot (the #99 format), its entries are imported into NVS, deduplicated by SSID. That is the field pre-loading path and the way real credentials reach the bench without touching git. +- **The file is never touched without asking (owner, 2026-09-09).** After an import the TFT shows a one-time pop-up: "Imported N networks from SD. Remove /config/wifi.txt?" with **Remove** in red and **Keep** (child 3). The `/wifi` page shows the same choice while the file is present (child 2). Until answered the file stays; a repeat import on the next boot is harmless because of the dedupe. +- The six literals go. Acceptance: no SSID or password literal in any tracked file, and a `main` build joins the home network after one import. + +**2. #296 — `/wifi` web endpoint** +- List saved networks (SSID, priority, never the password), add, delete, move up. POST forms in the style of `/geocaches`. Same trust model as every other endpoint: plain HTTP on the LAN. +- Forget all networks with the same two-step confirm as the screen. The Remove/Keep choice for the SD file while it is present. + +**3. #297 — Settings → WiFi sub-screen** (closes #34) +- Status block: SSID, IP, RSSI. A Scan button starts an async scan and fills an `lv_list`: SSID, RSSI bars, lock glyph for secured networks. +- Tap a network → `lv_textarea` (masked, show/hide) with `lv_keyboard` (lower, upper, numbers, symbols come with the widget; keys are about 48 × 40 px at this size) → Connect → spinner → "Connected, " or "Failed" with retry. Success saves to the store, replacing the oldest when full. +- Saved-network list with delete, so a bad entry can be removed without the web page. +- **Clear all saved networks (owner, 2026-09-09):** a red "Forget all networks" button at the bottom of the sub-screen. Tapping it opens an `lv_msgbox`: "This removes every saved network from the device. Are you sure?" with a red **Forget all** and **Cancel**. Only the confirm wipes the store, and it disconnects if connected. +- The RSSI display is the usable half of #34; the rest of #34 is retired by the 5 GHz fact above. + +**4. #298 — Epic integration test on hardware** (CLAUDE.md § Epic Integration Testing, a dedicated task) +- Fresh board with no credentials → SD import → web add and delete → screen scan, connect, reboot, reconnect → loop pass never waits on WiFi (measured in the status line), touch stays live during a connect. Owner signs off. + +## Two questions answered + +**Do we need to encrypt on NVS?** Not for this. NVS is plain text on the flash chip, so the exposure is: someone with the device in hand, a USB cable, and esptool can dump the NVS partition and read the passwords. Against that: the same passwords were public from 2026-02 until the #245 rebuild on 2026-09-07 and still sit in the private archive (risk accepted 2026-08-08; the public repository is clean, verified 2026-09-09: none of the three 2026-02 commits that carry the value is an ancestor of any public branch, and GitHub has no such object in the public repo), the SD alternative is worse (any card reader, no tools), and the fix, ESP32 flash encryption plus NVS encryption, burns eFuses, is irreversible in release mode, needs an `nvs_keys` partition in both partition tables, and changes every flash we do through the wrapper. If the threat model ever becomes "device stolen, home network at risk", that is the answer, and it is its own epic. For now: NVS unencrypted. + +**Does the Feather support 5 GHz?** No, see Diagnosis. + +## Out of scope (accepted by the owner, 2026-09-09) + +- A captive-portal access point for first provisioning. SD import and the web page cover it. +- Encryption at rest, per the answer above. +- Anything 5 GHz. + +## Size + +Child 1 about 300 lines, 2 about 250, 3 about 400. Each gets the Gemini gate and a flash. 1 and 2 can be verified in one bench session. diff --git a/src/src.ino b/src/src.ino index 87c974f..9b3769d 100644 --- a/src/src.ino +++ b/src/src.ino @@ -215,7 +215,7 @@ void setup() { initFRAM(); // SPI FRAM 256KB (shared bus with TFT/SD) initRTC(); // Adalogger RTC - sets system time if RTC has valid time initSerialLog(rtcAvailable); // Serial log to SD (#59) - needs SD + RTC; RTC state passed in (#263) - initWiFi(); // Will sync NTP if connected, then sync RTC + initWiFi(); // Loads saved networks (NVS, SD import) and starts the first attempt; checkWiFi() in loop() does the rest (#295) // Flush any FRAM data from previous session to SD if (framAvailable && sdAvailable) { diff --git a/src/web.cpp b/src/web.cpp index 001c9c3..e2650c8 100644 --- a/src/web.cpp +++ b/src/web.cpp @@ -17,16 +17,10 @@ #include "display.h" #include "touch.h" #include "geo.h" +#include "wifi_store.h" // saved networks live in NVS, not here (#295, epic #99) -const char* WIFI_SSID_1 = "REDACTED_SSID_1"; -const char* WIFI_PASS_1 = "REDACTED_WIFI_PASSWORD"; -const char* WIFI_SSID_2 = "REDACTED_SSID_2"; -const char* WIFI_PASS_2 = "REDACTED_WIFI_PASSWORD"; -const char* WIFI_SSID_3 = "REDACTED_SSID_3"; -const char* WIFI_PASS_3 = "REDACTED_HOTSPOT_PASSWORD"; const char* NTP_SERVER = "pool.ntp.org"; WebServer webServer(WEB_SERVER_PORT); -unsigned long lastWiFiAttempt = 0; bool wifiConnected = false; bool ntpSynced = false; bool webServerStarted = false; @@ -36,89 +30,133 @@ static String gpxUploadBuffer; static bool gpxUploadSuccess = false; static String gpxUploadError; -void initWiFi() { - logPrint("Connecting to WiFi"); - - // Try networks in order - const char* ssids[] = {WIFI_SSID_1, WIFI_SSID_2, WIFI_SSID_3}; - const char* passwords[] = {WIFI_PASS_1, WIFI_PASS_2, WIFI_PASS_3}; - - for (int net = 0; net < 3; net++) { - logPrintf(" [%s]", ssids[net]); - - WiFi.begin(ssids[net], passwords[net]); - - int attempts = 0; - while (WiFi.status() != WL_CONNECTED && attempts < 15) { - delay(500); - logPrint("."); - attempts++; - } - - if (WiFi.status() == WL_CONNECTED) break; +// ---- WiFi: one non-blocking state machine (#295, epic #99) ----------------- +// The old initWiFi() blocked setup() for up to 22.5 s and checkWiFi() blocked +// loop() for 5 s every 30 s while the network was down (#292 measured it). +// Nothing here waits: initWiFi() starts the first attempt and returns, and +// checkWiFi(), called every loop pass, moves the machine along by looking at +// WiFi.status() and the clock. Saved networks are tried in store order; when +// none answers the machine rests WIFI_RECONNECT_INTERVAL and starts over. A +// network added at runtime (#296, #297) is picked up from IDLE or on the next +// round. NTP is started on connect and checked on later passes, never waited on. +enum WifiState { WIFI_ST_IDLE, WIFI_ST_CONNECTING, WIFI_ST_CONNECTED, WIFI_ST_WAIT }; +static WifiState wifiState = WIFI_ST_IDLE; +static int wifiTryIndex = 0; +static unsigned long wifiStateSince = 0; +static bool ntpPending = false; +static unsigned long ntpSince = 0; +#define WIFI_CONNECT_TIMEOUT_MS 8000 // per network, about what the old 15 x 500 ms gave +#define NTP_TIMEOUT_MS 15000 +#define NTP_EPOCH_SANE 1700000000UL // 2023-11-14: the SNTP client has answered + +static void wifiStartAttempt(int idx) { + WifiCred c; + if (!wifiStoreGet(idx, c)) { + wifiState = WIFI_ST_WAIT; + wifiStateSince = millis(); + return; } + logPrintf("[WIFI] Connecting to %s (%d of %d)\n", c.ssid, idx + 1, wifiStoreCount()); + WiFi.begin(c.ssid, c.pass); + wifiTryIndex = idx; + wifiState = WIFI_ST_CONNECTING; + wifiStateSince = millis(); +} - if (WiFi.status() == WL_CONNECTED) { - wifiConnected = true; - logPrintln(" OK"); - logPrintf(" IP: %s\n", WiFi.localIP().toString().c_str()); +void initWiFi() { + WiFi.mode(WIFI_STA); + WiFi.persistent(false); // the core keeps no copy of the credentials in its own NVS area + WiFi.setAutoReconnect(false); // this machine is the only thing that reconnects, so the log is truthful + wifiStoreInit(); + wifiStoreImportFromSD(); + if (wifiStoreCount() == 0) { + logPrintf("[WIFI] No saved networks. Put %s on the SD card (see docs/plans/2026-09-09-wifi-configuration.md)\n", + WIFI_IMPORT_PATH); + wifiState = WIFI_ST_IDLE; + return; + } + wifiStartAttempt(0); +} - // Sync NTP time - logPrint("Syncing NTP time... "); +static void wifiOnConnected() { + wifiConnected = true; + wifiState = WIFI_ST_CONNECTED; + wifiStateSince = millis(); + logPrintf("[WIFI] Connected to %s, IP %s, RSSI %d dBm\n", + WiFi.SSID().c_str(), WiFi.localIP().toString().c_str(), WiFi.RSSI()); + if (!ntpSynced) { configTime(0, 0, NTP_SERVER); // NTP provides UTC; POSIX TZ handles offset (#98) applyTimezone(); // Ensure TZ is set after configTime + ntpPending = true; + ntpSince = millis(); + } + if (!webServerStarted) initWebServer(); +} - struct tm timeinfo; - if (getLocalTime(&timeinfo, 5000)) { - ntpSynced = true; - logPrintln("OK"); - - // Sync RTC from NTP (if GPS hasn't already synced it) - if (!rtcSyncedFromGPS && !rtcSyncedFromNTP) { - syncRTCFromSystemTime("NTP"); - rtcSyncedFromNTP = true; - } - } else { - logPrintln("FAILED"); +static void wifiServiceNtp() { + if (!ntpPending) return; + if ((unsigned long)time(nullptr) > NTP_EPOCH_SANE) { + ntpPending = false; + ntpSynced = true; + logPrintln("[NTP] Synced"); + // Sync RTC from NTP (if GPS hasn't already synced it) + if (!rtcSyncedFromGPS && !rtcSyncedFromNTP) { + syncRTCFromSystemTime("NTP"); + rtcSyncedFromNTP = true; } - } else { - logPrintln(" FAILED"); + } else if (millis() - ntpSince > NTP_TIMEOUT_MS) { + ntpPending = false; + logPrintln("[NTP] No answer in 15 s; GPS/RTC time stands"); } - - lastWiFiAttempt = millis(); } void checkWiFi() { - // Update connection status - wifiConnected = (WiFi.status() == WL_CONNECTED); - - // Attempt reconnect if disconnected - if (!wifiConnected && (millis() - lastWiFiAttempt > WIFI_RECONNECT_INTERVAL)) { - logPrintln("WiFi disconnected, attempting reconnect..."); - WiFi.reconnect(); - lastWiFiAttempt = millis(); - - // Wait briefly for connection - int attempts = 0; - while (WiFi.status() != WL_CONNECTED && attempts < 10) { - delay(500); - attempts++; - } + wl_status_t st = WiFi.status(); + unsigned long now = millis(); + switch (wifiState) { + case WIFI_ST_IDLE: + if (wifiStoreCount() > 0) wifiStartAttempt(0); // a network was added since boot + break; + + case WIFI_ST_CONNECTING: + if (st == WL_CONNECTED) { + wifiOnConnected(); + break; + } + if (st == WL_CONNECT_FAILED || st == WL_NO_SSID_AVAIL || + now - wifiStateSince > WIFI_CONNECT_TIMEOUT_MS) { + WifiCred c; + wifiStoreGet(wifiTryIndex, c); + logPrintf("[WIFI] %s: %s\n", c.ssid, + st == WL_NO_SSID_AVAIL ? "not in range" : + st == WL_CONNECT_FAILED ? "rejected (password?)" : "no answer in 8 s"); + int next = wifiTryIndex + 1; + if (next < wifiStoreCount()) { + wifiStartAttempt(next); + } else { + WiFi.disconnect(); + wifiState = WIFI_ST_WAIT; + wifiStateSince = now; + logPrintf("[WIFI] No saved network reachable; trying again in %d s\n", WIFI_RECONNECT_INTERVAL / 1000); + } + } + break; - wifiConnected = (WiFi.status() == WL_CONNECTED); - if (wifiConnected) { - logPrintln("WiFi reconnected!"); - // Start web server if not already running - if (!webServerStarted) { - initWebServer(); + case WIFI_ST_CONNECTED: + if (st != WL_CONNECTED) { + wifiConnected = false; + logPrintln("[WIFI] Connection lost; reconnecting"); + wifiStartAttempt(0); } - } - } + break; - // Ensure web server is started if WiFi is connected - if (wifiConnected && !webServerStarted) { - initWebServer(); + case WIFI_ST_WAIT: + if (now - wifiStateSince > WIFI_RECONNECT_INTERVAL) wifiStartAttempt(0); + break; } + + wifiServiceNtp(); + if (wifiConnected && !webServerStarted) initWebServer(); } void handleWebRoot() { diff --git a/src/wifi_store.cpp b/src/wifi_store.cpp new file mode 100644 index 0000000..35e5755 --- /dev/null +++ b/src/wifi_store.cpp @@ -0,0 +1,190 @@ +// wifi_store.cpp -- saved WiFi networks in NVS, and the one-shot SD import (#295, epic #99). +#include "wifi_store.h" +#include "logging.h" +#include +#include + +static const char* kNamespace = "wifi"; + +static Preferences prefs; +static WifiCred creds[WIFI_STORE_MAX]; +static int credCount = 0; + +int wifiImportedCount = 0; +bool wifiImportFilePresent = false; + +// Keys: "n" = count, "s0".."s4" = SSIDs, "p0".."p4" = passwords. The whole +// list is rewritten on every change; five entries is not worth a delta. +static void persist() { + if (!prefs.begin(kNamespace, false)) { + logPrintln("[WIFI] store: NVS open for write failed; change not saved"); + return; + } + prefs.clear(); + prefs.putUChar("n", (uint8_t)credCount); + char key[4]; + for (int i = 0; i < credCount; i++) { + snprintf(key, sizeof key, "s%d", i); + prefs.putString(key, creds[i].ssid); + snprintf(key, sizeof key, "p%d", i); + prefs.putString(key, creds[i].pass); + } + prefs.end(); +} + +void wifiStoreInit() { + credCount = 0; + memset(creds, 0, sizeof creds); + // Read-only open fails when the namespace has never been written: a fresh + // chip. That is "no saved networks", not an error. + if (!prefs.begin(kNamespace, true)) { + logPrintln("[WIFI] store: empty (nothing saved yet)"); + return; + } + int n = prefs.getUChar("n", 0); + if (n > WIFI_STORE_MAX) n = WIFI_STORE_MAX; + char key[4]; + for (int i = 0; i < n; i++) { + WifiCred c = {}; + snprintf(key, sizeof key, "s%d", i); + prefs.getString(key, c.ssid, sizeof c.ssid); + snprintf(key, sizeof key, "p%d", i); + prefs.getString(key, c.pass, sizeof c.pass); + if (c.ssid[0]) creds[credCount++] = c; // compacts any hole left by a bad write + } + prefs.end(); + logPrintf("[WIFI] store: %d saved network(s)\n", credCount); + for (int i = 0; i < credCount; i++) logPrintf(" %d: %s\n", i, creds[i].ssid); +} + +int wifiStoreCount() { + return credCount; +} + +bool wifiStoreGet(int idx, WifiCred& out) { + if (idx < 0 || idx >= credCount) return false; + out = creds[idx]; + return true; +} + +static int findSsid(const char* ssid) { + for (int i = 0; i < credCount; i++) { + if (strcmp(creds[i].ssid, ssid) == 0) return i; + } + return -1; +} + +bool wifiStoreAdd(const char* ssid, const char* pass) { + if (!ssid || !ssid[0] || strlen(ssid) > WIFI_SSID_MAX) return false; + if (!pass) pass = ""; + if (strlen(pass) > WIFI_PASS_MAX) return false; + int idx = findSsid(ssid); + if (idx >= 0) { + strlcpy(creds[idx].pass, pass, sizeof creds[idx].pass); // same network, new password; priority unchanged + } else { + if (credCount < WIFI_STORE_MAX) { + idx = credCount++; + } else { + idx = WIFI_STORE_MAX - 1; // full: the lowest-priority slot goes + logPrintf("[WIFI] store full; replacing %s\n", creds[idx].ssid); + } + memset(&creds[idx], 0, sizeof creds[idx]); + strlcpy(creds[idx].ssid, ssid, sizeof creds[idx].ssid); + strlcpy(creds[idx].pass, pass, sizeof creds[idx].pass); + } + persist(); + logPrintf("[WIFI] store: saved %s at priority %d\n", ssid, idx); + return true; +} + +bool wifiStoreRemove(int idx) { + if (idx < 0 || idx >= credCount) return false; + logPrintf("[WIFI] store: removed %s\n", creds[idx].ssid); + for (int i = idx; i < credCount - 1; i++) creds[i] = creds[i + 1]; + credCount--; + memset(&creds[credCount], 0, sizeof creds[credCount]); + persist(); + return true; +} + +bool wifiStoreMoveUp(int idx) { + if (idx <= 0 || idx >= credCount) return false; + WifiCred t = creds[idx - 1]; + creds[idx - 1] = creds[idx]; + creds[idx] = t; + persist(); + return true; +} + +void wifiStoreClear() { + credCount = 0; + memset(creds, 0, sizeof creds); + persist(); + logPrintln("[WIFI] store: all saved networks forgotten"); +} + +// ---- SD import ------------------------------------------------------------- + +// One line, CR/LF stripped, truncated to the buffer. Returns false at EOF +// with nothing read. +static bool readLine(File& f, char* buf, size_t len) { + size_t n = 0; + bool any = false; + while (f.available()) { + int c = f.read(); + if (c < 0) break; + any = true; + if (c == '\n') break; + if (c == '\r') continue; + if (n < len - 1) buf[n++] = (char)c; + } + buf[n] = '\0'; + return any; +} + +int wifiStoreImportFromSD() { + wifiImportedCount = 0; + wifiImportFilePresent = false; + if (!sdHealth.available) return -1; + if (!SD.exists(WIFI_IMPORT_PATH)) return -1; + wifiImportFilePresent = true; + + File f = sdOpenSafe(WIFI_IMPORT_PATH, "r", true); + if (!f) { + logPrintf("[WIFI] import: %s present but would not open\n", WIFI_IMPORT_PATH); + return -1; + } + + // "[n]" headers are ignored; every "ssid=" starts an entry and the next + // "ssid=" or the end of the file closes it. "#" lines are comments. + char line[WIFI_SSID_MAX + WIFI_PASS_MAX + 8]; + WifiCred cur = {}; + bool open = false; + int imported = 0; + while (readLine(f, line, sizeof line)) { + if (!line[0] || line[0] == '#' || line[0] == '[') continue; + if (strncmp(line, "ssid=", 5) == 0) { + if (open && cur.ssid[0] && wifiStoreAdd(cur.ssid, cur.pass)) imported++; + memset(&cur, 0, sizeof cur); + strlcpy(cur.ssid, line + 5, sizeof cur.ssid); + open = true; + } else if (strncmp(line, "pass=", 5) == 0) { + strlcpy(cur.pass, line + 5, sizeof cur.pass); + } + } + if (open && cur.ssid[0] && wifiStoreAdd(cur.ssid, cur.pass)) imported++; + f.close(); + + wifiImportedCount = imported; + logPrintf("[WIFI] import: %d network(s) from %s; file left in place until you say Remove\n", + imported, WIFI_IMPORT_PATH); + return imported; +} + +bool wifiImportFileRemove() { + if (!sdHealth.available) return false; + bool ok = SD.remove(WIFI_IMPORT_PATH); + if (ok) wifiImportFilePresent = false; + logPrintf("[WIFI] import: %s %s\n", WIFI_IMPORT_PATH, ok ? "removed" : "NOT removed"); + return ok; +} diff --git a/src/wifi_store.h b/src/wifi_store.h new file mode 100644 index 0000000..f9f6162 --- /dev/null +++ b/src/wifi_store.h @@ -0,0 +1,45 @@ +#pragma once +// wifi_store.h -- saved WiFi networks in NVS, and the one-shot SD import (#295, epic #99). +// +// The store is the ESP32's NVS through Preferences, namespace "wifi": up to +// WIFI_STORE_MAX entries in priority order (index 0 is tried first). Nothing +// here is in git or on the removable card; it is plain text on the flash chip +// by decision (docs/plans/2026-09-09-wifi-configuration.md, "encrypt on NVS?"). +// +// Pre-loading: /config/wifi.txt on the SD card, the #99 format: +// [0] +// ssid=MyNetwork +// pass=secret +// [1] +// ... +// wifiStoreImportFromSD() reads it at boot and adds the entries (deduplicated +// by SSID). The file is never modified or removed here: the owner is asked +// Remove or Keep on the screen (#297) and the web page (#296), and +// wifiImportFileRemove() is what those call. A repeat import is harmless. +// +// SSIDs may be logged. Passwords are never logged, never sent to a page, and +// never leave this unit except into WiFi.begin(). +#include + +#define WIFI_STORE_MAX 5 +#define WIFI_SSID_MAX 32 // 802.11 SSID limit +#define WIFI_PASS_MAX 63 // WPA2 passphrase limit +#define WIFI_IMPORT_PATH "/config/wifi.txt" + +struct WifiCred { + char ssid[WIFI_SSID_MAX + 1]; + char pass[WIFI_PASS_MAX + 1]; +}; + +void wifiStoreInit(); // open NVS, load the list (call once, before initWiFi's first attempt) +int wifiStoreCount(); +bool wifiStoreGet(int idx, WifiCred& out); // false if idx is out of range +bool wifiStoreAdd(const char* ssid, const char* pass); // same SSID: password updated in place; full: last slot replaced +bool wifiStoreRemove(int idx); +bool wifiStoreMoveUp(int idx); // swap with idx-1 (higher priority) +void wifiStoreClear(); // the "forget all" -- callers confirm first + +int wifiStoreImportFromSD(); // entries imported, or -1 when there is no file / no card +bool wifiImportFileRemove(); // delete /config/wifi.txt (the owner's Remove) +extern int wifiImportedCount; // result of the boot import, for the prompts +extern bool wifiImportFilePresent; // /config/wifi.txt still on the card From deac3d13c94d4159ee0e90bdb23f5c3a7065e1df Mon Sep 17 00:00:00 2001 From: Strycher Date: Wed, 9 Sep 2026 01:19:40 -0400 Subject: [PATCH 2/3] fix(#295): crash-safe store writes, BOM and whitespace in the import, NTP judged by the SNTP client Gemini gate on #295, four findings fixed: persist() no longer clears the namespace first and writes the count last, so a power cut mid-write leaves the previous list instead of an empty one; the import strips a UTF-8 BOM, trims trailing whitespace from an SSID, and logs when a password begins or ends with whitespace (kept as written); NTP sync is read from sntp_get_sync_status(), because the RTC sets the system clock at boot and a sane clock proved nothing. Three findings justified in the PR body: NVS wear (wear-levelled, unchanged values not rewritten), lost-link hysteresis (one begin per 8 s at worst, then the 30 s rest), unhandled transient statuses (the 8 s timeout bounds them). Both envs rebuilt: um 22.9/27.7, adafruit 31.8/64.9. Co-Authored-By: Claude Fable 5.1 --- src/web.cpp | 8 ++++++-- src/wifi_store.cpp | 42 ++++++++++++++++++++++++++++++------------ 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/web.cpp b/src/web.cpp index e2650c8..497f008 100644 --- a/src/web.cpp +++ b/src/web.cpp @@ -18,6 +18,7 @@ #include "touch.h" #include "geo.h" #include "wifi_store.h" // saved networks live in NVS, not here (#295, epic #99) +#include // sntp_get_sync_status(): did a time server actually answer const char* NTP_SERVER = "pool.ntp.org"; WebServer webServer(WEB_SERVER_PORT); @@ -47,7 +48,6 @@ static bool ntpPending = false; static unsigned long ntpSince = 0; #define WIFI_CONNECT_TIMEOUT_MS 8000 // per network, about what the old 15 x 500 ms gave #define NTP_TIMEOUT_MS 15000 -#define NTP_EPOCH_SANE 1700000000UL // 2023-11-14: the SNTP client has answered static void wifiStartAttempt(int idx) { WifiCred c; @@ -95,7 +95,11 @@ static void wifiOnConnected() { static void wifiServiceNtp() { if (!ntpPending) return; - if ((unsigned long)time(nullptr) > NTP_EPOCH_SANE) { + // The clock being sane proves nothing here: the RTC sets system time at + // boot and GPS may have too (review finding on #295). The SNTP client's own + // status says whether a server answered; it reads COMPLETED once, then + // resets, so it is polled every pass and acted on the pass it appears. + if (sntp_get_sync_status() == SNTP_SYNC_STATUS_COMPLETED) { ntpPending = false; ntpSynced = true; logPrintln("[NTP] Synced"); diff --git a/src/wifi_store.cpp b/src/wifi_store.cpp index 35e5755..6c0c94c 100644 --- a/src/wifi_store.cpp +++ b/src/wifi_store.cpp @@ -13,22 +13,25 @@ static int credCount = 0; int wifiImportedCount = 0; bool wifiImportFilePresent = false; -// Keys: "n" = count, "s0".."s4" = SSIDs, "p0".."p4" = passwords. The whole -// list is rewritten on every change; five entries is not worth a delta. +// Keys: "n" = count, "s0".."s4" = SSIDs, "p0".."p4" = passwords. Every entry +// is written in place and the count last, so a power cut mid-write leaves the +// previous count pointing at previous entries rather than an emptied +// namespace (review finding on #295). Stale keys above the count are ignored +// on load. NVS skips a write whose value is unchanged, so five put calls cost +// only the slots that moved. static void persist() { if (!prefs.begin(kNamespace, false)) { logPrintln("[WIFI] store: NVS open for write failed; change not saved"); return; } - prefs.clear(); - prefs.putUChar("n", (uint8_t)credCount); char key[4]; for (int i = 0; i < credCount; i++) { - snprintf(key, sizeof key, "s%d", i); - prefs.putString(key, creds[i].ssid); snprintf(key, sizeof key, "p%d", i); - prefs.putString(key, creds[i].pass); + prefs.putString(key, creds[i].pass); // password first: a cut between the two leaves the + snprintf(key, sizeof key, "s%d", i); // old SSID with the old password, never a new SSID + prefs.putString(key, creds[i].ssid); // with a stale one } + prefs.putUChar("n", (uint8_t)credCount); prefs.end(); } @@ -160,16 +163,31 @@ int wifiStoreImportFromSD() { char line[WIFI_SSID_MAX + WIFI_PASS_MAX + 8]; WifiCred cur = {}; bool open = false; + bool first = true; int imported = 0; while (readLine(f, line, sizeof line)) { - if (!line[0] || line[0] == '#' || line[0] == '[') continue; - if (strncmp(line, "ssid=", 5) == 0) { + char* p = line; + if (first) { // Notepad and friends write a UTF-8 BOM; it is not part of "ssid=" + first = false; + if ((uint8_t)p[0] == 0xEF && (uint8_t)p[1] == 0xBB && (uint8_t)p[2] == 0xBF) p += 3; + } + if (!p[0] || p[0] == '#' || p[0] == '[') continue; + if (strncmp(p, "ssid=", 5) == 0) { if (open && cur.ssid[0] && wifiStoreAdd(cur.ssid, cur.pass)) imported++; memset(&cur, 0, sizeof cur); - strlcpy(cur.ssid, line + 5, sizeof cur.ssid); + strlcpy(cur.ssid, p + 5, sizeof cur.ssid); + for (int n = strlen(cur.ssid); n > 0 && (cur.ssid[n - 1] == ' ' || cur.ssid[n - 1] == '\t'); n--) { + cur.ssid[n - 1] = '\0'; // an SSID never ends in whitespace on purpose; an editor's does + } open = true; - } else if (strncmp(line, "pass=", 5) == 0) { - strlcpy(cur.pass, line + 5, sizeof cur.pass); + } else if (strncmp(p, "pass=", 5) == 0) { + strlcpy(cur.pass, p + 5, sizeof cur.pass); + // A passphrase may legitimately contain spaces, so it is kept exactly; + // but say so, because a stray trailing space is the classic "rejected". + size_t n = strlen(cur.pass); + if (n && (cur.pass[0] == ' ' || cur.pass[n - 1] == ' ' || cur.pass[n - 1] == '\t')) { + logPrintf("[WIFI] import: password for %s begins or ends with whitespace, kept as written\n", cur.ssid); + } } } if (open && cur.ssid[0] && wifiStoreAdd(cur.ssid, cur.pass)) imported++; From e39a9f49c42b473b54d23c9d69d4bf0d4f4741e5 Mon Sep 17 00:00:00 2001 From: Strycher Date: Wed, 9 Sep 2026 03:01:59 -0400 Subject: [PATCH 3/3] fix(#295): two-bank store writes; BOM on any line; read the first build's layout Second gate pass: writing entries in place could still pair an old SSID with a new password when a power cut landed between the two keys of a shifted entry (remove, move-up, add-when-full). The list now goes whole into the bank that is not live and one key flips the live bank. A BOM is stripped at the start of any line, not only the first. The single-bank layout the bench build deac3d1 wrote is still read when no bank key exists. Both envs rebuilt: um 22.9/27.7, adafruit 31.8/65.0. Co-Authored-By: Claude Fable 5.1 --- src/wifi_store.cpp | 59 ++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/src/wifi_store.cpp b/src/wifi_store.cpp index 6c0c94c..f3fdb4f 100644 --- a/src/wifi_store.cpp +++ b/src/wifi_store.cpp @@ -13,25 +13,38 @@ static int credCount = 0; int wifiImportedCount = 0; bool wifiImportFilePresent = false; -// Keys: "n" = count, "s0".."s4" = SSIDs, "p0".."p4" = passwords. Every entry -// is written in place and the count last, so a power cut mid-write leaves the -// previous count pointing at previous entries rather than an emptied -// namespace (review finding on #295). Stale keys above the count are ignored -// on load. NVS skips a write whose value is unchanged, so five put calls cost -// only the slots that moved. +// Two banks, one atomic switch. Bank 0 uses keys "n0", "s0".."s4", "p0".."p4"; +// bank 1 uses "n1", "t0".."t4", "q0".."q4"; "bank" says which is live. A change +// is written whole into the bank that is NOT live, then "bank" is flipped, one +// NVS key write, which NVS makes atomic. A power cut anywhere before the flip +// leaves the live bank untouched; after it, the new bank is complete. The +// earlier in-place scheme could pair an old SSID with a new password when a +// cut landed between the two keys of a shifted entry (review finding on #295). +static uint8_t liveBank = 0; + +static void bankKey(char* key, size_t len, uint8_t bank, char kind, int i) { + // kind 's' = SSID, 'p' = password; bank 1 uses 't' and 'q' + char c = (bank == 0) ? kind : (kind == 's' ? 't' : 'q'); + snprintf(key, len, "%c%d", c, i); +} + static void persist() { if (!prefs.begin(kNamespace, false)) { logPrintln("[WIFI] store: NVS open for write failed; change not saved"); return; } + uint8_t target = liveBank ? 0 : 1; char key[4]; for (int i = 0; i < credCount; i++) { - snprintf(key, sizeof key, "p%d", i); - prefs.putString(key, creds[i].pass); // password first: a cut between the two leaves the - snprintf(key, sizeof key, "s%d", i); // old SSID with the old password, never a new SSID - prefs.putString(key, creds[i].ssid); // with a stale one + bankKey(key, sizeof key, target, 's', i); + prefs.putString(key, creds[i].ssid); + bankKey(key, sizeof key, target, 'p', i); + prefs.putString(key, creds[i].pass); } - prefs.putUChar("n", (uint8_t)credCount); + snprintf(key, sizeof key, "n%d", target); + prefs.putUChar(key, (uint8_t)credCount); + prefs.putUChar("bank", target); // the switch + liveBank = target; prefs.end(); } @@ -44,14 +57,22 @@ void wifiStoreInit() { logPrintln("[WIFI] store: empty (nothing saved yet)"); return; } - int n = prefs.getUChar("n", 0); - if (n > WIFI_STORE_MAX) n = WIFI_STORE_MAX; + liveBank = prefs.getUChar("bank", 0) ? 1 : 0; char key[4]; + snprintf(key, sizeof key, "n%d", liveBank); + int n = prefs.getUChar(key, 0); + if (!prefs.isKey("bank") && prefs.isKey("n")) { + // Written by the first #295 build (deac3d1, one bank, count under "n"): + // read it as bank 0; the next change rewrites it in the two-bank form. + n = prefs.getUChar("n", 0); + liveBank = 0; + } + if (n > WIFI_STORE_MAX) n = WIFI_STORE_MAX; for (int i = 0; i < n; i++) { WifiCred c = {}; - snprintf(key, sizeof key, "s%d", i); + bankKey(key, sizeof key, liveBank, 's', i); prefs.getString(key, c.ssid, sizeof c.ssid); - snprintf(key, sizeof key, "p%d", i); + bankKey(key, sizeof key, liveBank, 'p', i); prefs.getString(key, c.pass, sizeof c.pass); if (c.ssid[0]) creds[credCount++] = c; // compacts any hole left by a bad write } @@ -163,14 +184,12 @@ int wifiStoreImportFromSD() { char line[WIFI_SSID_MAX + WIFI_PASS_MAX + 8]; WifiCred cur = {}; bool open = false; - bool first = true; int imported = 0; while (readLine(f, line, sizeof line)) { char* p = line; - if (first) { // Notepad and friends write a UTF-8 BOM; it is not part of "ssid=" - first = false; - if ((uint8_t)p[0] == 0xEF && (uint8_t)p[1] == 0xBB && (uint8_t)p[2] == 0xBF) p += 3; - } + // Notepad and friends write a UTF-8 BOM at the top of a file, and a block + // pasted from such a file carries one mid-file; neither is part of "ssid=". + if ((uint8_t)p[0] == 0xEF && (uint8_t)p[1] == 0xBB && (uint8_t)p[2] == 0xBF) p += 3; if (!p[0] || p[0] == '#' || p[0] == '[') continue; if (strncmp(p, "ssid=", 5) == 0) { if (open && cur.ssid[0] && wifiStoreAdd(cur.ssid, cur.pass)) imported++;