fix: command op replies that said ok when nothing was done

From the review of #100:
- setMyPosition on a moving shade replies busy ("The shade is moving"),
  and setPositions with only a tilt, for a shade without one, replies
  bad request ("The shade has no tilt"). Both said ok before.
- PROTOCOL.md lists where the ops differ from the routes, and how
  sendRemoteCommand is answered.
- The LoopQueue hold comment covers protocol 2 replies and an unpair
  held through a frequency scan.
- One shadeRequestIsForGroup() for the group cases; ProtoOps shares one
  reader for "one or both" fields and one for ids, and the radio
  refused text.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
cjkas 2026-10-24 12:49:23 +02:00
parent 598d708b12
commit 5737a58790
12 changed files with 103 additions and 49 deletions

View file

@ -76,7 +76,7 @@ Events go out to every client from the moment it connects, so they can arrive du
## Commands
Every op carries an `id` and gets one reply. The ops take what the HTTP routes of the same name take, under the dictionary's keys:
Every op carries an `id` and gets one reply. The ops do what the HTTP routes of the same name do, with these arguments:
| Op | Payload | Does |
|---|---|---|
@ -94,8 +94,13 @@ Every op carries an `id` and gets one reply. The ops take what the HTTP routes o
- **An op reads only the fields that were sent.** Unlike what the device sends, a field left out of an op is not its schema default: it is not given, so `setPositions` with only `position` leaves the tilt as it is. A client sends every field it means, defaults included.
- `command` is a command's name, in any case: `My`, `Up`, `Down`, `MyUp`, `MyDown`, `UpDown`, `MyUpDown`, `Prog`, `SunFlag`, `Flag`, `StepUp`, `StepDown`, `Toggle`, `Sensor`, `Favorite`, `Stop`, or the spelling `shadeCommanded` reports (`Step Up`, `My+Up`).
- `repeat` left out means the shade's or the group's own repeats.
- Where they differ from the routes:
- The keys come from the dictionary: `setMyPosition` takes `myPos` and `myTiltPos` (the route: `pos`, `tilt`); `setRollingCode` and `sendRemoteCommand` take `lastRollingCode` (the routes: `rollingCode`, `rcode`); `sendRemoteCommand` takes `remoteAddress` (the route: `address`).
- `groupCommand`, `repeatCommand` and `sendRemoteCommand` need a `command`, where the routes send My without one.
- `setMyPosition` takes a tilt on its own and keeps the lift favorite, where the route needs a lift position.
- A field of the wrong kind, a value out of range, or both of two alternatives, is a bad request, and `desc` names the field.
- The reply comes once the command has gone to the radio, or was refused: not found (no shade or group with that id), radio unavailable (the radio is paused, as during a frequency scan, or didn't take it) or busy (the device's command queue is full). It carries no state: the shade's `shadeState` events follow as it moves.
- The reply comes once the command has gone to the radio, or was refused: not found (no shade or group with that id), radio unavailable (the radio is paused, as during a frequency scan, or didn't take it), busy (the device's command queue is full, or `setMyPosition` on a shade that is moving) or bad request (`setPositions` with only a tilt, for a shade without one). It carries no state: the shade's `shadeState` events follow as it moves.
- `sendRemoteCommand` is answered as soon as the radio has taken the frame, not after the ops queued before it, and with radio unavailable when the radio is full rather than waiting for room.
## Replies

View file

