8121137b68
- /cap_bssids.txt append on save; handshakeLoadPersistedBssids after SD mount - Boot order: SD then load file then first WiFi scan (reds match SD) - Pre-save walk of in-memory PCAP counting EAPOL-Key frames - SD_AppendLine / SD_ForEachLine; drop g_sd_ready clear on write fail - README + MUST_DO updates Made-with: Cursor
117 lines
6.5 KiB
Markdown
117 lines
6.5 KiB
Markdown
# Must-do improvements (firmware review)
|
||
|
||
Full pass over `handshake-capture-c6`: capture logic, SD, UI, display SPI, and main loop. Items below are **simple in concept** (some need careful implementation). Ordered by **severity**.
|
||
|
||
## Status vs **v1.0.3** (re-review)
|
||
|
||
| # | Item | Status |
|
||
|---|------|--------|
|
||
| 1 | Serialize shared SPI (LCD + SD) | **Done** — `SPI_Bus_Lock.cpp` recursive mutex; all `LCD_Write*` paths lock; `LCD_addWindow` wraps full flush; `SD_Init` / `SD_WriteFileAtomic` lock + `SPIBus_SetDisplayPaused` so `Timer_Loop` skips LVGL during SD. |
|
||
| 2 | Cap / document `eapol_count` | **Done** — increment only while `eapol_count < HANDSHAKE_EAPOL_FRAMES`; README notes heuristic. |
|
||
| 3 | 2.4 GHz–only sweep | **Done** — `isSupportedCaptureChannel`, `isCaptureable` filters; list shows `5G+` + grey for off-band secure APs; status line `off-band` count. |
|
||
| 4 | Production vs debug serial | **Done** — `HANDSHAKE_DEBUG` (default 0) + `HANDSHAKE_LOGF` / `HANDSHAKE_LOGLN`; boot version line always. |
|
||
| 5 | `USE_SOFTAP` default | **Done** — default `0` in `HandshakeCapture.h`. |
|
||
| 6 | Long SD write / WDT / UI | **Partially** — `yield()` after large write in `SD_WriteFileAtomic`; WDT not explicitly fed (usually OK on Arduino main task). |
|
||
| 7 | Rescan vs `network_index` | **Unchanged** — still no mid-capture rescan; README covers behavior. |
|
||
| 8 | Magic numbers | **Done** — timing + channel range centralized in `HandshakeCapture.h`. |
|
||
|
||
### Remaining nits (optional follow-ups)
|
||
|
||
1. **`SD_WriteFileAtomic`:** On failure it sets `g_sd_ready = false` even for a **single-file** error (e.g. disk full, bad filename) while the card may still be mounted. Consider clearing `g_sd_ready` only on errors that imply unmount, or always pair with a defined recovery path.
|
||
2. **`SD_Card.cpp` → `HandshakeCapture.h`:** Include is only for log macros — a tiny `HandshakeLog.h` would remove coupling.
|
||
3. **`LCD_Reset`:** Still toggles CS without the SPI mutex (boot-only; low risk).
|
||
|
||
---
|
||
|
||
## 1. **Serialize shared SPI (LCD + SD)** — *critical*
|
||
|
||
**What:** ST7789 and the SD card share `SPI` (MOSI/SCLK/MISO; separate CS). **LVGL** flushes the display from `Timer_Loop()` → `LCD_*` → SPI, while **`SD.open` / `write` / `close`** use the same bus.
|
||
|
||
**Risk:** Interleaved transactions corrupt SD I/O or garble a display frame → failed mounts, bogus PCAP writes, or rare crashes.
|
||
|
||
**Do:** Add a single **mutex** (e.g. `portMUX_TYPE` or FreeRTOS mutex) that **every** LCD SPI transfer and **every** SD filesystem call takes around the full operation (including `SD.begin` path if the core does SPI under the hood). Alternatively: **pause LVGL** (skip `lv_timer_handler` / block flush) for the duration of each SD block in `handshakeCaptureProcessPending()`.
|
||
|
||
**Files:** `Display_ST7789.cpp`, `SD_Card.cpp`, `handshake-capture-c6.ino` (or one small `spi_bus_lock.h` used by both).
|
||
|
||
---
|
||
|
||
## 2. **Cap or validate `eapol_count` semantics** — *high*
|
||
|
||
**What:** Any unencrypted EAPOL-shaped frame increments the counter. **Four frames ≠ guaranteed one valid 4-way** (could be duplicates, other STAs, or rekeys).
|
||
|
||
**Do (minimal):** Document the limitation in `README.md`. **Better (still small):** stop incrementing after `HANDSHAKE_EAPOL_FRAMES` (saves buffer space and makes UI honest). **Better later:** parse EAPOL Key Info / replay counter to count **distinct** steps of the handshake (more code).
|
||
|
||
**Files:** `HandshakeCapture.cpp` (promisc callback EAPOL branch).
|
||
|
||
---
|
||
|
||
## 3. **2.4 GHz–only channel sweep** — *high for real environments*
|
||
|
||
**What:** `getNextChannelSweep()` only considers channels **1–14**. Scan results on **5 GHz** (or 6 GHz, if ever reported) are never visited, so those BSSIDs are never captured.
|
||
|
||
**Do:** Either **exclude** non-2.4G networks from the UI / `isCaptureable` path with a clear status, or **extend** sweep to supported `WiFi.channel()` values for your core/regulatory config (larger change: channel hop rules, dwell time).
|
||
|
||
**Files:** `HandshakeCapture.cpp` (`getNextChannelSweep`, `startMultiCapture`).
|
||
|
||
---
|
||
|
||
## 4. **Production vs debug serial** — *medium*
|
||
|
||
**What:** `[SD]`, `[CAP]`, `[SETUP]` strings and deauth logs are useful on the bench but noisy and slightly slow in the field.
|
||
|
||
**Do:** Wrap verbose `Serial.printf` / `println` in `#ifdef HANDSHAKE_DEBUG` (or a build flag) default **off** for “release” builds; keep boot **version line** always.
|
||
|
||
**Files:** `SD_Card.cpp`, `HandshakeCapture.cpp`, `handshake-capture-c6.ino`.
|
||
|
||
---
|
||
|
||
## 5. **`USE_SOFTAP` default** — *medium*
|
||
|
||
**What:** `WiFi.mode(WIFI_AP_STA)` runs a soft AP **without** a clear use case in this sketch (capture is STA + promisc). Extra RF/channel behavior and attack surface.
|
||
|
||
**Do:** Default `#define USE_SOFTAP 0` unless you explicitly need AP mode; document how to enable for lab tests.
|
||
|
||
**Files:** `HandshakeCapture.cpp`, `README.md`.
|
||
|
||
---
|
||
|
||
## 6. **Long SD write vs watchdog / UI** — *medium*
|
||
|
||
**What:** Writing ~4 KB PCAP can block the main loop for tens of ms. LVGL stalls; on some configs the **task watchdog** could trip if SD is slow or the card hangs.
|
||
|
||
**Do:** After large `file.write`, optional `yield()`; if WDT issues appear, `esp_task_wdt_reset()` in the SD path **only** with measured care, or increase WDT timeout in `setup`. Prefer fixing SPI contention (#1) first.
|
||
|
||
**Files:** `HandshakeCapture.cpp` (`handshakeCaptureProcessPending`), `setup()`.
|
||
|
||
---
|
||
|
||
## 7. **Rescan invalidates “ready” list vs slots** — *low*
|
||
|
||
**What:** During capture, the network list is static. If user triggers rescan (or you add a button later), `network_index` in slots could desync from rebuilt `networks[]`.
|
||
|
||
**Do:** Today there is no mid-capture rescan from UI — **document** that. If rescan is added, **stop capture** first and clear slots.
|
||
|
||
**Files:** `README.md` / future UI code.
|
||
|
||
---
|
||
|
||
## 8. **Magic numbers centralization** — *low (maintainability)*
|
||
|
||
**What:** Timing (`OBSERVE_DURATION_MS`, `CAPTURE_TIMEOUT_MS`, …) and `CHANNEL_TIMEOUT_MS` in the `.ino` are related but defined in two places; easy to desync.
|
||
|
||
**Do:** Move all timing `#define`s to one header (e.g. `HandshakeCapture.h` or `capture_config.h`) and include from the sketch.
|
||
|
||
**Files:** `HandshakeCapture.cpp`, `handshake-capture-c6.ino`.
|
||
|
||
---
|
||
|
||
### Already in decent shape
|
||
|
||
- Spinlock around promiscuous vs main-loop slot state; SD I/O only on main loop.
|
||
- `SD_IsReady()` vs flaky `cardType()` after WiFi; FAT filename sanitization; discard on failed save (no pending deadlock).
|
||
- PCAP fixed buffer; append failure doesn’t increment EAPOL count.
|
||
|
||
---
|
||
|
||
*Add new items here as you close these; bump `HANDSHAKE_FIRMWARE_VERSION` when behavior changes.*
|