From 4a33b0d7ddbeeee80a3a0a858aa301244b804cbf Mon Sep 17 00:00:00 2001 From: cjkas Date: Wed, 14 Oct 2026 16:01:55 +0200 Subject: [PATCH] fix: set rolling codes only on the main loop 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 --- docs/KNOWN_ISSUES.md | 29 +-------- src/Web.cpp | 108 ++++++++++++++++++++++--------- src/WebCommand.h | 27 +++++++- test/test_webcmd/test_webcmd.cpp | 25 ++++--- 4 files changed, 120 insertions(+), 69 deletions(-) diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index 9c8bc86..5d5bd1e 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -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. diff --git a/src/Web.cpp b/src/Web.cpp index d122592..f4d4025 100644 --- a/src/Web.cpp +++ b/src/Web.cpp @@ -51,12 +51,12 @@ static QueueHandle_t webCmdQueue = nullptr; static void sendQueueFull(AsyncWebServerRequest *request); static void sendRadioUnavailable(AsyncWebServerRequest *request); -// Hands a command that transmits to the main loop. When it can't be the -// request is answered with the reason and false is returned. -static bool queueRadioCommand(AsyncWebServerRequest *request, const web_command_t &cmd) { +// Hands a command to the main loop, which owns the shades. When it can't be +// the request is answered with the reason and false is returned. +static bool queueLoopCommand(AsyncWebServerRequest *request, const web_command_t &cmd) { // Nothing can go on air during a frequency scan so tell the client now rather // than dropping the command once it reaches the radio. - if(somfy.transceiver.txSuspended()) { sendRadioUnavailable(request); return false; } + if(webCommandUsesRadio(cmd.type) && somfy.transceiver.txSuspended()) { sendRadioUnavailable(request); return false; } if(!webServer.queueCommand(cmd)) { sendQueueFull(request); return false; } return true; } @@ -67,7 +67,7 @@ static bool queueSetMyPosition(AsyncWebServerRequest *request, SomfyShade *shade cmd.shadeId = shade->getShadeId(); cmd.position = pos; cmd.tiltPosition = tilt; - return queueRadioCommand(request, cmd); + return queueLoopCommand(request, cmd); } // The main loop clears paired and saves it once the Prog is queued. The // shade belongs to the main loop so it isn't changed here. @@ -75,7 +75,32 @@ static bool queueUnpairShade(AsyncWebServerRequest *request, SomfyShade *shade) web_command_t cmd = {}; cmd.type = web_cmd_t::unpair_shade; cmd.shadeId = shade->getShadeId(); - return queueRadioCommand(request, cmd); + return queueLoopCommand(request, cmd); +} +// Sending a command reads and commits the rolling code on the main loop, so +// a new code is set there too. +static bool queueSetRollingCode(AsyncWebServerRequest *request, SomfyShade *shade, uint16_t rollingCode) { + web_command_t cmd = {}; + cmd.type = web_cmd_t::set_rolling_code; + cmd.shadeId = shade->getShadeId(); + cmd.rollingCode = rollingCode; + return queueLoopCommand(request, cmd); +} +// A restore's options travel through the command queue as RESTORE_* bits. +static uint8_t restoreParts(const restore_options_t &opts) { + return (opts.settings ? RESTORE_SETTINGS : 0) | (opts.shades ? RESTORE_SHADES : 0) | + (opts.network ? RESTORE_NETWORK : 0) | (opts.transceiver ? RESTORE_TRANSCEIVER : 0) | + (opts.repeaters ? RESTORE_REPEATERS : 0) | (opts.mqtt ? RESTORE_MQTT : 0); +} +static restore_options_t restoreOptions(uint8_t parts) { + restore_options_t opts; + opts.settings = parts & RESTORE_SETTINGS; + opts.shades = parts & RESTORE_SHADES; + opts.network = parts & RESTORE_NETWORK; + opts.transceiver = parts & RESTORE_TRANSCEIVER; + opts.repeaters = parts & RESTORE_REPEATERS; + opts.mqtt = parts & RESTORE_MQTT; + return opts; } // The radio and MQTT are shut down once an upload starts and only a reboot // brings them back. The completion handler schedules it but never runs if the @@ -220,6 +245,22 @@ void Web::processQueue() { } break; } + case web_cmd_t::set_rolling_code: { + SomfyShade *shade = somfy.getShadeById(cmd.shadeId); + if(shade) shade->setRollingCode(cmd.rollingCode); + break; + } + case web_cmd_t::restore_shades: { + restore_options_t opts = restoreOptions(cmd.restoreParts); + ShadeConfigFile::restore(&somfy, "/shades.tmp", opts); + ESP_LOGI(TAG, "Rebooting ESP for restored settings..."); + rebootDelay.reboot = true; + rebootDelay.rebootTime = millis() + 1000; + break; + } + case web_cmd_t::load_shades: + somfy.loadShadesFile("/shades.tmp"); + break; } if(!sent) ESP_LOGW(TAG, "Radio didn't take web command %u", static_cast(cmd.type)); } @@ -310,10 +351,10 @@ static void serializeShadeRef(SomfyShade *shade, JsonFormatter &json) { json.addElem("hasLight", shade->hasLight()); json.addElem("repeats", shade->repeats); } -// paired is passed in for a response that reports the change the main loop -// is about to make. The unpair is held until it goes on air (holdWebCommand), -// so the answer comes true. -static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired) { +// paired and lastRollingCode are passed in for a response that reports the +// change the main loop is about to make. An unpair is held until it goes on +// air (holdWebCommand), so the answer comes true. +static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired, uint16_t lastRollingCode) { json.addElem("shadeId", shade->getShadeId()); json.addElem("roomId", shade->roomId); json.addElem("name", shade->name); @@ -321,7 +362,7 @@ static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired) json.addElem("upTime", (uint32_t)shade->upTime); json.addElem("downTime", (uint32_t)shade->downTime); json.addElem("paired", paired); - json.addElem("lastRollingCode", (uint32_t)shade->lastRollingCode); + json.addElem("lastRollingCode", (uint32_t)lastRollingCode); json.addElem("position", shade->transformPosition(shade->currentPos)); json.addElem("tiltType", static_cast(shade->tiltType)); json.addElem("tiltPosition", shade->transformPosition(shade->currentTiltPos)); @@ -361,7 +402,7 @@ static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired) } json.endArray(); } -static void serializeShade(SomfyShade *shade, JsonFormatter &json) { serializeShade(shade, json, shade->paired); } +static void serializeShade(SomfyShade *shade, JsonFormatter &json) { serializeShade(shade, json, shade->paired, shade->lastRollingCode); } static void serializeGroupRef(SomfyGroup *group, JsonFormatter &json) { group->updateFlags(); json.addElem("groupId", group->getGroupId()); @@ -890,7 +931,7 @@ void Web::handleShadeCommand(AsyncWebServerRequest *request, JsonVariant &json) cmd.command = command; cmd.repeat = repeat; cmd.stepSize = stepSize; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); @@ -931,7 +972,7 @@ void Web::handleGroupCommand(AsyncWebServerRequest *request, JsonVariant &json) cmd.command = command; cmd.repeat = repeat; cmd.stepSize = stepSize; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); @@ -968,7 +1009,7 @@ void Web::handleTiltCommand(AsyncWebServerRequest *request, JsonVariant &json) { cmd.shadeId = shadeId; cmd.target = target; cmd.command = command; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); @@ -1009,7 +1050,7 @@ void Web::handleRepeatCommand(AsyncWebServerRequest *request, JsonVariant &json) cmd.command = command; cmd.repeat = repeat; cmd.stepSize = stepSize; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); @@ -1027,7 +1068,7 @@ void Web::handleRepeatCommand(AsyncWebServerRequest *request, JsonVariant &json) cmd.command = command; cmd.repeat = repeat; cmd.stepSize = stepSize; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); @@ -1166,7 +1207,7 @@ void Web::handleSetSensor(AsyncWebServerRequest *request, JsonVariant &json) { cmd.sunny = sunny; cmd.windy = windy; cmd.repeat = repeat; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; // The main loop applies the command after we answer so this is the state // before it. Clients get the result from the shadeState socket event. AsyncJsonResp resp; @@ -1187,7 +1228,7 @@ void Web::handleSetSensor(AsyncWebServerRequest *request, JsonVariant &json) { cmd.sunny = sunny; cmd.windy = windy; cmd.repeat = repeat; - if(!queueRadioCommand(request, cmd)) return; + if(!queueLoopCommand(request, cmd)) return; // As above, this is the state before the command and groupState follows. AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); @@ -1449,7 +1490,6 @@ void Web::begin() { asyncServer.on("/restore", HTTP_POST, [](AsyncWebServerRequest *request) { if(webServer.uploadSuccess) { - request->send(200, _encoding_json, "{\"status\":\"Success\",\"desc\":\"Restoring Shade settings\"}"); restore_options_t opts; if(asyncHasParam(request, "data")) { String dataStr = asyncParam(request, "data"); @@ -1469,10 +1509,13 @@ void Web::begin() { ESP_LOGD(TAG, "No restore options sent. Using defaults..."); opts.shades = true; } - ShadeConfigFile::restore(&somfy, "/shades.tmp", opts); - ESP_LOGI(TAG, "Rebooting ESP for restored settings..."); - rebootDelay.reboot = true; - rebootDelay.rebootTime = millis() + 1000; + // The main loop restores, since it owns the shades and their rolling + // codes, and then reboots. + web_command_t cmd = {}; + cmd.type = web_cmd_t::restore_shades; + cmd.restoreParts = restoreParts(opts); + if(!queueLoopCommand(request, cmd)) return; + request->send(200, _encoding_json, "{\"status\":\"Success\",\"desc\":\"Restoring Shade settings\"}"); } }, [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) { @@ -1790,11 +1833,11 @@ void Web::begin() { request->send(500, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Shade not found to set rolling code\"}")); } else { - shade->setRollingCode(rollingCode); + if(!queueSetRollingCode(request, shade, rollingCode)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); - serializeShade(shade, resp); + serializeShade(shade, resp, shade->paired, rollingCode); resp.endObject(); resp.endResponse(); } @@ -1812,11 +1855,11 @@ void Web::begin() { request->send(500, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Shade not found to set rolling code\"}")); } else { - shade->setRollingCode(rollingCode); + if(!queueSetRollingCode(request, shade, rollingCode)) return; AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); - serializeShade(shade, resp); + serializeShade(shade, resp, shade->paired, rollingCode); resp.endObject(); resp.endResponse(); } @@ -1890,7 +1933,7 @@ void Web::begin() { AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); - serializeShade(shade, resp, false); + serializeShade(shade, resp, false, shade->lastRollingCode); resp.endObject(); resp.endResponse(); } @@ -1909,7 +1952,7 @@ void Web::begin() { AsyncJsonResp resp; resp.beginResponse(request, g_async_content, sizeof(g_async_content)); resp.beginObject(); - serializeShade(shade, resp, false); + serializeShade(shade, resp, false, shade->lastRollingCode); resp.endObject(); resp.endResponse(); } @@ -2282,7 +2325,10 @@ void Web::begin() { fup.close(); } if(final) { - somfy.loadShadesFile("/shades.tmp"); + // The main loop loads it, since it owns the shades. + web_command_t cmd = {}; + cmd.type = web_cmd_t::load_shades; + webServer.queueCommand(cmd); } }); diff --git a/src/WebCommand.h b/src/WebCommand.h index 982a4c8..580c0aa 100644 --- a/src/WebCommand.h +++ b/src/WebCommand.h @@ -22,8 +22,19 @@ enum class web_cmd_t : uint8_t { group_sensor, // group sensor command set_my_position, // shade setMyPosition (position/tiltPosition) unpair_shade, // shade unpair (Prog) and clear paired flag + set_rolling_code, // set a shade's rolling code + restore_shades, // restore from /shades.tmp, then reboot + load_shades, // replace the shades with /shades.tmp }; +// What a restore_shades command restores, as bits of restoreParts. +#define RESTORE_SETTINGS 0x01 +#define RESTORE_SHADES 0x02 +#define RESTORE_NETWORK 0x04 +#define RESTORE_TRANSCEIVER 0x08 +#define RESTORE_REPEATERS 0x10 +#define RESTORE_MQTT 0x20 + struct web_command_t { web_cmd_t type; uint8_t shadeId; @@ -36,10 +47,22 @@ struct web_command_t { int8_t tiltPosition; // for setPositions/tilt int8_t sunny; // for sensor int8_t windy; // for sensor + uint16_t rollingCode; // for set_rolling_code + uint8_t restoreParts; // for restore_shades: RESTORE_* bits }; -// Only setting positions never goes on air. -inline bool webCommandUsesRadio(web_cmd_t type) { return type != web_cmd_t::set_positions; } +// Whether the command sends anything on air. +inline bool webCommandUsesRadio(web_cmd_t type) { + switch(type) { + case web_cmd_t::set_positions: + case web_cmd_t::set_rolling_code: + case web_cmd_t::restore_shades: + case web_cmd_t::load_shades: + return false; + default: + return true; + } +} // Whether the command at the head of the queue waits for the radio. The // client was already told it was accepted so it stays queued until the radio // has room rather than have the radio drop it. While TX is suspended nothing diff --git a/test/test_webcmd/test_webcmd.cpp b/test/test_webcmd/test_webcmd.cpp index 7073ad2..40c86d3 100644 --- a/test/test_webcmd/test_webcmd.cpp +++ b/test/test_webcmd/test_webcmd.cpp @@ -10,17 +10,24 @@ static const web_cmd_t RADIO_COMMANDS[] = { void setUp() {} void tearDown() {} -static void test_only_set_positions_stays_off_the_radio() { +static const web_cmd_t LOOP_COMMANDS[] = { + web_cmd_t::set_positions, web_cmd_t::set_rolling_code, web_cmd_t::restore_shades, web_cmd_t::load_shades +}; + +static void test_only_loop_commands_stay_off_the_radio() { int offRadio = 0; - for(uint8_t t = 0; t <= static_cast(web_cmd_t::unpair_shade); t++) { + for(uint8_t t = 0; t <= static_cast(web_cmd_t::load_shades); t++) { if(!webCommandUsesRadio(static_cast(t))) offRadio++; } - TEST_ASSERT_EQUAL_INT(1, offRadio); - TEST_ASSERT_FALSE(webCommandUsesRadio(web_cmd_t::set_positions)); + TEST_ASSERT_EQUAL_INT(4, offRadio); + for(web_cmd_t t : LOOP_COMMANDS) TEST_ASSERT_FALSE(webCommandUsesRadio(t)); } -static void test_set_positions_is_never_held() { - TEST_ASSERT_FALSE(holdWebCommand(web_cmd_t::set_positions, false, false)); - TEST_ASSERT_FALSE(holdWebCommand(web_cmd_t::set_positions, true, false)); +static void test_loop_commands_are_never_held() { + // Nothing goes on air, so a full or suspended radio doesn't matter. + for(web_cmd_t t : LOOP_COMMANDS) { + TEST_ASSERT_FALSE(holdWebCommand(t, false, false)); + TEST_ASSERT_FALSE(holdWebCommand(t, true, false)); + } } static void test_radio_command_waits_for_room() { for(web_cmd_t t : RADIO_COMMANDS) { @@ -46,8 +53,8 @@ int main(int argc, char **argv) { (void)argc; (void)argv; UNITY_BEGIN(); - RUN_TEST(test_only_set_positions_stays_off_the_radio); - RUN_TEST(test_set_positions_is_never_held); + RUN_TEST(test_only_loop_commands_stay_off_the_radio); + RUN_TEST(test_loop_commands_are_never_held); RUN_TEST(test_radio_command_waits_for_room); RUN_TEST(test_commands_are_not_held_while_suspended); RUN_TEST(test_unpair_waits_out_a_suspend);