feat(protocol): multi-band support — WHOOP 5 (gen5) framing + records - #16
Merged
Conversation
WHOOP 5 is WHOOP 4 in a different envelope: only the outer frame header
changes (gen4 4-byte + crc8 → gen5 8-byte + crc16-modbus); the payload
CRC32 and the command opcodes are shared. Introduce a BandProfile so the
frame layer carries that difference and the rest of the stack stays
band-agnostic. gen4 is the default profile, so WHOOP 4 is byte-identical
and all 2934 decode-parity cases still pass.
- crc.dart: crc16Modbus (verified 0x71E6 on the gen5 hello header)
- band.dart: DeviceType{gen4,gen5}, BandProfile, GattProfile
(gen4 6108… / gen5 fd4b…)
- framing.dart: buildFrame / parseFrame / FrameReassembler take a
BandProfile (default gen4)
- commands.dart: gen5ClientHello (GET_HELLO 0x91, reproduces the canonical
hello byte-for-byte) + gen5 empty-payload offload helpers;
profile threaded through command + history-result builders
- records.dart: parseGen5Record — thin K24 (HR@17 + timing only). Motion
(K10/K21) uses the existing R10 offsets; SpO2/temp/RR are
honest-null pending validated offsets (never fabricated)
- test: 16 gen5 cases (crc16, hello, framing round-trip, gen4
regression, gen5 reassembly, history ACK token echo, K24)
📝 WalkthroughWalkthroughAdds gen5 support for GATT profiles, CRC16-Modbus framing, commands, control-plane decoding, historical records, console logs, public exports, and end-to-end tests. Gen4 remains the default. ChangesGen5 protocol support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FrameReassembler
participant parseFrame
participant decodeFrame
participant parseGen5Historical
FrameReassembler->>parseFrame: parse profile-specific Gen5 frame
parseFrame->>decodeFrame: provide validated inner payload
decodeFrame->>parseGen5Historical: decode historical record
parseGen5Historical-->>decodeFrame: return structured Gen5 record
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/src/commands.dart (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
buildCommandwith the namedprofileconvention.
buildCommandis the only builder that makesprofilepositional, so callers have to passpayloadfirst just to override the frame profile. Make it a named parameter like the other helpers, and update thebuildHistoryResultFailcall to useprofile:too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/commands.dart` around lines 18 - 30, Update buildCommand so profile is a named parameter while preserving payload as the existing positional optional argument and retaining the default BandProfile.gen4 value. Update the buildHistoryResultFail call site to pass the selected profile using the profile: named argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/src/commands.dart`:
- Around line 18-30: Update buildCommand so profile is a named parameter while
preserving payload as the existing positional optional argument and retaining
the default BandProfile.gen4 value. Update the buildHistoryResultFail call site
to pass the selected profile using the profile: named argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c916b9c-27bd-496a-8453-a7691ba41f90
📒 Files selected for processing (7)
lib/openstrap_protocol.dartlib/src/band.dartlib/src/commands.dartlib/src/crc.dartlib/src/framing.dartlib/src/records.darttest/gen5_test.dart
|
Cross-checked the gen5 framing here against two independent public WHOOP RE efforts (a WHOOP 4.0 findings write-up and a WHOOP 4.0/5.0 iOS+Rust decoder) and it lines up byte-for-byte. A few things I confirmed independently:
One future surface, not blocking: packet type Nice work on this. |
# Conflicts: # lib/src/framing.dart
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/records.dart (1)
131-144: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLimit accepted R-R intervals to four.
kMaxRrPerRecord = 8permits the loop to decode bytes outside the documented four-interval R-R region. A declared count of six or more reads the PPG block atinner[29]as beat timing. If those bytes are within the physiological range, the decoder emits fabricated RR intervals.Set the limit to
4.Proposed fix
-const int kMaxRrPerRecord = 8; +const int kMaxRrPerRecord = 4;Also applies to: 324-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/records.dart` around lines 131 - 144, Update the kMaxRrPerRecord constant used by the historical RR decoder loop to 4, ensuring declared counts above four produce no RR intervals and cannot read into subsequent record fields; leave the existing interval bounds and decoding behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/framing.dart`:
- Around line 96-99: Update the documentation for the reassembler’s resyncs
counter to replace the profile-specific “crc8 did not match” wording with the
neutral “header CRC did not match” description, while preserving the surrounding
explanations of skipped bytes and Frame.valid.
---
Outside diff comments:
In `@lib/src/records.dart`:
- Around line 131-144: Update the kMaxRrPerRecord constant used by the
historical RR decoder loop to 4, ensuring declared counts above four produce no
RR intervals and cannot read into subsequent record fields; leave the existing
interval bounds and decoding behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b062639-186b-446d-9067-3502613f6b28
📒 Files selected for processing (3)
lib/src/commands.dartlib/src/framing.dartlib/src/records.dart
Comment on lines
96
to
99
| @@ -86,6 +99,8 @@ class FrameReassembler { | |||
| /// length is discarded here, so it never reaches [Frame.valid]. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a profile-neutral header CRC description.
Gen5 frames use CRC16-Modbus, not CRC8. Replace “crc8 did not match” with “header CRC did not match” so resyncs describes both profiles correctly.
Proposed fix
- /// length field whose crc8 did not match). Callers use this to detect a degraded link — a bad
+ /// length field whose header CRC did not match). Callers use this to detect a degraded link — a bad🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/framing.dart` around lines 96 - 99, Update the documentation for the
reassembler’s resyncs counter to replace the profile-specific “crc8 did not
match” wording with the neutral “header CRC did not match” description, while
preserving the surrounding explanations of skipped bytes and Frame.valid.
…/v21/v26 decoders
_gen5NormalHistoryVersions = {9, 12, 24} targeted WHOOP4's thin/rich
HR-only and full-optical layouts, not anything a real WHOOP 5.0/MG strap
ships. Real gen5 historical data (packet type 0x2F) ships hist_version
bytes 18 (per-second biometric summary), 20 (6-channel optical deep
buffer), 21 (100Hz IMU deep buffer), 26 (24Hz PPG waveform) -- confirmed
independently by two hardware-tested reference implementations and
re-verified here byte-by-byte (CRC16 + CRC32) against 3 real captures.
gen5_records.dart owns the new decoders plus a small RecordDecoder
interface (matches()-then-decode(), v21 identified by paired
100-sample counts rather than trusting hist_version, per both
references' own dispatch convention). parseR24/records.dart is back to
gen4-only, its original scope.
…y gate Byte-verified against 8 real gen5 fixtures: header bytes[4:6] are NOT a universal [0x00,0x01] constant as both upstream reference repos assumed -- every host->strap COMMAND frame carries [0x00,0x01], every strap->host frame of any other packet type carries [0x01,0x00] instead. BandProfile now carries this as outboundDirectionMarker / inboundDirectionMarker data instead of a literal buried in buildHeader, and nothing gates inbound-frame validity on it. Also adds OpcodeSafety (whoop-rs's forbidden/destructive opcode lists, published as data for a call site to enforce -- not enforced here), the gen5-exclusive opcode values (SET_CLOCK_MAVERICK, GET_CLOCK_GEN5, RUN_HAPTIC_PATTERN_MAVERICK, SET_DEVICE_CONFIG_VALUE, SET_FF_VALUE), and the BLE_REALTIME_HR_ON/OFF event ids.
…ery event CONSOLE_LOGS (0x32) had no decoder in either generation -- added parseConsoleLog + a ConsoleLogReassembler for log lines that straddle multiple consecutive record_index frames. parseCommandResponse now takes a BandProfile: GET_BATTERY_LEVEL is direct-percent on gen5 vs gen4's deci-percent (same opcode, different wire scale), and GET_HELLO (gen5's 0x91, a different opcode from gen4's GET_HELLO_HARVARD) decodes device_name/fw_version. Also adds the BATTERY_LEVEL event's soc/battery_mV/charging fields and GET_DATA_RANGE's ring-buffer backlog telemetry -- both are inner-relative and band-agnostic, so they apply to gen4 captures too. decodeFrame takes an optional profile and now dispatches gen5 historical records through parseGen5Historical.
…lders buildR22EnableSequence sends the 16 SET_CONFIG flags a real gen5 strap needs before it will ever emit v20/v21/v26 (the official app never sends this -- a fresh connection otherwise only yields v18). Also: cmdSetClockGen5/cmdGetClockGen5 (gen5's distinct opcode values, gen4 payload shape as the best-supported assumption pending hardware verification), cmdBuzzGen5Maverick (opcode 0x13, a different opcode from gen4's cmdBuzz/0x4F despite similar purpose), and a `profile` param on cmdSetAlarm/cmdSetAlarmSimple/cmdRunAlarm/cmdDisableAlarm so their already-shared opcodes (66/69) can be framed for either generation.
…tures gen5_test.dart drops the old K24-thin tests (parseGen5Record is gone) and covers framing/commands/opcode-safety/event-vocabulary instead. gen5_historical_test.dart is new: golden parity tests for the real, independently byte-verified v18 and v26 captures (every field asserted against the fixture's actual bytes, not just "doesn't throw"), plus structural tests for v20/v21 (no full real capture was available for this task, so these exercise the byte-verified offsets/scales with synthetic buffers), the REALTIME_DATA/METADATA fixtures proving gen4's inner-relative offsets already work unchanged for gen5, and the CONSOLE_LOGS/COMMAND_RESPONSE/decodeFrame additions.
…tighten GET_DATA_RANGE scan, gate activity_class, clamp maverick loop count, flag v20 as unresolved
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/commands.dart`:
- Around line 429-459: Clarify the introductory comment above cmdSetConfigGen5
to state that the 40-byte body count excludes the leading revision1/[0x01] byte,
while the complete payload after the opcode is 41 bytes. Keep the existing field
layout and per-function frame documentation consistent with this distinction.
- Around line 461-480: Extract the duplicated ASCII name validation,
single-character value validation, and 32-byte NUL-padded name construction from
cmdSetConfigGen5 and cmdSetDeviceConfigValueGen5 into one private helper, then
update both builders to reuse it while preserving their existing payload formats
and validation behavior.
In `@lib/src/constants.dart`:
- Around line 131-152: Update the `forbidden` set to reference the named `Cmd`
constants for opcodes 10, 25, 29, 32, 77, 119, 120, 123, and 146, while
retaining unnamed values 99 and 142–144 as numeric literals with the existing
comment. Add a brief documentation note identifying opcode 45 as an unnamed
destructive opcode in the `destructive` declaration.
In `@lib/src/control.dart`:
- Around line 789-806: Update ConsoleLogReassembler.add in
lib/src/control.dart:789-806 to return the completed run as String? when a
record_index discontinuity occurs, preserving the prior buffer instead of
clearing it irretrievably; return null for contiguous additions, and
parenthesize the modulo-65536 successor expression. In
test/gen5_historical_test.dart:454-468, update boolean-contract assertions for
the new return type and add coverage feeding indices 1 → 2 → 4 without an
intermediate flush, asserting the completed run is returned.
- Line 636: Replace the fixed 1900000000 value in _maxPlausibleUnixForRange with
a runtime-derived upper bound based on the current time, while retaining the
intended tight plausibility window used by _plausibleUnixRange. Ensure future
real strap timestamps are accepted without allowing an unbounded range; if a
compile-time constant must remain, add coverage that exposes the cutoff before
it is reached.
- Around line 878-892: Replace the unstable g.runtimeType.toString() assignment
in _decodeDataRecord with the stable Gen5RecordDecoder.name value, or an
explicit mapping for Gen5HistorySample, Gen5OpticalBuffer, Gen5ImuBuffer, and
Gen5PpgWaveform, preserving an explicit fallback for unknown records. Document
that Gen5PpgWaveform.recordIndex is a u16 at inner[3] while other kinds use a
u32, and add a test assertion verifying the emitted inner kind value.
- Around line 762-775: Update parseConsoleLog to bound the text slice using
chunkLen from u16(inner, 10), clamped to the available bytes after offset 13, so
trailing real NUL bytes are preserved. Apply the existing 2048-byte cap within
that bounded slice before passing it to _consoleLogText, while keeping the
current packet validation and metadata parsing unchanged.
In `@lib/src/gen5_records.dart`:
- Around line 444-491: Document the one trailing byte in each 422-byte block by
adding an explicit layout comment near _kV20BlockLen or _decodeOpticalBlock,
identifying it as the known field or unknown padding. Keep the existing 21-byte
metadata and 200-byte channel regions and 422-byte stride unchanged.
- Around line 726-766: Update parseGen5Historical to dispatch through
kGen5HistoricalDecoders in its declared order instead of hardcoding the v21
probe and version switch. Ensure each Gen5RecordDecoder is given the opportunity
to match and decode, preserving the existing short-input and
null-for-unsupported behavior, so registering a decoder in the public registry
automatically makes it dispatchable.
In `@lib/src/records.dart`:
- Around line 488-508: Add a CHANGELOG entry for the removed public
parseGen5Record API, stating that it used the incorrect version set and
directing consumers to parseGen5Historical. Bump the package version in the
package’s version metadata to reflect this breaking API change.
In `@test/gen5_historical_test.dart`:
- Around line 16-23: Extract the duplicated top-level hex helper from
gen5_historical_test.dart and gen5_test.dart into a shared test utility such as
hex_helper.dart, preserving its current whitespace removal and radix-16 parsing
behavior. Import the shared helper in both test suites and remove their local
definitions.
- Around line 454-468: Add a separate test near the existing
ConsoleLogReassembler coverage that feeds record indices 1, 2, and 4 without
calling flush between additions, then assert the final flush result according to
the proposed contract. Keep the existing contiguous and explicitly flushed gap
test unchanged, and use ConsoleLogReassembler.add and flush to verify the
unflushed earlier run behavior.
- Around line 163-192: In test/gen5_historical_test.dart lines 163-192,
right-pad each truncated fixture to kGen5V26MinInnerLen, parse it with
parseGen5Historical, and assert recordIndex through the production parser
instead of computing inner offsets inline. In test/gen5_historical_test.dart
lines 311-324, assert bodyStart + 5 * blockLen equals the exported
kGen5V20InnerLen. Consider exporting the v20/v21 layout offsets from
gen5_records.dart so both test suites can validate production constants
directly.
In `@test/gen5_test.dart`:
- Around line 177-180: Update the test named “rejects an over-length name /
non-ASCII / multi-char value” to add separate invalid-input assertions covering
a non-ASCII name and a non-ASCII value, exercising both non-ASCII validation
branches in cmdSetConfigGen5 while preserving the existing over-length and
multi-character cases.
- Around line 213-227: Extend the test for cmdSetClockGen5 with deterministic
assertions for the encoded timestamp payload: verify the little-endian u32 epoch
bytes at setParsed.inner[0..3] and the 32768-tick subsecond bytes at
setParsed.inner[4..5] for the fixed UTC date. Keep the existing opcode
assertions and cmdGetClockGen5 coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b706e607-6c55-4140-8314-8386fcc3c248
📒 Files selected for processing (9)
lib/openstrap_protocol.dartlib/src/band.dartlib/src/commands.dartlib/src/constants.dartlib/src/control.dartlib/src/gen5_records.dartlib/src/records.darttest/gen5_historical_test.darttest/gen5_test.dart
Comment on lines
+429
to
459
| // ── gen5 SET_CONFIG (opcode 120) + the R22 deep-buffer enable sequence ───── | ||
| // | ||
| // Opcode-identical to gen4's SET_FF_VALUE, but gen5's R22 deep buffers | ||
| // (v20 optical / v21 IMU / v26 PPG — see gen5_records.dart) are OFF by | ||
| // default even in the official WHOOP app; a strap will only ever emit v18 | ||
| // unless this 16-flag sequence is sent first. Byte-verified body shape (a | ||
| // real `enable_r22_packets` capture): 40-byte body = ASCII key name | ||
| // NUL-padded to 32 bytes, + 1 value byte @ offset 32, + 7 zero bytes. | ||
|
|
||
| /// One SET_CONFIG (120) frame: `[0x23][seq][120][0x01][name:32B NUL-padded] | ||
| /// [value:1B][zero:7B]`. [name] must fit in 31 bytes (leaving room for the | ||
| /// NUL terminator within the 32-byte field) and [value] must be a single | ||
| /// ASCII character (this package's config values are always `'1'` or `'2'`). | ||
| Uint8List cmdSetConfigGen5(int seq, String name, String value) { | ||
| if (name.codeUnits.any((c) => c > 0x7f) || name.length > 31) { | ||
| throw ArgumentError.value( | ||
| name, 'name', 'must be <=31 ASCII chars (32-byte NUL-padded field)'); | ||
| } | ||
| if (value.length != 1 || value.codeUnitAt(0) > 0x7f) { | ||
| throw ArgumentError.value( | ||
| value, 'value', 'must be a single ASCII character'); | ||
| } | ||
| final nameBytes = Uint8List(32)..setRange(0, name.length, name.codeUnits); | ||
| final payload = <int>[ | ||
| revision1, | ||
| ...nameBytes, | ||
| value.codeUnitAt(0), | ||
| 0, 0, 0, 0, 0, 0, 0, // 7 zero bytes | ||
| ]; | ||
| return buildCommand(seq, Cmd.setFfValue, payload, BandProfile.gen5); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Align the documented body length with the emitted bytes.
The comment at Line 435-436 states a "40-byte body = ASCII key name NUL-padded to 32 bytes, + 1 value byte @ offset 32, + 7 zero bytes" (32+1+7 = 40). The emitted payload adds revision1 first, so the bytes after the opcode are 41. The per-function doc at Line 438-439 already shows the [0x01] byte separately. Clarify whether the 40-byte count excludes the [0x01] revision byte, so a future reader comparing against a capture does not shift every offset by one.
📝 Proposed doc clarification
-// default even in the official WHOOP app; a strap will only ever emit v18
-// unless this 16-flag sequence is sent first. Byte-verified body shape (a
-// real `enable_r22_packets` capture): 40-byte body = ASCII key name
-// NUL-padded to 32 bytes, + 1 value byte @ offset 32, + 7 zero bytes.
+// default even in the official WHOOP app; a strap will only ever emit v18
+// unless this 16-flag sequence is sent first. Byte-verified body shape (a
+// real `enable_r22_packets` capture): a leading revision byte (0x01), then a
+// 40-byte body = ASCII key name NUL-padded to 32 bytes, + 1 value byte @
+// body offset 32, + 7 zero bytes (41 bytes after the opcode in total).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ── gen5 SET_CONFIG (opcode 120) + the R22 deep-buffer enable sequence ───── | |
| // | |
| // Opcode-identical to gen4's SET_FF_VALUE, but gen5's R22 deep buffers | |
| // (v20 optical / v21 IMU / v26 PPG — see gen5_records.dart) are OFF by | |
| // default even in the official WHOOP app; a strap will only ever emit v18 | |
| // unless this 16-flag sequence is sent first. Byte-verified body shape (a | |
| // real `enable_r22_packets` capture): 40-byte body = ASCII key name | |
| // NUL-padded to 32 bytes, + 1 value byte @ offset 32, + 7 zero bytes. | |
| /// One SET_CONFIG (120) frame: `[0x23][seq][120][0x01][name:32B NUL-padded] | |
| /// [value:1B][zero:7B]`. [name] must fit in 31 bytes (leaving room for the | |
| /// NUL terminator within the 32-byte field) and [value] must be a single | |
| /// ASCII character (this package's config values are always `'1'` or `'2'`). | |
| Uint8List cmdSetConfigGen5(int seq, String name, String value) { | |
| if (name.codeUnits.any((c) => c > 0x7f) || name.length > 31) { | |
| throw ArgumentError.value( | |
| name, 'name', 'must be <=31 ASCII chars (32-byte NUL-padded field)'); | |
| } | |
| if (value.length != 1 || value.codeUnitAt(0) > 0x7f) { | |
| throw ArgumentError.value( | |
| value, 'value', 'must be a single ASCII character'); | |
| } | |
| final nameBytes = Uint8List(32)..setRange(0, name.length, name.codeUnits); | |
| final payload = <int>[ | |
| revision1, | |
| ...nameBytes, | |
| value.codeUnitAt(0), | |
| 0, 0, 0, 0, 0, 0, 0, // 7 zero bytes | |
| ]; | |
| return buildCommand(seq, Cmd.setFfValue, payload, BandProfile.gen5); | |
| } | |
| // ── gen5 SET_CONFIG (opcode 120) + the R22 deep-buffer enable sequence ───── | |
| // | |
| // Opcode-identical to gen4's SET_FF_VALUE, but gen5's R22 deep buffers | |
| // (v20 optical / v21 IMU / v26 PPG — see gen5_records.dart) are OFF by | |
| // default even in the official WHOOP app; a strap will only ever emit v18 | |
| // unless this 16-flag sequence is sent first. Byte-verified body shape (a | |
| // real `enable_r22_packets` capture): a leading revision byte (0x01), then a | |
| // 40-byte body = ASCII key name NUL-padded to 32 bytes, + 1 value byte @ | |
| // body offset 32, + 7 zero bytes (41 bytes after the opcode in total). | |
| /// One SET_CONFIG (120) frame: `[0x23][seq][120][0x01][name:32B NUL-padded] | |
| /// [value:1B][zero:7B]`. [name] must fit in 31 bytes (leaving room for the | |
| /// NUL terminator within the 32-byte field) and [value] must be a single | |
| /// ASCII character (this package's config values are always `'1'` or `'2'`). | |
| Uint8List cmdSetConfigGen5(int seq, String name, String value) { | |
| if (name.codeUnits.any((c) => c > 0x7f) || name.length > 31) { | |
| throw ArgumentError.value( | |
| name, 'name', 'must be <=31 ASCII chars (32-byte NUL-padded field)'); | |
| } | |
| if (value.length != 1 || value.codeUnitAt(0) > 0x7f) { | |
| throw ArgumentError.value( | |
| value, 'value', 'must be a single ASCII character'); | |
| } | |
| final nameBytes = Uint8List(32)..setRange(0, name.length, name.codeUnits); | |
| final payload = <int>[ | |
| revision1, | |
| ...nameBytes, | |
| value.codeUnitAt(0), | |
| 0, 0, 0, 0, 0, 0, 0, // 7 zero bytes | |
| ]; | |
| return buildCommand(seq, Cmd.setFfValue, payload, BandProfile.gen5); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/commands.dart` around lines 429 - 459, Clarify the introductory
comment above cmdSetConfigGen5 to state that the 40-byte body count excludes the
leading revision1/[0x01] byte, while the complete payload after the opcode is 41
bytes. Keep the existing field layout and per-function frame documentation
consistent with this distinction.
Comment on lines
+461
to
+480
| /// SET_DEVICE_CONFIG_VALUE (119) — a distinct, SMALLER sibling of SET_CONFIG | ||
| /// (120): a 33-byte body with NO trailing padding (`[name:32B NUL-padded] | ||
| /// [value:1B]`, vs 120's 40-byte body). ASSUMPTION: the multiband spec | ||
| /// confirms the body is 33 bytes with no padding but does not give a | ||
| /// byte-verified real capture for this opcode specifically — this mirrors | ||
| /// 120's name/value convention as the best-supported guess. Verify against a | ||
| /// real capture before relying on it. | ||
| Uint8List cmdSetDeviceConfigValueGen5(int seq, String name, String value) { | ||
| if (name.codeUnits.any((c) => c > 0x7f) || name.length > 31) { | ||
| throw ArgumentError.value( | ||
| name, 'name', 'must be <=31 ASCII chars (32-byte NUL-padded field)'); | ||
| } | ||
| if (value.length != 1 || value.codeUnitAt(0) > 0x7f) { | ||
| throw ArgumentError.value( | ||
| value, 'value', 'must be a single ASCII character'); | ||
| } | ||
| final nameBytes = Uint8List(32)..setRange(0, name.length, name.codeUnits); | ||
| final payload = <int>[...nameBytes, value.codeUnitAt(0)]; | ||
| return buildCommand(seq, Cmd.setDeviceConfigValue, payload, BandProfile.gen5); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the shared name/value validation.
cmdSetConfigGen5 and cmdSetDeviceConfigValueGen5 repeat the same two validation blocks and the same 32-byte NUL-padded name construction. Extract one private helper. This keeps the two builders in sync if the field width or the ASCII rule changes.
♻️ Proposed refactor
+Uint8List _gen5ConfigNameField(String name) {
+ if (name.codeUnits.any((c) => c > 0x7f) || name.length > 31) {
+ throw ArgumentError.value(
+ name, 'name', 'must be <=31 ASCII chars (32-byte NUL-padded field)');
+ }
+ return Uint8List(32)..setRange(0, name.length, name.codeUnits);
+}
+
+int _gen5ConfigValueByte(String value) {
+ if (value.length != 1 || value.codeUnitAt(0) > 0x7f) {
+ throw ArgumentError.value(
+ value, 'value', 'must be a single ASCII character');
+ }
+ return value.codeUnitAt(0);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/commands.dart` around lines 461 - 480, Extract the duplicated ASCII
name validation, single-character value validation, and 32-byte NUL-padded name
construction from cmdSetConfigGen5 and cmdSetDeviceConfigValueGen5 into one
private helper, then update both builders to reuse it while preserving their
existing payload formats and validation behavior.
Comment on lines
+131
to
+152
| static const Set<int> forbidden = { | ||
| 10, | ||
| 146, | ||
| 25, | ||
| 29, | ||
| 32, | ||
| 45, | ||
| 77, | ||
| 119, | ||
| 120, | ||
| 99, | ||
| 123, | ||
| 142, | ||
| 143, | ||
| 144, | ||
| }; | ||
|
|
||
| /// The subset of [forbidden] that is actively destructive (data loss / | ||
| /// bricking), not merely "don't auto-fire". Opcodes 142-144 have no named | ||
| /// meaning in either reference codebase — treat as permanently blocked, | ||
| /// unknown-but-dangerous. | ||
| static const Set<int> destructive = {25, 45, 142, 143, 144}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use Cmd symbols for the opcodes that already have names.
Several entries in forbidden map to existing Cmd constants: 10 → Cmd.setClock, 25 → Cmd.forceTrim, 29 → Cmd.rebootStrap, 32 → Cmd.powerCycleStrap, 77 → Cmd.setAdvertisingNameHarvard, 119 → Cmd.setDeviceConfigValue, 120 → Cmd.setFfValue, 123 → Cmd.selectWrist, 146 → Cmd.setClockMaverick. Raw decimals mixed with hex-defined opcodes make an editing mistake hard to see. Keep the unnamed values (99, 142-144) as literals with the existing comment.
Also add a short note for opcode 45 in destructive. The doc explains 142-144 but not 45, and 45 has no Cmd symbol.
♻️ Proposed refactor
static const Set<int> forbidden = {
- 10,
- 146,
- 25,
- 29,
- 32,
- 45,
- 77,
- 119,
- 120,
- 99,
- 123,
+ Cmd.setClock, // 10
+ Cmd.setClockMaverick, // 146
+ Cmd.forceTrim, // 25
+ Cmd.rebootStrap, // 29
+ Cmd.powerCycleStrap, // 32
+ 45, // unnamed in both reference codebases
+ Cmd.setAdvertisingNameHarvard, // 77
+ Cmd.setDeviceConfigValue, // 119
+ Cmd.setFfValue, // 120
+ 99, // unnamed in both reference codebases
+ Cmd.selectWrist, // 123
142,
143,
144,
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/constants.dart` around lines 131 - 152, Update the `forbidden` set to
reference the named `Cmd` constants for opcodes 10, 25, 29, 32, 77, 119, 120,
123, and 146, while retaining unnamed values 99 and 142–144 as numeric literals
with the existing comment. Add a brief documentation note identifying opcode 45
as an unnamed destructive opcode in the `destructive` declaration.
| // than the u32 grid) picked up an off-grid straddle word neither field | ||
| // actually occupies. Both fixed here without touching GET_CLOCK's own | ||
| // (deliberately more permissive) scan. | ||
| const int _maxPlausibleUnixForRange = 1900000000; // ~2030 — tighter than 2100 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_maxPlausibleUnixForRange expires in 2030.
The constant hard-codes 1900000000, which is 2030-03-17. After that date a real newest timestamp from a strap exceeds the bound. _plausibleUnixRange then drops it and reports a stale newest, or returns null when both fields are rejected. The failure is silent, because the function has no way to signal "the bound rejected a plausible value".
Derive the bound from the current time instead, so the scan stays tight without an expiry date.
🐛 Proposed fix
-const int _maxPlausibleUnixForRange = 1900000000; // ~2030 — tighter than 2100
+/// Upper bound for a GET_DATA_RANGE timestamp: a small margin past "now",
+/// which keeps the scan tight without a hard-coded expiry date. A strap
+/// cannot legitimately report data from the future beyond clock skew.
+int get _maxPlausibleUnixForRange =>
+ DateTime.now().millisecondsSinceEpoch ~/ 1000 + 86400 * 2;If a compile-time constant is required, keep the literal but add a test that fails before the cutoff date, so the expiry is not discovered in production.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const int _maxPlausibleUnixForRange = 1900000000; // ~2030 — tighter than 2100 | |
| /// Upper bound for a GET_DATA_RANGE timestamp: a small margin past "now", | |
| /// which keeps the scan tight without a hard-coded expiry date. A strap | |
| /// cannot legitimately report data from the future beyond clock skew. | |
| int get _maxPlausibleUnixForRange => | |
| DateTime.now().millisecondsSinceEpoch ~/ 1000 + 86400 * 2; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/control.dart` at line 636, Replace the fixed 1900000000 value in
_maxPlausibleUnixForRange with a runtime-derived upper bound based on the
current time, while retaining the intended tight plausibility window used by
_plausibleUnixRange. Ensure future real strap timestamps are accepted without
allowing an unbounded range; if a compile-time constant must remain, add
coverage that exposes the cutoff before it is reached.
Comment on lines
+762
to
+775
| ConsoleLogChunk? parseConsoleLog(Uint8List inner) { | ||
| if (inner.length < 13 || inner[0] != PacketType.consoleLogs) return null; | ||
| final text = inner.length > 13 | ||
| ? _consoleLogText(Uint8List.sublistView(inner, 13)) | ||
| : ''; | ||
| return ConsoleLogChunk( | ||
| recordIndex: u16(inner, 1), | ||
| unix: u32(inner, 4), | ||
| subsec: u16(inner, 8), | ||
| chunkLen: u16(inner, 10), | ||
| channel: inner[12], | ||
| text: text, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any real CONSOLE_LOGS (0x32) capture or documented layout in-repo.
rg -n -i -C4 'CONSOLE_LOG|0x32|chunk_len' --glob '!**/*.g.dart'Repository: OpenStrap/protocol
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | sed -n '1,120p'
printf '\ncontrol.dart outline around parseConsoleLog:\n'
if [ -f lib/src/control.dart ]; then
wc -l lib/src/control.dart
ast-grep outline lib/src/control.dart --match parseConsoleLog --view expanded || true
sed -n '720,810p' lib/src/control.dart | cat -n
fi
printf '\nRelevant symbols/usages:\n'
rg -n "ConsoleLogChunk|parseConsoleLog|_consoleLogText|PacketType\.consoleLogs|consoleLogs|u16\(|chunkLen|console" lib/src/control.dart || trueRepository: OpenStrap/protocol
Length of output: 6375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'console log tests:\n'
rg -n -C5 "ConsoleLogChunk|consoleLogs|CONSOLE_LOG|console log|chunk_len|chunkLen|subsec|channel" test lib/src/records.dart lib/src/gen5_records.dart || true
printf '\nframing padding behavior candidates:\n'
rg -n -C4 "/4|-pad|padding|pad|PacketType.consoleLogs|consoleLogs" lib/src test/ --glob '!**/*.g.dart' || trueRepository: OpenStrap/protocol
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'console log tests section:\n'
sed -n '423,475p' test/gen5_historical_test.dart | cat -n
printf '\nframing padding helpers:\n'
sed -n '36,58p' lib/src/framing.dart | cat -n
printf '\nParseFrame flow relevant to inner:\n'
sed -n '1,130p' lib/src/framing.dart | cat -n
printf '\nBehavior probe: padding + current decode vs length-gated decode:\n'
python3 - <<'PY'
def current(inner):
# Simulate _consoleLogText: trim trailing zeros, cap 2048, Latin-1-ish codepoints.
end = len(inner[13:])
while end > 0 and inner[13:][end - 1] == 0:
end -= 1
capped = min(end, 2048)
return bytes(inner[13:13+capped]).decode('latin-1')
def declared(inner):
if len(inner) < 13:
return ''
declared_len = (inner[10] | (inner[11] << 8))
available = len(inner) - 13
text_len = declared_len if declared_len < available else available
if text_len == 0:
return ''
return bytes(inner[13:13+text_len]).decode('latin-1')
cases = {
'exact_text': bytearray([0x32,0,1,0,0,0,0,0,0,0,3,0,1,97,98,99]),
'trailing_real_nul': bytearray([0x32,0,1,0,0,0,0,0,0,0,4,0,1,97,98,99,0]),
'oversized_inner_padding': bytearray([0x32,0,1,0,0,0,0,0,0,0,3,0,1,97,98,99,0,0,0]),
}
for name, inner in cases.items():
print(f"{name}: inner_len={len(inner)} chunk_len={(inner[10]|inner[11]<<8)} current={current(inner)!r} declared={declared(inner)!r}")
PYRepository: OpenStrap/protocol
Length of output: 10084
Use chunkLen as the consoleLogs text boundary.
chunkLen documents the text bytes beyond inner[13]. _consoleLogText trims a trailing NUL run, so a chunk ending in a real NUL currently loses that final character. Slice by u16(inner, 10) clamped to inner.length - 13, then apply the existing 2048-byte cap inside the slice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/control.dart` around lines 762 - 775, Update parseConsoleLog to bound
the text slice using chunkLen from u16(inner, 10), clamped to the available
bytes after offset 13, so trailing real NUL bytes are preserved. Apply the
existing 2048-byte cap within that bounded slice before passing it to
_consoleLogText, while keeping the current packet validation and metadata
parsing unchanged.
Comment on lines
+16
to
+23
| Uint8List hex(String s) { | ||
| final clean = s.replaceAll(' ', ''); | ||
| final out = Uint8List(clean.length ~/ 2); | ||
| for (int i = 0; i < out.length; i++) { | ||
| out[i] = int.parse(clean.substring(i * 2, i * 2 + 2), radix: 16); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the duplicated hex helper into a shared test utility.
test/gen5_test.dart lines 11-18 defines a byte-for-byte identical hex function. Both are public top-level functions in the test package. Move one copy to a shared file, for example test/hex_helper.dart, and import it in both suites. This prevents the two copies from diverging in whitespace or radix handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gen5_historical_test.dart` around lines 16 - 23, Extract the duplicated
top-level hex helper from gen5_historical_test.dart and gen5_test.dart into a
shared test utility such as hex_helper.dart, preserving its current whitespace
removal and radix-16 parsing behavior. Import the shared helper in both test
suites and remove their local definitions.
Comment on lines
+163
to
+192
| test( | ||
| 'record_index is a clean consecutive counter across real consecutive ' | ||
| 'frames (whoop-rs real_frames.json ppg_frames — the strongest ground ' | ||
| 'truth: three real captured frames one second apart give record_id ' | ||
| '48077,48078,48079; a u32 read of the same bytes would NOT be ' | ||
| 'consecutive since inner[5:7] is a distinct field)', () { | ||
| final hexes = [ | ||
| 'aa015000010035412f1a80cdbb7601e700556a33', | ||
| 'aa015000010035412f1a80cebb7601e800556a33', | ||
| 'aa015000010035412f1a80cfbb7601e900556a33', | ||
| ]; | ||
| final expectedIds = [48077, 48078, 48079]; | ||
| final expectedUnix = [1783955687, 1783955688, 1783955689]; | ||
| for (var i = 0; i < hexes.length; i++) { | ||
| // These fixtures are truncated (real-world capture excerpt, only the | ||
| // header + record_index/unix bytes) — long enough to exercise the | ||
| // shared header parse without needing the full 61-byte v26 payload. | ||
| final inner = hex(hexes[i]).sublist(8); // strip the 8-byte gen5 header | ||
| final hdr = Gen5HistoricalHeader.tryParse(inner); | ||
| expect(hdr, isNotNull); | ||
| expect(hdr!.version, 26); | ||
| // The shared header still reads the WRONG u32 value (inner[3:7]) — | ||
| // that's fine, only Gen5V26Decoder.decode() applies the v26-specific | ||
| // u16 correction. Confirm that distinction explicitly here. | ||
| expect(hdr.recordIndex, isNot(expectedIds[i])); | ||
| final u16RecordIndex = inner[3] | (inner[4] << 8); | ||
| expect(u16RecordIndex, expectedIds[i]); | ||
| expect(hdr.unix, expectedUnix[i]); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both offset tests restate production constants instead of reading them, so offset regressions cannot fail. The shared root cause is that gen5_records.dart keeps its block and field offsets private, so the tests mirror the literals and then assert those literals against other literals. A change to a production offset leaves both tests green.
test/gen5_historical_test.dart#L163-L192: right-pad the truncated fixtures tokGen5V26MinInnerLenand assertrecordIndexthroughparseGen5Historical, removing the inlineinner[3] | (inner[4] << 8)computation.test/gen5_historical_test.dart#L311-L324: assert that the test'sbodyStart + 5 * blockLenequals the exportedkGen5V20InnerLen, so the mirrored stride is anchored to the library.
Consider exporting the v20 and v21 layout offsets from lib/src/gen5_records.dart so both suites can assert them directly.
📍 Affects 1 file
test/gen5_historical_test.dart#L163-L192(this comment)test/gen5_historical_test.dart#L311-L324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gen5_historical_test.dart` around lines 163 - 192, In
test/gen5_historical_test.dart lines 163-192, right-pad each truncated fixture
to kGen5V26MinInnerLen, parse it with parseGen5Historical, and assert
recordIndex through the production parser instead of computing inner offsets
inline. In test/gen5_historical_test.dart lines 311-324, assert bodyStart + 5 *
blockLen equals the exported kGen5V20InnerLen. Consider exporting the v20/v21
layout offsets from gen5_records.dart so both test suites can validate
production constants directly.
Comment on lines
+454
to
+468
| test( | ||
| 'ConsoleLogReassembler joins contiguous record_index chunks and flushes on a gap', | ||
| () { | ||
| final r = ConsoleLogReassembler(); | ||
| expect(r.add(parseConsoleLog(buildConsoleLog(1, 1, 'hello '))!), | ||
| isFalse); // first chunk | ||
| expect(r.add(parseConsoleLog(buildConsoleLog(2, 1, 'world'))!), | ||
| isTrue); // contiguous | ||
| expect(r.flush(), 'hello world'); | ||
| // A gap (index jumps from 2 to 2 again, i.e. non-contiguous) starts a | ||
| // fresh run rather than splicing. | ||
| expect(r.add(parseConsoleLog(buildConsoleLog(2, 1, 'x'))!), isFalse); | ||
| expect(r.add(parseConsoleLog(buildConsoleLog(3, 1, 'y'))!), isTrue); | ||
| expect(r.flush(), 'xy'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for a gap without a preceding flush.
Every add call here is followed by a flush before the next discontinuity, so the test only exercises the path where the caller cooperates. ConsoleLogReassembler.add clears the buffer on a gap, so a caller that feeds chunks continuously and flushes only at the end loses the earlier run. That behavior is untested.
Add a case that feeds 1 -> 2 -> 4 with no intermediate flush and asserts what the caller can still retrieve. This test pins the contract discussed on lib/src/control.dart lines 789-806 and will need updating with the fix proposed there.
💚 Proposed test
test('a gap with no intervening flush does not silently drop the run', () {
final r = ConsoleLogReassembler();
r.add(parseConsoleLog(buildConsoleLog(1, 1, 'first '))!);
r.add(parseConsoleLog(buildConsoleLog(2, 1, 'line'))!);
// index jumps 2 -> 4: the caller never called flush().
r.add(parseConsoleLog(buildConsoleLog(4, 1, 'second'))!);
// Pin the current behavior explicitly so a contract change is visible.
expect(r.flush(), 'second');
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gen5_historical_test.dart` around lines 454 - 468, Add a separate test
near the existing ConsoleLogReassembler coverage that feeds record indices 1, 2,
and 4 without calling flush between additions, then assert the final flush
result according to the proposed contract. Keep the existing contiguous and
explicitly flushed gap test unchanged, and use ConsoleLogReassembler.add and
flush to verify the unflushed earlier run behavior.
Comment on lines
+177
to
+180
| test('rejects an over-length name / non-ASCII / multi-char value', () { | ||
| expect(() => cmdSetConfigGen5(1, 'x' * 32, '2'), throwsArgumentError); | ||
| expect(() => cmdSetConfigGen5(1, 'ok', '22'), throwsArgumentError); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name claims non-ASCII coverage that is absent.
The name says "rejects an over-length name / non-ASCII / multi-char value". The body covers over-length (Line 178) and multi-char value (Line 179). It never passes a non-ASCII name or a non-ASCII value. cmdSetConfigGen5 has two distinct non-ASCII branches: name.codeUnits.any((c) => c > 0x7f) and value.codeUnitAt(0) > 0x7f. Both are untested.
💚 Proposed additions
test('rejects an over-length name / non-ASCII / multi-char value', () {
expect(() => cmdSetConfigGen5(1, 'x' * 32, '2'), throwsArgumentError);
expect(() => cmdSetConfigGen5(1, 'ok', '22'), throwsArgumentError);
+ expect(() => cmdSetConfigGen5(1, 'énable', '2'), throwsArgumentError);
+ expect(() => cmdSetConfigGen5(1, 'ok', 'é'), throwsArgumentError);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('rejects an over-length name / non-ASCII / multi-char value', () { | |
| expect(() => cmdSetConfigGen5(1, 'x' * 32, '2'), throwsArgumentError); | |
| expect(() => cmdSetConfigGen5(1, 'ok', '22'), throwsArgumentError); | |
| }); | |
| test('rejects an over-length name / non-ASCII / multi-char value', () { | |
| expect(() => cmdSetConfigGen5(1, 'x' * 32, '2'), throwsArgumentError); | |
| expect(() => cmdSetConfigGen5(1, 'ok', '22'), throwsArgumentError); | |
| expect(() => cmdSetConfigGen5(1, 'énable', '2'), throwsArgumentError); | |
| expect(() => cmdSetConfigGen5(1, 'ok', 'é'), throwsArgumentError); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gen5_test.dart` around lines 177 - 180, Update the test named “rejects
an over-length name / non-ASCII / multi-char value” to add separate
invalid-input assertions covering a non-ASCII name and a non-ASCII value,
exercising both non-ASCII validation branches in cmdSetConfigGen5 while
preserving the existing over-length and multi-character cases.
Comment on lines
+213
to
+227
| test( | ||
| 'cmdSetClockGen5 / cmdGetClockGen5 use the gen5-exclusive opcode values', | ||
| () { | ||
| final setFrame = cmdSetClockGen5(1, now: DateTime.utc(2026, 1, 1)); | ||
| final setParsed = parseFrame(setFrame, profile: BandProfile.gen5)!; | ||
| expect(setParsed.valid, isTrue); | ||
| expect(setParsed.inner[2], Cmd.setClockMaverick); | ||
| expect(Cmd.setClockMaverick, 146); | ||
|
|
||
| final getFrame = cmdGetClockGen5(1); | ||
| final getParsed = parseFrame(getFrame, profile: BandProfile.gen5)!; | ||
| expect(getParsed.valid, isTrue); | ||
| expect(getParsed.inner[2], Cmd.getClockGen5); | ||
| expect(Cmd.getClockGen5, 147); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the encoded clock payload, not only the opcode.
The test passes a fixed DateTime.utc(2026, 1, 1) but checks only setParsed.inner[2]. The part of cmdSetClockGen5 that can actually be wrong is the payload arithmetic: the u32 epoch in little-endian at bytes 0-3, and the 32768-tick subsecond at bytes 4-5. Neither is asserted. A sign error, a byte-order mistake, or a wrong subsecond divisor all pass this test.
Because now is fixed, the expected bytes are deterministic.
💚 Proposed addition
final setFrame = cmdSetClockGen5(1, now: DateTime.utc(2026, 1, 1));
final setParsed = parseFrame(setFrame, profile: BandProfile.gen5)!;
expect(setParsed.valid, isTrue);
expect(setParsed.inner[2], Cmd.setClockMaverick);
expect(Cmd.setClockMaverick, 146);
+ // Payload = [u32 epoch LE][u16 subsec `@32768/s`][u16 pad]. 2026-01-01Z
+ // is exactly on the second, so subsec must be 0.
+ final epoch =
+ DateTime.utc(2026, 1, 1).millisecondsSinceEpoch ~/ 1000;
+ final payload = ByteData.sublistView(setParsed.inner, 3, 11);
+ expect(payload.getUint32(0, Endian.little), epoch);
+ expect(payload.getUint16(4, Endian.little), 0);
+ expect(payload.getUint16(6, Endian.little), 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test( | |
| 'cmdSetClockGen5 / cmdGetClockGen5 use the gen5-exclusive opcode values', | |
| () { | |
| final setFrame = cmdSetClockGen5(1, now: DateTime.utc(2026, 1, 1)); | |
| final setParsed = parseFrame(setFrame, profile: BandProfile.gen5)!; | |
| expect(setParsed.valid, isTrue); | |
| expect(setParsed.inner[2], Cmd.setClockMaverick); | |
| expect(Cmd.setClockMaverick, 146); | |
| final getFrame = cmdGetClockGen5(1); | |
| final getParsed = parseFrame(getFrame, profile: BandProfile.gen5)!; | |
| expect(getParsed.valid, isTrue); | |
| expect(getParsed.inner[2], Cmd.getClockGen5); | |
| expect(Cmd.getClockGen5, 147); | |
| }); | |
| test( | |
| 'cmdSetClockGen5 / cmdGetClockGen5 use the gen5-exclusive opcode values', | |
| () { | |
| final setFrame = cmdSetClockGen5(1, now: DateTime.utc(2026, 1, 1)); | |
| final setParsed = parseFrame(setFrame, profile: BandProfile.gen5)!; | |
| expect(setParsed.valid, isTrue); | |
| expect(setParsed.inner[2], Cmd.setClockMaverick); | |
| expect(Cmd.setClockMaverick, 146); | |
| // Payload = [u32 epoch LE][u16 subsec `@32768/s`][u16 pad]. 2026-01-01Z | |
| // is exactly on the second, so subsec must be 0. | |
| final epoch = | |
| DateTime.utc(2026, 1, 1).millisecondsSinceEpoch ~/ 1000; | |
| final payload = ByteData.sublistView(setParsed.inner, 3, 11); | |
| expect(payload.getUint32(0, Endian.little), epoch); | |
| expect(payload.getUint16(4, Endian.little), 0); | |
| expect(payload.getUint16(6, Endian.little), 0); | |
| final getFrame = cmdGetClockGen5(1); | |
| final getParsed = parseFrame(getFrame, profile: BandProfile.gen5)!; | |
| expect(getParsed.valid, isTrue); | |
| expect(getParsed.inner[2], Cmd.getClockGen5); | |
| expect(Cmd.getClockGen5, 147); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gen5_test.dart` around lines 213 - 227, Extend the test for
cmdSetClockGen5 with deterministic assertions for the encoded timestamp payload:
verify the little-endian u32 epoch bytes at setParsed.inner[0..3] and the
32768-tick subsecond bytes at setParsed.inner[4..5] for the fixed UTC date. Keep
the existing opcode assertions and cmdGetClockGen5 coverage unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds WHOOP 5 (gen5 / "fd4b") support to the pure-Dart protocol library. The key finding: WHOOP 5 is WHOOP 4 in a different envelope — only the outer frame header changes, so this is a small, well-bounded
BandProfileabstraction rather than a second protocol.[AA][u16 size][crc8][AA][01][u16 size][00][01][crc16-modbus]6108000x-…fd4b000x-…Changes
crc.dart—crc16Modbus(verified0x71E6on the gen5 hello header).band.dart(new) —DeviceType,BandProfile,GattProfile.framing.dart—buildFrame/parseFrame/FrameReassemblertake aBandProfile, defaulting to gen4 → WHOOP 4 byte-identical.commands.dart—gen5ClientHello()(reproduces the canonical hello byte-for-byte), gen5 empty-payload offload helpers; profile threaded through command + history-result builders.records.dart—parseGen5Record: thin K24 (HR@17 + timing). Motion uses the existing R10 offsets; SpO2/temp/RR honest-null pending validated offsets (never fabricated).Testing
dart test→ 87 pass (16 new gen5 cases + all 2934 TS-parity cases). No WHOOP 4 regression. The gen5 decode path was additionally validated end-to-end against a real WHOOP 5.0 owned capture (HR/counter/timestamp decode correctly; CRC16+CRC32 both check out).Notes
🤖 Generated with Claude Code
Summary by CodeRabbit