From 8fa087aa89727e940fedd4ac509776e231e6facc Mon Sep 17 00:00:00 2001 From: cjkas Date: Sat, 24 Oct 2026 18:10:06 +0200 Subject: [PATCH] fix: edit ops review findings From the review of 7723711: - The assembler is locked: a client closed for a full queue is closed by the task that was sending, loop(), whose disconnect dropped the buffer while async_tcp could be copying into it. A whole message is taken out of it, so a disconnect can't free it while it is read. - loop() expires a message stalled for 10 s, freeing its buffer and telling its client, instead of holding it until another client came. A binary message is refused before it can touch another's buffer. - Each edit takes only the fields its route reads; one it would ignore is refused by name. linkRemote's rolling code goes in under the route's name, so applyModelEdit is unchanged again. - An edit refused for an id that doesn't exist replies not found (edit_result_t::missing). A reply that doesn't fit its buffer goes as an internal error, so the op still gets one. - The TYPE table says what becomes of each op (proto_op_t), which the socket switches on. The reply address is one proto_reply_to_t. The routes and the ops start and queue an edit the same way (newModelEdit, submitModelEdit). The full-list texts and the offered remote address rule live in ModelEditor. - e2e: the sort order test skips a list that it would renumber. Co-Authored-By: Claude Opus 5.5 --- docs/PROTOCOL.md | 6 +- src/LoopQueue.cpp | 4 +- src/LoopQueue.h | 10 +- src/ModelEditor.cpp | 3 + src/ModelEditor.h | 16 +- src/ModelSnapshot.cpp | 2 +- src/ProtoAssembly.cpp | 42 ++--- src/ProtoAssembly.h | 36 ++-- src/ProtoEdits.cpp | 92 +++++++++-- src/ProtoFrame.h | 7 + src/ProtoOps.cpp | 4 +- src/ProtoSchema.cpp | 21 ++- src/ProtoSchema.h | 131 ++++++++------- src/ShadeController.cpp | 2 +- src/SomfyEditor.h | 13 +- src/SomfyJson.cpp | 53 ++++-- src/entryPoints/WebUiRoutes.cpp | 18 +- src/outputs/ProtoSocket.cpp | 156 +++++++++++------- src/outputs/ProtoSocket.h | 7 +- test/e2e/test_edits.py | 16 +- test/test_proto/test_proto.cpp | 22 +++ .../test_protoassembly/test_protoassembly.cpp | 58 ++++--- test/test_protoedits/test_protoedits.cpp | 24 +++ 23 files changed, 486 insertions(+), 257 deletions(-) diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index a17bb9c..302616f 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -9,7 +9,7 @@ Why the protocol looks like this: [ADR 0005](adr/0005-one-websocket-protocol.md) - `ws://host/ws` on port 80. Under Secured the upgrade needs `?apikey=` from `/login`; without a valid one it is refused with 401 ([ADR 0006](adr/0006-two-security-modes-eleven-http-routes.md)). - Optional `?schema=`: the client already has the schema with that hash and doesn't need it again. - At most 5 clients. A sixth is closed as soon as it connects. -- Text messages of at most 4 KB. A message may be sent as several frames, and a frame larger than one TCP segment (about 1.4 KB) reaches the device in pieces anyway; the device puts it back together. It does that for one message at a time across all clients: while it is, another client's long message is answered with busy, and the client sends it again. A message that takes over 10 s to arrive may lose its place to another. These refusals have no `id`, since the message couldn't be read. A binary encoding, if one comes, would be asked for at the upgrade (for example `?enc=bin`) and use binary frames. +- Text messages of at most 4 KB. A message may be sent as several frames, and a frame larger than one TCP segment (about 1.4 KB) reaches the device in pieces anyway; the device puts it back together. It does that for one message at a time across all clients: while it is, another client's long message is answered with busy, and the client sends it again. A message that hasn't arrived in full within 10 s is given up, and its client is told with a bad request. These refusals have no `id`, since the message couldn't be read. A binary encoding, if one comes, would be asked for at the upgrade (for example `?enc=bin`) and use binary frames. ## Envelope @@ -107,9 +107,9 @@ 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, 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 (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. - 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 is a bad request with the route's reason in `desc`, and for a shade whose settings don't add up, the reason's number in `validation`. +- 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. ## Reads diff --git a/src/LoopQueue.cpp b/src/LoopQueue.cpp index 8fed019..6ed4215 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.replyClient) protoSocket.replyRequest(item.replyClient, item.replyId, item.request, result); + if(item.replyTo.client) protoSocket.replyRequest(item.replyTo, item.request, result); break; } case loop_item_type_t::restore_shades: { @@ -85,7 +85,7 @@ void LoopQueue::drain() { runModelEdit(item.edit); break; case loop_item_type_t::proto_read: - protoSocket.replyRead(item.replyClient, item.replyId, item.readOp, item.readId); + protoSocket.replyRead(item.replyTo, item.readOp, item.groupId); break; } } diff --git a/src/LoopQueue.h b/src/LoopQueue.h index ca125e7..e9cdbb6 100644 --- a/src/LoopQueue.h +++ b/src/LoopQueue.h @@ -2,6 +2,7 @@ #define LOOP_QUEUE_H #include #include "ShadeRequests.h" +#include "ProtoFrame.h" // The one queue through which other tasks hand work to loop(), which alone // changes the shade model (docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md). @@ -34,12 +35,11 @@ struct loop_item_t { uint8_t restoreParts = 0; // restore_shades: RESTORE_* bits uint8_t slot = 0; // snapshot_fill: the SnapshotSlots slot model_edit_t *edit = nullptr; // model_edit: loop() runs and frees it - // shade_request and proto_read from a protocol 2 client: loop() replies to - // this socket client with replyId once it has run. 0 for no reply. - uint32_t replyClient = 0; - uint32_t replyId = 0; + // shade_request and proto_read from a protocol 2 client: loop() replies + // once it has run. + proto_reply_to_t replyTo; uint8_t readOp = 0; // proto_read: the op's type - uint8_t readId = 0; // proto_read: the group, for groupOptions + uint8_t groupId = 0; // proto_read: groupOptions' group }; // Whether the item sends anything on air. diff --git a/src/ModelEditor.cpp b/src/ModelEditor.cpp index d8123fa..d7eeaf2 100644 --- a/src/ModelEditor.cpp +++ b/src/ModelEditor.cpp @@ -63,6 +63,9 @@ uint32_t freeRemoteAddress(uint32_t candidate, const uint32_t *used, size_t coun } return candidate; } +uint32_t offeredRemoteAddress(uint32_t startingAddress, uint8_t id, const uint32_t *used, size_t count) { + return freeRemoteAddress(startingAddress + id, used, count); +} template static bool addTo(T *list, size_t size, T item) { diff --git a/src/ModelEditor.h b/src/ModelEditor.h index ffa90ba..ad374d9 100644 --- a/src/ModelEditor.h +++ b/src/ModelEditor.h @@ -26,9 +26,10 @@ enum class model_edit_type_t : uint8_t { // What an edit did, sent to the clients as requestResult. id is the room, // shade or group it added or changed, 0 when none. shadeId is the shade a // group gained or lost. code is the validation error of a shade that wasn't -// saved, 0 otherwise. +// saved, 0 otherwise. notFound: it was refused for an id that doesn't exist. struct edit_result_t { bool ok = true; + bool notFound = false; uint8_t id = 0; uint8_t shadeId = 0; int8_t code = 0; @@ -40,7 +41,17 @@ struct edit_result_t { r.code = code; return r; } + static edit_result_t missing(const char *desc) { + edit_result_t r = error(desc); + r.notFound = true; + return r; + } }; +// Why an add, or the read that offers one, is refused when every slot of its +// kind is taken. +#define EDIT_ROOMS_FULL "Maximum number of rooms exceeded." +#define EDIT_SHADES_FULL "Maximum number of shades exceeded." +#define EDIT_GROUPS_FULL "Maximum number of groups exceeded." // The lists an edit can put in a new order. enum class sort_list_t : uint8_t { none, rooms, shades, groups }; @@ -64,6 +75,9 @@ int8_t nextSortOrder(const int8_t *orders, size_t count); // candidate, or the next address after it that isn't in used. No two shades or // groups may share a remote address. uint32_t freeRemoteAddress(uint32_t candidate, const uint32_t *used, size_t count); +// The remote address a new shade or group with that id is offered: the +// starting address plus its id, or the next free one after it. +uint32_t offeredRemoteAddress(uint32_t startingAddress, uint8_t id, const uint32_t *used, size_t count); // A list of ids or addresses with 0 for an empty slot, kept packed at the front. // Adding one that is already there changes nothing and succeeds; false when the diff --git a/src/ModelSnapshot.cpp b/src/ModelSnapshot.cpp index d40abae..f891cdb 100644 --- a/src/ModelSnapshot.cpp +++ b/src/ModelSnapshot.cpp @@ -145,7 +145,7 @@ uint32_t nextRemoteAddress(const model_snapshot_t &snap, uint8_t id) { size_t count = 0; for(uint8_t i = 0; i < snap.shadeCount; i++) used[count++] = snap.shades[i].remoteAddress; for(uint8_t i = 0; i < snap.groupCount; i++) used[count++] = snap.groups[i].remoteAddress; - return freeRemoteAddress(snap.startingAddress + id, used, count); + return offeredRemoteAddress(snap.startingAddress, id, used, count); } /********************************************************************* diff --git a/src/ProtoAssembly.cpp b/src/ProtoAssembly.cpp index 38f29f4..ac99de1 100644 --- a/src/ProtoAssembly.cpp +++ b/src/ProtoAssembly.cpp @@ -2,41 +2,43 @@ #include #include "ProtoAssembly.h" -void MessageAssembler::free() { - ::free(this->buf); +void MessageAssembler::release() { + free(this->buf); this->buf = nullptr; this->len = 0; this->owner = 0; } void MessageAssembler::drop(uint32_t clientId) { - if(this->owner == clientId) this->free(); - if(this->stalled == clientId) this->stalled = 0; + if(this->owner == clientId) this->release(); +} +uint32_t MessageAssembler::expire(uint32_t now) { + if(!this->owner || now - this->startedAt < PROTO_ASSEMBLY_TIMEOUT_MS) return 0; + const uint32_t stalled = this->owner; + this->release(); + return stalled; +} +char *MessageAssembler::take(size_t &len) { + char *message = this->buf; + len = this->len; + this->buf = nullptr; + this->release(); + return message; } assembly_t MessageAssembler::add(const proto_piece_t &piece, uint32_t now) { if(piece.first) { - if(this->stalled == piece.clientId) this->stalled = 0; - if(this->owner && this->owner != piece.clientId) { - if(now - this->startedAt < PROTO_ASSEMBLY_TIMEOUT_MS) return assembly_t::busy; - // A message that stalled gives the buffer up after a while. - this->stalled = this->owner; - } - this->free(); + // Refused without taking the buffer from anyone. if(!piece.text) return assembly_t::notText; + if(this->owner && this->owner != piece.clientId) return assembly_t::busy; + this->release(); this->buf = static_cast(malloc(this->limit + 1)); if(!this->buf) return assembly_t::noMemory; this->owner = piece.clientId; this->startedAt = now; } - else if(this->owner != piece.clientId) { - if(this->stalled == piece.clientId) { - this->stalled = 0; - return assembly_t::timedOut; - } - // The rest of a message that was already answered. - return assembly_t::ignored; - } + // The rest of a message that was already answered. + else if(this->owner != piece.clientId) return assembly_t::ignored; if(this->len + piece.len > this->limit) { - this->free(); + this->release(); return assembly_t::tooLarge; } memcpy(this->buf + this->len, piece.data, piece.len); diff --git a/src/ProtoAssembly.h b/src/ProtoAssembly.h index 671ce01..22c87eb 100644 --- a/src/ProtoAssembly.h +++ b/src/ProtoAssembly.h @@ -6,13 +6,12 @@ // Putting a protocol 2 message back together from the pieces the web server // hands over: a frame larger than a TCP segment, or a message sent as several // frames (docs/PROTOCOL.md). One message at a time across every client, in -// one buffer taken when its first piece arrives and freed when it is whole, so -// nothing is held while no long message is on its way. async_tcp only. Kept -// free of the web server so it can be unit tested on the PC -// (pio test -e native). +// one buffer taken when its first piece arrives and given up when it is whole, +// refused or stalled, so nothing is held while no long message is on its way. +// Not thread safe: the caller locks it. Kept free of the web server so it can +// be unit tested on the PC (pio test -e native). -// How long a message may take to arrive in full before another client's may -// take the buffer. +// How long a message may take to arrive in full. #define PROTO_ASSEMBLY_TIMEOUT_MS 10000 // One piece of a message. @@ -26,36 +25,35 @@ struct proto_piece_t { }; enum class assembly_t : uint8_t { more, // wait for the next piece - complete, // the message is whole: text() and length(), then done() + complete, // the message is whole: take() it ignored, // a piece of a message that was already answered busy, // another client's message holds the buffer tooLarge, // over the limit: the rest of it is ignored notText, // a binary message: the rest of it is ignored - noMemory, // the buffer couldn't be had: the rest of it is ignored - timedOut // it stalled and another client's took the buffer + noMemory // the buffer couldn't be had: the rest of it is ignored }; class MessageAssembler { public: explicit MessageAssembler(size_t limit) : limit(limit) {} - ~MessageAssembler() { this->free(); } - // Each reply-worthy result (busy, tooLarge, notText, noMemory, timedOut) - // comes once per message, from the piece that caused it. + ~MessageAssembler() { this->release(); } + // Each refusal (busy, tooLarge, notText, noMemory) comes once per message, + // from the piece that caused it, so the client gets one reply. assembly_t add(const proto_piece_t &piece, uint32_t now); - // The whole message, terminated, until done(). - const char *text() const { return this->buf; } - size_t length() const { return this->len; } - void done() { this->free(); } + // The complete message, terminated, for the caller to free(): the + // assembler holds nothing after it. + char *take(size_t &len); // The client has gone: its part of a message goes with it. void drop(uint32_t clientId); + // Gives up a message that has taken too long, returning its client, which + // is owed a reply; 0 when none has. The rest of it is ignored. + uint32_t expire(uint32_t now); bool holding() const { return this->owner != 0; } private: size_t limit; uint32_t owner = 0; uint32_t startedAt = 0; - // The client whose stalled message lost the buffer, still to be told. - uint32_t stalled = 0; char *buf = nullptr; size_t len = 0; - void free(); + void release(); }; #endif diff --git a/src/ProtoEdits.cpp b/src/ProtoEdits.cpp index a5546d4..aaca079 100644 --- a/src/ProtoEdits.cpp +++ b/src/ProtoEdits.cpp @@ -1,4 +1,5 @@ #include +#include #include "ProtoEdits.h" struct edit_op_t { @@ -34,15 +35,65 @@ bool protoEditType(uint8_t type, model_edit_type_t &out) { } return false; } -bool protoIsReadOp(uint8_t type) { - switch(static_cast(type)) { - case proto_type_t::getNextRoom: - case proto_type_t::getNextShade: - case proto_type_t::getNextGroup: - case proto_type_t::groupOptions: - return true; - default: - return false; +bool protoIsReadOp(uint8_t type) { return protoOpKind(type) == proto_op_t::read; } + +// The fields each edit's route reads, so a field it would ignore is refused +// rather than dropped without a word. +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, + proto_field_t::tiltType, proto_field_t::bitLength, proto_field_t::proto, proto_field_t::upTime, + proto_field_t::downTime, proto_field_t::tiltTime, proto_field_t::stepSize, proto_field_t::flipCommands, + proto_field_t::flipPosition, proto_field_t::sunSensor, proto_field_t::light, proto_field_t::simMy, + proto_field_t::repeats, proto_field_t::gpioUp, proto_field_t::gpioDown, proto_field_t::gpioMy, + proto_field_t::gpioLLTrigger +}; +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 +}; +template +static bool in(proto_field_t field, const proto_field_t (&fields)[N]) { + for(proto_field_t f : fields) + if(f == field) return true; + return false; +} +static bool takes(model_edit_type_t type, proto_field_t f) { + switch(type) { + case model_edit_type_t::add_room: return in(f, ROOM_FIELDS); + case model_edit_type_t::save_room: return f == proto_field_t::roomId || in(f, ROOM_FIELDS); + case model_edit_type_t::add_shade: return in(f, SHADE_FIELDS); + case model_edit_type_t::save_shade: return f == proto_field_t::shadeId || in(f, SHADE_FIELDS); + case model_edit_type_t::add_group: return in(f, GROUP_FIELDS); + case model_edit_type_t::save_group: return f == proto_field_t::groupId || in(f, GROUP_FIELDS); + case model_edit_type_t::delete_room: return f == proto_field_t::roomId; + case model_edit_type_t::delete_shade: return f == proto_field_t::shadeId; + case model_edit_type_t::delete_group: return f == proto_field_t::groupId; + case model_edit_type_t::link_remote: + return f == proto_field_t::shadeId || f == proto_field_t::remoteAddress || f == proto_field_t::lastRollingCode; + case model_edit_type_t::unlink_remote: return f == proto_field_t::shadeId || f == proto_field_t::remoteAddress; + case model_edit_type_t::link_to_group: + case model_edit_type_t::unlink_from_group: return f == proto_field_t::groupId || f == proto_field_t::shadeId; + case model_edit_type_t::link_repeater: + case model_edit_type_t::unlink_repeater: return f == proto_field_t::remoteAddress; + case model_edit_type_t::set_paired: return f == proto_field_t::shadeId || f == proto_field_t::paired; + default: return false; + } +} +// The name a field has in the edit route's request. Only linkRemote's rolling +// code is named apart from the dictionary. +static const char *routeName(model_edit_type_t type, proto_field_t f) { + if(type == model_edit_type_t::link_remote && f == proto_field_t::lastRollingCode) return "rollingCode"; + return protoField(f).name; +} +static const char *kindName(char kind) { + switch(kind) { + case 'b': return "boolean (1 or 0)"; + case 'i': return "whole number"; + case 'n': return "number"; + case 's': return "string"; + case 'l': return "list"; + default: return "object"; } } @@ -66,9 +117,6 @@ static bool expandValue(const proto_field_info_t &f, JsonVariantConst v, JsonVar case 's': if(v.is()) return out.set(v); return false; - case 'l': - if(v.is()) return out.set(v); - return false; default: return false; } @@ -80,6 +128,11 @@ const char *expandEditPayload(model_edit_type_t type, JsonObjectConst d, JsonDoc case model_edit_type_t::shade_order: case model_edit_type_t::group_order: { // 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; + } JsonVariantConst ids = d[protoField(proto_field_t::ids).key]; if(!ids.is()) return "ids is missing"; for(JsonVariantConst id : ids.as()) @@ -92,15 +145,18 @@ const char *expandEditPayload(model_edit_type_t type, JsonObjectConst d, JsonDoc } JsonObject obj = doc.to(); for(JsonPairConst kv : d) { - const proto_field_info_t *f = protoFieldByKey(kv.key().c_str()); - if(!f) { + proto_field_t field; + if(!protoFieldOfKey(kv.key().c_str(), field)) { snprintf(why, sizeof(why), "Unknown field %s", kv.key().c_str()); return why; } - if(!expandValue(*f, kv.value(), obj[f->name].to())) { - snprintf(why, sizeof(why), "%s is not a %s", f->name, - f->kind == 'b' ? "boolean (1 or 0)" : f->kind == 'i' ? "whole number" : f->kind == 'n' ? "number" : - f->kind == 's' ? "string" : "list"); + const proto_field_info_t &f = protoField(field); + if(!takes(type, field)) { + snprintf(why, sizeof(why), "%s can't be set by this edit", f.name); + return why; + } + if(!expandValue(f, kv.value(), obj[routeName(type, field)].to())) { + snprintf(why, sizeof(why), "%s is not a %s", f.name, kindName(f.kind)); return why; } } diff --git a/src/ProtoFrame.h b/src/ProtoFrame.h index 8ffeb5e..12c26e7 100644 --- a/src/ProtoFrame.h +++ b/src/ProtoFrame.h @@ -8,6 +8,13 @@ // nothing of the shade model (docs/PROTOCOL.md). Kept free of the shade model // and the web server so it can be unit tested on the PC (pio test -e native). +// Who an op's reply goes to when loop() sends it: the socket client and the +// op's id. A client of 0 wants no reply. +struct proto_reply_to_t { + uint32_t client = 0; + uint32_t id = 0; +}; + // A frame: {"v":2,"t":,"id":,"d":{ ... }}. id 0 leaves it out. void beginFrame(JsonFormatter &json, proto_type_t type, uint32_t id = 0); void endFrame(JsonFormatter &json); diff --git a/src/ProtoOps.cpp b/src/ProtoOps.cpp index 70e0cc4..f367669 100644 --- a/src/ProtoOps.cpp +++ b/src/ProtoOps.cpp @@ -103,9 +103,7 @@ bool protoCommand(const char *name, somfy_commands &cmd) { return false; } -bool protoIsShadeOp(uint8_t type) { - return type >= static_cast(proto_type_t::shadeCommand) && type <= static_cast(proto_type_t::setRollingCode); -} +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) { diff --git a/src/ProtoSchema.cpp b/src/ProtoSchema.cpp index c27dc90..3139a35 100644 --- a/src/ProtoSchema.cpp +++ b/src/ProtoSchema.cpp @@ -5,13 +5,13 @@ #define PROTO_FIELD_ROW(name, key, kind, def) {#name, key, kind, def}, static const proto_field_info_t FIELDS[] = { PROTO_FIELDS(PROTO_FIELD_ROW) }; #undef PROTO_FIELD_ROW -// Errors and feeds have no sender, so it is left empty and starts as 0. -#define PROTO_NAMED_ROW(name, number, ...) {number, #name, PROTO_SENDER(__VA_ARGS__, 0)}, -#define PROTO_SENDER(sender, ...) sender -static const proto_named_t TYPES[] = { PROTO_TYPES(PROTO_NAMED_ROW) }; +#define PROTO_TYPE_ROW(name, number, sender, op) {number, #name, sender, op}, +static const proto_named_t TYPES[] = { PROTO_TYPES(PROTO_TYPE_ROW) }; +#undef PROTO_TYPE_ROW +// Errors and feeds have no sender or op. +#define PROTO_NAMED_ROW(name, number) {number, #name, 0, 0}, static const proto_named_t ERRORS[] = { PROTO_ERRORS(PROTO_NAMED_ROW) }; static const proto_named_t FEEDS[] = { PROTO_FEEDS(PROTO_NAMED_ROW) }; -#undef PROTO_SENDER #undef PROTO_NAMED_ROW static_assert(sizeof(FIELDS) / sizeof(FIELDS[0]) == static_cast(proto_field_t::count), "a field without a row"); @@ -21,6 +21,12 @@ const proto_field_info_t *protoFieldByKey(const char *key) { if(strcmp(f.key, key) == 0) return &f; return nullptr; } +bool protoFieldOfKey(const char *key, proto_field_t &out) { + const proto_field_info_t *f = protoFieldByKey(key); + if(!f) return false; + out = static_cast(f - FIELDS); + return true; +} uint8_t protoTypeCount() { return sizeof(TYPES) / sizeof(TYPES[0]); } const proto_named_t &protoTypeAt(uint8_t index) { return TYPES[index]; } uint8_t protoErrorCount() { return sizeof(ERRORS) / sizeof(ERRORS[0]); } @@ -32,6 +38,11 @@ const char *protoTypeName(uint8_t number) { if(t.number == number) return t.name; return nullptr; } +proto_op_t protoOpKind(uint8_t type) { + for(const proto_named_t &t : TYPES) + if(t.number == type) return static_cast(t.op); + return proto_op_t::none; +} bool protoClientMaySend(uint8_t type) { for(const proto_named_t &t : TYPES) if(t.number == type) return t.sender == 'c' || t.sender == 'b'; diff --git a/src/ProtoSchema.h b/src/ProtoSchema.h index 035e28b..9a61de2 100644 --- a/src/ProtoSchema.h +++ b/src/ProtoSchema.h @@ -15,66 +15,67 @@ // Message types: 1-15 control, 16-127 client ops, 128-255 device events. A // number is never reused; a retired type stays reserved. -// TYPE(name, number, sender): sender is 'c' client, 'd' device, 'b' both. +// TYPE(name, number, sender, op): sender is 'c' client, 'd' device, 'b' both; +// op is what becomes of a client op (proto_op_t), '-' for the rest. #define PROTO_TYPES(TYPE) \ - TYPE(hello, 1, 'd') \ - TYPE(reply, 2, 'd') \ - TYPE(next, 3, 'c') \ - TYPE(sync, 4, 'b') \ - TYPE(syncEnd, 5, 'd') \ - TYPE(subscribe, 6, 'c') \ - TYPE(unsubscribe, 7, 'c') \ - TYPE(shadeCommand, 16, 'c') \ - TYPE(groupCommand, 17, 'c') \ - TYPE(tiltCommand, 18, 'c') \ - TYPE(repeatCommand, 19, 'c') \ - TYPE(setPositions, 20, 'c') \ - TYPE(setSensor, 21, 'c') \ - TYPE(setMyPosition, 22, 'c') \ - TYPE(unpairShade, 23, 'c') \ - TYPE(setRollingCode, 24, 'c') \ - TYPE(sendRemoteCommand, 25, 'c') \ - TYPE(addRoom, 26, 'c') \ - TYPE(addShade, 27, 'c') \ - TYPE(addGroup, 28, 'c') \ - TYPE(saveRoom, 29, 'c') \ - TYPE(saveShade, 30, 'c') \ - TYPE(saveGroup, 31, 'c') \ - TYPE(deleteRoom, 32, 'c') \ - TYPE(deleteShade, 33, 'c') \ - TYPE(deleteGroup, 34, 'c') \ - TYPE(linkRemote, 35, 'c') \ - TYPE(unlinkRemote, 36, 'c') \ - TYPE(linkToGroup, 37, 'c') \ - TYPE(unlinkFromGroup, 38, 'c') \ - TYPE(linkRepeater, 39, 'c') \ - TYPE(unlinkRepeater, 40, 'c') \ - TYPE(setPaired, 41, 'c') \ - TYPE(roomSortOrder, 42, 'c') \ - TYPE(shadeSortOrder, 43, 'c') \ - TYPE(groupSortOrder, 44, 'c') \ - TYPE(getNextRoom, 45, 'c') \ - TYPE(getNextShade, 46, 'c') \ - TYPE(getNextGroup, 47, 'c') \ - TYPE(groupOptions, 48, 'c') \ - TYPE(shadeState, 128, 'd') \ - TYPE(shade, 129, 'd') \ - TYPE(shadeRemoved, 130, 'd') \ - TYPE(group, 131, 'd') \ - TYPE(groupState, 132, 'd') \ - TYPE(groupRemoved, 133, 'd') \ - TYPE(room, 134, 'd') \ - TYPE(roomRemoved, 135, 'd') \ - TYPE(roomOrder, 136, 'd') \ - TYPE(shadeOrder, 137, 'd') \ - TYPE(groupOrder, 138, 'd') \ - TYPE(shadeCommanded, 139, 'd') \ - TYPE(memory, 140, 'd') \ - TYPE(wifi, 141, 'd') \ - TYPE(ethernet, 142, 'd') \ - TYPE(frame, 143, 'd') \ - TYPE(frequencyScan, 144, 'd') \ - TYPE(repeaters, 145, 'd') + TYPE(hello, 1, 'd', '-') \ + TYPE(reply, 2, 'd', '-') \ + TYPE(next, 3, 'c', '-') \ + TYPE(sync, 4, 'b', '-') \ + TYPE(syncEnd, 5, 'd', '-') \ + TYPE(subscribe, 6, 'c', '-') \ + TYPE(unsubscribe, 7, 'c', '-') \ + TYPE(shadeCommand, 16, 'c', 'q') \ + TYPE(groupCommand, 17, 'c', 'q') \ + TYPE(tiltCommand, 18, 'c', 'q') \ + TYPE(repeatCommand, 19, 'c', 'q') \ + TYPE(setPositions, 20, 'c', 'q') \ + TYPE(setSensor, 21, 'c', 'q') \ + TYPE(setMyPosition, 22, 'c', 'q') \ + TYPE(unpairShade, 23, 'c', 'q') \ + TYPE(setRollingCode, 24, 'c', 'q') \ + TYPE(sendRemoteCommand, 25, 'c', 'f') \ + TYPE(addRoom, 26, 'c', 'e') \ + TYPE(addShade, 27, 'c', 'e') \ + TYPE(addGroup, 28, 'c', 'e') \ + TYPE(saveRoom, 29, 'c', 'e') \ + TYPE(saveShade, 30, 'c', 'e') \ + TYPE(saveGroup, 31, 'c', 'e') \ + TYPE(deleteRoom, 32, 'c', 'e') \ + TYPE(deleteShade, 33, 'c', 'e') \ + TYPE(deleteGroup, 34, 'c', 'e') \ + TYPE(linkRemote, 35, 'c', 'e') \ + TYPE(unlinkRemote, 36, 'c', 'e') \ + TYPE(linkToGroup, 37, 'c', 'e') \ + TYPE(unlinkFromGroup, 38, 'c', 'e') \ + TYPE(linkRepeater, 39, 'c', 'e') \ + TYPE(unlinkRepeater, 40, 'c', 'e') \ + TYPE(setPaired, 41, 'c', 'e') \ + TYPE(roomSortOrder, 42, 'c', 'e') \ + TYPE(shadeSortOrder, 43, 'c', 'e') \ + TYPE(groupSortOrder, 44, 'c', 'e') \ + TYPE(getNextRoom, 45, 'c', 'r') \ + TYPE(getNextShade, 46, 'c', 'r') \ + TYPE(getNextGroup, 47, 'c', 'r') \ + TYPE(groupOptions, 48, 'c', 'r') \ + TYPE(shadeState, 128, 'd', '-') \ + TYPE(shade, 129, 'd', '-') \ + TYPE(shadeRemoved, 130, 'd', '-') \ + TYPE(group, 131, 'd', '-') \ + TYPE(groupState, 132, 'd', '-') \ + TYPE(groupRemoved, 133, 'd', '-') \ + TYPE(room, 134, 'd', '-') \ + TYPE(roomRemoved, 135, 'd', '-') \ + TYPE(roomOrder, 136, 'd', '-') \ + TYPE(shadeOrder, 137, 'd', '-') \ + TYPE(groupOrder, 138, 'd', '-') \ + TYPE(shadeCommanded, 139, 'd', '-') \ + TYPE(memory, 140, 'd', '-') \ + TYPE(wifi, 141, 'd', '-') \ + TYPE(ethernet, 142, 'd', '-') \ + TYPE(frame, 143, 'd', '-') \ + TYPE(frequencyScan, 144, 'd', '-') \ + TYPE(repeaters, 145, 'd', '-') // Fields, in one dictionary for every type. FIELD(name, key, kind, default): // kind is b bool (sent as 1/0), i integer, n number with a fraction, s string, @@ -222,9 +223,20 @@ struct proto_named_t { uint8_t number; const char *name; char sender; // types only + char op; // types only +}; +// What becomes of a client op, from the TYPE table. +enum class proto_op_t : char { + none = '-', // not an op + shadeRequest = 'q', // a command: a shade request loop() runs + remoteFrame = 'f', // sendRemoteCommand: a frame that goes to the radio at once + edit = 'e', // an edit loop() applies + read = 'r' // a read loop() answers from the shade model }; const proto_field_info_t &protoField(proto_field_t field); const proto_field_info_t *protoFieldByKey(const char *key); +// The field with that key; false when there is none. +bool protoFieldOfKey(const char *key, proto_field_t &out); // The tables as the schema lists them, in declaration order. uint8_t protoTypeCount(); const proto_named_t &protoTypeAt(uint8_t index); @@ -235,6 +247,7 @@ const proto_named_t &protoFeedAt(uint8_t index); const char *protoTypeName(uint8_t number); // What a client may send: control messages it sends and the ops. bool protoClientMaySend(uint8_t type); +proto_op_t protoOpKind(uint8_t type); // Hex FNV-1a of every row of the schema: 8 characters and a terminator. const char *protoSchemaHash(); diff --git a/src/ShadeController.cpp b/src/ShadeController.cpp index 596ec6b..e331c00 100644 --- a/src/ShadeController.cpp +++ b/src/ShadeController.cpp @@ -183,7 +183,7 @@ uint32_t SomfyShadeController::getNextRemoteAddress(uint8_t id) { if(this->shades[i].getShadeId() != 255) used[count++] = this->shades[i].getRemoteAddress(); for(uint8_t i = 0; i < SOMFY_MAX_GROUPS; i++) if(this->groups[i].getGroupId() != 255) used[count++] = this->groups[i].getRemoteAddress(); - return freeRemoteAddress(this->startingAddress + id, used, count); + return offeredRemoteAddress(this->startingAddress, id, used, count); } uint8_t SomfyShadeController::getNextRoomId() { uint8_t ids[SOMFY_MAX_ROOMS]; diff --git a/src/SomfyEditor.h b/src/SomfyEditor.h index 2a7e54d..b6db6ba 100644 --- a/src/SomfyEditor.h +++ b/src/SomfyEditor.h @@ -2,6 +2,7 @@ #define SOMFY_EDITOR_H #include #include "ModelEditor.h" +#include "ProtoFrame.h" // ModelEditor on the device: an edit of the shade model handed from the web // server task to loop(), which applies it with the ModelEditor rules and tells @@ -9,13 +10,11 @@ // One edit on its way to loop(): the edit route and a copy of its request. // From an edit route its result goes to every client as requestResult; from a -// protocol 2 edit op, as the reply to replyId for the socket client -// replyClient. +// protocol 2 edit op, as its reply. struct model_edit_t { model_edit_type_t type = model_edit_type_t::add_room; uint32_t requestId = 0; - uint32_t replyClient = 0; - uint32_t replyId = 0; + proto_reply_to_t replyTo; JsonDocument doc; }; @@ -25,6 +24,12 @@ bool beginModelEdit(); uint32_t nextEditRequestId(); // Gives up an edit that was begun but never queued. Any task. void cancelModelEdit(model_edit_t *edit); +// Begins an edit of that type: nullptr when one is already waiting or running +// (busy), or there was no memory for it. Any task. +model_edit_t *newModelEdit(model_edit_type_t type, bool &busy); +// Hands a begun edit to loop(); false when the queue is full, and it is given +// up. Any task. +bool submitModelEdit(model_edit_t *edit); // Applies the edit, saves it, tells how it went and frees it. loop() only. void runModelEdit(model_edit_t *edit); // Applies the edit to the shade model and saves it. loop() only. diff --git a/src/SomfyJson.cpp b/src/SomfyJson.cpp index 736c934..26b5de4 100644 --- a/src/SomfyJson.cpp +++ b/src/SomfyJson.cpp @@ -6,11 +6,13 @@ #include "SomfyEditor.h" #include "ShadeChanges.h" #include "outputs/ProtoSocket.h" +#include "LoopQueue.h" static const char *TAG = "Somfy"; extern SomfyShadeController somfy; extern ConfigSettings settings; +extern LoopQueue loopQueue; int8_t SomfyShade::validateJSON(JsonObject &obj) { int8_t ret = 0; @@ -228,6 +230,25 @@ void cancelModelEdit(model_edit_t *edit) { delete edit; editBusy.store(false); } +model_edit_t *newModelEdit(model_edit_type_t type, bool &busy) { + busy = !beginModelEdit(); + if(busy) return nullptr; + model_edit_t *edit = new (std::nothrow) model_edit_t(); + if(!edit) { + cancelModelEdit(nullptr); + return nullptr; + } + edit->type = type; + return edit; +} +bool submitModelEdit(model_edit_t *edit) { + loop_item_t item; + item.type = loop_item_type_t::model_edit; + item.edit = edit; + if(!edit->doc.overflowed() && loopQueue.submit(item)) return true; + cancelModelEdit(edit); + return false; +} // What protocol 2 clients get after an edit (editChanges). A shade or group // goes out whole from the change fan-out, by its config bit, so it is sent // once with whatever else changed; the rest goes out now. @@ -246,7 +267,7 @@ void runModelEdit(model_edit_t *edit) { const edit_result_t result = applyModelEdit(*edit); if(!result.ok) ESP_LOGW(TAG, "Edit %u refused: %s", static_cast(edit->type), result.desc); else markEdited(*edit, result); - if(edit->replyClient) protoSocket.replyEdit(edit->replyClient, edit->replyId, edit->type, result); + if(edit->replyTo.client) protoSocket.replyEdit(edit->replyTo, edit->type, result); else emitRequestResult(edit->requestId, result); cancelModelEdit(edit); } @@ -273,7 +294,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { switch(edit.type) { case model_edit_type_t::add_room: { SomfyRoom *room = somfy.addRoom(); - if(!room) return edit_result_t::error("Maximum number of rooms exceeded."); + if(!room) return edit_result_t::error(EDIT_ROOMS_FULL); room->fromJSON(obj); room->save(); room->emitState("roomAdded"); @@ -281,7 +302,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { } case model_edit_type_t::add_shade: { SomfyShade *shade = somfy.addShade(); - if(!shade) return edit_result_t::error("Maximum number of shades exceeded."); + if(!shade) return edit_result_t::error(EDIT_SHADES_FULL); const int8_t err = shade->fromJSON(obj); if(err != 0) { // Not added after all: free the slot it took. @@ -294,7 +315,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { } case model_edit_type_t::add_group: { SomfyGroup *group = somfy.addGroup(); - if(!group) return edit_result_t::error("Maximum number of groups exceeded."); + if(!group) return edit_result_t::error(EDIT_GROUPS_FULL); group->fromJSON(obj); group->save(); group->emitState("groupAdded"); @@ -303,7 +324,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::save_room: { if(obj["roomId"].isNull()) return edit_result_t::error("No room id was supplied."); SomfyRoom *room = somfy.getRoomById(obj["roomId"]); - if(!room) return edit_result_t::error("Room Id not found."); + if(!room) return edit_result_t::missing("Room Id not found."); room->fromJSON(obj); room->save(); room->emitState(); @@ -312,7 +333,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::save_shade: { 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::error("Shade Id not found."); + if(!shade) return edit_result_t::missing("Shade Id not found."); const int8_t err = shade->fromJSON(obj); if(err != 0) return edit_result_t::error("Data Error.", err); shade->save(); @@ -322,7 +343,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::save_group: { if(obj["groupId"].isNull()) return edit_result_t::error("No group id was supplied."); SomfyGroup *group = somfy.getGroupById(obj["groupId"]); - if(!group) return edit_result_t::error("Group Id not found."); + if(!group) return edit_result_t::missing("Group Id not found."); group->fromJSON(obj); group->save(); group->emitState(); @@ -331,7 +352,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::delete_room: { if(obj["roomId"].isNull()) return edit_result_t::error("No room id was supplied."); const uint8_t roomId = obj["roomId"]; - if(!somfy.getRoomById(roomId)) return edit_result_t::error("Room with the specified id not found."); + if(!somfy.getRoomById(roomId)) return edit_result_t::missing("Room with the specified id not found."); somfy.deleteRoom(roomId); return done(roomId); } @@ -339,7 +360,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { if(obj["shadeId"].isNull()) return edit_result_t::error("No shade id was supplied."); const uint8_t shadeId = obj["shadeId"]; SomfyShade *shade = somfy.getShadeById(shadeId); - if(!shade) return edit_result_t::error("Shade with the specified id not found."); + if(!shade) return edit_result_t::missing("Shade with the specified id not found."); if(shade->isInGroup()) return edit_result_t::error("This shade is a member of a group and cannot be deleted."); somfy.deleteShade(shadeId); return done(shadeId); @@ -347,7 +368,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::delete_group: { if(obj["groupId"].isNull()) return edit_result_t::error("No group id was supplied."); const uint8_t groupId = obj["groupId"]; - if(!somfy.getGroupById(groupId)) return edit_result_t::error("Group with the specified id not found."); + if(!somfy.getGroupById(groupId)) return edit_result_t::missing("Group with the specified id not found."); somfy.deleteGroup(groupId); return done(groupId); } @@ -355,13 +376,11 @@ edit_result_t applyModelEdit(model_edit_t &edit) { case model_edit_type_t::unlink_remote: { 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::error("Shade Id not found."); + if(!shade) return edit_result_t::missing("Shade Id not found."); if(obj["remoteAddress"].isNull()) return edit_result_t::error("Remote address not provided."); const uint32_t address = obj["remoteAddress"]; - // The route's name, or protocol 2's. - const uint16_t rollingCode = obj["rollingCode"] | (obj["lastRollingCode"] | (uint16_t)0); if(edit.type == model_edit_type_t::unlink_remote) shade->unlinkRemote(address); - else if(!shade->linkRemote(address, rollingCode)) + else if(!shade->linkRemote(address, obj["rollingCode"] | (uint16_t)0)) return edit_result_t::error("This shade already has the most linked remotes it can."); return done(shade->getShadeId()); } @@ -372,8 +391,8 @@ edit_result_t applyModelEdit(model_edit_t &edit) { if(groupId == 0) return edit_result_t::error("Group id not provided."); if(shadeId == 0) return edit_result_t::error("Shade id not provided."); SomfyGroup *group = somfy.getGroupById(groupId); - if(!group) return edit_result_t::error("Group id not found."); - if(!somfy.getShadeById(shadeId)) return edit_result_t::error("Shade id not found."); + if(!group) return edit_result_t::missing("Group id not found."); + if(!somfy.getShadeById(shadeId)) return edit_result_t::missing("Shade id not found."); if(edit.type == model_edit_type_t::unlink_from_group) group->unlinkShade(shadeId); else if(!group->linkShade(shadeId)) return edit_result_t::error("This group already has the most shades it can."); somfy.commit(); @@ -393,7 +412,7 @@ edit_result_t applyModelEdit(model_edit_t &edit) { } case model_edit_type_t::set_paired: { SomfyShade *shade = obj["shadeId"].isNull() ? nullptr : somfy.getShadeById(obj["shadeId"]); - if(!shade) return edit_result_t::error("Shade not found to pair"); + if(!shade) return edit_result_t::missing("Shade not found to pair"); shade->paired = obj["paired"] | false; shade->save(); shade->emitState(); diff --git a/src/entryPoints/WebUiRoutes.cpp b/src/entryPoints/WebUiRoutes.cpp index 26d9c73..b2683b5 100644 --- a/src/entryPoints/WebUiRoutes.cpp +++ b/src/entryPoints/WebUiRoutes.cpp @@ -81,25 +81,17 @@ static void onUploadDisconnect() { // The reply only says it was accepted; the result reaches every client as a // requestResult event carrying the same requestId once loop() has applied it. static void queueModelEdit(AsyncWebServerRequest *request, model_edit_type_t type, JsonVariantConst body) { - if(!beginModelEdit()) { - request->send(503, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Busy, try the change again\"}")); - return; - } - model_edit_t *edit = new (std::nothrow) model_edit_t(); + bool busy; + model_edit_t *edit = newModelEdit(type, busy); if(!edit) { - cancelModelEdit(nullptr); - sendQueueFull(request); + if(busy) request->send(503, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Busy, try the change again\"}")); + else sendQueueFull(request); return; } - edit->type = type; edit->requestId = nextEditRequestId(); edit->doc.set(body); const uint32_t requestId = edit->requestId; - loop_item_t item; - item.type = loop_item_type_t::model_edit; - item.edit = edit; - if(edit->doc.overflowed() || !loopQueue.submit(item)) { - cancelModelEdit(edit); + if(!submitModelEdit(edit)) { sendQueueFull(request); return; } diff --git a/src/outputs/ProtoSocket.cpp b/src/outputs/ProtoSocket.cpp index 32e2e11..2821b39 100644 --- a/src/outputs/ProtoSocket.cpp +++ b/src/outputs/ProtoSocket.cpp @@ -1,6 +1,8 @@ #include #include #include +#include +#include #include "esp_log.h" #include "ProtoSocket.h" #include "ProtoSync.h" @@ -40,8 +42,16 @@ static_assert(MAX_SOCK_RESPONSE >= PROTO_MAX_FRAME, "protocol 2 frames share the static proto_slot_t slots[PROTO_MAX_CLIENTS]; // What loop() formats events and syncEnd with, into the socket event buffer. static BufferedJsonFormatter eventJson; -// Messages that arrive in pieces. async_tcp only. +// Messages that arrive in pieces. async_tcp adds to it, and a disconnect can +// drop from it on another task (a client closed for a full queue is closed by +// whoever was sending), so it is only touched under its lock, and never while +// replying. static MessageAssembler assembler(PROTO_MAX_CLIENT_FRAME); +static SemaphoreHandle_t assemblerMutex = nullptr; +struct AssemblerLock { + AssemblerLock() { xSemaphoreTake(assemblerMutex, portMAX_DELAY); } + ~AssemblerLock() { xSemaphoreGive(assemblerMutex); } +}; /********************************************************************* * The shade model as a sync source @@ -163,8 +173,7 @@ static void queueShadeOp(AsyncWebSocketClient *client, const proto_message_t &re replyError(client, req.id, proto_error_t::radioUnavailable, "The radio is paused"); return; } - item.replyClient = client->id(); - item.replyId = req.id; + item.replyTo = {client->id(), req.id}; if(!loopQueue.submit(item)) replyError(client, req.id, proto_error_t::busy, "Command queue full"); } // A frame as a remote that isn't one of the shades. It touches nothing of the @@ -182,32 +191,21 @@ static void sendRemoteCommand(AsyncWebSocketClient *client, const proto_message_ } // An edit op: loop() applies it as its edit route does, and replies. static void queueEditOp(AsyncWebSocketClient *client, const proto_message_t &msg, model_edit_type_t type, JsonObjectConst d) { - if(!beginModelEdit()) { - replyError(client, msg.id, proto_error_t::busy, "Another edit is running, send it again"); - return; - } - model_edit_t *edit = new (std::nothrow) model_edit_t(); + bool busy; + model_edit_t *edit = newModelEdit(type, busy); if(!edit) { - cancelModelEdit(nullptr); - replyError(client, msg.id, proto_error_t::internal, "Out of memory"); + if(busy) replyError(client, msg.id, proto_error_t::busy, "Another edit is running, send it again"); + else replyError(client, msg.id, proto_error_t::internal, "Out of memory"); return; } const char *bad = expandEditPayload(type, d, edit->doc); if(bad) { - replyError(client, msg.id, proto_error_t::badRequest, bad); cancelModelEdit(edit); + replyError(client, msg.id, proto_error_t::badRequest, bad); return; } - edit->type = type; - edit->replyClient = client->id(); - edit->replyId = msg.id; - loop_item_t item; - item.type = loop_item_type_t::model_edit; - item.edit = edit; - if(!loopQueue.submit(item)) { - cancelModelEdit(edit); - replyError(client, msg.id, proto_error_t::busy, "Command queue full"); - } + edit->replyTo = {client->id(), msg.id}; + if(!submitModelEdit(edit)) replyError(client, msg.id, proto_error_t::busy, "Command queue full"); } // A read op: loop() answers it from the shade model. static void queueReadOp(AsyncWebSocketClient *client, const proto_message_t &msg, JsonObjectConst d) { @@ -218,10 +216,9 @@ static void queueReadOp(AsyncWebSocketClient *client, const proto_message_t &msg 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.readId = groupId.as(); + item.groupId = groupId.as(); } - item.replyClient = client->id(); - item.replyId = msg.id; + item.replyTo = {client->id(), msg.id}; if(!loopQueue.submit(item)) replyError(client, msg.id, proto_error_t::busy, "Command queue full"); } static bool authorized(AsyncWebServerRequest *request) { @@ -249,7 +246,10 @@ static void onConnect(AsyncWebSocketClient *client, AsyncWebServerRequest *reque ESP_LOGI(TAG, "Socket [%lu] connected from %u.%u.%u.%u%s", (unsigned long)id, ip[0], ip[1], ip[2], ip[3], withSchema ? "" : " (has the schema)"); } static void onDisconnect(AsyncWebSocketClient *client) { - assembler.drop(client->id()); + { + AssemblerLock lock; + assembler.drop(client->id()); + } proto_slot_t *slot = slotOf(slots, PROTO_MAX_CLIENTS, client->id()); if(!slot) return; freeSlot(*slot); @@ -286,27 +286,30 @@ static void onMessage(AsyncWebSocketClient *client, const char *text, size_t len replyOk(client, msg.id); return; } - case proto_type_t::sendRemoteCommand: + default: + break; + } + // The ops, by what becomes of them (the TYPE table). + switch(protoOpKind(msg.type)) { + case proto_op_t::shadeRequest: + queueShadeOp(client, msg, doc["d"]); + return; + case proto_op_t::remoteFrame: sendRemoteCommand(client, msg, doc["d"]); return; - default: { - if(protoIsShadeOp(msg.type)) { - queueShadeOp(client, msg, doc["d"]); - return; - } + case proto_op_t::edit: { model_edit_type_t edit; - if(protoEditType(msg.type, edit)) { - queueEditOp(client, msg, edit, doc["d"]); - return; - } - if(protoIsReadOp(msg.type)) { - queueReadOp(client, msg, doc["d"]); - return; - } + if(protoEditType(msg.type, edit)) queueEditOp(client, msg, edit, doc["d"]); + else replyError(client, msg.id, proto_error_t::internal, "An edit op without an edit"); + return; + } + case proto_op_t::read: + queueReadOp(client, msg, doc["d"]); + return; + case proto_op_t::none: // The settings ops arrive with the protocol core's next PR. replyError(client, msg.id, proto_error_t::unknownOp, "Unknown message type"); return; - } } } static void onFrame(AsyncWebSocketClient *client, AwsFrameInfo *info, uint8_t *data, size_t len) { @@ -327,10 +330,18 @@ static void onFrame(AsyncWebSocketClient *client, AwsFrameInfo *info, uint8_t *d piece.len = len; // Each refusal comes once per message, so the client gets one reply. The // message couldn't be read, so the reply has no id. - switch(assembler.add(piece, millis())) { + assembly_t result; + char *message = nullptr; + size_t messageLen = 0; + { + AssemblerLock lock; + result = assembler.add(piece, millis()); + if(result == assembly_t::complete) message = assembler.take(messageLen); + } + switch(result) { case assembly_t::complete: - onMessage(client, assembler.text(), assembler.length()); - assembler.done(); + onMessage(client, message, messageLen); + free(message); return; case assembly_t::busy: replyError(client, 0, proto_error_t::busy, "Another long message is on its way, send it again"); @@ -344,9 +355,6 @@ static void onFrame(AsyncWebSocketClient *client, AwsFrameInfo *info, uint8_t *d case assembly_t::noMemory: replyError(client, 0, proto_error_t::internal, "Out of memory"); return; - case assembly_t::timedOut: - replyError(client, 0, proto_error_t::badRequest, "The message took too long to arrive"); - return; case assembly_t::more: case assembly_t::ignored: return; @@ -363,6 +371,7 @@ void ProtoSocket::wsEvent(AsyncWebSocket *server, AsyncWebSocketClient *client, } void ProtoSocket::begin(AsyncWebServer &server) { + assemblerMutex = xSemaphoreCreateMutex(); ws.handleHandshake(authorized); ws.onEvent(ProtoSocket::wsEvent); server.addHandler(&ws); @@ -404,25 +413,48 @@ void ProtoSocket::loop() { break; } } + // A long message that stalled gives its buffer up, and its client hears so. + uint32_t stalled; + { + AssemblerLock lock; + stalled = assembler.expire(millis()); + } + if(stalled) { + char buf[160]; + BufferedJsonFormatter json; + json.setBuffer(buf, sizeof(buf)); + writeErrorReply(json, 0, proto_error_t::badRequest, "The message took too long to arrive"); + ws.text(stalled, buf, json.length()); + } ws.cleanupClients(PROTO_MAX_CLIENTS); } -void ProtoSocket::replyRequest(uint32_t client, uint32_t id, const shade_request_t &req, shade_request_result_t result) { - if(!sockEmit.onLoopTask() || id == 0) return; +// Sends a reply formatted on loop(). One that didn't fit goes as an internal +// error instead, so the op still gets its one reply. A client that has gone +// is skipped. +static void sendReply(const proto_reply_to_t &to, BufferedJsonFormatter &json, char *buf, size_t size) { + if(json.overflowed()) { + json.setBuffer(buf, size); + writeErrorReply(json, to.id, proto_error_t::internal, "Reply too large"); + } + ws.text(to.client, buf, json.length()); +} +void ProtoSocket::replyRequest(const proto_reply_to_t &to, const shade_request_t &req, shade_request_result_t result) { + if(!sockEmit.onLoopTask() || to.id == 0) return; char buf[192]; BufferedJsonFormatter json; json.setBuffer(buf, sizeof(buf)); const proto_error_t code = protoRequestError(result); - if(code == proto_error_t::none) writeOkReply(json, id); - else writeErrorReply(json, id, code, protoRequestErrorDesc(req, result)); - // A client that has gone is skipped. - if(!json.overflowed()) ws.text(client, buf, json.length()); + if(code == proto_error_t::none) writeOkReply(json, to.id); + else writeErrorReply(json, to.id, code, protoRequestErrorDesc(req, result)); + sendReply(to, json, buf, sizeof(buf)); } // What an edit op's reply says: for an add, the new item's id; for a refused // edit, the route's reason and the shade's validation code when it has one. -void ProtoSocket::replyEdit(uint32_t client, uint32_t id, model_edit_type_t type, const edit_result_t &result) { - if(!sockEmit.onLoopTask() || id == 0) return; +void ProtoSocket::replyEdit(const proto_reply_to_t &to, model_edit_type_t type, const edit_result_t &result) { + if(!sockEmit.onLoopTask() || to.id == 0) return; + const uint32_t id = to.id; char buf[256]; BufferedJsonFormatter json; json.setBuffer(buf, sizeof(buf)); @@ -430,7 +462,7 @@ void ProtoSocket::replyEdit(uint32_t client, uint32_t id, model_edit_type_t type ProtoWriter w(json); w.add(proto_field_t::ok, result.ok); if(!result.ok) { - w.add(proto_field_t::code, static_cast(proto_error_t::badRequest)); + w.add(proto_field_t::code, static_cast(result.notFound ? proto_error_t::notFound : proto_error_t::badRequest)); w.add(proto_field_t::desc, result.desc); w.add(proto_field_t::validation, result.code); } @@ -438,13 +470,13 @@ void ProtoSocket::replyEdit(uint32_t client, uint32_t id, model_edit_type_t type else if(type == model_edit_type_t::add_shade) w.add(proto_field_t::shadeId, result.id); else if(type == model_edit_type_t::add_group) w.add(proto_field_t::groupId, result.id); endFrame(json); - // A client that has gone is skipped. - if(!json.overflowed()) ws.text(client, buf, json.length()); + sendReply(to, json, buf, sizeof(buf)); } // The reads an edit form needs: what a new room, shade or group would get, // and the shades a group could take. -void ProtoSocket::replyRead(uint32_t client, uint32_t id, uint8_t op, uint8_t groupId) { - if(!sockEmit.onLoopTask() || id == 0) return; +void ProtoSocket::replyRead(const proto_reply_to_t &to, uint8_t op, uint8_t groupId) { + if(!sockEmit.onLoopTask() || to.id == 0) return; + const uint32_t id = to.id; char buf[256]; BufferedJsonFormatter json; json.setBuffer(buf, sizeof(buf)); @@ -455,7 +487,7 @@ void ProtoSocket::replyRead(uint32_t client, uint32_t id, uint8_t op, uint8_t gr switch(static_cast(op)) { case proto_type_t::getNextRoom: { const uint8_t roomId = somfy.getNextRoomId(); - if(roomId == 0) { refused = "Maximum number of rooms exceeded."; break; } + if(roomId == 0) { refused = EDIT_ROOMS_FULL; break; } w.add(proto_field_t::ok, true); w.add(proto_field_t::roomId, roomId); break; @@ -464,7 +496,7 @@ void ProtoSocket::replyRead(uint32_t client, uint32_t id, uint8_t op, uint8_t gr case proto_type_t::getNextGroup: { const bool isShade = op == static_cast(proto_type_t::getNextShade); const uint8_t newId = isShade ? somfy.getNextShadeId() : somfy.getNextGroupId(); - if(newId == 255) { refused = isShade ? "Maximum number of shades exceeded." : "Maximum number of groups exceeded."; break; } + if(newId == 255) { refused = isShade ? EDIT_SHADES_FULL : EDIT_GROUPS_FULL; break; } w.add(proto_field_t::ok, true); w.add(isShade ? proto_field_t::shadeId : proto_field_t::groupId, newId); w.add(proto_field_t::remoteAddress, somfy.getNextRemoteAddress(newId)); @@ -493,7 +525,7 @@ void ProtoSocket::replyRead(uint32_t client, uint32_t id, uint8_t op, uint8_t gr writeErrorReply(json, id, code, refused); } else endFrame(json); - if(!json.overflowed()) ws.text(client, buf, json.length()); + sendReply(to, json, buf, sizeof(buf)); } /********************************************************************* diff --git a/src/outputs/ProtoSocket.h b/src/outputs/ProtoSocket.h index c949d1e..44ce2ce 100644 --- a/src/outputs/ProtoSocket.h +++ b/src/outputs/ProtoSocket.h @@ -4,6 +4,7 @@ #include "ProtoSchema.h" #include "ModelEditor.h" #include "ShadeRequests.h" +#include "ProtoFrame.h" class SomfyShade; class SomfyGroup; @@ -25,11 +26,11 @@ class ProtoSocket { void loop(); bool hasClients() const; // The reply to a command op once loop() has run its shade request. - void replyRequest(uint32_t client, uint32_t id, const shade_request_t &req, shade_request_result_t result); + void replyRequest(const proto_reply_to_t &to, const shade_request_t &req, shade_request_result_t result); // The reply to an edit op once loop() has applied it. - void replyEdit(uint32_t client, uint32_t id, model_edit_type_t type, const edit_result_t &result); + void replyEdit(const proto_reply_to_t &to, model_edit_type_t type, const edit_result_t &result); // The reply to a read op, from the shade model. loop() only. - void replyRead(uint32_t client, uint32_t id, uint8_t op, uint8_t groupId); + void replyRead(const proto_reply_to_t &to, uint8_t op, uint8_t groupId); // Events, loop() only. void shadeState(SomfyShade &shade); void shade(SomfyShade &shade); diff --git a/test/e2e/test_edits.py b/test/e2e/test_edits.py index e02012b..eb32a74 100644 --- a/test/e2e/test_edits.py +++ b/test/e2e/test_edits.py @@ -75,15 +75,27 @@ def test_an_edit_of_a_missing_shade_says_why(connect, schema): """Refused by the edit itself on loop(), as its route would be.""" c, shades, _ = synced_model(connect, schema) reply = op(c, schema, "saveShade", {"shadeId": unused(shades, "shadeId"), "name": "x"}) - assert error_name(schema, reply) == "badRequest" + assert error_name(schema, reply) == "notFound" assert reply["d"][schema.keys["desc"]] == "Shade Id not found." +def test_a_field_an_edit_doesnt_take_is_refused(connect, schema): + c, shades, _ = synced_model(connect, schema) + reply = op(c, schema, "saveShade", {"shadeId": unused(shades, "shadeId"), "paired": 1}) + assert error_name(schema, reply) == "badRequest" + assert reply["d"][schema.keys["desc"]] == "paired can't be set by this edit" + + def test_a_sort_order_set_to_what_it_is(connect, schema): c, shades, _ = synced_model(connect, schema) if not shades: pytest.skip("the device has no shades") - ids = [s["shadeId"] for s in sorted(shades, key=lambda s: s.get("sortOrder", 0))] + ordered = sorted(shades, key=lambda s: s.get("sortOrder", 0)) + # A sort order numbers the shades 0, 1, 2...: only a list already numbered + # so stays as it is. + if [s.get("sortOrder", 0) for s in ordered] != list(range(len(ordered))): + pytest.skip("the shades' sort orders have gaps or repeats, which a sort order would renumber") + ids = [s["shadeId"] for s in ordered] reply = op(c, schema, "shadeSortOrder", {"ids": ids}) assert is_ok(schema, reply) order = c.recv_until(lambda f: f["t"] == schema.type_numbers["shadeOrder"])[-1] diff --git a/test/test_proto/test_proto.cpp b/test/test_proto/test_proto.cpp index 7d17139..4e1a2fb 100644 --- a/test/test_proto/test_proto.cpp +++ b/test/test_proto/test_proto.cpp @@ -50,6 +50,26 @@ static void test_type_names_are_unique() { std::set names; for(uint8_t i = 0; i < protoTypeCount(); i++) TEST_ASSERT_TRUE_MESSAGE(names.insert(protoTypeAt(i).name).second, protoTypeAt(i).name); } +static void test_every_op_says_what_becomes_of_it() { + for(uint8_t i = 0; i < protoTypeCount(); i++) { + const proto_named_t &t = protoTypeAt(i); + const bool op = t.number >= 16 && t.number <= 127; + TEST_ASSERT_EQUAL_MESSAGE(op, protoOpKind(t.number) != proto_op_t::none, t.name); + } + TEST_ASSERT_EQUAL(proto_op_t::shadeRequest, protoOpKind(static_cast(proto_type_t::shadeCommand))); + TEST_ASSERT_EQUAL(proto_op_t::remoteFrame, protoOpKind(static_cast(proto_type_t::sendRemoteCommand))); + TEST_ASSERT_EQUAL(proto_op_t::edit, protoOpKind(static_cast(proto_type_t::saveShade))); + TEST_ASSERT_EQUAL(proto_op_t::read, protoOpKind(static_cast(proto_type_t::groupOptions))); + TEST_ASSERT_EQUAL(proto_op_t::none, protoOpKind(250)); +} +static void test_a_field_by_its_key() { + proto_field_t f; + TEST_ASSERT_TRUE(protoFieldOfKey("s", f)); + TEST_ASSERT_EQUAL(proto_field_t::shadeId, f); + TEST_ASSERT_TRUE(protoFieldOfKey("vc", f)); + TEST_ASSERT_EQUAL(proto_field_t::validation, f); + TEST_ASSERT_FALSE(protoFieldOfKey("zz", f)); +} static void test_error_and_feed_numbers_are_unique() { std::set errors, feeds; for(uint8_t i = 0; i < protoErrorCount(); i++) TEST_ASSERT_TRUE(errors.insert(protoErrorAt(i).number).second); @@ -386,6 +406,8 @@ int main(int argc, char **argv) { RUN_TEST(test_every_field_has_a_known_kind); RUN_TEST(test_type_numbers_are_unique_and_in_their_sender_range); RUN_TEST(test_type_names_are_unique); + RUN_TEST(test_every_op_says_what_becomes_of_it); + RUN_TEST(test_a_field_by_its_key); RUN_TEST(test_error_and_feed_numbers_are_unique); RUN_TEST(test_bootstrap_keys_are_fixed); RUN_TEST(test_schema_hash_is_eight_hex_digits_and_stable); diff --git a/test/test_protoassembly/test_protoassembly.cpp b/test/test_protoassembly/test_protoassembly.cpp index beee78d..30e4c78 100644 --- a/test/test_protoassembly/test_protoassembly.cpp +++ b/test/test_protoassembly/test_protoassembly.cpp @@ -1,4 +1,5 @@ #include +#include #include #include "ProtoAssembly.h" @@ -18,26 +19,32 @@ static proto_piece_t piece(uint32_t client, const char *text, bool first, bool l p.len = strlen(text); return p; } +// Takes the complete message and checks it. +static void assertTaken(MessageAssembler &a, const char *expected) { + size_t len = 0; + char *message = a.take(len); + TEST_ASSERT_EQUAL_STRING(expected, message); + TEST_ASSERT_EQUAL_UINT32(strlen(expected), len); + free(message); +} static void test_pieces_make_the_message() { MessageAssembler a(64); TEST_ASSERT_EQUAL(assembly_t::more, a.add(piece(1, "{\"v\":2,", true, false), 0)); TEST_ASSERT_EQUAL(assembly_t::more, a.add(piece(1, "\"t\":3", false, false), 0)); TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(1, "}", false, true), 0)); - TEST_ASSERT_EQUAL_STRING("{\"v\":2,\"t\":3}", a.text()); - TEST_ASSERT_EQUAL_UINT32(13, a.length()); - a.done(); - TEST_ASSERT_FALSE(a.holding()); + assertTaken(a, "{\"v\":2,\"t\":3}"); } -static void test_nothing_is_held_between_messages() { +static void test_nothing_is_held_once_the_message_is_taken() { MessageAssembler a(64); TEST_ASSERT_FALSE(a.holding()); a.add(piece(1, "ab", true, false), 0); TEST_ASSERT_TRUE(a.holding()); a.add(piece(1, "c", false, true), 0); - a.done(); + assertTaken(a, "abc"); TEST_ASSERT_FALSE(a.holding()); - TEST_ASSERT_NULL(a.text()); + // A client leaving now can't free what was taken. + a.drop(1); } static void test_another_clients_message_waits() { MessageAssembler a(64); @@ -46,7 +53,7 @@ static void test_another_clients_message_waits() { TEST_ASSERT_EQUAL(assembly_t::busy, a.add(piece(2, "xy", true, false), 10)); TEST_ASSERT_EQUAL(assembly_t::ignored, a.add(piece(2, "z", false, true), 10)); TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(1, "c", false, true), 20)); - TEST_ASSERT_EQUAL_STRING("abc", a.text()); + assertTaken(a, "abc"); } static void test_too_large_is_said_once() { MessageAssembler a(4); @@ -60,28 +67,40 @@ static void test_a_binary_message_is_refused_once() { TEST_ASSERT_EQUAL(assembly_t::notText, a.add(piece(1, "ab", true, false, false), 0)); TEST_ASSERT_EQUAL(assembly_t::ignored, a.add(piece(1, "c", false, true, false), 0)); } -static void test_a_stalled_message_gives_the_buffer_up() { +static void test_a_binary_message_leaves_another_clients_alone() { MessageAssembler a(64); a.add(piece(1, "ab", true, false), 0); - TEST_ASSERT_EQUAL(assembly_t::busy, a.add(piece(2, "x", true, true), PROTO_ASSEMBLY_TIMEOUT_MS - 1)); - TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(2, "x", true, true), PROTO_ASSEMBLY_TIMEOUT_MS)); - a.done(); - // The stalled client is told once, when its next piece comes. - TEST_ASSERT_EQUAL(assembly_t::timedOut, a.add(piece(1, "c", false, false), PROTO_ASSEMBLY_TIMEOUT_MS + 1)); - TEST_ASSERT_EQUAL(assembly_t::ignored, a.add(piece(1, "d", false, true), PROTO_ASSEMBLY_TIMEOUT_MS + 1)); + TEST_ASSERT_EQUAL(assembly_t::notText, a.add(piece(2, "x", true, true, false), 0)); + TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(1, "c", false, true), 0)); + assertTaken(a, "abc"); +} +static void test_a_stalled_message_expires_and_its_client_is_owed_a_reply() { + MessageAssembler a(64); + a.add(piece(1, "ab", true, false), 0); + TEST_ASSERT_EQUAL_UINT32(0, a.expire(PROTO_ASSEMBLY_TIMEOUT_MS - 1)); + TEST_ASSERT_TRUE(a.holding()); + TEST_ASSERT_EQUAL_UINT32(1, a.expire(PROTO_ASSEMBLY_TIMEOUT_MS)); + TEST_ASSERT_FALSE(a.holding()); + TEST_ASSERT_EQUAL_UINT32(0, a.expire(2 * PROTO_ASSEMBLY_TIMEOUT_MS)); + // Its late pieces were answered by the expiry. + TEST_ASSERT_EQUAL(assembly_t::ignored, a.add(piece(1, "c", false, true), PROTO_ASSEMBLY_TIMEOUT_MS + 1)); + // And the buffer is free for anyone. + TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(2, "xy", true, true), PROTO_ASSEMBLY_TIMEOUT_MS + 1)); + assertTaken(a, "xy"); } static void test_a_client_that_leaves_lets_go() { MessageAssembler a(64); a.add(piece(1, "ab", true, false), 0); a.drop(1); TEST_ASSERT_FALSE(a.holding()); + TEST_ASSERT_EQUAL_UINT32(0, a.expire(PROTO_ASSEMBLY_TIMEOUT_MS)); TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(2, "xy", true, true), 1)); + assertTaken(a, "xy"); } static void test_a_message_of_one_piece_is_whole() { - // A whole binary frame, or one piece too large for the rest of the code. MessageAssembler a(64); TEST_ASSERT_EQUAL(assembly_t::complete, a.add(piece(1, "{}", true, true), 0)); - TEST_ASSERT_EQUAL_STRING("{}", a.text()); + assertTaken(a, "{}"); } int main(int argc, char **argv) { @@ -89,11 +108,12 @@ int main(int argc, char **argv) { (void)argv; UNITY_BEGIN(); RUN_TEST(test_pieces_make_the_message); - RUN_TEST(test_nothing_is_held_between_messages); + RUN_TEST(test_nothing_is_held_once_the_message_is_taken); RUN_TEST(test_another_clients_message_waits); RUN_TEST(test_too_large_is_said_once); RUN_TEST(test_a_binary_message_is_refused_once); - RUN_TEST(test_a_stalled_message_gives_the_buffer_up); + RUN_TEST(test_a_binary_message_leaves_another_clients_alone); + RUN_TEST(test_a_stalled_message_expires_and_its_client_is_owed_a_reply); RUN_TEST(test_a_client_that_leaves_lets_go); RUN_TEST(test_a_message_of_one_piece_is_whole); return UNITY_END(); diff --git a/test/test_protoedits/test_protoedits.cpp b/test/test_protoedits/test_protoedits.cpp index 60be81d..a0d4761 100644 --- a/test/test_protoedits/test_protoedits.cpp +++ b/test/test_protoedits/test_protoedits.cpp @@ -91,6 +91,26 @@ static void test_values_are_checked_against_their_kind() { static void test_an_unknown_key_is_refused() { TEST_ASSERT_EQUAL_STRING("Unknown field zz", expand(model_edit_type_t::save_room, R"({"r":1,"zz":5})")); } +static void test_a_field_the_route_would_ignore_is_refused() { + // In the sync, but not something a save sets. + TEST_ASSERT_EQUAL_STRING("linkedRemotes can't be set by this edit", expand(model_edit_type_t::save_shade, R"({"s":3,"lk":[1]})")); + TEST_ASSERT_EQUAL_STRING("paired can't be set by this edit", expand(model_edit_type_t::save_shade, R"({"s":3,"i":1})")); + 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"})")); +} +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})")); + TEST_ASSERT_EQUAL_STRING(R"({"shadeId":3,"remoteAddress":1193046,"rollingCode":12})", written().c_str()); + TEST_ASSERT_EQUAL_STRING("lastRollingCode can't be set by this edit", expand(model_edit_type_t::unlink_remote, R"({"s":3,"a":5,"c":12})")); +} +static void test_a_sort_order_takes_only_ids() { + TEST_ASSERT_EQUAL_STRING("A sort order takes only ids", expand(model_edit_type_t::room_order, R"({"is":[1],"r":2})")); +} +static void test_a_shade_save_takes_what_the_form_sends() { + TEST_ASSERT_GOOD(expand(model_edit_type_t::save_shade, + R"({"s":3,"n":"Office","r":1,"a":5,"st":1,"y":2,"bl":80,"pr":1,"U":1,"D":2,"Y":3,"z":5,"fc":1,"fp":0,"ss":1,"li":0,"sm":1,"rp":2,"gu":4,"gd":5,"gm":6,"gl":1})")); +} int main(int argc, char **argv) { (void)argc; @@ -105,5 +125,9 @@ int main(int argc, char **argv) { RUN_TEST(test_a_sort_order_is_its_list_of_ids); 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_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); return UNITY_END(); }