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);