7e93324288
- SPI_Bus_Lock: serialize LCD + SD on shared SPI; pause display during SD - SD_Init after WiFi scan; SD_IsReady gating; serial [SD]/[CAP] diagnostics - MUST_DO_IMPROVEMENTS.md full firmware review backlog; README link - Bump HANDSHAKE_FIRMWARE_VERSION to 1.0.2 (header) Made-with: Cursor
98 lines
4.8 KiB
Markdown
98 lines
4.8 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**.
|
||
|
||
---
|
||
|
||
## 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.*
|