feat(edge): detect + speak WHOOP 5 (gen5) alongside WHOOP 4 - #97
feat(edge): detect + speak WHOOP 5 (gen5) alongside WHOOP 4#97abdulsaheel wants to merge 28 commits into
Conversation
Scan filters both service UUIDs (gen4 6108xxxx / gen5 fd4bxxxx); at discovery the session pins its generation and rebuilds the frame reassemblers with the matching header shape. The BandProfile is threaded through the frame path, the command builder, and the history-result ACK (the safe-trim token echo). Adds the gen5 handshake branch (client-hello + empty-payload offload) and routes gen5 records through parseGen5Record; unknown/motion kinds fall through to raw_archive as before. The WHOOP 4 path is unchanged. The gen5 connect/handshake path is not yet validated on physical hardware (marked in-code) — pending a WHOOP 5 band.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBLE scanning, service discovery, protocol handling, initialization, historical decoding, diagnostics, and iOS accessory selection now support WHOOP gen4 and gen5 bands through generation-specific profiles, commands, UUIDs, and transfer flows. ChangesWHOOP generation support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BLEScanner
participant ServiceDiscovery
participant BLESession
participant WHOOPBand
participant HistoricalDecoder
BLEScanner->>ServiceDiscovery: discover gen4 or gen5 WHOOP services
ServiceDiscovery->>BLESession: applyBand(BandProfile)
BLESession->>BLESession: rebuild frame reassemblers
BLESession->>WHOOPBand: send generation-specific initialization
BLESession->>WHOOPBand: request generation-specific historical data
WHOOPBand-->>BLESession: return framed historical records
BLESession->>HistoricalDecoder: decode records using pinned profile
HistoricalDecoder-->>BLESession: return supported Sample records
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ble/ble_engine.dart`:
- Around line 2380-2394: Centralize the generation-specific GET_DATA_RANGE and
SEND_HISTORICAL_DATA writes in a helper, using empty gen5 payloads and the
existing gen4 payloads as appropriate. Update the gen5 initial flow near
gen5ClientHello() and _startHistoricalRefresh() to call this helper so periodic,
manual, and retry refreshes preserve the gen5 command format.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd244272-7c1d-4b50-b23c-9dbae531602f
📒 Files selected for processing (1)
lib/ble/ble_engine.dart
…paths CodeRabbit review: the initial handshake used gen5 empty-payload GET_DATA_RANGE/SEND_HISTORICAL_DATA, but the periodic backfill, manual refresh, and retry paths still sent the gen4 [0x00] payload (only the frame envelope was band-correct). Extract _sendGetDataRange / _sendHistoricalData helpers that pick the payload by generation (gen4 [0x00], gen5 empty) and route the init, refresh, backfill, and retry paths through them, so the gen5 offload format is identical everywhere.
|
Addressed in 67fa1cb — extracted @coderabbitai review |
|
✅ Action performedReview finished.
|
…t gen5 branch The BLE engine could speak gen5, but a WHOOP 5 band could not be paired on iOS: AccessorySetupKit only advertised the gen4 6108 service, so a fd4b band never appeared in the picker (and with no ASK provisioning the restore central is never created → no connection at all). - AccessorySetup.swift: offer one ASPickerDisplayItem per generation (gen4 6108 + gen5 fd4b) so either band can be provisioned; the provisioned CoreBluetooth identifier is generation-agnostic. - Info.plist: add the gen5 service to NSAccessorySetupBluetoothServices (required for the descriptor criterion). - pubspec.yaml: point openstrap_protocol at feat/multiband-whoop5 for the experimental build (revert to main once protocol#16 merges). Android needs no change: CDM associates by MAC (generation-agnostic) and the Flutter scan is already fd4b-aware. iOS restore reconnects by peripheral identifier, also generation-agnostic. Still hardware-unvalidated end-to-end — pending a physical WHOOP 5 band.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pubspec.yaml (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBranch ref makes builds non-reproducible; pin a commit for release.
Using
ref: feat/multiband-whoop5means a force-push to that branch silently changes the resolved dependency, making builds non-reproducible. This is acceptable for experimental development, but before any release or merge to a production branch, pin to a specific commit SHA instead of a branch ref.🔒️ Suggested pinning for reproducibility
openstrap_protocol: git: url: https://github.com/OpenStrap/protocol.git - # EXPERIMENTAL: WHOOP 5 (gen5) multi-band support. Point back to `main` - # once OpenStrap/protocol#16 merges. - ref: feat/multiband-whoop5 + # EXPERIMENTAL: WHOOP 5 (gen5) multi-band support. Point back to `main` + # once OpenStrap/protocol#16 merges. + # Pinned to a specific commit for reproducibility; update intentionally. + ref: <commit-sha-of-branch-tip>🤖 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 `@pubspec.yaml` around lines 27 - 29, Replace the branch-based ref for the OpenStrap dependency in the WHOOP 5 experimental configuration with the specific reviewed commit SHA before release or production-branch merge. Keep the dependency source and existing experimental intent unchanged, and remove the non-reproducible feat/multiband-whoop5 branch reference.
🤖 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 `@pubspec.yaml`:
- Around line 27-29: Replace the branch-based ref for the OpenStrap dependency
in the WHOOP 5 experimental configuration with the specific reviewed commit SHA
before release or production-branch merge. Keep the dependency source and
existing experimental intent unchanged, and remove the non-reproducible
feat/multiband-whoop5 branch reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bd7319d-ad6c-41b3-85ec-f7333f0d0e93
📒 Files selected for processing (3)
ios/Runner/AccessorySetup.swiftios/Runner/Info.plistpubspec.yaml
Pin the committed lock's openstrap_protocol dependency to the gen5 branch commit (687aa46) so CI/release resolves the exact experimental protocol revision. Only the protocol ref/resolved-ref changed; analytics stays on main. (Locally the gitignored pubspec_overrides.yaml still redirects to ../protocol for dev; the committed lock is what release resolution uses.)
|
I have a whoop 5 that I am willing to use to validate against if that would help? |
# Conflicts: # lib/ble/ble_engine.dart # pubspec.lock # pubspec.yaml
PR Reviewer Guide 🔍(Review updated until commit 82b094c)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
…ted engine Extends BleEngine (not a parallel copy) so a gen5 link gets a real, working lifecycle end to end instead of stopping at "connected": - SET_CLOCK/GET_CLOCK now use gen5's own opcodes (SET_CLOCK_MAVERICK/ GET_CLOCK_GEN5) instead of silently sending gen4's — the bug that would have left a gen5 strap's RTC forever unlatched and refusing history. - Historical-record ingestion now calls protocol's real parseGen5Historical (v18/v20/v21/v26) via a new sampleFromGen5Historical mapper, replacing the old parseGen5Record call that targeted gen4's version numbers and would have silently archived every real gen5 record. - Added the opt-in R22 deep-buffer enable sequence (default OFF, gated by a constructor toggle) and a gen5 Maverick haptic buzz path. - _send now also blocks OpcodeSafety.destructive band-agnostically, alongside the existing gen4 dangerousCmds list. - decodeFrame is now called with the session's BandProfile so gen5's direct-percent battery / GET_HELLO shape decode correctly; added a small edge-side augment for GET_CLOCK_GEN5's clock_epoch (protocol doesn't populate it yet) and debug-only logging for gen5 console/hello frames. - DeviceState.generation + a band_generation ledger field surface which WHOOP generation a session/batch came from, with no schema migration (rides the existing sync_ledger meta_json blob). - New gen5_sample_mapping_test.dart covers the v18->Sample mapping against a real byte-verified capture, plus the deep-buffer/null fall-through. gen4 behavior is unchanged (every branch above is band-gated); full suite green (1059 tests, 2 pre-existing skips). Real-hardware validation of the handshake and R22 sequence is still outstanding — see inline doc comments.
The previous pin (7f1a2db) predates protocol's real v18/v20/v21/v26 decoders, gen5 clock/haptics/SET_CONFIG opcodes, R22 sequence builder, OpcodeSafety gate, and CONSOLE_LOGS decoder — all of which the previous edge commit's BleEngine changes call directly. Verified with a clean `flutter pub get` (no local path override) against this SHA: analyze and the full test suite (1059 tests) both green.
|
Persistent review updated to latest commit 9d13f1b |
|
Persistent review updated to latest commit aa18782 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ble/ble_engine.dart (2)
970-992: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
GattProfileprefixes instead of hardcoded magic strings for scan matching.The scan callback hardcodes
'61080001'and'fd4b0001'as service-UUID prefixes. Service discovery at Line 1173 and Line 1178 usesGattProfile.gen4.servicePrefixandGattProfile.gen5.servicePrefixfor the same purpose. Two sources of truth exist for identical values. If either prefix constant changes, the scan's early-stop optimization silently desyncs from the actual discovery match, and the scan runs to its full timeout instead of stopping early.Reference the same constants in both places.
♻️ Proposed fix
if (found == null && (name.contains('whoop') || advNames.any((s) => - s.startsWith('61080001') || s.startsWith('fd4b0001')))) { + s.startsWith(GattProfile.gen4.servicePrefix.toLowerCase()) || + s.startsWith(GattProfile.gen5.servicePrefix.toLowerCase())))) {🤖 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/ble/ble_engine.dart` around lines 970 - 992, Update the scan callback in the BLE discovery flow to replace the hardcoded “61080001” and “fd4b0001” checks with GattProfile.gen4.servicePrefix and GattProfile.gen5.servicePrefix, matching the constants used by service discovery. Keep the existing case-normalized prefix matching and early-stop behavior unchanged.
1426-1426: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd regression coverage for the gen5 refresh offload payload.
lib/ble/ble_engine.dartsends[0x00]for gen4 and[]for gen5, but the test suite only exercises gen5 historical mapping throughtest/gen5_sample_mapping_test.dart. Add a gen5-pinned refresh/backfill/retry regression that assertsGET_DATA_RANGE/SEND_HISTORICAL_DATAare sent with an empty payload, not only the initialsendInitpath.🤖 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/ble/ble_engine.dart` at line 1426, Add regression coverage in the gen5 refresh/backfill/retry test flow around _sendGetDataRange, pinning the device generation to gen5 and asserting GET_DATA_RANGE and SEND_HISTORICAL_DATA use empty payloads. Exercise the refresh path beyond the initial sendInit sequence, including backfill and retry behavior, while preserving the existing gen4 payload expectations.Source: Coding guidelines
🤖 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 `@pubspec.yaml`:
- Around line 38-44: Update the stale protocol-generation comments near the
WHOOP dependency to describe the active experimental WHOOP 5 multi-band
configuration and pinned protocol commit, removing any WHOOP-4-only or ref: main
claims. Keep the dependency pinned to the existing full commit SHA and do not
introduce a branch reference.
- Around line 38-44: Before release, validate the pinned experimental protocol
from the WHOOP 5 flow through gen5 connection, handshake, and R22 behavior on
physical hardware, including GATT discovery and device-specific framing; update
the release readiness or dependency decision around the pubspec ref only after
those checks pass.
- Around line 38-44: The protocol dependency pin in the pubspec must reference a
commit that actually contains the claimed gen5 framing and CRC implementation
changes. Update the ref under the WHOOP 5 multi-band dependency comments to the
correct SHA containing the crc8 length-field validation, odd-length hex
rejection, and profile-aware header-CRC fixes; preserve the pinned-commit
approach.
---
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 970-992: Update the scan callback in the BLE discovery flow to
replace the hardcoded “61080001” and “fd4b0001” checks with
GattProfile.gen4.servicePrefix and GattProfile.gen5.servicePrefix, matching the
constants used by service discovery. Keep the existing case-normalized prefix
matching and early-stop behavior unchanged.
- Line 1426: Add regression coverage in the gen5 refresh/backfill/retry test
flow around _sendGetDataRange, pinning the device generation to gen5 and
asserting GET_DATA_RANGE and SEND_HISTORICAL_DATA use empty payloads. Exercise
the refresh path beyond the initial sendInit sequence, including backfill and
retry behavior, while preserving the existing gen4 payload expectations.
🪄 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: 118d5963-8206-46f1-8518-4e8f4e54f1af
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
ios/Runner/Info.plistlib/ble/ble_engine.dartlib/data/models.dartpubspec.yamltest/gen5_sample_mapping_test.dart
PR Code Suggestions ✨ |
1 similar comment
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit e21bbaf |
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit 260cd0c |
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit 63a87ed |
|
Persistent review updated to latest commit 82b094c |
PR Code Suggestions ✨ |
PR Code Suggestions ✨ |
|
I was able to connect with my whoop 5. so far i can see heart rate data live. I will keep it connected to see recovery/strain and other data points. |
Great !! Thanks, lets continue this discussion here: #178 |
Protocol's strict gravity gate rejected every v18 record in a hardware export (fw 50.40.1.0) while v20/v26 deep buffers dominated offload. Recover HR/RR via a hardware fallback with alternate unix offset and honest absent accel when gravity fails validation.
Drop the misaligned unix@6 fallback that could invent timestamps, and prepend revision1 on gen5 SET_CLOCK/GET_CLOCK only (WHOOP 4 path unchanged). Hardware showed Invalid revision when body[0] was the epoch low byte; a later connect with the 9-byte form correlated with drift=0.
Gen5 v18/lenient samples lack gen4 optics so R24 decode fails and decoded_onehz stayed empty — fall back to the BLE-preferred Sample when tsEpoch > 0, excluding gen4 R10-lite hr-only records. Align gen5V18UnixFromInner with RecordGate's isPlausibleUnix(wallNow) so implausible timestamps archive instead of silently dropping after decode. Restore git-sourced analytics lock (cbbe06a).
Parse raw hex and gate Gen4 R10-lite records before accepting a complete preferred sample into decoded_onehz. Exercise lenient v18 recovery through decodeGen5HistoricalSample with HR+RR in hardware tests.
Cover strict dynamic-accel rejection with valid gravity through the production decodeGen5HistoricalSample path.
Strict parseGen5Historical accepts high dynamic accel when gravity is valid; lenient fallback is exercised via the gravity-gate case only.
Prove _decodeOneHzSample rejects Gen4 R10-lite bytes before the hasDecodedOneHz early return, not only when preferred is HR-only.
R10-lite reject must parse hex first, but hexToBytes failure should fall through to preferred/hasDecodedOneHz — not return null. Restores decoded_onehz for fixture inserts with placeholder hex. Also fix clamp→int in the gen5 persistence helper.
WHOOP 5 rejects rich SET_ALARM at slot 0 (`arm info is invalid, 0xb`); HCI capture of the official app uses index 1 and emits event 56. Gen5 test buzz uses Maverick 0x13 (RUN_ALARM is a no-op on-wrist). Gen4 path unchanged. Profile sheet now rebuilds on confirmation flags.
int.clamp returns num; toInt() keeps the List<int> payload type-safe.
WHOOP 5 rejects bare toggleImuMode (Invalid rev 0); arm with [revision1, on] and skip unhandled R10/R11 realtime. Live accel arrives as 0x2B rec 0x15 (100 Hz planar), not gen4 0x33 — decode that layout so calibration counts. Gen4 0x33/R10 still use frameAccel unchanged.
P0. `sampleFromGen5V18Lenient` correctly abstains on a gravity vector that
fails the magnitude gate (ax/ay/az stay null) while keeping HR/RR -- but
`decoded_onehz.ax/ay/az` are REAL NOT NULL, so `_queueDecodedOneHz` writes
`decoded.ax ?? 0` and the substrate loader reads `?? 0` back. "We did not
measure this" becomes "the wrist was at exactly (0,0,0)".
That is not an inert default. `zAngle(0,0,0)` is exactly 0.0 in Dart --
atan2(0,0) is 0.0, not NaN -- so a run of absent seconds has a perfectly
CONSTANT z-angle, which is the van Hees immobility criterion satisfied
maximally. Measured against the pinned analytics: 8 h of (0,0,0) scores 28501
immobile seconds and `vanHeesSleepWindow.present == true`, i.e. a fabricated
~7.9 h night, fully staged, out of data that does not exist. This PR's own
commit message notes the strict gate rejected EVERY v18 record on fw
50.40.1.0, so for that firmware it is the ordinary case, not a corner.
Exact (0,0,0) is an unambiguous ABSENT marker rather than a reading: gravity
always has magnitude ~1 g and every decoder that emits a vector gates on
`magSq >= 0.25`. So `Substrate.accelPresentAt` / `accelPresentFraction` can
recover the distinction the NOT NULL column erased, with no schema change --
and it heals rows already persisted.
TWO OTHER SENTINELS WERE TRIED AND REJECTED, both measured rather than assumed:
* NaN fails OPEN. The rule asks "did the angle change by >= threshold", and
every comparison against NaN is false, so it never trips -- NaN scores the
SAME 28501 immobile seconds as zeros. Pinned in a test so nobody "fixes"
this that way later.
* Omitting the seconds does not work either: `immobilityMask` is a pure
index-wise angle rule with no timestamp/gap awareness (unlike `nap.dart`'s
`stillAt`, which does check `absAt(k) - absAt(k-1) == 1`), so it just joins
across the hole.
Since analytics has no validity input to be told any of this, the honest move
at this layer is not to let absent accel ANCHOR a window: below
`kMinAccelCoverageForVanHees` the accel-led path is skipped entirely and the
day falls through to the EXISTING HR-led fallback, which is already the
low-confidence degraded mode for exactly this situation. Half rather than
something tiny, because van Hees picks the LONGEST immobile block and absent
seconds are maximally immobile, so a mostly-absent window would reliably hand
the answer to the missing data.
The complete fix belongs upstream -- `immobilityMask` should take an optional
validity mask and mark invalid seconds via the `immobileUnknown` machinery it
already has. Noted for analytics; this keeps the P0 off the field meanwhile.
P1, same commit: lenient records bypassed `raw_archive`. A partial decode is
not a full decode -- HR/RR are kept but the gravity bytes the strict gate
rejected were discarded the moment we ACK'd the trim, with `raw_records` gone
and `decoded_onehz` having nowhere to put a null accel. They are now archived
as well as sampled, in the same safe-trim transaction, so a future decoder can
recover what this one could not. Extracted `_archiveHistoricalFrame` so both
the undecodable and partial paths share one implementation.
7 tests added; the 6 pre-existing failures in notification_dedupe_test also
reproduce on origin/main unmodified and are unrelated.
fix(gen5): SET_CLOCK revision byte + honest v18 decode
fix(gen5): WHOOP 5 wake alarm (slot index 1 + Maverick test buzz)
`'33' + ('00' * 80)` trips prefer_interpolation_to_compose_strings. That is only
an INFO, but CI runs a bare `flutter analyze`, which exits 1 on any issue
including infos -- so this alone would have turned the job red.
Note for the record: Copilot flagged this same line claiming `'00' * 80` "relies
on a non-standard String operator * and will fail to compile". That part is
wrong -- `String.operator*` is dart:core, and `'00' * 4` evaluates to
'00000000'. The line compiles fine; it is the lint, not the language.
fix(gen5): arm and decode Maverick live IMU for step calibration
Summary
Teaches the BLE engine to detect and talk to WHOOP 5 (gen5 / "fd4b") bands as well as WHOOP 4. Pairs with OpenStrap/protocol#16 (the
BandProfileabstraction). Analytics needs no changes — WHOOP 5 just feeds the samedecoded_onehz/decoded_rrtables.Changes (all in
lib/ble/ble_engine.dart)6108xxxx/ gen5fd4bxxxx)._Session.applyBand) and rebuilds the frame reassemblers with the matching header shape; characteristics resolved via the generation's GATT prefixes.BandProfilethreaded through the frame path, the command builder (_send), and the history-result ACK (safe-trim token echo).sendInit: client-hello (0x91) + empty-payloadGET_DATA_RANGE/SEND_HISTORICAL_DATA.parseGen5Record; unknown/motion kinds fall through toraw_archiveas before.Testing
flutter test --concurrency=1→ 483 pass (no regression).flutter analyze lib/ble/→ clean.Invariants preserved
decoded_*schema unchanged.The gen5 connect/handshake path is not yet validated on physical hardware (no WHOOP 5 band available) — clearly marked in-code. Decode is validated against real owned captures + synthetic vectors. Requires an on-device pairing pass (bond → handshake → offload → ACK → trim → decode → day derive) before shipping to gen5 users.
🤖 Generated with Claude Code
Summary by CodeRabbit