refactor: pin which radio job holds a shade's simulated movement

Review flagged the outer My/Favorite losing to the nested Up/Down it queues.
Every such nested send is from a simMy shade, where the Up/Down is what
starts the motor, so the behaviour stays.  The hold moves into TxHold in
ShadeMotion with a comment explaining the order and unit tests pinning it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/ShadeMotion.cpp

@@ -29,3 +29,13 @@ 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));
 }
+void TxHold::mark(uint32_t jobSeq, bool moving, uint32_t now) {
+  if(jobSeq == 0 || this->seq != 0 || moving) return;
+  this->seq = jobSeq;
+  this->since = now;
+}
+bool TxHold::release(uint32_t jobSeq) {
+  if(jobSeq == 0 || jobSeq != this->seq) return false;
+  this->seq = 0;
+  return true;
+}

src/ShadeMotion.h

@@ -12,4 +12,23 @@ 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 radio job that set an idle shade moving.  Its simulated movement is held
+// until the job goes on air, since that is when the motor starts.
+struct TxHold {
+  uint32_t seq = 0;    // 0 when nothing is held.
+  uint32_t since = 0;  // millis() when the hold started.
+  // Holds for job seq unless the shade was already moving (its motor carries on
+  // with the earlier command) or a job is already held.  The first job held is
+  // the one that starts the motor.  A command sent while processing another
+  // registers before it: for a simMy shade My/Favorite queues its Up/Down from
+  // moveToMyPosition() first, and that Up/Down is what starts a motor with no
+  // favorite of its own, so the outer My/Favorite is ignored on purpose.
+  void mark(uint32_t jobSeq, bool moving, uint32_t now);
+  // Ends the hold when jobSeq is the held job.  True when it was.
+  bool release(uint32_t jobSeq);
+  bool held() const { return this->seq != 0; }
+  bool timedOut(uint32_t now, uint32_t timeoutMs) const { return now - this->since >= timeoutMs; }
+  void clear() { this->seq = 0; }
+};
 #endif

src/Somfy.cpp

