mirror of
https://github.com/rstrouse/ESPSomfy-RTS.git
synced 2026-10-31 13:52:15 +01:00
fix: give every NVS user its own Preferences handle
loop() and the async web server task shared one global Preferences. A begin() from one task failed while the other had a namespace open, and the clear() or remove() after it then hit the other task's namespace: saving the radio settings while a command committed its rolling code could erase every rolling code. Each function now opens NVS with a local Preferences. NVS serializes access internally and the destructor closes the handle, so no lock is added (see docs/adr/0001). No new tests: nothing here builds natively, and deleting the global makes any missed use a compile error. The read-then-commit races on rolling codes across tasks remain and are tracked in docs/KNOWN_ISSUES.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
4135f21ee7
commit
c1b47f898c
6 changed files with 94 additions and 27 deletions
|
|
@ -57,3 +57,26 @@ Unlike the `/controller` HTTP crash, this path does **not** abort — there is n
|
|||
|
||||
- Not the same code path as the `/controller` crash. Solve independently.
|
||||
- Worth grepping for `JsonSockEvent` usages that iterate collections (see references in `Somfy.cpp`, `ESPNetwork.cpp`, `GitOTA.cpp`) to identify the most at-risk events.
|
||||
|
||||
## Rolling codes written from two tasks
|
||||
|
||||
**Status:** open
|
||||
**Location:** `SomfyRemote::sendCommandAfter` and `setRollingCode` in [src/Somfy.cpp](../src/Somfy.cpp), the `/setRollingCode` handlers in [src/Web.cpp](../src/Web.cpp), and `ShadeConfigFile::readShadeRecord`/`readGroupRecord` in [src/ConfigFile.cpp](../src/ConfigFile.cpp).
|
||||
|
||||
### Summary
|
||||
|
||||
Sending a command reads the next rolling code, queues the frame, then commits the code (`peekNextRollingCode` → `submitTx` → `commitRollingCode`), all on `loop()`. `/setRollingCode` and a shades restore change the same remote's `lastRollingCode` and its `ShadeCodes` NVS key from the async web server task, with no lock against that sequence. Each NVS write is safe on its own, since every function opens its own handle (see [ADR-0001](adr/0001-local-nvs-handles-no-wrapper.md)), but the read-then-commit sequence is not.
|
||||
|
||||
### Concrete failure modes
|
||||
|
||||
1. `/setRollingCode` lands between the peek and the commit of a command. The command's commit overwrites the code just set, so the set has no effect.
|
||||
2. A restore reads `ShadeCodes` while a command commits a newer code. The restored shade can come back with the older code, and the motor rejects its frames until the code passes the newer one.
|
||||
|
||||
### Fix options (pick one later)
|
||||
|
||||
- **(a) Run these handlers on `loop()`.** Queue `/setRollingCode` and restore through the web command queue like shade commands, so `loop()` is the only task that touches a remote's rolling code. Part of making `loop()` the only owner of the shade model.
|
||||
- **(b) A rolling-code lock.** Hold a mutex across peek → queue → commit, and in `setRollingCode` and restore. Narrower, but it adds a lock to the command path.
|
||||
|
||||
### Related
|
||||
|
||||
- Same root cause as [Concurrent mutation during chunked `/controller` response](#concurrent-mutation-during-chunked-controller-response): the web server task changes the shade model that `loop()` owns.
|
||||
|
|
|
|||
12
docs/adr/0001-local-nvs-handles-no-wrapper.md
Normal file
12
docs/adr/0001-local-nvs-handles-no-wrapper.md
Normal file
|
|
@ -0,0 +1,12 @@
|
|||
# Each function opens NVS with its own Preferences handle, with no lock or wrapper
|
||||
|
||||
A single global `Preferences pref` was used from both `loop()` and the async web server task. A `begin()` from one task could fail while the other task had a namespace open, and its `clear()` or `remove()` then hit the wrong namespace (a radio settings save could erase every rolling code). We replaced the global with a local `Preferences` in each function that uses NVS. We did not put an app-level lock or a wrapper module around it, because ESP-IDF's NVS API already serializes access internally (`nvs::Lock`), each `Preferences` holds its own handle, and its destructor calls `end()`.
|
||||
|
||||
## Considered Options
|
||||
|
||||
- **Global handle plus a mutex-backed `NvsSession`.** Rejected: the lock would only protect the shared handle, which is the actual bug, and it would block `loop()`'s rolling-code writes while `ConfigFile` keeps `ShadeCodes` open across LittleFS reads. Remove the shared handle and nothing is left for the lock to do.
|
||||
- **An `NvsStore` seam with typed accessors and an in-memory adapter for native tests.** Rejected: it would have exactly one real adapter, so the seam would be hypothetical, and deleting it would just give back `Preferences`. The only NVS logic worth testing (the next rolling code) is already tested natively through `RollingCode.h`, which takes its loader as a parameter.
|
||||
|
||||
## Consequences
|
||||
|
||||
Separate handles make each NVS operation safe on its own, but not a read-then-write sequence across tasks (rolling-code peek/commit in `loop()` against `/setRollingCode` or a restore on the web server task). Those races belong to the model, not to NVS, and are tracked in `docs/KNOWN_ISSUES.md`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue