fix: bound, dispatch and abstain across the record/response decoders

lib/src/commands.dart

@@ -158,8 +158,21 @@ Uint8List cmdToggleImu(int seq, bool on) =>
     buildCommand(seq, Cmd.toggleImuMode, [on ? 0x01 : 0x00]);
 Uint8List cmdEnableOptical(int seq, bool on) =>
     buildCommand(seq, Cmd.enableOpticalData, [revision1, on ? 0x01 : 0x00]);
-Uint8List cmdBuzz(int seq, [int pattern = hapticShortPulse]) =>
-    buildCommand(seq, Cmd.runHapticsPattern, [pattern, 0, 0, 0, 0]);
+/// Play a haptic waveform effect (RUN_HAPTICS_PATTERN = 0x4F).
+///
+/// [pattern] is a single u8 waveform-effect id. It is RANGE-CHECKED rather than
+/// masked: `buildFrame` would happily wrap `cmdBuzz(0, 300)` to effect 44 and
+/// hand the strap a perfectly CRC-valid packet playing the wrong effect, with
+/// nothing to tell the caller. A value that does not fit a u8 is a caller bug,
+/// so throw. (Contrast [cmdSetAlarm], which masks because its payload is a
+/// pattern LIST already validated for length.)
+Uint8List cmdBuzz(int seq, [int pattern = hapticShortPulse]) {
+  if (pattern < 0 || pattern > 0xff) {
+    throw ArgumentError.value(
+        pattern, 'pattern', 'haptic waveform effect must fit in a u8 (0-255)');
+  }
+  return buildCommand(seq, Cmd.runHapticsPattern, [pattern, 0, 0, 0, 0]);
+}
 
 // ── On-device haptic alarm (SET_ALARM_TIME = 0x42) ─────────────────────────
 //

lib/src/control.dart

@@ -163,6 +163,11 @@ R10Lite? parseR10Lite(Uint8List inner) {
 }
 
 // ── Compact realtime HR (small 0x28 packet body) ─────────────────────────────