@@ -762,20 +762,20 @@ void SomfyShade::checkMovement() {
   uint8_t currPos = floor(this->currentPos);
   uint8_t currTiltPos = floor(this->currentTiltPos);
   if(this->direction != 0) this->lastMovement = this->direction;
-  if(this->txPendingSeq != 0 && this->direction == 0 && this->tiltDirection == 0) {
+  if(this->txHold.held() && this->direction == 0 && this->tiltDirection == 0) {
     // The command didn't set the shade moving so there is nothing to hold.
-    this->txPendingSeq = 0;
+    this->txHold.clear();
   }
-  if(this->txPendingSeq != 0) {
+  if(this->txHold.held()) {
     // The command that set us moving hasn't gone on air yet so the motor hasn't
     // started.  Hold the simulated position until it does.
-    if((uint32_t)curTime - this->txPendingSince < TX_PENDING_TIMEOUT) {
+    if(!this->txHold.timedOut((uint32_t)curTime, TX_PENDING_TIMEOUT)) {
       this->moveStart = this->tiltStart = curTime;
       if(currDir != this->direction || currTiltDir != this->tiltDirection) this->emitState();
       return;
     }
     ESP_LOGW(TAG, "[Shade %u] Timed out waiting for the radio, moving anyway", this->shadeId);
-    this->txPendingSeq = 0;
+    this->txHold.clear();
   }
   if (sunFlag) {
     if (isSunny && !isWindy) {  // It is sunny and there is no wind so we should be extended
@@ -2335,20 +2335,15 @@ void SomfyShade::setTiltMovement(int8_t dir) {
   }
 }
 void SomfyShade::markTxPending(uint32_t seq) {
-  // The motor starts on the first of several queued commands to go on air.
-  if(this->txPendingSeq != 0) return;
   // This runs before the caller sets the new target so direction still says
-  // whether the shade was moving.  A moving motor carries on with its earlier
+  // whether the shade was moving.  A reversal is simulated as soon as it is
-  // command so its simulation must not be held.  A reversal is simulated as
+  // queued even though the motor turns once it is on air.  See TxHold::mark
-  // soon as it is queued even though the motor turns once it is on air.
+  // for which of several queued commands is held.
-  if(this->direction != 0 || this->tiltDirection != 0) return;
+  this->txHold.mark(seq, this->direction != 0 || this->tiltDirection != 0, millis());
-  this->txPendingSeq = seq;
-  this->txPendingSince = millis();
 }
 void SomfyShade::processTxEvent(const somfy_tx_event_t &evt) {
   if(evt.seq == 0) return;
-  if(this->txPendingSeq == evt.seq) {
+  if(this->txHold.release(evt.seq)) {
-    this->txPendingSeq = 0;
     if(evt.type == tx_event_t::started) {
       // The motor starts now so the simulated movement does too.
       this->moveStart = this->tiltStart = evt.at;

src/Somfy.h

@@ -23,6 +23,7 @@
 
 #include "SomfyFrame.h"
 #include "TxBuffer.h"
+#include "ShadeMotion.h"
 enum class group_types : byte {
   channel = 0x00
 };
@@ -240,9 +241,8 @@ class SomfyShade : public SomfyRemote {
     bool settingTiltPos = false;
     uint32_t awaitMy = 0;
     // Radio job that set the shade moving.  The simulated position is held
-    // until it goes on air.  0 when nothing is waiting.
+    // until it goes on air.
-    uint32_t txPendingSeq = 0;
+    TxHold txHold;
-    uint32_t txPendingSince = 0;
     // Stop sent when the simulated target was reached.  If it never goes on
     // air the motor runs on to its end limit.  0 once it is on air.
     uint32_t txStopSeq = 0;

test/test_motion/test_motion.cpp

@@ -110,6 +110,74 @@ static void test_flipCommand_keeps_the_rest() {
   for(somfy_commands cmd : same) ASSERT_CMD(cmd, flipCommand(cmd));
 }
 
+/*********************************************************************
+ * TxHold
+ ********************************************************************/
+static void test_txhold_idle_shade_is_held() {
+  TxHold hold;
+  hold.mark(7, false, 1000);
+  TEST_ASSERT_TRUE(hold.held());
+  TEST_ASSERT_EQUAL_UINT32(7, hold.seq);
+  TEST_ASSERT_EQUAL_UINT32(1000, hold.since);
+}
+static void test_txhold_moving_shade_is_not_held() {
+  TxHold hold;
+  hold.mark(7, true, 1000);
+  TEST_ASSERT_FALSE(hold.held());
+}
+static void test_txhold_ignores_seq_zero() {
+  TxHold hold;
+  hold.mark(0, false, 1000);
+  TEST_ASSERT_FALSE(hold.held());
+  hold.mark(7, false, 1000);
+  TEST_ASSERT_FALSE(hold.release(0));
+  TEST_ASSERT_TRUE(hold.held());
+}
+static void test_txhold_first_job_wins() {
+  // simMy Favorite: the Up/Down queued from inside processFrame (N+1) is
+  // marked before the outer Favorite (N), and the hold stays on the Up/Down.
+  TxHold hold;
+  hold.mark(11, false, 1000);
+  hold.mark(10, false, 1005);
+  TEST_ASSERT_EQUAL_UINT32(11, hold.seq);
+  TEST_ASSERT_EQUAL_UINT32(1000, hold.since);
+  TEST_ASSERT_FALSE(hold.release(10));
+  TEST_ASSERT_TRUE(hold.held());
+  TEST_ASSERT_TRUE(hold.release(11));
+  TEST_ASSERT_FALSE(hold.held());
+}
+static void test_txhold_release_only_matches_the_held_job() {
+  TxHold hold;
+  hold.mark(5, false, 0);
+  TEST_ASSERT_FALSE(hold.release(4));
+  TEST_ASSERT_FALSE(hold.release(6));
+  TEST_ASSERT_TRUE(hold.held());
+  TEST_ASSERT_TRUE(hold.release(5));
+  TEST_ASSERT_FALSE(hold.release(5));
+}
+static void test_txhold_new_hold_after_release() {
+  TxHold hold;
+  hold.mark(5, false, 0);
+  hold.release(5);
+  hold.mark(6, false, 200);
+  TEST_ASSERT_EQUAL_UINT32(6, hold.seq);
+  TEST_ASSERT_EQUAL_UINT32(200, hold.since);
+}
+static void test_txhold_times_out() {
+  TxHold hold;
+  hold.mark(5, false, 1000);
+  TEST_ASSERT_FALSE(hold.timedOut(1000, 30000));
+  TEST_ASSERT_FALSE(hold.timedOut(30999, 30000));
+  TEST_ASSERT_TRUE(hold.timedOut(31000, 30000));
+}
+static void test_txhold_times_out_across_millis_wrap() {
+  TxHold hold;
+  hold.mark(5, false, 0xFFFFF000u);
+  TEST_ASSERT_FALSE(hold.timedOut(0x00001000u, 30000));  // 8192ms later
+  TEST_ASSERT_FALSE(hold.timedOut(0x00006000u, 30000));  // 28672ms later
+  TEST_ASSERT_TRUE(hold.timedOut(0x00007000u, 30000));   // 32768ms later
+}
+
 int main(int argc, char **argv) {
   (void)argc;
   (void)argv;
@@ -126,5 +194,13 @@ int main(int argc, char **argv) {
   RUN_TEST(test_displayPosition);
   RUN_TEST(test_flipCommand_swaps_directions);
   RUN_TEST(test_flipCommand_keeps_the_rest);
+  RUN_TEST(test_txhold_idle_shade_is_held);
+  RUN_TEST(test_txhold_moving_shade_is_not_held);
+  RUN_TEST(test_txhold_ignores_seq_zero);
+  RUN_TEST(test_txhold_first_job_wins);
+  RUN_TEST(test_txhold_release_only_matches_the_held_job);
+  RUN_TEST(test_txhold_new_hold_after_release);
+  RUN_TEST(test_txhold_times_out);
+  RUN_TEST(test_txhold_times_out_across_millis_wrap);
   return UNITY_END();
 }