diff --git a/docs/adr.md b/docs/adr.md index be3c455..0eae792 100644 --- a/docs/adr.md +++ b/docs/adr.md @@ -358,9 +358,16 @@ derived key. - **FatFS caveat (Spike 3, verified 2026-07-11):** FatFS's `f_rename` returns `FR_EXIST` on an existing destination — it does **not** replace like POSIX `rename(2)`. So the atomic save must `f_unlink` the target before renaming the - `*.tmp` over it, and pair that with **boot recovery**: a lingering `*.tmp` at - startup means the last save didn't finish → promote it (it is the newest - complete, fsync'd copy). See the + `*.tmp` over it, and pair that with **boot recovery** of a lingering `*.tmp`. + Recovery is *not* simply "promote the tmp": a crash *during* the tmp write + leaves a partial tmp, so the choice depends on whether the target survived — + - **tmp + target both present** → the crash could have been mid-write, so the + tmp is untrustworthy. Keep the committed target, discard the tmp (this is the + documented "you get the previous version" behaviour). + - **tmp only, target absent** → the target was already unlinked, so the tmp is + the newest complete, fsync'd copy and the only one left. Promote it. + + Implemented in `firmware::persistence::Storage::{save,recover}`. See the [Spike 3 postmortem](postmortems/2026-07-05-spike3-sd-cmd59.md#resolution-2026-07-11). - **SD-card compatibility:** use a genuine card, ideally **≤32 GB (SDHC/FAT32)**. Large or counterfeit SDXC cards may reject `CMD59` (SPI-mode CRC) and fail to diff --git a/docs/postmortems/2026-07-05-spike3-sd-cmd59.md b/docs/postmortems/2026-07-05-spike3-sd-cmd59.md index 744b561..6891b53 100644 --- a/docs/postmortems/2026-07-05-spike3-sd-cmd59.md +++ b/docs/postmortems/2026-07-05-spike3-sd-cmd59.md @@ -39,10 +39,18 @@ as-is on FAT; the destination must be unlinked first. That unlink opens a crash window: between `f_unlink(target)` and `f_rename`, the target is momentarily gone while `*.tmp` holds the complete, fsync'd new content. -The consequence for the real persistence module is a **boot-recovery** rule: a -lingering `*.tmp` at startup means the last save didn't finish → promote it -(it's the newest complete copy). The spike now does unlink-then-rename and -tolerates a missing target, so re-runs are idempotent; recorded in +The consequence for the real persistence module is a **boot-recovery** rule for a +lingering `*.tmp`. Implementing it (`firmware::persistence::Storage::recover`) +surfaced that "promote the tmp" is too blunt: a crash *during* the tmp write (a +second window, before the fsync) leaves a **partial** tmp, so recovery must look +at whether the target survived — +- **tmp + target both present** → crash point ambiguous, tmp may be partial → + keep the committed target, discard the tmp (the "previous version" guarantee); +- **tmp only, target absent** → target was already unlinked → tmp is the newest + complete copy → promote it. + +The spike now does unlink-then-rename and tolerates a missing target, so re-runs +are idempotent; recorded in [ADR-007](../adr.md#adr-007-storage-split--fat-on-sd-for-working-copy-littlefs-on-flash-for-config). ## Summary diff --git a/docs/v0.1-mvp-technical.md b/docs/v0.1-mvp-technical.md index cc794bd..5336080 100644 --- a/docs/v0.1-mvp-technical.md +++ b/docs/v0.1-mvp-technical.md @@ -163,11 +163,18 @@ in [qfd.md §3](qfd.md#3-house-of-quality--whats--hows). Storage split rationale: [ADR-007](adr.md#adr-007-storage-split--fat-on-sd-for-working-copy-littlefs-on-flash-for-config). -- Atomic save: write to `notes.md.tmp`, fsync, rename. We accept FAT's - weakness here; on power loss between rename and dir flush, the user gets - the previous version, which is the documented behavior. +Implemented in [`firmware::persistence`](../firmware/src/persistence.rs) +(`Storage::{mount, load, save, recover}`), graduated from Spike 3 and +hardware-verified 2026-07-11. + +- Atomic save: write to `notes.md.tmp`, fsync, unlink the target (FatFS + `f_rename` won't overwrite — ADR-007), rename. On power loss the user gets + the previous version, the documented behavior. Boot recovery reconciles a + leftover `*.tmp`: kept-target-if-both-present, promote-if-target-gone (see + ADR-007 for why "just promote" is unsafe). - The file is read fully into the rope at boot. v0.1 caps file size at - 256 KB; larger files refuse to open with a clear message. + 256 KB; larger files refuse to open with a clear message. Saving is not + capped — the buffer is always persistable. ### `wifi` — on-demand station diff --git a/firmware/README.md b/firmware/README.md index 362abca..3b7fc79 100644 --- a/firmware/README.md +++ b/firmware/README.md @@ -66,13 +66,15 @@ of it. `sdkconfig.defaults` gains the full certificate bundle and a bigger main task stack for the mbedtls handshake — a one-time esp-idf reconfigure on the next build. -**Spike 3 — SD card (FAT) on shared SPI2: verified 2026-07-11.** A separate +**Spike 3 — SD card (FAT) on dedicated SPI3: verified 2026-07-11.** A separate binary — [`src/bin/sd_fat.rs`](src/bin/sd_fat.rs), flashed with `just flash-sd` — -brings up the SD card, mounts FAT at `/sd`, and exercises the persistence -module's atomic save (write `*.tmp` → fsync → rename → read-back). Per ADR-012 -the SD runs on its **own SPI3 host** — **SCK 14 · MOSI 15 · MISO 13 · SD CS 10** -— leaving the EPD alone on SPI2. Verified on the dedicated SPI3 bus 2026-07-11 -(same mount + round-trip result as the initial shared-SPI2 bring-up). +is now a thin on-device harness over the real +[`firmware::persistence`](src/persistence.rs) module: it mounts the card, reports +FAT usage, and round-trips an atomic save/load (write `*.tmp` → fsync → unlink → +rename → read-back). Per ADR-012 the SD runs on its **own SPI3 host** — +**SCK 14 · MOSI 15 · MISO 13 · SD CS 10** — leaving the EPD alone on SPI2. +Verified on the dedicated SPI3 bus 2026-07-11 (same mount + round-trip result as +the initial shared-SPI2 bring-up). Bench result (genuine 32 GB SDHC card): mounts at 10 MHz, `29806 MiB total`, atomic round-trip byte-identical. Two findings baked into the code: @@ -82,8 +84,11 @@ atomic round-trip byte-identical. Two findings baked into the code: with a swap-the-card message rather than run over an unchecked bus. See the [Spike 3 postmortem](../docs/postmortems/2026-07-05-spike3-sd-cmd59.md). - **FatFS rename ≠ POSIX rename.** `f_rename` won't overwrite an existing - target (returns `FR_EXIST`), so the atomic save unlinks the destination first; - the real persistence module must add `*.tmp` boot-recovery. Long filenames + target (returns `FR_EXIST`), so the atomic save unlinks the destination first. + `firmware::persistence` pairs this with `*.tmp` boot-recovery + (`Storage::recover`): if a `*.tmp` is found *alongside* the target the crash + may have been mid-write, so it keeps the committed file and discards the tmp; + it only promotes the tmp when the target was already unlinked. Long filenames (`CONFIG_FATFS_LFN_HEAP`) are required for the two-dot `*.md.tmp` name. **Arbitration resolved (ADR-012):** the EPD driver holds an exclusive SPI2 lock