+
+/// RR slots the compact 0x28 form can hold: [10] [12] [14] [16], stopping
+/// before the `wearing` byte at [18].
+const int _maxRealtimeRr = 4;
+
 class RealtimeHr {
   final int hrBpm;
   final double hrPrecise;
@@ -188,13 +193,23 @@ RealtimeHr? parseRealtimeHr(Uint8List inner) {
   // be one byte out of bounds. no rr_count byte just means no RR intervals,
   // not "reject this decode" (copilot review caught this, real bug).
   final n = inner.length > 9 ? inner[9] : 0;
-  if (n > 0 && inner.length >= 12) {
-    final v = u16(inner, 10);
-    if (v >= 200 && v <= 2500) rr.add(v);
-  }
-  if (n > 1 && inner.length >= 14) {
-    final v = u16(inner, 12);
-    if (v >= 200 && v <= 2500) rr.add(v);
+  // Read ALL declared intervals, not just the first two — this used to stop
+  // after slots 1 and 2, silently discarding beats 3 and 4 that live.dart's
+  // realtimeRr returns from the same bytes (fewer beats = a different RMSSD).
+  //
+  // The wire form has room for exactly four slots, at [10] [12] [14] [16],
+  // bounded by the `wearing` byte at [18]. A declared count above that cannot
+  // fit the layout, so — like live.dart, which rejects a large count as
+  // "wrong offset" — we emit NO intervals rather than reading `wearing` (or
+  // whatever follows) as a heartbeat. Values are gated to the same
+  // physiological range records.dart uses.
+  if (n > 0 && n <= _maxRealtimeRr) {
+    for (int i = 0; i < n; i++) {
+      final off = 10 + 2 * i;
+      if (off + 2 > inner.length) break;
+      final v = u16(inner, off);
+      if (v >= kMinRrMs && v <= kMaxRrMs) rr.add(v);
+    }
   }
   final wearing = inner.length > 18 ? inner[18] == 1 : true;
   return RealtimeHr(hr, hr.toDouble(), rr, wearing, ts);
@@ -232,6 +247,12 @@ class HelloInfo {
   });
 }
 
+/// A battery percentage is 0..100. Anything else is a mis-read field, not a
+/// battery level — callers must omit it, never clamp it (a clamp would report
+/// a confident 100% for garbage bytes).
+bool _validBatteryPct(double pct) =>
+    pct.isFinite && pct >= 0.0 && pct <= 100.0;
+
 List<String> _asciiRuns(Uint8List data, int start, int minlen) {
   final runs = <String>[];
   final cur = StringBuffer();
@@ -254,12 +275,20 @@ HelloInfo parseHello(Uint8List payload) {
   final info = HelloInfo(rawHex: _hex(payload));
   if (payload.length < 10) return info;
 
+  // Battery is a u16 in tenths of a percent whose offset drifts across
+  // firmware, so we scan for the first field that could BE one. The upper
+  // bound used to be 1009 (= 100.9%), which let an impossible reading through;
+  // a percentage is 0..100 by definition, so 1000 is the ceiling. Out of
+  // range = not the battery field, keep scanning / leave batteryPct null.
   for (int off = 1; off < 10; off++) {
     if (off + 2 <= payload.length) {
       final v = u16(payload, off);
-      if (v >= 10 && v <= 1009) {
-        info.batteryPct = _round(v / 10.0, 1);
-        break;
+      if (v >= 10 && v <= 1000) {
+        final pct = _round(v / 10.0, 1);
+        if (_validBatteryPct(pct)) {
+          info.batteryPct = pct;
+          break;
+        }
       }
     }
   }
@@ -397,12 +426,30 @@ CmdResponse? parseCommandResponse(Uint8List inner) {
   final payload = Uint8List.sublistView(inner, 3);
   final dec = <String, dynamic>{};
   if (op == Cmd.getBatteryLevel && inner.length >= 7) {
-    dec['battery_pct'] = _round(u16(inner, 5) / 10.0, 1); // u16 LE @[5:7] / 10
+    // u16 LE @[5:7] in tenths of a percent. A battery percentage outside
+    // 0..100 is not a battery percentage — `ff ff` here used to surface as
+    // 6553.5%. Emit nothing rather than a number the UI would render.
+    final pct = _round(u16(inner, 5) / 10.0, 1);
+    if (_validBatteryPct(pct)) dec['battery_pct'] = pct;
   } else if (op == Cmd.getHelloHarvard) {
     final h = parseHello(payload);
     dec['hello'] = h;
-  } else if (op == Cmd.getAlarmTime && payload.length >= 5) {
-    dec['alarm_epoch'] = u32(payload, 1);
+  } else if (op == Cmd.getAlarmTime && payload.isNotEmpty) {
+    // GET_ALARM_TIME echoes whichever alarm form the strap holds, and the
+    // epoch offset DIFFERS between them (this package writes both — see
+    // cmdSetAlarmSimple / cmdSetAlarm):
+    //   0x01  simple, 7 B:  [0x01][u32 epoch][u16 subsec]        → epoch @1
+    //   0x04  rich,  20 B:  [0x04][u8 index][u32 epoch][u16 subsec][12 B haptics]
+    //                                                             → epoch @2
+    // Reading @1 unconditionally decoded the rich form's [index][epoch:3] as
+    // the epoch. An unrecognised leading byte is an unknown form: emit no
+    // alarm_epoch rather than guessing an offset.
+    final form = payload[0];
+    if (form == 0x01 && payload.length >= 5) {
+      dec['alarm_epoch'] = u32(payload, 1);
+    } else if (form == 0x04 && payload.length >= 6) {
+      dec['alarm_epoch'] = u32(payload, 2);
+    }
   } else if (op == Cmd.getAdvertisingNameHarvard) {
     dec['strap_name'] = _decodeAdvName(payload);
   } else if (op == Cmd.getClock) {

lib/src/live.dart

@@ -12,10 +12,21 @@ import 'records.dart';
 class DecodedSample {
   final int ts; // unix seconds
   final int hr; // bpm (0 = off-wrist / no reading)
-  final double activity; // motion magnitude (stddev of |accel(g)|), 0 if no IMU
-  final int stepsInc; // steps detected in this record's IMU window (R10 only)
+
+  /// Motion magnitude (stddev of |accel(g)|) over this record's IMU window.
+  ///
+  /// NULL means "this record carried no usable IMU data" — an R10 frame that
+  /// was truncated before the accel arrays. That is NOT the same claim as
+  /// `0.0`, which means "the IMU was read and the wrist was still". The old
+  /// code returned 0.0 for both, so a truncated frame was indistinguishable
+  /// from a genuine zero-motion reading downstream.
+  final double? activity;
+
+  /// Steps detected in this record's IMU window (R10 only). Null when the IMU
+  /// window was unreadable (see [activity]); `0` means "read, no gait found".
+  final int? stepsInc;
   final bool wristOn; // worn proxy (hr>0)
-  final int recType; // 10 | 24 | 28
+  final int recType; // 7 | 9 | 10 | 12 | 18 | 24 | 25 | 28
 
   DecodedSample({
     required this.ts,
@@ -170,8 +181,13 @@ class _Motion {
 }
 
 // Decode the R10 IMU arrays into (activity, steps) over the 100-sample window.
-_Motion _r10Motion(ByteData view, int len) {
-  if (len < 685) return _Motion(0, 0);
+// Returns NULL when the frame is too short to contain the accel arrays at all
+// — "we could not measure motion", which the caller surfaces as a null
+// activity/steps rather than a fabricated 0.0/0 that reads like a real
+// zero-motion sample. A non-null result with steps==0 IS a measurement: the
+// window was read and no gait rhythm was found.
+_Motion? _r10Motion(ByteData view, int len) {
+  if (len < 685) return null;
   const acc = 1 / 4096;
   List<int> arr(int off) {
     final out = <int>[];
@@ -184,7 +200,7 @@ _Motion _r10Motion(ByteData view, int len) {
 
   final ax = arr(85), ay = arr(285), az = arr(485);
   final n = math.min(ax.length, math.min(ay.length, az.length));
-  if (n == 0) return _Motion(0, 0);
+  if (n == 0) return null; // nothing measured — same absence as a short frame.
   final mags = <double>[];
   for (int i = 0; i < n; i++) {
     final x = ax[i] * acc, y = ay[i] * acc, z = az[i] * acc;
@@ -283,8 +299,13 @@ DecodedSample? decodeRecord(String hex) {
 
   if (b.length < 18) return null;
 
-  // WHOOP 4 historical telemetry (v24 / v25 auto-routed by parseR24).
-  if (recType == 24 || recType == 25) {
+  // WHOOP 4 historical telemetry — EVERY layout version parseR24 has a field
+  // map for, not just v24/v25. v12 in particular is real, shipping firmware
+  // (Record.r12); routing it to null here cost the caller the record's own
+  // timestamp, so a multi-day backfill collapsed onto the capture time.
+  // parseR24 owns the per-version HR offset and the plausibility gate, so the
+  // versions it cannot decode honestly still come back null.
+  if (kKnownRecordVersions.contains(recType)) {
     final d = parseR24(b);
     if (d == null) return null;
     return DecodedSample(
@@ -301,12 +322,14 @@ DecodedSample? decodeRecord(String hex) {
   if (recType == 10) {
     final ts = view.getUint32(7, Endian.little);
     final hr = b[17];
+    // Null motion = the frame carried no readable IMU window; propagate that
+    // absence instead of reporting a measured zero.
     final m = _r10Motion(view, b.length);
     return DecodedSample(
       ts: ts,
       hr: hr,
-      activity: m.activity,
-      stepsInc: m.steps,
+      activity: m?.activity,
+      stepsInc: m?.steps,
       wristOn: hr > 0,
       recType: 10,
     );

lib/src/records.dart

@@ -13,6 +13,12 @@ class R24 {
   final int hr; // heart rate bpm @ inner[17] (frame[21], v24/v12)
 
   /// Beat-to-beat (R-R) intervals in ms for this 1 s record, 0–4 of them.
+  ///
+  /// [rrCount] is the number of intervals we ACCEPTED, i.e. always
+  /// `rrIntervalsMs.length` — not the raw declared count byte. A record whose
+  /// declared count is implausible (see [kMaxRrPerRecord]) or whose interval
+  /// values fall outside [kMinRrMs]..[kMaxRrMs] contributes nothing here rather
+  /// than handing HRV a fabricated beat.
   final int rrCount;
   final List<int> rrIntervalsMs;
 
@@ -91,14 +97,55 @@ class R24 {
 // Replicate JavaScript Math.round semantics: round half toward +Infinity
 // (Math.round(2.5)=3, Math.round(-2.5)=-2). Dart's roundToDouble rounds half
 // away from zero, so we implement the JS rule explicitly.
-double _jsRound(double v) => (v + 0.5).floorToDouble();
+//
+// NaN / ±Infinity are passed through UNCHANGED rather than being folded to 0.0
+// (which is what control.dart's `_round` does). Rounding cannot manufacture a
+// real number out of a non-finite one: a NaN accel component means the four
+// bytes we read are not a float32 field, and silently emitting 0.0 would turn
+// "these bytes are not accel" into "the wrist was perfectly still" — a
+// fabricated measurement that then propagates through every downstream
+// mean/std. control.dart can afford 0.0 because its callers only use it for
+// display scalars; here the honest answer is to keep the non-finite value
+// visible so [_parseV24Layout] can REJECT the whole record (see the
+// `isFinite` gate there), which is an absence the API can already express
+// (parseR24 returns null).
+double _jsRound(double v) {
+  if (!v.isFinite) return v;
+  return (v + 0.5).floorToDouble();
+}
 
 /// Round `v` to `decimals` places using JS `Math.round(v*p)/p` semantics.
+/// Non-finite input is returned unchanged — see [_jsRound].
 double _round(double v, int decimals) {
+  if (!v.isFinite) return v;
   final p = _pow10(decimals);
   return _jsRound(v * p) / p;
 }
 
+/// Largest R-R interval count a single 1 s historical record can plausibly
+/// declare. The sibling realtime decoder (live.dart `realtimeRr`) applies the
+/// same ceiling for the same reason: the wire form carries 0–4 beats, so a
+/// larger count byte means we are reading the wrong offset — not a second
+/// containing dozens of heartbeats. A record declaring more than this yields
+/// NO intervals at all (absence), never a truncated read of whatever bytes
+/// happen to follow (ppg, accel float32s, skin contact, spo2, temp, ambient).
+const int kMaxRrPerRecord = 8;
+
+/// Physiologically possible beat-to-beat interval bounds, ms — 2500 ms = 24 bpm,
+/// 200 ms = 300 bpm. Identical to the bound control.dart's `parseRealtimeHr`
+/// applies to the realtime RR slots, so both decoders agree on what a beat is.
+const int kMinRrMs = 200;
+const int kMaxRrMs = 2500;
+
+/// The historical-record layout versions this package has a field map for.
+/// v25 has its own dedicated layout ([_parseV25]); the rest share the v24 field
+/// map with a per-version HR offset ([_hrOffsetByVersion]). Used by live.dart's
+/// `decodeRecord` so every version [parseR24] can decode is actually routed to
+/// it (v12 in particular is real firmware and used to fall through to null,
+/// which cost callers the record's own timestamp).
+final Set<int> kKnownRecordVersions =
+    Set.unmodifiable({..._hrOffsetByVersion.keys, 25});
+
 double _pow10(int n) {
   double p = 1;
   for (int i = 0; i < n; i++) {
@@ -264,12 +311,25 @@ R24? _parseV24Layout(
   );
 
   // R-R intervals: rr_count @ [18], then rr_count signed int16 LE from [19].
-  final rrCount = inner[18];
+  //
+  // The declared count is UNTRUSTED. Taken raw it addresses up to 255 int16s
+  // starting at [19], which walks straight through ppg@29/31, the accel
+  // float32s@36/40/44, skin contact@51, spo2@64/66, skin temp@68 and
+  // ambient@70 — reinterpreting all of them as "beats" that then feed
+  // RMSSD/HRV. So: reject an implausible count outright, and accept only
+  // values inside the physiological interval range. Both guards run on EVERY
+  // path, including the "trusted" v24/v12 one that skips
+  // [_physiologicallyPlausible].
+  final declaredRrCount = inner[18];
   final rrIntervalsMs = <int>[];
-  for (int i = 0; i < rrCount && 19 + 2 * i + 2 <= inner.length; i++) {
-    final v = view.getInt16(19 + 2 * i, Endian.little);
-    if (v > 0) rrIntervalsMs.add(v);
+  if (declaredRrCount <= kMaxRrPerRecord) {
+    for (int i = 0; i < declaredRrCount && 19 + 2 * i + 2 <= inner.length; i++) {
+      final v = view.getInt16(19 + 2 * i, Endian.little);
+      if (v >= kMinRrMs && v <= kMaxRrMs) rrIntervalsMs.add(v);
+    }
   }
+  // rrCount reports what we accepted, never what the byte claimed.
+  final rrCount = rrIntervalsMs.length;
 
   final hr = inner[hrOffset];
   final accelG = [
@@ -278,6 +338,16 @@ R24? _parseV24Layout(
     _round(view.getFloat32(44, Endian.little), 4),
   ];
 
+  // A NaN / ±Infinity accel component means bytes [36:48] are not the float32
+  // vector this field map claims. Emitting 0.0 (or the raw NaN) would poison
+  // every downstream mean/std with a value that reads as a real measurement,
+  // so we reject the record instead — the caller already handles a null decode
+  // by archiving the raw bytes. This runs BEFORE the validate gate so it also
+  // covers the trusted v24/v12 path, which skips [_physiologicallyPlausible].
+  for (final c in accelG) {
+    if (!c.isFinite) return null;
+  }
+
   if (validate && !_physiologicallyPlausible(accelG, hr)) {
     return null;
   }