@ -43,11 +43,14 @@ struct loop_item_t {
inline bool loopItemUsesRadio(const loop_item_t &item) {
return item.type == loop_item_type_t::shade_request && shadeRequestUsesRadio(item.request.type);
}
// Whether the item at the head of the queue waits for the radio. The client
// was already told it was accepted so it stays queued until the radio has room
// rather than have the radio drop it. While TX is suspended nothing gets
// through anyway so nothing is held back then, except an unpair: the client was
// told the shade is unpaired, so it waits until TX resumes.
// Whether the item at the head of the queue waits for the radio. An HTTP or
// MQTT client was already told it was accepted, so it stays queued until the
// radio has room rather than have the radio drop it; a protocol 2 client gets
// its reply once it has run. While TX is suspended nothing gets through
// anyway so nothing is held back then, except an unpair: an HTTP client was
// told the shade is unpaired, so it waits until TX resumes. Everything queued
// behind it waits too, protocol 2 replies included, so a frequency scan
// started after an unpair delays them until it ends.
inline bool holdLoopItem(const loop_item_t &item, bool txSuspended, bool txHasRoom) {
if(txSuspended) return item.type == loop_item_type_t::shade_request && item.request.type == shade_request_type_t::unpair;
return loopItemUsesRadio(item) && !txHasRoom;

View file

@ -64,9 +64,10 @@ class OpReader {
return v.is<const char *>() ? v.as<const char *>() : nullptr;
}
};
// What the reply's desc points at once parseShadeOp has returned.
// The reader's problem, kept once the reader has gone so the reply can send
// it. One buffer for every parse: they all run on async_tcp, one at a time.
static char lastError[64];
static const char *result(OpReader &r) {
static const char *errorOf(OpReader &r) {
if(!r.error) return nullptr;
snprintf(lastError, sizeof(lastError), "%s", r.error);
return lastError;
@ -116,17 +117,17 @@ static void readStep(OpReader &r, shade_request_t &req) {
// Steps are 7 bits on air.
if(r.get(proto_field_t::stepSize, 0, 127, n)) req.stepSize = static_cast<uint8_t>(n);
}
static void readShade(OpReader &r, shade_request_t &req) {
static void readId(OpReader &r, proto_field_t field, shade_request_t &req) {
int32_t n;
if(r.need(proto_field_t::shadeId, 0, 255, n)) req.id = static_cast<uint8_t>(n);
if(r.need(field, 0, 255, n)) req.id = static_cast<uint8_t>(n);
}
static void readShade(OpReader &r, shade_request_t &req) { readId(r, proto_field_t::shadeId, req); }
// A shade or a group; req.type becomes the one that was given.
static void readShadeOrGroup(OpReader &r, shade_request_t &req, shade_request_type_t shade, shade_request_type_t group) {
if(!r.either(proto_field_t::shadeId, proto_field_t::groupId)) return;
int32_t n;
const bool isShade = r.has(proto_field_t::shadeId);
req.type = isShade ? shade : group;
if(r.need(isShade ? proto_field_t::shadeId : proto_field_t::groupId, 0, 255, n)) req.id = static_cast<uint8_t>(n);
readId(r, isShade ? proto_field_t::shadeId : proto_field_t::groupId, req);
}
// A command, or a position 0-100 to move to.
static void readCommandOrTarget(OpReader &r, shade_request_t &req) {
@ -135,15 +136,16 @@ static void readCommandOrTarget(OpReader &r, shade_request_t &req) {
if(r.has(proto_field_t::command)) r.command(req.command);
else if(r.get(proto_field_t::target, 0, 100, n)) req.target = static_cast<int8_t>(n);
}
// Two positions 0-100 of which at least one is given; -1 for the other.
static void readPositions(OpReader &r, proto_field_t lift, proto_field_t tilt, int8_t &liftOut, int8_t &tiltOut) {
if(!r.has(lift) && !r.has(tilt)) {
r.fail("Give %s, %s or both", protoField(lift).name, protoField(tilt).name);
// Two values in lo..hi of which at least one is given; the other is left as
// it was.
static void readOneOrBoth(OpReader &r, proto_field_t a, proto_field_t b, int32_t lo, int32_t hi, int8_t &aOut, int8_t &bOut) {
if(!r.has(a) && !r.has(b)) {
r.fail("Give %s, %s or both", protoField(a).name, protoField(b).name);
return;
}
int32_t n;
if(r.get(lift, 0, 100, n)) liftOut = static_cast<int8_t>(n);
if(r.get(tilt, 0, 100, n)) tiltOut = static_cast<int8_t>(n);
if(r.get(a, lo, hi, n)) aOut = static_cast<int8_t>(n);
if(r.get(b, lo, hi, n)) bOut = static_cast<int8_t>(n);
}
const char *parseShadeOp(uint8_t type, JsonObjectConst d, shade_request_t &req) {
@ -165,7 +167,7 @@ const char *parseShadeOp(uint8_t type, JsonObjectConst d, shade_request_t &req)
break;
case proto_type_t::groupCommand:
req.type = shade_request_type_t::group_command;
if(r.need(proto_field_t::groupId, 0, 255, n)) req.id = static_cast<uint8_t>(n);
readId(r, proto_field_t::groupId, req);
r.command(req.command);
readRepeat(r, req);
readStep(r, req);
@ -179,19 +181,17 @@ const char *parseShadeOp(uint8_t type, JsonObjectConst d, shade_request_t &req)
case proto_type_t::setPositions:
req.type = shade_request_type_t::set_positions;
readShade(r, req);
readPositions(r, proto_field_t::position, proto_field_t::tiltPosition, req.position, req.tiltPosition);
readOneOrBoth(r, proto_field_t::position, proto_field_t::tiltPosition, 0, 100, req.position, req.tiltPosition);
break;
case proto_type_t::setSensor:
readShadeOrGroup(r, req, shade_request_type_t::shade_sensor, shade_request_type_t::group_sensor);
if(!r.has(proto_field_t::sunny) && !r.has(proto_field_t::windy)) r.fail("Give sunny, windy or both");
if(r.get(proto_field_t::sunny, 0, 1, n)) req.sunny = static_cast<int8_t>(n);
if(r.get(proto_field_t::windy, 0, 1, n)) req.windy = static_cast<int8_t>(n);
readOneOrBoth(r, proto_field_t::sunny, proto_field_t::windy, 0, 1, req.sunny, req.windy);
readRepeat(r, req);
break;
case proto_type_t::setMyPosition:
req.type = shade_request_type_t::set_favorite;
readShade(r, req);
readPositions(r, proto_field_t::myPos, proto_field_t::myTiltPos, req.position, req.tiltPosition);
readOneOrBoth(r, proto_field_t::myPos, proto_field_t::myTiltPos, 0, 100, req.position, req.tiltPosition);
break;
case proto_type_t::unpairShade:
req.type = shade_request_type_t::unpair;
@ -206,7 +206,7 @@ const char *parseShadeOp(uint8_t type, JsonObjectConst d, shade_request_t &req)
r.fail("Not a shade op");
break;
}
return result(r);
return errorOf(r);
}
const char *parseRemoteCommandOp(JsonObjectConst d, somfy_frame_t &frame, uint8_t &repeats) {
@ -220,28 +220,25 @@ const char *parseRemoteCommandOp(JsonObjectConst d, somfy_frame_t &frame, uint8_
if(r.need(proto_field_t::lastRollingCode, 1, UINT16_MAX, n)) frame.rollingCode = static_cast<uint16_t>(n);
if(r.get(proto_field_t::encKey, 0, UINT8_MAX, n)) frame.encKey = static_cast<uint8_t>(n);
if(r.get(proto_field_t::repeats, 0, UINT8_MAX, n)) repeats = static_cast<uint8_t>(n);
return result(r);
return errorOf(r);
}
proto_error_t protoRequestError(shade_request_result_t result) {
switch(result) {
case shade_request_result_t::not_found: return proto_error_t::notFound;
case shade_request_result_t::radio_refused: return proto_error_t::radioUnavailable;
// At rest it would take it: the client may try again.
case shade_request_result_t::shade_moving: return proto_error_t::busy;
case shade_request_result_t::no_tilt: return proto_error_t::badRequest;
default: return proto_error_t::none;
}
}
const char *protoRequestErrorDesc(const shade_request_t &req, shade_request_result_t result) {
switch(result) {
case shade_request_result_t::not_found:
switch(req.type) {
case shade_request_type_t::group_command:
case shade_request_type_t::group_repeat:
case shade_request_type_t::group_sensor:
return "No group with that id";
default:
return "No shade with that id";
}
case shade_request_result_t::radio_refused: return "The radio didn't take the command";
case shade_request_result_t::not_found: return shadeRequestIsForGroup(req.type) ? "No group with that id" : "No shade with that id";
case shade_request_result_t::radio_refused: return PROTO_RADIO_REFUSED;
case shade_request_result_t::shade_moving: return "The shade is moving";
case shade_request_result_t::no_tilt: return "The shade has no tilt";
default: return nullptr;
}
}

View file

@ -27,6 +27,9 @@ const char *parseRemoteCommandOp(JsonObjectConst d, somfy_frame_t &frame, uint8_
// in any case. False for a name that isn't a command.
bool protoCommand(const char *name, somfy_commands &cmd);
// What a reply says when the radio didn't take a command.
#define PROTO_RADIO_REFUSED "The radio didn't take the command"
// The reply to a shade request that loop() has run: none when it was done.
proto_error_t protoRequestError(shade_request_result_t result);
// What that reply says.

View file

@ -10,6 +10,16 @@ bool shadeRequestUsesRadio(shade_request_type_t type) {
return true;
}
}
bool shadeRequestIsForGroup(shade_request_type_t type) {
switch(type) {
case shade_request_type_t::group_command:
case shade_request_type_t::group_repeat:
case shade_request_type_t::group_sensor:
return true;
default:
return false;
}
}
int8_t requestRepeat(int32_t repeat) {
if(repeat < 0) return -1;
return repeat > INT8_MAX ? INT8_MAX : static_cast<int8_t>(repeat);
@ -26,16 +36,10 @@ static shade_request_result_t sent(bool taken) { return taken ? shade_request_re
shade_request_result_t ShadeRequests::run(const shade_request_t &req) {
shade_info_t shade;
uint8_t groupRepeats = 0;
switch(req.type) {
case shade_request_type_t::group_command:
case shade_request_type_t::group_repeat:
case shade_request_type_t::group_sensor:
if(!this->ops.findGroup(req.id, groupRepeats)) return shade_request_result_t::not_found;
break;
default:
if(!this->ops.findShade(req.id, shade)) return shade_request_result_t::not_found;
break;
if(shadeRequestIsForGroup(req.type)) {
if(!this->ops.findGroup(req.id, groupRepeats)) return shade_request_result_t::not_found;
}
else if(!this->ops.findShade(req.id, shade)) return shade_request_result_t::not_found;
const bool flip = shade.flipPosition;
switch(req.type) {
case shade_request_type_t::shade_command:
@ -62,6 +66,8 @@ shade_request_result_t ShadeRequests::run(const shade_request_t &req) {
case shade_request_type_t::set_positions: {
const float pos = inRange(req.position) ? storedPosition(req.position, flip) : -1.0f;
const float tilt = inRange(req.tiltPosition) && shade.tiltType != tilt_types::none ? storedPosition(req.tiltPosition, flip) : -1.0f;
// A tilt on its own for a shade without one has nothing to set.
if(pos < 0.0f && inRange(req.tiltPosition) && shade.tiltType == tilt_types::none) return shade_request_result_t::no_tilt;
if(pos >= 0.0f || tilt >= 0.0f) this->ops.setPositions(req.id, pos, tilt);
return shade_request_result_t::done;
}
@ -70,6 +76,8 @@ shade_request_result_t ShadeRequests::run(const shade_request_t &req) {
case shade_request_type_t::group_sensor:
return sent(this->ops.sendGroupSensorCommand(req.id, req.windy, req.sunny, repeatsOr(req.repeat, groupRepeats)));
case shade_request_type_t::set_favorite: {
// The shade only takes a favorite at rest, and would ignore it.
if(!shade.idle) return shade_request_result_t::shade_moving;
const int8_t tilt = requestedFavoriteTilt(req.tiltPosition, shade.tiltType, shade.myTiltPos, flip);
// Without a lift position only the tilt changes: the lift favorite stays,
// or is set where the shade is when it has none.

View file

@ -40,6 +40,8 @@ struct shade_request_t {
// Whether the request sends anything on air.
bool shadeRequestUsesRadio(shade_request_type_t type);
// Whether the request's id is a group id rather than a shade id.
bool shadeRequestIsForGroup(shade_request_type_t type);
// A repeat count from a client: negative for the default, capped at what fits.
int8_t requestRepeat(int32_t repeat);
// A target from a client: -1 unless it is 0-100.
@ -55,6 +57,7 @@ struct shade_info_t {
float currentPos = 0.0f;
float myPos = -1.0f;
float myTiltPos = -1.0f;
bool idle = true; // not moving, and at its target
};
// The seam between ShadeRequests and the shades and groups it acts on. On the
@ -89,7 +92,9 @@ class ShadeOps {
enum class shade_request_result_t : uint8_t {
done,
not_found, // no shade or group with that id; nothing was done
radio_refused // the radio didn't take a command it sent
radio_refused, // the radio didn't take a command it sent
shade_moving, // a favorite is only set on a shade at rest; nothing was done
no_tilt // only a tilt was given, for a shade without one; nothing was done
};
// Carries out shade requests, whichever entry point they came from: finds

View file

@ -105,6 +105,7 @@ class SomfyShade : public SomfyRemote, public ShadeSettings, private MotorLink,
uint8_t getShadeId() { return shadeId; }
// Where the shade is and where it is going.
const ShadeBehaviour::State &state() const { return this->behaviour.state(); }
bool isIdle() const { return this->behaviour.isIdle(); }
uint8_t getFlags() override;
// Sets the setting and state bits from a saved or sent flags byte.
void setFlags(uint8_t flags);

View file

@ -16,6 +16,7 @@ bool SomfyShadeOps::findShade(uint8_t shadeId, shade_info_t &info) {
info.currentPos = shade->state().currentPos;
info.myPos = shade->state().myPos;
info.myTiltPos = shade->state().myTiltPos;
info.idle = shade->isIdle();
return true;
}
bool SomfyShadeOps::findGroup(uint8_t groupId, uint8_t &repeats) {

View file

@ -170,7 +170,7 @@ static void sendRemoteCommand(AsyncWebSocketClient *client, const proto_message_
const char *bad = parseRemoteCommandOp(d, frame, repeats);
if(bad) { replyError(client, req.id, proto_error_t::badRequest, bad); return; }
if(!somfy.sendFrame(frame, repeats)) {
replyError(client, req.id, proto_error_t::radioUnavailable, "The radio didn't take the command");
replyError(client, req.id, proto_error_t::radioUnavailable, PROTO_RADIO_REFUSED);
return;
}
replyOk(client, req.id);

View file

@ -13,6 +13,6 @@ Runs against a real device on your network, by hand, never in CI. It checks sock
| `--radio --shade <id>` | also send real commands: that shade moves a little and back |
| `--record` | save hello, the sync and the schema to `test/fixtures/proto2/`, for the web UI and Home Assistant tests |
A plain run sends nothing on air, changes no settings and doesn't reboot the device. The command tests send ops that are refused before they reach the radio, and one `setPositions` that sets a shade to the position it already has. It does fill every socket slot for a few seconds, and replays a page load.
A plain run sends nothing on air, changes no settings and doesn't reboot the device. The command tests send ops that are refused before they reach the radio, and one `setPositions` that sets a shade to the position the sync gave: that drops the fraction of a percent the shade's believed position may have had. It does fill every socket slot for a few seconds, and replays a page load.
The tests run in whatever security mode the device is in; the suite never changes it. To test both modes, run the suite once in each.

View file

@ -215,6 +215,12 @@ static void test_request_results_become_replies() {
TEST_ASSERT_EQUAL_STRING("No shade with that id", protoRequestErrorDesc(shade, shade_request_result_t::not_found));
TEST_ASSERT_EQUAL_STRING("No group with that id", protoRequestErrorDesc(group, shade_request_result_t::not_found));
TEST_ASSERT_EQUAL(proto_error_t::radioUnavailable, protoRequestError(shade_request_result_t::radio_refused));
TEST_ASSERT_EQUAL_STRING("The radio didn't take the command", protoRequestErrorDesc(shade, shade_request_result_t::radio_refused));
// A favorite waits for the shade to stop: the client may try again.
TEST_ASSERT_EQUAL(proto_error_t::busy, protoRequestError(shade_request_result_t::shade_moving));
TEST_ASSERT_EQUAL_STRING("The shade is moving", protoRequestErrorDesc(shade, shade_request_result_t::shade_moving));
TEST_ASSERT_EQUAL(proto_error_t::badRequest, protoRequestError(shade_request_result_t::no_tilt));
TEST_ASSERT_EQUAL_STRING("The shade has no tilt", protoRequestErrorDesc(shade, shade_request_result_t::no_tilt));
}
int main(int argc, char **argv) {

View file

@ -174,6 +174,13 @@ static void test_set_positions_leaves_the_tilt_of_a_shade_without_one() {
requests.run(req);
TEST_ASSERT_EQUAL_STRING("setPositions 4 20 -1", ops.calls.c_str());
}
static void test_set_positions_tilt_alone_on_a_shade_without_one_is_refused() {
ops.shades[4] = rollerShade();
shade_request_t req = request(shade_request_type_t::set_positions);
req.tiltPosition = 40;
TEST_ASSERT_EQUAL(shade_request_result_t::no_tilt, requests.run(req));
TEST_ASSERT_EQUAL_STRING("", ops.calls.c_str());
}
static void test_set_positions_with_nothing_in_range_does_nothing() {
ops.shades[4] = tiltShade(tilt_types::integrated);
shade_request_t req = request(shade_request_type_t::set_positions);
@ -279,6 +286,21 @@ static void test_group_sensor_uses_the_groups_repeats() {
/*********************************************************************
* The favorite
********************************************************************/
static void test_favorite_on_a_moving_shade_is_refused() {
ops.shades[4] = rollerShade();
ops.shades[4].idle = false;
shade_request_t req = request(shade_request_type_t::set_favorite);
req.position = 50;
TEST_ASSERT_EQUAL(shade_request_result_t::shade_moving, requests.run(req));
TEST_ASSERT_EQUAL_STRING("", ops.calls.c_str());
}
static void test_group_requests_take_a_group_id() {
TEST_ASSERT_TRUE(shadeRequestIsForGroup(shade_request_type_t::group_command));
TEST_ASSERT_TRUE(shadeRequestIsForGroup(shade_request_type_t::group_repeat));
TEST_ASSERT_TRUE(shadeRequestIsForGroup(shade_request_type_t::group_sensor));
TEST_ASSERT_FALSE(shadeRequestIsForGroup(shade_request_type_t::shade_sensor));
TEST_ASSERT_FALSE(shadeRequestIsForGroup(shade_request_type_t::unpair));
}
static void test_favorite_on_a_flipped_shade_is_turned_round() {
ops.shades[4] = flippedShade();
shade_request_t req = request(shade_request_type_t::set_favorite);
@ -374,6 +396,7 @@ int main(int argc, char **argv) {
RUN_TEST(test_tilt_without_a_target_sends_the_tilt_command);
RUN_TEST(test_set_positions_on_a_flipped_shade_are_turned_round);
RUN_TEST(test_set_positions_leaves_the_tilt_of_a_shade_without_one);
RUN_TEST(test_set_positions_tilt_alone_on_a_shade_without_one_is_refused);
RUN_TEST(test_set_positions_with_nothing_in_range_does_nothing);
RUN_TEST(test_client_values_are_checked_on_the_way_in);
RUN_TEST(test_no_repeat_means_the_shades_own);
@ -385,6 +408,8 @@ int main(int argc, char **argv) {
RUN_TEST(test_group_repeat_repeats_the_last_frame);
RUN_TEST(test_sensor_sends_what_was_given);
RUN_TEST(test_group_sensor_uses_the_groups_repeats);
RUN_TEST(test_favorite_on_a_moving_shade_is_refused);
RUN_TEST(test_group_requests_take_a_group_id);
RUN_TEST(test_favorite_on_a_flipped_shade_is_turned_round);
RUN_TEST(test_favorite_without_a_tilt_keeps_the_favorite_tilt);
RUN_TEST(test_favorite_tilt_alone_keeps_the_favorite_lift);