Merge pull request #37 from cjkas/fix/set-my-tilt-rolling-codes

Set rolling codes only on the main loop; default a set-favorite tilt to the favorite tilt

docs/KNOWN_ISSUES.md

@@ -32,12 +32,11 @@ The new `ControllerChunker` reads state as the response drains. On a slow link o
 - Same exposure exists in any other endpoint converted to chunked responses next (`/discovery`, `/shades`). Resolve this issue before expanding the pattern.
 - Other handlers on the web server task still change the shade model that `loop()` owns:
   - `/addRoom`, `/saveRoom`, `/deleteRoom`, `/addShade`, `/saveShade`, `/deleteShade`, `/addGroup`, `/saveGroup` and `/deleteGroup` add, change and remove rooms, shades and groups.
-  - `/linkRemote` and `/unlinkRemote` change a shade's linked remotes, and `/linkRemote` also sets a linked remote's rolling code.
+  - `/linkRemote` and `/unlinkRemote` change a shade's linked remotes. `/linkRemote` also sets a linked remote's rolling code, which `loop()` updates whenever it receives a frame from that remote: the last write wins, so a received frame can undo the code just set.
   - `/linkToGroup` and `/unlinkFromGroup` change a group's linked shades.
   - `/setPaired` sets `paired`.
-  - `/setRollingCode` and a shades restore change rolling codes (see [Rolling codes written from two tasks](#rolling-codes-written-from-two-tasks)).
 
-  Shade commands, set-My and unpair already go through the web command queue to `loop()`.
+  Shade commands, set-My, unpair, `/setRollingCode`, a shades restore and `/updateShadeConfig` already go through the web command queue to `loop()`.
 - `/controller` leaves out a room, shade, group or linked shade that doesn't fit in its chunk buffer, see [Oversized `/controller` items are left out](#oversized-controller-items-are-left-out).
 
 ## Oversized `/controller` items are left out
@@ -91,40 +90,3 @@ Unlike the `/controller` HTTP crash, this path does **not** abort — there is n
 
 - Not the same code path as the `/controller` crash. Solve independently.
 - Worth grepping for `JsonSockEvent` usages that iterate collections (see references in `SomfyEvents.cpp`, `Transceiver.cpp`, `ESPNetwork.cpp`, `GitOTA.cpp`) to identify the most at-risk events.
-
-## Rolling codes written from two tasks
-
-**Status:** open
-**Location:** `SomfyRemote::sendCommandAfter` and `setRollingCode` in [src/SomfyRemote.cpp](../src/SomfyRemote.cpp), the `/setRollingCode` handlers in [src/Web.cpp](../src/Web.cpp), and `ShadeConfigFile::readShadeRecord`/`readGroupRecord` in [src/ConfigFile.cpp](../src/ConfigFile.cpp).
-
-### Summary
-
-Sending a command reads the next rolling code, queues the frame, then commits the code (`peekNextRollingCode` → `submitTx` → `commitRollingCode`), all on `loop()`. `/setRollingCode` and a shades restore change the same remote's `lastRollingCode` and its `ShadeCodes` NVS key from the async web server task, with no lock against that sequence. Each NVS write is safe on its own, since every function opens its own handle (see [ADR-0001](adr/0001-local-nvs-handles-no-wrapper.md)), but the read-then-commit sequence is not.
-
-### Concrete failure modes
-
-1. `/setRollingCode` lands between the peek and the commit of a command. The command's commit overwrites the code just set, so the set has no effect.
-2. A restore reads `ShadeCodes` while a command commits a newer code. The restored shade can come back with the older code, and the motor rejects its frames until the code passes the newer one.
-
-### Fix options (pick one later)
-
-- **(a) Run these handlers on `loop()`.** Queue `/setRollingCode` and restore through the web command queue like shade commands, so `loop()` is the only task that touches a remote's rolling code. Part of making `loop()` the only owner of the shade model.
-- **(b) A rolling-code lock.** Hold a mutex across peek → queue → commit, and in `setRollingCode` and restore. Narrower, but it adds a lock to the command path.
-
-### Related
-
-- Same root cause as [Concurrent mutation during chunked `/controller` response](#concurrent-mutation-during-chunked-controller-response): the web server task changes the shade model that `loop()` owns.
-- `/linkRemote` sets a linked remote's rolling code on the web server task, while `loop()` updates the same code when it receives a frame from that remote. The last write wins, so a received frame can undo the code just set.
-
-## `/setMyPosition` without a tilt uses the lift favorite
-
-**Status:** open
-**Location:** the `/setMyPosition` handlers in [src/Web.cpp](../src/Web.cpp). This was already in the code before the radio task work.
-
-### Summary
-
-When a request for a shade with tilt leaves out `tilt`, the handler fills it in with `shade->myPos`, the lift favorite, where `shade->myTiltPos` looks intended. The favorite tilt is then set to the favorite lift position. The web UI always sends `tilt`, so only API callers that leave it out see this.
-
-### Fix
-
-Use `shade->myTiltPos` in both handlers, after checking that no API client depends on the current behaviour.

src/ShadeMotion.cpp

@@ -29,6 +29,11 @@ int8_t displayPosition(float pos, bool flip) {
   if(pos < 0) return -1;
   return static_cast<int8_t>(flip && pos >= 0.00f ? floor(100.0f - pos) : floor(pos));
 }
+int8_t requestedFavoriteTilt(int8_t requestTilt, tilt_types tilt, float myTiltPos, bool flip) {
+  if(tilt == tilt_types::none) return -1;
+  if(requestTilt >= 0) return requestTilt;
+  return displayPosition(myTiltPos, flip);
+}
 float limitTarget(int8_t dir) { return dir > 0 ? 100.0f : 0.0f; }
 bool stepPosition(float pos, int8_t dir, uint32_t travelMs, uint16_t stepSize, uint8_t frameStepSize, float &out) {
   if(travelMs == 0 || stepSize == 0) return false;

src/ShadeMotion.h

@@ -14,6 +14,10 @@ float travelPosition(float startPos, int8_t dir, uint32_t elapsedMs, int32_t tra
 // A position as reported to clients: -1 when unset, otherwise the whole
 // percentage, counted from the other end when flipped.
 int8_t displayPosition(float pos, bool flip);
+// The tilt a request to set the favorite asks for, as a display position: the
+// requested tilt, or the favorite tilt when the request leaves it out.  -1 for
+// a shade without tilt or with no favorite tilt.
+int8_t requestedFavoriteTilt(int8_t requestTilt, tilt_types tilt, float myTiltPos, bool flip);
 // Where a shade (or its tilt) moving in dir ends up when nothing stops it.
 float limitTarget(int8_t dir);
 // Where a step command moves a shade (or its tilt) from pos, in out.  dir > 0

src/Web.cpp

@@ -51,12 +51,12 @@ static QueueHandle_t webCmdQueue = nullptr;
 
 static void sendQueueFull(AsyncWebServerRequest *request);
 static void sendRadioUnavailable(AsyncWebServerRequest *request);
-// Hands a command that transmits to the main loop.  When it can't be the
+// Hands a command to the main loop, which owns the shades.  When it can't be
-// request is answered with the reason and false is returned.
+// the request is answered with the reason and false is returned.
-static bool queueRadioCommand(AsyncWebServerRequest *request, const web_command_t &cmd) {
+static bool queueLoopCommand(AsyncWebServerRequest *request, const web_command_t &cmd) {
   // Nothing can go on air during a frequency scan so tell the client now rather
   // than dropping the command once it reaches the radio.
-  if(somfy.transceiver.txSuspended()) { sendRadioUnavailable(request); return false; }
+  if(webCommandUsesRadio(cmd.type) && somfy.transceiver.txSuspended()) { sendRadioUnavailable(request); return false; }
   if(!webServer.queueCommand(cmd)) { sendQueueFull(request); return false; }
   return true;
 }
@@ -67,7 +67,7 @@ static bool queueSetMyPosition(AsyncWebServerRequest *request, SomfyShade *shade
   cmd.shadeId = shade->getShadeId();
   cmd.position = pos;
   cmd.tiltPosition = tilt;
-  return queueRadioCommand(request, cmd);
+  return queueLoopCommand(request, cmd);
 }
 // The main loop clears paired and saves it once the Prog is queued.  The
 // shade belongs to the main loop so it isn't changed here.
@@ -75,7 +75,32 @@ static bool queueUnpairShade(AsyncWebServerRequest *request, SomfyShade *shade)
   web_command_t cmd = {};
   cmd.type = web_cmd_t::unpair_shade;
   cmd.shadeId = shade->getShadeId();
-  return queueRadioCommand(request, cmd);
+  return queueLoopCommand(request, cmd);
+}
+// Sending a command reads and commits the rolling code on the main loop, so
+// a new code is set there too.
+static bool queueSetRollingCode(AsyncWebServerRequest *request, SomfyShade *shade, uint16_t rollingCode) {
+  web_command_t cmd = {};
+  cmd.type = web_cmd_t::set_rolling_code;
+  cmd.shadeId = shade->getShadeId();
+  cmd.rollingCode = rollingCode;
+  return queueLoopCommand(request, cmd);
+}
+// A restore's options travel through the command queue as RESTORE_* bits.
+static uint8_t restoreParts(const restore_options_t &opts) {
+  return (opts.settings ? RESTORE_SETTINGS : 0) | (opts.shades ? RESTORE_SHADES : 0) |
+    (opts.network ? RESTORE_NETWORK : 0) | (opts.transceiver ? RESTORE_TRANSCEIVER : 0) |
+    (opts.repeaters ? RESTORE_REPEATERS : 0) | (opts.mqtt ? RESTORE_MQTT : 0);
+}
+static restore_options_t restoreOptions(uint8_t parts) {
+  restore_options_t opts;
+  opts.settings = parts & RESTORE_SETTINGS;
+  opts.shades = parts & RESTORE_SHADES;
+  opts.network = parts & RESTORE_NETWORK;
+  opts.transceiver = parts & RESTORE_TRANSCEIVER;
+  opts.repeaters = parts & RESTORE_REPEATERS;
+  opts.mqtt = parts & RESTORE_MQTT;
+  return opts;
 }
 // The radio and MQTT are shut down once an upload starts and only a reboot
 // brings them back.  The completion handler schedules it but never runs if the
@@ -220,6 +245,22 @@ void Web::processQueue() {
         }
         break;
       }
+      case web_cmd_t::set_rolling_code: {
+        SomfyShade *shade = somfy.getShadeById(cmd.shadeId);
+        if(shade) shade->setRollingCode(cmd.rollingCode);
+        break;
+      }
+      case web_cmd_t::restore_shades: {
+        restore_options_t opts = restoreOptions(cmd.restoreParts);
+        ShadeConfigFile::restore(&somfy, "/shades.tmp", opts);
+        ESP_LOGI(TAG, "Rebooting ESP for restored settings...");
+        rebootDelay.reboot = true;
+        rebootDelay.rebootTime = millis() + 1000;
+        break;
+      }
+      case web_cmd_t::load_shades:
+        somfy.loadShadesFile("/shades.tmp");
+        break;
     }
     if(!sent) ESP_LOGW(TAG, "Radio didn't take web command %u", static_cast<uint8_t>(cmd.type));
   }
@@ -310,10 +351,10 @@ static void serializeShadeRef(SomfyShade *shade, JsonFormatter &json) {
   json.addElem("hasLight", shade->hasLight());
   json.addElem("repeats", shade->repeats);
 }
-// paired is passed in for a response that reports the change the main loop
+// paired and lastRollingCode are passed in for a response that reports the
-// is about to make.  The unpair is held until it goes on air (holdWebCommand),
+// change the main loop is about to make.  An unpair is held until it goes on
-// so the answer comes true.
+// air (holdWebCommand), so the answer comes true.
-static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired) {
+static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired, uint16_t lastRollingCode) {
   json.addElem("shadeId", shade->getShadeId());
   json.addElem("roomId", shade->roomId);
   json.addElem("name", shade->name);
@@ -321,7 +362,7 @@ static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired)
   json.addElem("upTime", (uint32_t)shade->upTime);
   json.addElem("downTime", (uint32_t)shade->downTime);
   json.addElem("paired", paired);
-  json.addElem("lastRollingCode", (uint32_t)shade->lastRollingCode);
+  json.addElem("lastRollingCode", (uint32_t)lastRollingCode);
   json.addElem("position", shade->transformPosition(shade->currentPos));
   json.addElem("tiltType", static_cast<uint8_t>(shade->tiltType));
   json.addElem("tiltPosition", shade->transformPosition(shade->currentTiltPos));
@@ -361,7 +402,7 @@ static void serializeShade(SomfyShade *shade, JsonFormatter &json, bool paired)
   }
   json.endArray();
 }
