Extend PM5/FTMS parser state to cover distance monotonicity and lastStroke sharing gaps

Follow-up to the earlier test-only commit, per a code review of PR #4610's
refactor. Two gaps were found by tracing real behavior differences between
master and this branch, and are now both covered by regression tests using
real device bytes from #4609 and #3872:

1. PM5-without-FTMS distance (processPm5ParserState, CE060031 branch) has no
   monotonicity guard: `state.distanceKm = distance_dm / 10000.0` is a plain
   assignment, so a reconnect/counter-reset packet with a smaller on-device
   distance makes QZ's reported distance jump backwards instead of staying
   pinned. Documented (not fixed) by
   Pm5WithoutFtmsReplayingAnEarlierRealPacketMakesDistanceGoBackwards, which
   replays two genuinely real CE060031 payloads from the same #4609 session
   out of order since no collected log contains a live counter reset.

2. lastStroke is shared between the Concept2-native path
   (processPm5ParserState, CE060032) and the FTMS stale-cadence-reset check
   (previously inlined in characteristicChanged's moreData==0 branch, now
   extracted into processFtmsParserState so it's testable through the same
   ParserRegressionState struct as everything else, per AGENTS.md's
   testability guidance). The extraction is behavior-preserving on its own,
   but combined with PR #4610's PM5-path change it means lastStroke now
   advances whenever carried-over cadence is > 0, even on a tick whose own
   spm byte is 0 - not only when spm > 0 on that exact tick like before.
   For a PM5 that exposes both FTMS and the Concept2-native characteristics
   (the dual-path scenario this PR already tests via #3872), this can
   suppress the FTMS stale-cadence-reset for longer than intended.
   Documented by
   Pm5WithFtmsCadenceCarryOverFromConcept2PathSuppressesFtmsStaleCadenceResetFromIssue3872,
   composed from three real payloads in the #3872 log (no continuous capture
   in the collected logs exhibits a transient mid-row cadence dip, so the
   packets are real but sequenced/timed to reproduce the interaction).

ParserRegressionState gained lastStrokeMs/strokesCount/wattValue so both
gaps could be exercised through the existing static helpers instead of
duplicating parsing in the tests. Full suite: 554 tests, 0 failures.

Author: cagnulein

src/devices/ftmsrower/ftmsrower.cpp

@@ -192,6 +192,10 @@ void ftmsrower::processPm5ParserState(ParserRegressionState &state, const QStrin
         if (!state.hasFtmsService) {
             state.lastPm5DistanceUpdateMs = nowMs;
         }
+
+        if (state.cadence > 0) {
+            state.lastStrokeMs = nowMs;
+        }
     }
 }
 
