From 970256e7f6cd8f322a860963ff51785c9c42a41e Mon Sep 17 00:00:00 2001 From: cjkas Date: Sat, 24 Oct 2026 18:29:34 +0200 Subject: [PATCH] fix: edit ops second review findings - A group's sunSensor and a shade's flags are refused by name: the routes don't set them. - setPaired needs paired, and without a shade id it is a bad request. - groupOptions' payload is parsed in ProtoEdits (parseReadPayload). - The assembler's mutex is static, so creating it can't fail. - protoIsReadOp and protoIsShadeOp are gone; the tests use protoOpKind. - Native tests for missing(), offeredRemoteAddress and the edit ops against the TYPE table. Co-Authored-By: Claude Opus 5.5 --- GLOSSARY.md | 2 +- docs/PROTOCOL.md | 4 +- src/LoopQueue.cpp | 2 +- src/ProtoEdits.cpp | 21 ++++++--- src/ProtoEdits.h | 8 ++-- src/ProtoOps.cpp | 2 - src/ProtoOps.h | 1 - src/SomfyJson.cpp | 3 +- src/outputs/ProtoSocket.cpp | 12 +++--- test/e2e/test_edits.py | 2 + test/test_modeleditor/test_modeleditor.cpp | 26 +++++++++++ test/test_protoedits/test_protoedits.cpp | 50 +++++++++++++++++++--- test/test_protoops/test_protoops.cpp | 8 ++-- 13 files changed, 107 insertions(+), 34 deletions(-) diff --git a/GLOSSARY.md b/GLOSSARY.md index a12a0ad..5fa4f78 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -170,5 +170,5 @@ A copy of the shade model taken for one read, so the reply describes a single mo _Avoid_: InstallationSnapshot **LoopQueue** -The single queue through which shade requests, model edits and snapshot requests reach `loop()` from other tasks, in the order they arrived. +The single queue through which shade requests, model edits, reads and snapshot requests reach `loop()` from other tasks, in the order they arrived. _Avoid_: web command queue diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index 302616f..bdbd934 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -106,8 +106,8 @@ Every op carries an `id` and gets one reply. The ops do what the HTTP routes of Each edit op is the edit route of the same name: `addRoom`, `addShade`, `addGroup`, `saveRoom`, `saveShade`, `saveGroup`, `deleteRoom`, `deleteShade`, `deleteGroup`, `linkRemote`, `unlinkRemote`, `linkToGroup`, `unlinkFromGroup`, `linkRepeater`, `unlinkRepeater`, `setPaired`, `roomSortOrder`, `shadeSortOrder`, `groupSortOrder`. -- The payload holds what the route's request does, under the dictionary's keys: a room's, shade's or group's fields as in the sync, and its id to save, delete or link it. `linkRemote` takes `remoteAddress` and `lastRollingCode`; `linkRepeater` takes `remoteAddress`. A sort order takes `ids`, the ids in their new order. -- As with the commands, only the fields that were sent are applied. A key that isn't in the dictionary, a field the edit doesn't set (a save doesn't set `paired` or `linkedRemotes`, for one), or a value of the wrong kind, is a bad request before anything is queued. +- The payload holds what the route's request does, under the dictionary's keys: a room's, shade's or group's fields as in the sync, and its id to save, delete or link it. `linkRemote` takes `remoteAddress` and `lastRollingCode`; `linkRepeater` takes `remoteAddress`; `setPaired` needs `paired`. A sort order takes `ids`, the ids in their new order. +- As with the commands, only the fields that were sent are applied. A key that isn't in the dictionary, a field the edit doesn't set, or a value of the wrong kind, is a bad request before anything is queued. A shade's save doesn't set `paired`, `linkedRemotes` or `flags` (`light` is its own field, and `setSensor` sets the sun and wind), and a group's doesn't set `sunSensor`, which comes from its shades. - One edit runs at a time, whichever client or route sent it: while one is waiting, another gets busy. - The reply comes once the edit is applied and saved. An add's reply carries the new item's id (`roomId`, `shadeId` or `groupId`). A refused edit has the route's reason in `desc`: not found for an id that doesn't exist, otherwise a bad request, with the reason's number in `validation` for a shade whose settings don't add up. - The edit's events follow: the whole item for an add or save, `*Removed` for a delete, the new order for a sort order, `repeaters` for a repeater. diff --git a/src/LoopQueue.cpp b/src/LoopQueue.cpp index 6ed4215..9b13de4 100644 --- a/src/LoopQueue.cpp +++ b/src/LoopQueue.cpp @@ -55,7 +55,7 @@ void LoopQueue::drain() { const shade_request_result_t result = shadeRequests.run(item.request); if(result == shade_request_result_t::radio_refused) ESP_LOGW(TAG, "Radio didn't take shade request %u", static_cast(item.request.type)); - if(item.replyTo.client) protoSocket.replyRequest(item.replyTo, item.request, result); + protoSocket.replyRequest(item.replyTo, item.request, result); break; } case loop_item_type_t::restore_shades: { diff --git a/src/ProtoEdits.cpp b/src/ProtoEdits.cpp index aaca079..b06fcb4 100644 --- a/src/ProtoEdits.cpp +++ b/src/ProtoEdits.cpp @@ -35,10 +35,20 @@ bool protoEditType(uint8_t type, model_edit_type_t &out) { } return false; } -bool protoIsReadOp(uint8_t type) { return protoOpKind(type) == proto_op_t::read; } +const char *parseReadPayload(uint8_t type, JsonObjectConst d, uint8_t &groupId) { + groupId = 0; + if(type != static_cast(proto_type_t::groupOptions)) return nullptr; + JsonVariantConst v = d[protoField(proto_field_t::groupId).key]; + if(v.isNull()) return "groupId is missing"; + if(!v.is()) return "groupId must be 0-255"; + groupId = v.as(); + return nullptr; +} // The fields each edit's route reads, so a field it would ignore is refused -// rather than dropped without a word. +// rather than dropped without a word. A shade's flags aren't among them: +// they hold light, which is set on its own, and the sun and wind state, which +// setSensor sets. A group's sunSensor is worked out from its shades. static const proto_field_t ROOM_FIELDS[] = {proto_field_t::name, proto_field_t::sortOrder}; static const proto_field_t SHADE_FIELDS[] = { proto_field_t::name, proto_field_t::roomId, proto_field_t::remoteAddress, proto_field_t::shadeType, @@ -50,7 +60,7 @@ static const proto_field_t SHADE_FIELDS[] = { }; static const proto_field_t GROUP_FIELDS[] = { proto_field_t::name, proto_field_t::roomId, proto_field_t::remoteAddress, proto_field_t::bitLength, - proto_field_t::proto, proto_field_t::flipCommands, proto_field_t::sunSensor, proto_field_t::repeats + proto_field_t::proto, proto_field_t::flipCommands, proto_field_t::repeats }; template static bool in(proto_field_t field, const proto_field_t (&fields)[N]) { @@ -130,8 +140,7 @@ const char *expandEditPayload(model_edit_type_t type, JsonObjectConst d, JsonDoc // The route takes the ids on their own, in their new order. for(JsonPairConst kv : d) { if(strcmp(kv.key().c_str(), protoField(proto_field_t::ids).key) == 0) continue; - snprintf(why, sizeof(why), "A sort order takes only ids"); - return why; + return "A sort order takes only ids"; } JsonVariantConst ids = d[protoField(proto_field_t::ids).key]; if(!ids.is()) return "ids is missing"; @@ -160,5 +169,7 @@ const char *expandEditPayload(model_edit_type_t type, JsonObjectConst d, JsonDoc return why; } } + // A left-out field is not given, so it can't stand for unpaired. + if(type == model_edit_type_t::set_paired && obj["paired"].isNull()) return "paired is missing"; return doc.overflowed() ? "Too large" : nullptr; } diff --git a/src/ProtoEdits.h b/src/ProtoEdits.h index 9e44b13..5bc7470 100644 --- a/src/ProtoEdits.h +++ b/src/ProtoEdits.h @@ -19,7 +19,9 @@ bool protoEditType(uint8_t type, model_edit_type_t &out); // a bad request reply. As with every op, only what was sent is in it. const char *expandEditPayload(model_edit_type_t type, JsonObjectConst d, JsonDocument &doc); -// getNextRoom, getNextShade, getNextGroup and groupOptions: answered by -// loop() from the shade model. -bool protoIsReadOp(uint8_t type); +// A read op's payload (getNextRoom, getNextShade, getNextGroup and +// groupOptions, answered by loop() from the shade model): groupOptions' group, +// left 0 for the others. nullptr when it is good; otherwise what is wrong +// with it. +const char *parseReadPayload(uint8_t type, JsonObjectConst d, uint8_t &groupId); #endif diff --git a/src/ProtoOps.cpp b/src/ProtoOps.cpp index f367669..e592ca7 100644 --- a/src/ProtoOps.cpp +++ b/src/ProtoOps.cpp @@ -103,8 +103,6 @@ bool protoCommand(const char *name, somfy_commands &cmd) { return false; } -bool protoIsShadeOp(uint8_t type) { return protoOpKind(type) == proto_op_t::shadeRequest; } - // The repeat count and step size a command may carry. static void readRepeat(OpReader &r, shade_request_t &req) { int32_t n; diff --git a/src/ProtoOps.h b/src/ProtoOps.h index b53a8c2..75e1941 100644 --- a/src/ProtoOps.h +++ b/src/ProtoOps.h @@ -16,7 +16,6 @@ // Every command op except sendRemoteCommand becomes a shade request that // loop() runs. -bool protoIsShadeOp(uint8_t type); // nullptr when the payload is good and req is filled in; otherwise what is // wrong with it, for a bad request reply. const char *parseShadeOp(uint8_t type, JsonObjectConst d, shade_request_t &req); diff --git a/src/SomfyJson.cpp b/src/SomfyJson.cpp index 26b5de4..881fd9a 100644 --- a/src/SomfyJson.cpp +++ b/src/SomfyJson.cpp @@ -411,7 +411,8 @@ edit_result_t applyModelEdit(model_edit_t &edit) { return done(); } case model_edit_type_t::set_paired: { - SomfyShade *shade = obj["shadeId"].isNull() ? nullptr : somfy.getShadeById(obj["shadeId"]); + if(obj["shadeId"].isNull()) return edit_result_t::error("No shade id was supplied."); + SomfyShade *shade = somfy.getShadeById(obj["shadeId"]); if(!shade) return edit_result_t::missing("Shade not found to pair"); shade->paired = obj["paired"] | false; shade->save(); diff --git a/src/outputs/ProtoSocket.cpp b/src/outputs/ProtoSocket.cpp index 2821b39..426de8f 100644 --- a/src/outputs/ProtoSocket.cpp +++ b/src/outputs/ProtoSocket.cpp @@ -47,6 +47,8 @@ static BufferedJsonFormatter eventJson; // whoever was sending), so it is only touched under its lock, and never while // replying. static MessageAssembler assembler(PROTO_MAX_CLIENT_FRAME); +// Static, so creating it can't fail. +static StaticSemaphore_t assemblerMutexBuffer; static SemaphoreHandle_t assemblerMutex = nullptr; struct AssemblerLock { AssemblerLock() { xSemaphoreTake(assemblerMutex, portMAX_DELAY); } @@ -212,12 +214,8 @@ static void queueReadOp(AsyncWebSocketClient *client, const proto_message_t &msg loop_item_t item; item.type = loop_item_type_t::proto_read; item.readOp = msg.type; - if(msg.type == static_cast(proto_type_t::groupOptions)) { - JsonVariantConst groupId = d[protoField(proto_field_t::groupId).key]; - if(groupId.isNull()) { replyError(client, msg.id, proto_error_t::badRequest, "groupId is missing"); return; } - if(!groupId.is()) { replyError(client, msg.id, proto_error_t::badRequest, "groupId must be 0-255"); return; } - item.groupId = groupId.as(); - } + const char *bad = parseReadPayload(msg.type, d, item.groupId); + if(bad) { replyError(client, msg.id, proto_error_t::badRequest, bad); return; } item.replyTo = {client->id(), msg.id}; if(!loopQueue.submit(item)) replyError(client, msg.id, proto_error_t::busy, "Command queue full"); } @@ -371,7 +369,7 @@ void ProtoSocket::wsEvent(AsyncWebSocket *server, AsyncWebSocketClient *client, } void ProtoSocket::begin(AsyncWebServer &server) { - assemblerMutex = xSemaphoreCreateMutex(); + assemblerMutex = xSemaphoreCreateMutexStatic(&assemblerMutexBuffer); ws.handleHandshake(authorized); ws.onEvent(ProtoSocket::wsEvent); server.addHandler(&ws); diff --git a/test/e2e/test_edits.py b/test/e2e/test_edits.py index eb32a74..71b8000 100644 --- a/test/e2e/test_edits.py +++ b/test/e2e/test_edits.py @@ -62,6 +62,8 @@ def test_group_options_of_a_missing_group(connect, schema): ("saveRoom", {"zz": 1}, "Unknown field zz"), ("setPaired", {"s": 1, "i": 2}, "paired is not a boolean (1 or 0)"), ("shadeSortOrder", {}, "ids is missing"), + ("setPaired", {"s": 1}, "paired is missing"), + ("saveGroup", {"g": 1, "ss": 1}, "sunSensor can't be set by this edit"), ]) def test_a_bad_payload_is_refused_before_it_is_queued(connect, schema, name, payload, why): c, _, _ = synced_model(connect, schema) diff --git a/test/test_modeleditor/test_modeleditor.cpp b/test/test_modeleditor/test_modeleditor.cpp index dbeac6c..3164b9f 100644 --- a/test/test_modeleditor/test_modeleditor.cpp +++ b/test/test_modeleditor/test_modeleditor.cpp @@ -31,6 +31,12 @@ static void test_free_remote_address_skips_every_taken_one() { TEST_ASSERT_EQUAL_UINT32(104, freeRemoteAddress(101, used, 4)); TEST_ASSERT_EQUAL_UINT32(100, freeRemoteAddress(100, used, 4)); } +static void test_a_new_one_is_offered_the_starting_address_plus_its_id() { + TEST_ASSERT_EQUAL_UINT32(1005, offeredRemoteAddress(1000, 5, nullptr, 0)); + // Taken by another shade or group, so the next free one. + const uint32_t used[] = { 1006, 1005 }; + TEST_ASSERT_EQUAL_UINT32(1007, offeredRemoteAddress(1000, 5, used, 2)); +} /********************************************************************* * Lists @@ -137,6 +143,23 @@ static void test_255_means_no_pin() { /********************************************************************* * What protocol 2 clients are sent after an edit ********************************************************************/ +/********************************************************************* + * Results + ********************************************************************/ +static void test_a_missing_id_is_not_found() { + const edit_result_t r = edit_result_t::missing("Shade Id not found."); + TEST_ASSERT_FALSE(r.ok); + TEST_ASSERT_TRUE(r.notFound); + TEST_ASSERT_EQUAL_STRING("Shade Id not found.", r.desc); +} +static void test_any_other_refusal_is_not_not_found() { + const edit_result_t r = edit_result_t::error("No shade id was supplied.", 3); + TEST_ASSERT_FALSE(r.ok); + TEST_ASSERT_FALSE(r.notFound); + TEST_ASSERT_EQUAL_INT8(3, r.code); + TEST_ASSERT_FALSE(edit_result_t().notFound); +} + static edit_result_t resultFor(uint8_t id, uint8_t shadeId = 0) { edit_result_t r; r.id = id; @@ -203,6 +226,9 @@ int main(int argc, char **argv) { RUN_TEST(test_first_free_id_is_0_when_every_one_is_taken); RUN_TEST(test_next_sort_order_goes_after_the_last); RUN_TEST(test_free_remote_address_skips_every_taken_one); + RUN_TEST(test_a_new_one_is_offered_the_starting_address_plus_its_id); + RUN_TEST(test_a_missing_id_is_not_found); + RUN_TEST(test_any_other_refusal_is_not_not_found); RUN_TEST(test_add_goes_into_the_first_empty_slot); RUN_TEST(test_adding_one_already_there_changes_nothing); RUN_TEST(test_adding_to_a_full_list_fails); diff --git a/test/test_protoedits/test_protoedits.cpp b/test/test_protoedits/test_protoedits.cpp index a0d4761..7242a4e 100644 --- a/test/test_protoedits/test_protoedits.cpp +++ b/test/test_protoedits/test_protoedits.cpp @@ -47,12 +47,37 @@ static void test_other_types_are_not_edits() { TEST_ASSERT_FALSE(protoEditType(static_cast(proto_type_t::getNextShade), t)); TEST_ASSERT_FALSE(protoEditType(static_cast(proto_type_t::shade), t)); } -static void test_the_read_ops() { - TEST_ASSERT_TRUE(protoIsReadOp(static_cast(proto_type_t::getNextRoom))); - TEST_ASSERT_TRUE(protoIsReadOp(static_cast(proto_type_t::getNextShade))); - TEST_ASSERT_TRUE(protoIsReadOp(static_cast(proto_type_t::getNextGroup))); - TEST_ASSERT_TRUE(protoIsReadOp(static_cast(proto_type_t::groupOptions))); - TEST_ASSERT_FALSE(protoIsReadOp(static_cast(proto_type_t::addRoom))); +static void test_the_edit_ops_are_the_schemas_edit_ops() { + // The TYPE table marks the edit ops; each needs its edit here, and only they + // have one. + model_edit_type_t t; + for(int type = 0; type <= 255; type++) { + const bool edit = protoOpKind(static_cast(type)) == proto_op_t::edit; + TEST_ASSERT_EQUAL_MESSAGE(edit, protoEditType(static_cast(type), t), protoTypeName(static_cast(type))); + } +} + +/********************************************************************* + * Reads + ********************************************************************/ +static uint8_t groupId; +static const char *readPayload(proto_type_t type, const char *json) { + payload.clear(); + TEST_ASSERT_EQUAL(DeserializationError::Ok, deserializeJson(payload, json).code()); + return parseReadPayload(static_cast(type), payload.as(), groupId); +} +static void test_group_options_reads_its_group() { + TEST_ASSERT_GOOD(readPayload(proto_type_t::groupOptions, R"({"g":4})")); + TEST_ASSERT_EQUAL_UINT8(4, groupId); + TEST_ASSERT_EQUAL_STRING("groupId is missing", readPayload(proto_type_t::groupOptions, R"({})")); + TEST_ASSERT_EQUAL_STRING("groupId must be 0-255", readPayload(proto_type_t::groupOptions, R"({"g":300})")); + TEST_ASSERT_EQUAL_STRING("groupId must be 0-255", readPayload(proto_type_t::groupOptions, R"({"g":"4"})")); +} +static void test_the_other_reads_need_nothing() { + TEST_ASSERT_GOOD(readPayload(proto_type_t::getNextShade, R"({})")); + TEST_ASSERT_EQUAL_UINT8(0, groupId); + TEST_ASSERT_GOOD(readPayload(proto_type_t::getNextRoom, R"({})")); + TEST_ASSERT_GOOD(readPayload(proto_type_t::getNextGroup, R"({})")); } /********************************************************************* @@ -98,6 +123,14 @@ static void test_a_field_the_route_would_ignore_is_refused() { TEST_ASSERT_EQUAL_STRING("shadeId can't be set by this edit", expand(model_edit_type_t::add_shade, R"({"s":3})")); TEST_ASSERT_EQUAL_STRING("upTime can't be set by this edit", expand(model_edit_type_t::save_group, R"({"g":1,"U":5})")); TEST_ASSERT_EQUAL_STRING("name can't be set by this edit", expand(model_edit_type_t::delete_room, R"({"r":1,"n":"x"})")); + // light is its own field, and setSensor sets the sun and wind. + TEST_ASSERT_EQUAL_STRING("flags can't be set by this edit", expand(model_edit_type_t::save_shade, R"({"s":3,"f":1})")); + // Worked out from the group's shades. + TEST_ASSERT_EQUAL_STRING("sunSensor can't be set by this edit", expand(model_edit_type_t::save_group, R"({"g":1,"ss":1})")); +} +static void test_set_paired_needs_paired() { + // Left out is not given, so it doesn't unpair the shade. + TEST_ASSERT_EQUAL_STRING("paired is missing", expand(model_edit_type_t::set_paired, R"({"s":3})")); } static void test_link_remote_takes_the_routes_rolling_code() { TEST_ASSERT_GOOD(expand(model_edit_type_t::link_remote, R"({"s":3,"a":1193046,"c":12})")); @@ -118,7 +151,9 @@ int main(int argc, char **argv) { UNITY_BEGIN(); RUN_TEST(test_every_edit_op_is_an_edit); RUN_TEST(test_other_types_are_not_edits); - RUN_TEST(test_the_read_ops); + RUN_TEST(test_the_edit_ops_are_the_schemas_edit_ops); + RUN_TEST(test_group_options_reads_its_group); + RUN_TEST(test_the_other_reads_need_nothing); RUN_TEST(test_short_keys_become_the_route_names); RUN_TEST(test_booleans_become_true_and_false); RUN_TEST(test_only_what_was_sent_is_in_the_request); @@ -126,6 +161,7 @@ int main(int argc, char **argv) { RUN_TEST(test_values_are_checked_against_their_kind); RUN_TEST(test_an_unknown_key_is_refused); RUN_TEST(test_a_field_the_route_would_ignore_is_refused); + RUN_TEST(test_set_paired_needs_paired); RUN_TEST(test_link_remote_takes_the_routes_rolling_code); RUN_TEST(test_a_sort_order_takes_only_ids); RUN_TEST(test_a_shade_save_takes_what_the_form_sends); diff --git a/test/test_protoops/test_protoops.cpp b/test/test_protoops/test_protoops.cpp index 6b4ece8..b8f8ed0 100644 --- a/test/test_protoops/test_protoops.cpp +++ b/test/test_protoops/test_protoops.cpp @@ -28,10 +28,10 @@ static void test_every_command_op_but_the_remote_frame_is_a_shade_request() { proto_type_t::setPositions, proto_type_t::setSensor, proto_type_t::setMyPosition, proto_type_t::unpairShade, proto_type_t::setRollingCode }; - for(proto_type_t t : shadeOps) TEST_ASSERT_TRUE(protoIsShadeOp(static_cast(t))); - TEST_ASSERT_FALSE(protoIsShadeOp(static_cast(proto_type_t::sendRemoteCommand))); - TEST_ASSERT_FALSE(protoIsShadeOp(static_cast(proto_type_t::subscribe))); - TEST_ASSERT_FALSE(protoIsShadeOp(static_cast(proto_type_t::shadeCommanded))); + for(proto_type_t t : shadeOps) TEST_ASSERT_EQUAL(proto_op_t::shadeRequest, protoOpKind(static_cast(t))); + TEST_ASSERT_EQUAL(proto_op_t::remoteFrame, protoOpKind(static_cast(proto_type_t::sendRemoteCommand))); + TEST_ASSERT_EQUAL(proto_op_t::none, protoOpKind(static_cast(proto_type_t::subscribe))); + TEST_ASSERT_EQUAL(proto_op_t::none, protoOpKind(static_cast(proto_type_t::shadeCommanded))); } /*********************************************************************