aaribaud/ESPSomfy-RTS
0
0
Fork 0
mirror of https://github.com/rstrouse/ESPSomfy-RTS.git synced 2026-10-31 13:52:15 +01:00
Code Issues Projects Releases Packages Wiki Activity

fix: set rolling codes only on the main loop

Browse source 
Sending a command reads a remote's next rolling code, queues the frame and
commits the code, all on loop(). /setRollingCode, a shades restore and
/updateShadeConfig wrote the same codes from the web server task, so a set
could be undone by a command's commit, and a restore could save an older
code to NVS that survived the reboot. All three now go through the web
command queue to loop(). They don't use the radio, so they are never held
back or refused while it is busy.

/restore now answers after queueing the restore. A broken options string
used to send an error after the success answer had already gone out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
cjkas 2026-10-14 16:01:55 +02:00
parent 1128431e8c
commit 4a33b0d7dd
4 changed files with 120 additions and 69 deletions
Download patch file Download diff file Expand all files Collapse all files

29
docs/KNOWN_ISSUES.md
View file

@ -32,12 +32,11 @@ The new `ControllerChunker` reads state as the response drains. On a slow link o
- Same exposure exists in any other endpoint converted to chunked responses next (`/discovery`, `/shades`). Resolve this issue before expanding the pattern.
- Other handlers on the web server task still change the shade model that `loop()` owns:
- `/addRoom`, `/saveRoom`, `/deleteRoom`, `/addShade`, `/saveShade`, `/deleteShade`, `/addGroup`, `/saveGroup` and `/deleteGroup` add, change and remove rooms, shades and groups.
- `/linkRemote` and `/unlinkRemote` change a shade's linked remotes, and `/linkRemote` also sets a linked remote's rolling code.
- `/linkRemote` and `/unlinkRemote` change a shade's linked remotes. `/linkRemote` also sets a linked remote's rolling code, which `loop()` updates whenever it receives a frame from that remote: the last write wins, so a received frame can undo the code just set.
- `/linkToGroup` and `/unlinkFromGroup` change a group's linked shades.
- `/setPaired` sets `paired`.
- `/setRollingCode` and a shades restore change rolling codes (see [Rolling codes written from two tasks](#rolling-codes-written-from-two-tasks)).
Shade commands, set-My and unpair already go through the web command queue to `loop()`.
Shade commands, set-My, unpair, `/setRollingCode`, a shades restore and `/updateShadeConfig` already go through the web command queue to `loop()`.
- `/controller` leaves out a room, shade, group or linked shade that doesn't fit in its chunk buffer, see [Oversized `/controller` items are left out](#oversized-controller-items-are-left-out).
## Oversized `/controller` items are left out
@ -91,27 +90,3 @@ 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 `SomfyEvents.cpp`, `Transceiver.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/SomfyRemote.cpp](../src/SomfyRemote.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.
- `/linkRemote` sets a linked remote's rolling code on the web server task, while `loop()` updates the same code when it receives a frame from that remote. The last write wins, so a received frame can undo the code just set.