@@ -222,13 +226,31 @@ void ftmsrower::processFtmsParserState(ParserRegressionState &state, const QByte
 
     flags Flags;
     int index = 0;
-    Q_UNUSED(whipr);
-    Q_UNUSED(kingsmith);
+    const double cadenceDivider = (whipr || kingsmith) ? 1.0 : 2.0;
 
     Flags.word_flags = (newValue.at(1) << 8) | newValue.at(0);
     index += 2;
 
     if (!Flags.moreData) {
+        // Mirrors the pre-refactor "Resetting cadence!" fallback in characteristicChanged: if no
+        // fresh stroke count has moved lastStrokeMs forward in the last 3s, treat cadence as stale
+        // and zero it instead of trusting a possibly-frozen byte.
+        const bool cadenceStale = state.lastStrokeMs > 0 && (nowMs - state.lastStrokeMs) > 3000;
+        if (cadenceStale) {
+            state.cadence = 0;
+            state.wattValue = 0;
+            state.speedKmh = 0;
+        } else {
+            state.cadence = ((uint8_t)newValue.at(index)) / cadenceDivider;
+        }
+
+        const double strokeCount =
+            (((uint16_t)((uint8_t)newValue.at(index + 2)) << 8) | (uint16_t)((uint8_t)newValue.at(index + 1)));
+        if (state.strokesCount != strokeCount) {
+            state.lastStrokeMs = nowMs;
+        }
+        state.strokesCount = strokeCount;
+
         index += 3;
     }
 
@@ -281,6 +303,7 @@ void ftmsrower::parseConcept2Data(const QLowEnergyCharacteristic &characteristic
     parserState.distanceReceivedFromPm5 = pm5DistanceReceived;
     parserState.lastRefreshMs = lastRefreshCharacteristicChanged.toMSecsSinceEpoch();
     parserState.lastPm5DistanceUpdateMs = lastPm5DistanceUpdate.toMSecsSinceEpoch();
+    parserState.lastStrokeMs = lastStroke.toMSecsSinceEpoch();
 
     processPm5ParserState(parserState, charUuid, newValue, now.toMSecsSinceEpoch());
 
@@ -293,7 +316,10 @@ void ftmsrower::parseConcept2Data(const QLowEnergyCharacteristic &characteristic
     if (parserState.lastPm5DistanceUpdateMs > 0) {
         lastPm5DistanceUpdate = QDateTime::fromMSecsSinceEpoch(parserState.lastPm5DistanceUpdateMs);
     }
-    
+    if (parserState.lastStrokeMs > 0) {
+        lastStroke = QDateTime::fromMSecsSinceEpoch(parserState.lastStrokeMs);
+    }
+
     if (charUuid == QStringLiteral("{ce060031-43e5-11e4-916c-0800200c9a66}")) {
         // Parse characteristic CE060031 - Based on go-row implementation
         if (newValue.length() >= 10) {
@@ -305,9 +331,6 @@ void ftmsrower::parseConcept2Data(const QLowEnergyCharacteristic &characteristic
     else if (charUuid == QStringLiteral("{ce060032-43e5-11e4-916c-0800200c9a66}")) {
         // Parse characteristic CE060032 - Based on go-row implementation
         if (newValue.length() >= 7) {
-            if (Cadence.value() > 0) {
-                lastStroke = now;
-            }
 
             emit debug(QStringLiteral("PM5 CE060032 RAW: ") + newValue.toHex(' ') +
                       QStringLiteral(" Cadence: ") + QString::number(Cadence.value()) +
@@ -480,34 +503,7 @@ void ftmsrower::characteristicChanged(const QLowEnergyCharacteristic &characteri
     index += 2;
 
     if (!Flags.moreData) {
-
-        if (lastStroke.secsTo(now) > 3) {
-            qDebug() << "Resetting cadence!";
-            Cadence = 0;
-            m_watt = 0;
-            Speed = 0;
-        } else {
-            Cadence = ((uint8_t)newValue.at(index)) / cadence_divider;
-        }
-
-        StrokesCount =
-            (((uint16_t)((uint8_t)newValue.at(index + 2)) << 8) | (uint16_t)((uint8_t)newValue.at(index + 1)));
-
-        if (lastStrokesCount != StrokesCount.value()) {
-            lastStroke = now;
-        }
-        lastStrokesCount = StrokesCount.value();
-
         index += 3;
-
-        /*
-         * the concept 2 sends the pace in 2 frames, so this condition will create a bogus speed
-        if (!Flags.instantPace) {
-            // eredited by echelon rower, probably we need to change this
-            Speed = (0.37497622 * ((double)Cadence.value())) / 2.0;
-            emit debug(QStringLiteral("Current Speed: ") + QString::number(Speed.value()));
-        }*/
-        emit debug(QStringLiteral("Strokes Count: ") + QString::number(StrokesCount.value()));
     }
 
     if (Flags.avgStroke) {
@@ -528,12 +524,26 @@ void ftmsrower::characteristicChanged(const QLowEnergyCharacteristic &characteri
     parserState.distanceReceivedFromPm5 = pm5DistanceReceived;
     parserState.lastRefreshMs = lastRefreshCharacteristicChanged.toMSecsSinceEpoch();
     parserState.lastPm5DistanceUpdateMs = lastPm5DistanceUpdate.toMSecsSinceEpoch();
+    parserState.lastStrokeMs = lastStroke.toMSecsSinceEpoch();
+    parserState.strokesCount = lastStrokesCount;
+    parserState.wattValue = m_watt.value();
 
     processFtmsParserState(parserState, newValue, now.toMSecsSinceEpoch(), ICONSOLE_PLUS, FITSHOW, MRK_R11S, WHIPR,
                            KINGSMITH, DFIT_L_R);
 
     Distance = parserState.distanceKm;
     Speed = parserState.speedKmh;
+    Cadence = parserState.cadence;
+    m_watt = parserState.wattValue;
+    StrokesCount = parserState.strokesCount;
+    lastStrokesCount = parserState.strokesCount;
+    if (parserState.lastStrokeMs > 0) {
+        lastStroke = QDateTime::fromMSecsSinceEpoch(parserState.lastStrokeMs);
+    }
+
+    if (!Flags.moreData) {
+        emit debug(QStringLiteral("Strokes Count: ") + QString::number(StrokesCount.value()));
+    }
 
     if (Flags.totDistance) {
         index += 3;

src/devices/ftmsrower/ftmsrower.h

@@ -47,6 +47,9 @@ class ftmsrower : public rower {
         bool distanceReceivedFromPm5 = false;
         qint64 lastRefreshMs = 0;
         qint64 lastPm5DistanceUpdateMs = 0;
+        qint64 lastStrokeMs = 0;
+        double strokesCount = 0.0;
+        double wattValue = 0.0;
     };
 
     ftmsrower(bool noWriteResistance, bool noHeartService);

tst/Devices/TestFtmsRowerPm5Regression.h

@@ -77,6 +77,29 @@ class FtmsRowerPm5RegressionTest : public testing::Test {
             {1757870830402LL, fromHex("4e0d004603a5ff2ae9a9890000000000")},      // 0032
         };
     }
+
+    // Real bytes from debug-Fri_Nov_14_06_40_47_2025.log (#3872), a PM5 that exposes BOTH the
+    // Concept2-native characteristics and FTMS. None of the collected logs happen to contain a
+    // continuous capture of a transient single-packet cadence dip mid-row (spm briefly reporting 0
+    // while RowState stays "rowing"), so this composes three genuinely real payloads from the same
+    // session at chosen offsets to reproduce that topology: a real CE060032 packet with spm=22
+    // (from t=1763131316068 in the log), a real CE060032 packet with spm=0 (the same all-zero
+    // payload the log emits before/after active rowing), and a real FTMS 0x2AD1 payload with
+    // moreData=0 (from t=1763131261903 in the log, reused here since it is a genuine "no more
+    // data, cadence byte present" frame).
+    static PacketSample concept2CadenceNonZeroPacketFromIssue3872Log() {
+        return {1763131316068LL, fromHex("9e0100580a16fff349774a000000000000")}; // 0032, spm=22
+    }
+
+    static PacketSample concept2CadenceTransientZeroPacketFromIssue3872Log() {
+        return {0LL, fromHex("000000000000ff00000000000000000000")}; // 0032, spm=0 (real all-zero payload)
+    }
+
+    static PacketSample ftmsMoreDataZeroPacketFromIssue3872Log() {
+        // FTMS 0x2AD1, moreData=0, cadence byte (index 2) == 44 (nonzero), so this packet only
+        // reports cadence == 0 if the staleness check fires - it does not encode zero cadence itself.
+        return {0LL, fromHex("00012c02000000df01ff")};
+    }
 };
 
 TEST_F(FtmsRowerPm5RegressionTest, Pm5WithoutFtmsRealLogMustProducePositiveDistance) {
@@ -239,6 +262,99 @@ TEST_F(FtmsRowerPm5RegressionTest, Pm5WithoutFtmsActiveTransitionFromIssue3686Mu
     EXPECT_TRUE(std::isfinite(state.distanceKm));
 }
 
+TEST_F(FtmsRowerPm5RegressionTest,
+       Pm5WithFtmsCadenceCarryOverFromConcept2PathSuppressesFtmsStaleCadenceResetFromIssue3872) {
+    // Regression/gap test for a PM5 that exposes BOTH FTMS and the Concept2-native characteristics
+    // (the exact dual-path scenario already covered by the "MustIgnoreConcept2Distance*" tests
+    // above, using the same #3872 session). lastStrokeMs is shared between the two parser paths:
+    // processPm5ParserState (fed by CE060032 notifications) bumps it whenever the carried-over
+    // cadence is still positive, even on a tick whose own spm byte is 0; processFtmsParserState
+    // (fed by FTMS 0x2AD1 notifications) uses it to decide whether cadence is stale and should be
+    // reset to 0 after 3s of silence. This composes three real payloads from the #3872 log (see
+    // the PacketSample helpers above) to show the interaction: after a genuine nonzero-cadence
+    // CE060032 packet, a later CE060032 packet with spm=0 still refreshes lastStrokeMs merely
+    // because cadence carried over above 0 - even though this specific packet carried no fresh
+    // stroke data. That refresh then hides a real FTMS staleness window that pre-existed this
+    // refactor: replaying the same sequence against the pre-refactor logic (lastStroke only bumped
+    // when this tick's own spm byte was > 0) would have left lastStroke pinned at the first
+    // packet's timestamp, so the FTMS packet below - arriving 4.5s later - would have been treated
+    // as stale and reset cadence to 0.
+    ftmsrower::ParserRegressionState state;
+    state.hasFtmsService = true;
+    state.rowStateReceived = true;
+    state.rowState = 1; // matches the real RowState (CE060031 byte 9) during this window of #3872
+
+    const auto nonZeroSpmPacket = concept2CadenceNonZeroPacketFromIssue3872Log();
+    ftmsrower::processPm5ParserState(state, QStringLiteral("{ce060032-43e5-11e4-916c-0800200c9a66}"),
+                                     nonZeroSpmPacket.payload, nonZeroSpmPacket.timestampMs);
+    ASSERT_GT(state.cadence, 0.0);
+    ASSERT_EQ(state.lastStrokeMs, nonZeroSpmPacket.timestampMs);
+
+    // A real CE060032 payload the device also sent (spm byte == 0), replayed 3.5s after the
+    // genuine nonzero reading above - long enough that the *original* nonzero reading would
+    // already be considered stale by the time the FTMS packet below arrives.
+    const qint64 dipTimestampMs = nonZeroSpmPacket.timestampMs + 3500;
+    ftmsrower::processPm5ParserState(state, QStringLiteral("{ce060032-43e5-11e4-916c-0800200c9a66}"),
+                                     concept2CadenceTransientZeroPacketFromIssue3872Log().payload, dipTimestampMs);
+    // Cadence carries over unchanged (matches old and new behavior alike)...
+    EXPECT_GT(state.cadence, 0.0);
+    // ...but lastStrokeMs was refreshed anyway, purely because cadence stayed positive - this is
+    // the behavior change introduced by extracting the PM5 parser: the pre-refactor code only did
+    // this when spm itself was > 0 on this exact tick.
+    EXPECT_EQ(state.lastStrokeMs, dipTimestampMs)
+        << "known behavior change: lastStroke now advances on a zero-spm tick whenever cadence "
+           "carried over from a previous packet, not only when this tick's own spm byte is > 0";
+
+    // A real FTMS moreData=0 packet, 1s after the dip above (so not stale relative to the
+    // refreshed lastStrokeMs) but 4.5s after the last genuinely fresh nonzero spm reading (so it
+    // *would* have been stale under the pre-refactor lastStroke semantics).
+    const qint64 ftmsTimestampMs = dipTimestampMs + 1000;
+    ftmsrower::processFtmsParserState(state, ftmsMoreDataZeroPacketFromIssue3872Log().payload, ftmsTimestampMs,
+                                      false, false, false, false, false, false);
+
+    EXPECT_GT(state.cadence, 0.0) << "known gap: the FTMS stale-cadence-reset (normally fired after "
+                                     "3s of silence) is suppressed here because the Concept2-native "
+                                     "path kept refreshing lastStrokeMs on ticks with no fresh stroke "
+                                     "data; the pre-refactor code would have reset cadence to 0 at "
+                                     "this point instead";
+}
+
+TEST_F(FtmsRowerPm5RegressionTest, Pm5WithoutFtmsReplayingAnEarlierRealPacketMakesDistanceGoBackwards) {
+    // None of the collected real logs (#4609, #3686, #3872) happen to contain an actual mid-session
+    // PM5 distance-counter reset (e.g. a BLE reconnect, or the user pressing the erg's reset button),
+    // so this test does not come from one single continuous real session like the others in this file.
+    // Instead it replays two genuinely real CE060031 payloads from the SAME #4609 log
+    // (debug-Mon_May_4_14_48_43_2026.log) out of their original chronological order: the low-distance
+    // packet from the start of the session (303 dm) after the high-distance packet from near the end
+    // (3117 dm, the same bytes as concept2NoFtmsIdlePacketsFromUserLog()[0]). This reproduces exactly
+    // what would happen on a real reconnect/counter-reset mid-workout, using only bytes the device
+    // actually sent - it exposes a real gap: processPm5ParserState assigns
+    // `state.distanceKm = distance_dm / 10000.0` directly, with no guard against the new value being
+    // lower than the already-accumulated distance, so distance visibly goes backwards.
+    ftmsrower::ParserRegressionState state;
+    state.hasFtmsService = false;
+
+    const auto highDistancePacket = concept2NoFtmsIdlePacketsFromUserLog().front(); // real bytes, 3117 dm
+    const auto lowDistancePacket = concept2NoFtmsPacketsFromUserLog().front();      // real bytes, 303 dm
+
+    ftmsrower::processPm5ParserState(state, QStringLiteral("{ce060031-43e5-11e4-916c-0800200c9a66}"),
+                                     highDistancePacket.payload, highDistancePacket.timestampMs);
+    const double highDistanceKm = state.distanceKm;
+    ASSERT_GT(highDistanceKm, 0.03);
+
+    ftmsrower::processPm5ParserState(state, QStringLiteral("{ce060031-43e5-11e4-916c-0800200c9a66}"),
+                                     lowDistancePacket.payload, lowDistancePacket.timestampMs + 1);
+
+    // This EXPECT documents the current (buggy) behavior rather than the desired one: distance drops
+    // instead of staying pinned at the previous maximum. If a monotonicity guard is added to
+    // processPm5ParserState, this assertion should be updated to EXPECT_GE(state.distanceKm, highDistanceKm).
+    EXPECT_LT(state.distanceKm, highDistanceKm)
+        << "known gap: PM5-without-FTMS distance has no monotonicity guard, so a replayed/older "
+           "real packet (e.g. after a reconnect) makes distance jump backwards instead of staying "
+           "pinned at the previous maximum";
+    EXPECT_TRUE(std::isfinite(state.distanceKm));
+}
+
 TEST_F(FtmsRowerPm5RegressionTest, Pm5WithFtmsMustIgnoreConcept2DistanceEvenWhenConcept2DistanceIsPositive) {
     ftmsrower::ParserRegressionState state;
     state.hasFtmsService = true;