feat(edge): detect + speak WHOOP 5 (gen5) alongside WHOOP 4 - #97
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:
📝 WalkthroughWalkthroughWHOOP Gen5 support now covers BLE discovery, generation-specific protocol flows, historical decoding, live IMU data, alarms, analytics, persistence, iOS accessory setup, and device presentation. Gen4 behavior remains available. 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
participant Database
BLEScanner->>ServiceDiscovery: discover Gen4 or Gen5 WHOOP services
ServiceDiscovery->>BLESession: apply generation profile
BLESession->>WHOOPBand: initialize and request historical data
WHOOPBand-->>BLESession: return framed records
BLESession->>HistoricalDecoder: decode records
HistoricalDecoder-->>BLESession: return Sample or archive record
BLESession->>Database: persist supported samples and metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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.
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 5dae5e5)Here are some key observations to aid the review process:
|
…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 ✨Explore these optional code suggestions:
|
1 similar comment
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
Persistent review updated to latest commit e21bbaf |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
Persistent review updated to latest commit 260cd0c |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
Persistent review updated to latest commit 63a87ed |
fix(gen5): arm and decode Maverick live IMU for step calibration
|
@coderabbitai full review Requesting a full pass rather than incremental: this branch has accumulated three merged PRs since your last review on 08-01, and merging PRs into a PR's head branch doesn't reliably fire the What landed since you last looked:
Two notes on your existing findings, so the pass doesn't re-litigate them:
Areas I'd especially value a look at, given this is the point where gen5 becomes user-facing:
|
|
I will verify the generation-specific offload routing, the gen5 derive and ingest paths, absent-data handling, and the alarm command and confirmation flow. I will also review the ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ble/ble_engine.dart (1)
519-547: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWire
gen5DeepBuffersEnabledinto the BLE engine construction paths.
sendInit()only sends the Gen5 R22 sequence whengen5DeepBuffersEnabled()returnstrue, but the app, background sync, and testBleEngineconstructions use the default callback and never override it. Add a persisted settings read to a production caller, such asAppState, so Gen5 deep-buffer draining can be enabled through the same session-trigger coverage required by the guidance.🤖 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 519 - 547, Wire the existing gen5DeepBuffersEnabled callback into the production BleEngine construction path, preferably in AppState, by reading the persisted user setting and returning its current enabled state. Ensure the callback is supplied wherever the app’s session-trigger flow creates BleEngine so sendInit() can enable the Gen5 R22 sequence; leave unrelated background and test construction paths unchanged unless required by that flow.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 `@lib/ui/profile/profile_screen.dart`:
- Around line 925-933: Replace the Row containing the status and generation
StatusChip widgets with a Wrap so the chips can flow onto multiple lines within
narrow layouts. Preserve the existing horizontal spacing using the Wrap spacing
property and keep the conditional generation chip behavior unchanged.
- Around line 1360-1373: Select the serial value needed by the profile row in
the context.select tuple instead of selecting the mutable AppState.device
object; use device.serial, or the existing fallback device.serial ??
paired?.serial. Update the corresponding tuple type and callback while leaving
the other selected alarm and connection values unchanged.
In `@test/gen5_decoded_onehz_persistence_test.dart`:
- Around line 159-230: The R10-lite regression test must also verify legacy
retention by querying samples for the same timestamp and asserting the row
remains available. Add a separate test covering LocalDb._decodeOneHzSample with
invalid RawRecord.hex and a timestamp-valid preferred Sample, asserting the
preferred sample is persisted successfully.
In `@test/substrate_accel_absence_test.dart`:
- Around line 149-163: Extend the test around the van Hees coverage-floor case
to exercise the automatic sleep-detection path in the relevant substrate
computation API, using an all-zero, no-override night and asserting it does not
produce an accel-led auto sleep result. Add a boundary case with exactly 50%
accelerometer coverage and verify the expected gate behavior, while retaining
the existing fraction assertions.
---
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 519-547: Wire the existing gen5DeepBuffersEnabled callback into
the production BleEngine construction path, preferably in AppState, by reading
the persisted user setting and returning its current enabled state. Ensure the
callback is supplied wherever the app’s session-trigger flow creates BleEngine
so sendInit() can enable the Gen5 R22 sequence; leave unrelated background and
test construction paths unchanged unless required by that flow.
🪄 Autofix
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: b7c5e8fb-fdb1-4a05-b93d-019717f11a67
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
ios/Runner.xcodeproj/project.pbxprojios/Runner/AccessorySetup.swiftios/Runner/Info.plistlib/ble/ble_engine.dartlib/ble/ble_state.dartlib/ble/gen5_live_imu.dartlib/compute/substrate.dartlib/data/db.dartlib/data/models.dartlib/state/app_state.dartlib/ui/profile/profile_screen.dartpubspec.yamltest/alarm_test.darttest/gen5_decoded_onehz_persistence_test.darttest/gen5_imu_mode_payload_test.darttest/gen5_live_imu_test.darttest/gen5_sample_mapping_test.darttest/gen5_v18_hardware_lenient_test.darttest/substrate_accel_absence_test.dart
Four of the five findings were real. Each verified against the code before
changing anything.
1. VACUOUS GUARD TEST (Major) -- substrate_accel_absence_test only exercised
`accelPresentFraction`, never the gate it feeds at substrate.dart:574-600.
The coverage floor could have been deleted and every assertion still passed
-- the same vacuity I flagged in other people's tests earlier in this stack,
in my own. Now drives `calendarDays` (the real entry point) and asserts on
`PhysioDay.sleepSource`.
Two things worth recording about building it. The first positive control
FAILED: a "near-still" night was not detected as sleep at all, which would
have made the negative test pass for the wrong reason. Fixed by giving the
fixture a genuine shape -- constant gravity through the night, a 10 deg/s
ramp while awake (a ramp, not an alternation: the mask smooths z-angle with
a 5-second rolling MEDIAN, which erases a 1 Hz square wave entirely).
Second, mutation testing showed the headline all-zero case was STILL not
discriminating -- with the gate removed it passed anyway, because a record
immobile end to end is rejected downstream regardless. The discriminating
shape is the realistic one: evening has gravity, the night's records do not.
Coverage lands below the floor, and those zeros would otherwise form a clean
multi-hour "immobile" block to anchor on. Removing the gate now fails 2
tests; both are pinned, with the non-discriminating one kept and labelled.
2. STALE SERIAL (Minor, real) -- `context.select` selected `a.device`, but
`select` compares with `==`, `DeviceState` declares no `==`/`hashCode`, and
`BleEngine` mutates `state.serial` IN PLACE. The selector returned the same
reference before and after, so no rebuild fired and the row could sit on a
stale serial indefinitely. Now selects the serial STRING the row renders,
which is the only value this sheet reads from `device`/`paired`.
3. CHIP OVERFLOW (Minor) -- two intrinsically-sized, non-flex chips in a Row,
the second carrying "WHOOP 5 (experimental)". At large text scales or on a
narrow device their combined width exceeds the Expanded column. Wrap
degrades to a second line instead.
4. TEST COVERAGE (Minor) -- the R10-lite case asserted only ABSENCE from
`decoded_onehz`, which would equally pass if the record were dropped
outright; retention in `samples` is the other half of that contract. Added,
plus a case protecting the unparseable-hex fallback in
`_decodeOneHzSample`.
NOT CHANGED, deliberately:
* `gen5DeepBuffersEnabled` is unwired in production -- verified true (zero
overrides in app_state or background_sync), but it is a deliberate
default-OFF opt-in for v20/v21/v26 buffers that are archived, not
interpreted. Wiring a settings toggle for a feature nothing consumes would
be premature.
* The protocol PIN. CodeRabbit's `pubspec.yaml` comment finding was real and
is fixed -- the file claimed this branch was "intentionally WHOOP-4-only"
while pinning the multiband commit twelve lines below. But the pin itself
is left alone: protocol#16 has landed, so the SHA is now reachable from
protocol main and SHOULD be repointed there (the current pin is a
PR-branch head on the branch this PR deletes on merge -- the exact
orphaning the same paragraph warns against). That belongs with the
main-merge, not this pass; recorded as a FOLLOW-UP in the file.
12 tests added; the gate tests are mutation-verified. analyze clean.
Suite 1085 passing / 6 failing -- the 6 are the notification_dedupe time bombs
this branch still carries because it predates #207 on main (19 hardcoded dates,
no `todayLabel()`). They clear when main is merged in.
|
@coderabbitai — worked through all five. Four were real and are fixed in ① Vacuous guard test (Major) — valid, and it was my ownYou were right: the file only exercised Two things worth recording from building it, because they'd bite anyone repeating it:
② Stale serial (Minor) — validConfirmed the whole chain: ③ Chip overflow (Minor) — valid,
|
|
Tip For best results, initiate chat on the files or code changes.
I acknowledge the four fixes in The updated sleep tests now exercise the I accept the decision not to wire The The six 🐇 ✓ ✏️ Learnings added
You are interacting with an AI system. |
The branch forked at 5faa4b0 and was 96 commits behind. The last two experimental releases (0.9.22, 0.9.23) were both cut from that fork point, so WHOOP 5 testers have been running builds without fixes main has had for weeks — including the ones that matter most on iOS: strength workouts never reaching Apple Health (#184), strain scored 0.0 after a backgrounded workout (#206), reconnect dying for the process lifetime after one throw (#208), and the crash batch in #212, whose Journey-timeline clamp is the most-hit issue in Crashlytics. This merge ends that drift before the next experimental. Five conflicts, three of them real, all resolved as keep-both: getBattery/getHello — take main's throttled _pollBatteryIfDue (a raw send here was 2,880 radio round-trips a day) and keep the branch's gen5 HELLO, which is a different opcode on Maverick. enableHrOnlyLive — both sides wanted a line at the same place: the branch's isGen5 lookup and main's _applyLinkPriority() step-down. Both are kept. app_state imports — union of the two `show` lists. One conflict git resolved silently and wrongly, caught by the analyzer rather than by the merge: gen5 changed `setAlarm` to return the armed instant (null = the write never reached the band) where main returns a bool, and main's alarm grace-retry — code the branch has never seen — assigned it straight to `bool rearmed`. Same signal either way, so the call site becomes `!= null` and the retry bookkeeping is unchanged. Dependency pins, both moved forward rather than merged blind: protocol -> 367d22b, protocol main @ #16 merge. gen5 is ON MAIN now, so the side-branch pin is obsolete; this SHA is a strict superset of the 7edcb3e edge main carried (crc8 length check, realtimeRr RR-bound, odd-length hex rejection) plus every gen5 decoder the branch needs. analytics -> main's 1fb34dc. The branch's cbbe06a is an ancestor of it, 15 commits behind, so this is a straight fast-forward. flutter analyze lib test clean. Full suite +1535 ~2, 0 failures.
Takes 0.9.25 for the experimental line, as 0.9.22/0.9.23 did before it, so main's next stable release is 0.9.26. versionCode 56 follows 55 (v0.9.24). pbxproj literals move with pubspec: the widget and Watch targets build with GENERATE_INFOPLIST_FILE=YES, so their versions come from project.pbxproj and NOT from $(FLUTTER_BUILD_NAME) — pointing them at that variable yields an EMPTY version, verified on compiled bytes. A pubspec-only bump ships stale embedded bundles and App Store validation bounces the upload.
|
Persistent review updated to latest commit 4005a8d |
PR Code Suggestions ✨Explore these optional code suggestions:
|
# Conflicts: # ios/Runner.xcodeproj/project.pbxproj # lib/ble/ble_engine.dart # lib/data/db.dart # pubspec.yaml
the plausibility gate threw records away. a record that decodes but has a timestamp we don't trust hit a bare return and was written nowhere, while the one two lines up that we couldn't parse at all got archived durably. so we treated "suspicious" worse than "unreadable". it archives now, and hadDurableRows counts banked records rather than archives, otherwise the no-progress refusal could never fire again. we were also archiving records we've always been able to decode — the ingest only routed 24/12/10 while parseR24 handles 7, 9, 18 and 25 too. a real export has 49,752 of those sitting as undecodable_rec_v25. routes on protocol's own exported set now, gen4 only, since gen5 uses 18 for something else. deleted the local copies of the gen5 clock payloads, the imu mode body and the lenient v18 decoder. protocol carries all three correctly now, and the lenient one only existed because the strict decoder used to bin a whole record when gravity looked off. gen5 clock failure was undetectable: we only surfaced clock_epoch when the value already looked plausible, so shouldOldClock never saw the unset rtc it exists to catch. emits it either way now and lets the policy judge. _readClock was sending the gen4 opcode too, so on a gen5 link the gate never got an answer. high-freq sync was framed for gen4 on both bands, which a gen5 strap can't parse, and we set the requested flag anyway so the logs claimed it worked. alarm disable/get and the strap rename now pick the form the band actually takes. gen4 stays byte for byte what it was. decoded_onehz keeps the per-second fields the band already sends and we were throwing away — steps, cadence, activity class, skin temp in c, wear and the hr quality flag. null on gen4 rather than 0, a fabricated zero step count is worse than nothing. schema 34. the decoded_rr query in the gen5 persistence test still used counter, which that table lost when it was re-keyed on rec_ts.
|
Persistent review updated to latest commit e99bd0e |
edge built its own 20-byte gen5 alarm body while protocol built 21 — gen5 carries a crescendo flag it validates as 0 or 1 and refuses otherwise, so one of the two was always going to be rejected by the strap. edge follows protocol's layout now; gen4 stays at the 20 bytes verified on hardware. hadDurableRows dropped archives entirely when gate-dropped records started archiving, which fixed the drop-only case and broke another: a burst made only of records we can't decode — an r10 historical, say — counts as no progress, so the band re-sends it forever. counts archives again except the plausibility drops. the clock read ignored the status byte protocol just started surfacing. a failed or unimplemented reply leaves the body unpopulated, and this path deliberately forwards implausible clocks so the unset-rtc case stays reachable, so nothing downstream would have caught a stale one.
|
Persistent review updated to latest commit 3d0675e |
the plausibility-drop fix landed the archive side but left both counters counting drops as progress. `records` schedules a derive pass and sets the ledger to partial, and `recordsThisOffload` feeds `banked` at history-complete — so a strap with a wandered rtc could drop every record, still look productive, and the empty-sync streak would reset so the rtc remedy never surfaced. a record we merely distrust isn't progress in either. v25 stays archived. the decode is right — checked against 20k of them, the timestamp steps by exactly 1s and gravity reads a mean |g| of 0.97 — but the record genuinely has no heart rate (scanned every offset across 18k consecutive pairs; what's in the middle is a raw waveform buffer). hr is NOT NULL and 0 is our off-skin sentinel, so banking it would claim the band was off the wrist for ~50k seconds while the real gravity keeps the coverage gate happy. wants a nullable hr column first. gen5 init now stops at the first failed write, like gen4. the caller pre-arms _offloadActive and rolls it back on false, which is only sound while false means the drain trigger never went out — firing send_historical after an earlier write failed floods the link while the caller tears the state down. the data-range reply was being byte-scanned for anything epoch-shaped, min and max. that's the bug protocol just replaced with a real field map, and its own comment records it landing on 2034 and pinning backlogRemains true forever. read the field. optical live-stream arming is gen4-only now — on gen5 that opcode is the save-to-history toggle, not the realtime stream, so every live session was writing a save enable next door to the persistent-optical footgun. clock read-gate is released on the reply rather than on a decoded epoch, so a reply we can't read costs nothing instead of 3s. the augmenter only forwards sub-floor values now; the scan it used to fall back to could only ever match a straddle. high-freq interval is 61 — 60 is the one value gen5 refuses. _innerHex is a table lookup; it runs ~50k times an offload on the ui isolate.
|
Persistent review updated to latest commit ee9a10f |
|
Persistent review updated to latest commit 22fc0b6 |
|
Persistent review updated to latest commit 5dae5e5 |
4 participants
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