-static void serializeShade(SomfyShade *shade, JsonFormatter &json) { serializeShade(shade, json, shade->paired); }
+static void serializeShade(SomfyShade *shade, JsonFormatter &json) { serializeShade(shade, json, shade->paired, shade->lastRollingCode); }
 static void serializeGroupRef(SomfyGroup *group, JsonFormatter &json) {
   group->updateFlags();
   json.addElem("groupId", group->getGroupId());
@@ -890,7 +931,7 @@ void Web::handleShadeCommand(AsyncWebServerRequest *request, JsonVariant &json)
     cmd.command = command;
     cmd.repeat = repeat;
     cmd.stepSize = stepSize;
-    if(!queueRadioCommand(request, cmd)) return;
+    if(!queueLoopCommand(request, cmd)) return;
     AsyncJsonResp resp;
     resp.beginResponse(request, g_async_content, sizeof(g_async_content));
     resp.beginObject();
@@ -931,7 +972,7 @@ void Web::handleGroupCommand(AsyncWebServerRequest *request, JsonVariant &json)
     cmd.command = command;
     cmd.repeat = repeat;
     cmd.stepSize = stepSize;
-    if(!queueRadioCommand(request, cmd)) return;
+    if(!queueLoopCommand(request, cmd)) return;
     AsyncJsonResp resp;
     resp.beginResponse(request, g_async_content, sizeof(g_async_content));
     resp.beginObject();
@@ -968,7 +1009,7 @@ void Web::handleTiltCommand(AsyncWebServerRequest *request, JsonVariant &json) {
     cmd.shadeId = shadeId;
     cmd.target = target;
     cmd.command = command;
-    if(!queueRadioCommand(request, cmd)) return;
+    if(!queueLoopCommand(request, cmd)) return;
     AsyncJsonResp resp;
     resp.beginResponse(request, g_async_content, sizeof(g_async_content));
     resp.beginObject();
@@ -1009,7 +1050,7 @@ void Web::handleRepeatCommand(AsyncWebServerRequest *request, JsonVariant &json)
     cmd.command = command;
     cmd.repeat = repeat;
     cmd.stepSize = stepSize;
-    if(!queueRadioCommand(request, cmd)) return;
+    if(!queueLoopCommand(request, cmd)) return;
     AsyncJsonResp resp;
     resp.beginResponse(request, g_async_content, sizeof(g_async_content));
     resp.beginObject();
@@ -1027,7 +1068,7 @@ void Web::handleRepeatCommand(AsyncWebServerRequest *request, JsonVariant &json)
     cmd.command = command;
     cmd.repeat = repeat;
     cmd.stepSize = stepSize;
-    if(!queueRadioCommand(request, cmd)) return;
+    if(!queueLoopCommand(request, cmd)) return;
     AsyncJsonResp resp;
     resp.beginResponse(request, g_async_content, sizeof(g_async_content));
     resp.beginObject();
@@ -1166,7 +1207,7 @@ void Web::handleSetSensor(AsyncWebServerRequest *request, JsonVariant &json) {
       cmd.sunny = sunny;
       cmd.windy = windy;
       cmd.repeat = repeat;
-      if(!queueRadioCommand(request, cmd)) return;
+      if(!queueLoopCommand(request, cmd)) return;
       // The main loop applies the command after we answer so this is the state
       // before it.  Clients get the result from the shadeState socket event.
       AsyncJsonResp resp;
@@ -1187,7 +1228,7 @@ void Web::handleSetSensor(AsyncWebServerRequest *request, JsonVariant &json) {
       cmd.sunny = sunny;
       cmd.windy = windy;
       cmd.repeat = repeat;
-      if(!queueRadioCommand(request, cmd)) return;
+      if(!queueLoopCommand(request, cmd)) return;
       // As above, this is the state before the command and groupState follows.
       AsyncJsonResp resp;
       resp.beginResponse(request, g_async_content, sizeof(g_async_content));
@@ -1449,7 +1490,6 @@ void Web::begin() {
   asyncServer.on("/restore", HTTP_POST,
     [](AsyncWebServerRequest *request) {
       if(webServer.uploadSuccess) {
-        request->send(200, _encoding_json, "{\"status\":\"Success\",\"desc\":\"Restoring Shade settings\"}");
         restore_options_t opts;
         if(asyncHasParam(request, "data")) {
           String dataStr = asyncParam(request, "data");
@@ -1469,10 +1509,13 @@ void Web::begin() {
           ESP_LOGD(TAG, "No restore options sent.  Using defaults...");
           opts.shades = true;
         }
-        ShadeConfigFile::restore(&somfy, "/shades.tmp", opts);
+        // The main loop restores, since it owns the shades and their rolling
-        ESP_LOGI(TAG, "Rebooting ESP for restored settings...");
+        // codes, and then reboots.
-        rebootDelay.reboot = true;
+        web_command_t cmd = {};
-        rebootDelay.rebootTime = millis() + 1000;
+        cmd.type = web_cmd_t::restore_shades;
+        cmd.restoreParts = restoreParts(opts);
+        if(!queueLoopCommand(request, cmd)) return;
+        request->send(200, _encoding_json, "{\"status\":\"Success\",\"desc\":\"Restoring Shade settings\"}");
       }
     },
     [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) {
@@ -1733,8 +1776,7 @@ void Web::begin() {
       }
       SomfyShade* shade = somfy.getShadeById(shadeId);
       if(shade) {
-        if(tilt < 0) tilt = shade->myPos;
+        tilt = requestedFavoriteTilt(tilt, shade->tiltType, shade->myTiltPos, shade->flipPosition);
-        if(shade->tiltType == tilt_types::none) tilt = -1;
         if(pos >= 0 && pos <= 100)
           if(!queueSetMyPosition(request, shade, shade->transformPosition(pos), shade->transformPosition(tilt))) return;
         AsyncJsonResp resp;
@@ -1760,8 +1802,7 @@ void Web::begin() {
     else { request->send(500, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"No shade object supplied.\"}")); return; }
     SomfyShade* shade = somfy.getShadeById(shadeId);
     if(shade) {
-      if(tilt < 0) tilt = shade->myPos;
+      tilt = requestedFavoriteTilt(tilt, shade->tiltType, shade->myTiltPos, shade->flipPosition);
-      if(shade->tiltType == tilt_types::none) tilt = -1;
       if(pos >= 0 && pos <= 100)
         if(!queueSetMyPosition(request, shade, shade->transformPosition(pos), shade->transformPosition(tilt))) return;
       AsyncJsonResp resp;
@@ -1792,11 +1833,11 @@ void Web::begin() {
         request->send(500, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Shade not found to set rolling code\"}"));
       }
       else {
-        shade->setRollingCode(rollingCode);
+        if(!queueSetRollingCode(request, shade, rollingCode)) return;
         AsyncJsonResp resp;
         resp.beginResponse(request, g_async_content, sizeof(g_async_content));
         resp.beginObject();
-        serializeShade(shade, resp);
+        serializeShade(shade, resp, shade->paired, rollingCode);
         resp.endObject();
         resp.endResponse();
       }
@@ -1814,11 +1855,11 @@ void Web::begin() {
       request->send(500, _encoding_json, F("{\"status\":\"ERROR\",\"desc\":\"Shade not found to set rolling code\"}"));
     }
     else {
-      shade->setRollingCode(rollingCode);
+      if(!queueSetRollingCode(request, shade, rollingCode)) return;
       AsyncJsonResp resp;
       resp.beginResponse(request, g_async_content, sizeof(g_async_content));
       resp.beginObject();
-      serializeShade(shade, resp);
+      serializeShade(shade, resp, shade->paired, rollingCode);
       resp.endObject();
       resp.endResponse();
     }
@@ -1892,7 +1933,7 @@ void Web::begin() {
         AsyncJsonResp resp;
         resp.beginResponse(request, g_async_content, sizeof(g_async_content));
         resp.beginObject();
-        serializeShade(shade, resp, false);
+        serializeShade(shade, resp, false, shade->lastRollingCode);
         resp.endObject();
         resp.endResponse();
       }
@@ -1911,7 +1952,7 @@ void Web::begin() {
       AsyncJsonResp resp;
       resp.beginResponse(request, g_async_content, sizeof(g_async_content));
       resp.beginObject();
-      serializeShade(shade, resp, false);
+      serializeShade(shade, resp, false, shade->lastRollingCode);
       resp.endObject();
       resp.endResponse();
     }
@@ -2284,7 +2325,10 @@ void Web::begin() {
         fup.close();
       }
       if(final) {
-        somfy.loadShadesFile("/shades.tmp");
+        // The main loop loads it, since it owns the shades.
+        web_command_t cmd = {};
+        cmd.type = web_cmd_t::load_shades;
+        webServer.queueCommand(cmd);
       }
     });
 

src/WebCommand.h

@@ -22,8 +22,19 @@ enum class web_cmd_t : uint8_t {
     group_sensor,       // group sensor command
     set_my_position,    // shade setMyPosition (position/tiltPosition)
     unpair_shade,       // shade unpair (Prog) and clear paired flag
+    set_rolling_code,   // set a shade's rolling code
+    restore_shades,     // restore from /shades.tmp, then reboot
+    load_shades,        // replace the shades with /shades.tmp
 };
 
+// What a restore_shades command restores, as bits of restoreParts.
+#define RESTORE_SETTINGS    0x01
+#define RESTORE_SHADES      0x02
+#define RESTORE_NETWORK     0x04
+#define RESTORE_TRANSCEIVER 0x08
+#define RESTORE_REPEATERS   0x10
+#define RESTORE_MQTT        0x20
+
 struct web_command_t {
     web_cmd_t type;
     uint8_t shadeId;
@@ -36,10 +47,22 @@ struct web_command_t {
     int8_t tiltPosition;    // for setPositions/tilt
     int8_t sunny;           // for sensor
     int8_t windy;           // for sensor
+    uint16_t rollingCode;   // for set_rolling_code
+    uint8_t restoreParts;   // for restore_shades: RESTORE_* bits
 };
 
-// Only setting positions never goes on air.
+// Whether the command sends anything on air.
-inline bool webCommandUsesRadio(web_cmd_t type) { return type != web_cmd_t::set_positions; }
+inline bool webCommandUsesRadio(web_cmd_t type) {
+  switch(type) {
+    case web_cmd_t::set_positions:
+    case web_cmd_t::set_rolling_code:
+    case web_cmd_t::restore_shades:
+    case web_cmd_t::load_shades:
+      return false;
+    default:
+      return true;
+  }
+}
 // Whether the command at the head of the queue waits for the radio.  The
 // client was already told it was accepted so it stays queued until the radio
 // has room rather than have the radio drop it.  While TX is suspended nothing

test/test_motion/test_motion.cpp

@@ -285,6 +285,25 @@ static void test_stepTargets_without_a_travel_time_or_step_size_is_ignored() {
   ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::none, DOWN, 100, 0, 1, 20000, 10000, 5000, 0).action);
 }
 
+/*********************************************************************
+ * requestedFavoriteTilt
+ ********************************************************************/
+static void test_favorite_tilt_keeps_the_requested_tilt() {
+  TEST_ASSERT_EQUAL_INT8(25, requestedFavoriteTilt(25, tilt_types::integrated, 70.0f, false));
+}
+static void test_favorite_tilt_defaults_to_the_favorite_tilt() {
+  // Not the lift favorite: that was the old default.
+  TEST_ASSERT_EQUAL_INT8(70, requestedFavoriteTilt(-1, tilt_types::tiltmotor, 70.0f, false));
+  // The request is in display positions, so a flipped shade gets its favorite flipped.
+  TEST_ASSERT_EQUAL_INT8(30, requestedFavoriteTilt(-1, tilt_types::tiltmotor, 70.0f, true));
+}
+static void test_favorite_tilt_without_a_favorite_is_unset() {
+  TEST_ASSERT_EQUAL_INT8(-1, requestedFavoriteTilt(-1, tilt_types::integrated, -1.0f, false));
+}
+static void test_favorite_tilt_of_a_shade_without_tilt_is_unset() {
+  TEST_ASSERT_EQUAL_INT8(-1, requestedFavoriteTilt(25, tilt_types::none, 70.0f, false));
+}
+
 int main(int argc, char **argv) {
   (void)argc;
   (void)argv;
@@ -312,6 +331,10 @@ int main(int argc, char **argv) {
   RUN_TEST(test_stepTargets_integrated_at_both_ends_is_ignored);
   RUN_TEST(test_stepTargets_without_a_travel_time_or_step_size_is_ignored);
   RUN_TEST(test_displayPosition);
+  RUN_TEST(test_favorite_tilt_keeps_the_requested_tilt);
+  RUN_TEST(test_favorite_tilt_defaults_to_the_favorite_tilt);
+  RUN_TEST(test_favorite_tilt_without_a_favorite_is_unset);
+  RUN_TEST(test_favorite_tilt_of_a_shade_without_tilt_is_unset);
   RUN_TEST(test_flipCommand_swaps_directions);
   RUN_TEST(test_flipCommand_keeps_the_rest);
   RUN_TEST(test_txhold_idle_shade_is_held);

test/test_webcmd/test_webcmd.cpp

@@ -10,17 +10,24 @@ static const web_cmd_t RADIO_COMMANDS[] = {
 void setUp() {}
 void tearDown() {}
 
-static void test_only_set_positions_stays_off_the_radio() {
+static const web_cmd_t LOOP_COMMANDS[] = {
+  web_cmd_t::set_positions, web_cmd_t::set_rolling_code, web_cmd_t::restore_shades, web_cmd_t::load_shades
+};
+
+static void test_only_loop_commands_stay_off_the_radio() {
   int offRadio = 0;
-  for(uint8_t t = 0; t <= static_cast<uint8_t>(web_cmd_t::unpair_shade); t++) {
+  for(uint8_t t = 0; t <= static_cast<uint8_t>(web_cmd_t::load_shades); t++) {
     if(!webCommandUsesRadio(static_cast<web_cmd_t>(t))) offRadio++;
   }
-  TEST_ASSERT_EQUAL_INT(1, offRadio);
+  TEST_ASSERT_EQUAL_INT(4, offRadio);
-  TEST_ASSERT_FALSE(webCommandUsesRadio(web_cmd_t::set_positions));
+  for(web_cmd_t t : LOOP_COMMANDS) TEST_ASSERT_FALSE(webCommandUsesRadio(t));
 }
-static void test_set_positions_is_never_held() {
+static void test_loop_commands_are_never_held() {
-  TEST_ASSERT_FALSE(holdWebCommand(web_cmd_t::set_positions, false, false));
+  // Nothing goes on air, so a full or suspended radio doesn't matter.
-  TEST_ASSERT_FALSE(holdWebCommand(web_cmd_t::set_positions, true, false));
+  for(web_cmd_t t : LOOP_COMMANDS) {
+    TEST_ASSERT_FALSE(holdWebCommand(t, false, false));
+    TEST_ASSERT_FALSE(holdWebCommand(t, true, false));
+  }
 }
 static void test_radio_command_waits_for_room() {
   for(web_cmd_t t : RADIO_COMMANDS) {
@@ -46,8 +53,8 @@ int main(int argc, char **argv) {
   (void)argc;
   (void)argv;
   UNITY_BEGIN();
-  RUN_TEST(test_only_set_positions_stays_off_the_radio);
+  RUN_TEST(test_only_loop_commands_stay_off_the_radio);
-  RUN_TEST(test_set_positions_is_never_held);
+  RUN_TEST(test_loop_commands_are_never_held);
   RUN_TEST(test_radio_command_waits_for_room);
   RUN_TEST(test_commands_are_not_held_while_suspended);
   RUN_TEST(test_unpair_waits_out_a_suspend);