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 <noreply@anthropic.com>

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

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.
+- 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 (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.
+- 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.

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<uint8_t>(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: {

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<uint8_t>(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<uint8_t>()) return "groupId must be 0-255";
+  groupId = v.as<uint8_t>();
+  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<size_t N>
 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 "A sort order takes only ids";
-        return why;
       }
       JsonVariantConst ids = d[protoField(proto_field_t::ids).key];
       if(!ids.is<JsonArrayConst>()) 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;
 }

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
+// A read op's payload (getNextRoom, getNextShade, getNextGroup and
-// loop() from the shade model.
+// groupOptions, answered by loop() from the shade model): groupOptions' group,
-bool protoIsReadOp(uint8_t type);
+// 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

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;

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

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

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<uint8_t>(proto_type_t::groupOptions)) {
+  const char *bad = parseReadPayload(msg.type, d, item.groupId);
-    JsonVariantConst groupId = d[protoField(proto_field_t::groupId).key];
+  if(bad) { replyError(client, msg.id, proto_error_t::badRequest, bad); return; }
-    if(groupId.isNull()) { replyError(client, msg.id, proto_error_t::badRequest, "groupId is missing"); return; }
-    if(!groupId.is<uint8_t>()) { replyError(client, msg.id, proto_error_t::badRequest, "groupId must be 0-255"); return; }
-    item.groupId = groupId.as<uint8_t>();
-  }
   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);

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)

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

test/test_protoedits/test_protoedits.cpp

@@ -47,12 +47,37 @@ static void test_other_types_are_not_edits() {
   TEST_ASSERT_FALSE(protoEditType(static_cast<uint8_t>(proto_type_t::getNextShade), t));
   TEST_ASSERT_FALSE(protoEditType(static_cast<uint8_t>(proto_type_t::shade), t));
 }
-static void test_the_read_ops() {
+static void test_the_edit_ops_are_the_schemas_edit_ops() {
-  TEST_ASSERT_TRUE(protoIsReadOp(static_cast<uint8_t>(proto_type_t::getNextRoom)));
+  // The TYPE table marks the edit ops; each needs its edit here, and only they
-  TEST_ASSERT_TRUE(protoIsReadOp(static_cast<uint8_t>(proto_type_t::getNextShade)));
+  // have one.
-  TEST_ASSERT_TRUE(protoIsReadOp(static_cast<uint8_t>(proto_type_t::getNextGroup)));
+  model_edit_type_t t;
-  TEST_ASSERT_TRUE(protoIsReadOp(static_cast<uint8_t>(proto_type_t::groupOptions)));
+  for(int type = 0; type <= 255; type++) {
-  TEST_ASSERT_FALSE(protoIsReadOp(static_cast<uint8_t>(proto_type_t::addRoom)));
+    const bool edit = protoOpKind(static_cast<uint8_t>(type)) == proto_op_t::edit;
+    TEST_ASSERT_EQUAL_MESSAGE(edit, protoEditType(static_cast<uint8_t>(type), t), protoTypeName(static_cast<uint8_t>(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<uint8_t>(type), payload.as<JsonObjectConst>(), 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);

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<uint8_t>(t)));
+  for(proto_type_t t : shadeOps) TEST_ASSERT_EQUAL(proto_op_t::shadeRequest, protoOpKind(static_cast<uint8_t>(t)));
-  TEST_ASSERT_FALSE(protoIsShadeOp(static_cast<uint8_t>(proto_type_t::sendRemoteCommand)));
+  TEST_ASSERT_EQUAL(proto_op_t::remoteFrame, protoOpKind(static_cast<uint8_t>(proto_type_t::sendRemoteCommand)));
-  TEST_ASSERT_FALSE(protoIsShadeOp(static_cast<uint8_t>(proto_type_t::subscribe)));
+  TEST_ASSERT_EQUAL(proto_op_t::none, protoOpKind(static_cast<uint8_t>(proto_type_t::subscribe)));
-  TEST_ASSERT_FALSE(protoIsShadeOp(static_cast<uint8_t>(proto_type_t::shadeCommanded)));
+  TEST_ASSERT_EQUAL(proto_op_t::none, protoOpKind(static_cast<uint8_t>(proto_type_t::shadeCommanded)));
 }
 
 /*********************************************************************