Skip to content

Fix WHOOP 4.0 HRV source selection - #2441

Closed
rodrigosa7 wants to merge 2 commits into
ryanbr:mainfrom
rodrigosa7:fix/whoop4-hrv-source-selection
Closed

rodrigosa7 wants to merge 2 commits into
ryanbr:mainfrom
rodrigosa7:fix/whoop4-hrv-source-selection

Conversation

@rodrigosa7

@rodrigosa7 rodrigosa7 commented Sep 24, 2026 •

Copy link
Copy Markdown

What this PR does

Labels WHOOP 4.0 type-47 historical R-R intervals separately from the overlapping unlabelled live BLE feed. HRV reads now select the historical stream consistently across the requested window, with a fallback to legacy unlabelled rows when no historical data is available. Exact-key historical replays promote existing legacy rows, and analysis fingerprints change so cached nights are rescored.

This addresses the supplied WHOOP log where a plausible whole-night RMSSD was withheld because combined historical/live coverage was classified as crossSecondOverCount.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

  • swift test in Packages/WhoopProtocol: 788 tests passed, 1 corpus-dependent test skipped.
  • swift test in Packages/WhoopStore: 609 tests passed.
  • GitHub Actions passed on the updated branch, including Android build-and-test, WhoopProtocol/WhoopStore tests, and macOS/iOS builds.
  • Local Android testFullDebugUnitTest: app and tests compiled; 6,308 tests ran, 6 skipped, and 2 RecoveryDriversTest cases failed (driverPointRoundingUsesNearestWithHalfTiesAwayFromZero, issue51NegativeHalfTieUsesDefaultArg8WithoutChangingScoreOrDriverFields). Those files are outside this change; the PR’s GitHub Actions Android job passed.
  • Local macOS Strand app build passed with existing warnings in unrelated files.
  • git diff --check passed.
  • No real strap validation was performed. The fix changes how the existing WHOOP 4.0 historical BLE stream is labelled and selected; it does not change BLE commands or transport behavior.

Checklist

  • Swift package tests pass for the packages touched.
  • Android unit tests pass in GitHub Actions; local run reported two unrelated recovery-driver failures noted above.
  • No new build warnings introduced in the changed files; the macOS build warnings were in unrelated existing files.
  • UI changes use only StrandDesign tokens — no UI changes.
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders.
  • Follows the conventions in docs/CONTRIBUTING.md.
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores.

Related issues

Refs #2335, #1118, #2336

@ryanbr

ryanbr commented Sep 26, 2026

Copy link
Copy Markdown
Owner

Landed on main. Your two commits are there unchanged as c1beffa51 and 5e063cd75, authorship intact, with a follow-up of mine at 1106f6ddd. Merged with rebase rather than squash so they stayed yours. The work went in through #2478, which is the only reason this one is not the merge itself: a push to your fork is refused (Permission to rodrigosa7/noop.git denied) even though the API reports maintainer_can_modify: true. Thanks @rodrigosa7.

The diagnosis was right, and it matches the shape #2423 settled on for Oura: when two sources cover the same beats, score ONE of them for the interval rather than splicing both and reading coverage above 1.0.

What the follow-up mostly did was the rebase. Your base predates #2423 and #2457, and both touch the single read this changes. Taking either side of that conflict literally would have reverted a merged fix without saying so. rrIntervals is now three-way: WHOOP 5 takes the lowest scorable transport, a WHOOP 4 takes its type-47 history (or its unlabelled rows when the window holds none), and everything else takes the Oura branch. OuraRedrainCollapse still wraps the result on both platforms; the Kotlin conflict would have dropped it from WhoopRepository too.

The parity gate needed five findings dispositioned, and it is worth knowing that as submitted this branch would not have run it: parity-governance.yml filters on Tools/parity_*, so the gate only fired once my commit touched parity_dispositions.json. extractHistoricalStreams took a declared twin reference rather than a disposition, because the Kotlin twin exists and adding family only moved the arity-keyed identity. The three Room DAO methods and their two @Query constants took platform_specific dispositions, since Room needs a generated method and a constant string where the Swift store inlines the same SQL.

One thing I checked and did not change. The family: DeviceFamily = .whoop4 default looks risky read alone, but it mirrors the Kotlin twin where that exact default has been on main at three signatures, and both production call sites pass it explicitly. Left as you wrote it.

A note on the two RecoveryDriversTest failures you saw locally. Worth knowing for next time: those, and the DebugExportSchedulerTest pair I hit on this branch, fail identically on untouched main. The scheduler ones are a DST-boundary bug in the test, filed as #2477. Your instinct to call them unrelated was right, and the way to show it is to run the same test on the base commit, which turns "outside this change" into a fact rather than a judgement.

Hardware proof is still owed, as you say: a whole-night RMSSD on a 4.0 whose window carries both a type-47 offload and live BLE, with hrv diag showing coverage at or below 1.0 instead of crossSecondOverCount. Worth posting when a night produces one.

One known trade is recorded in #2478: promotion relabels only exact key matches, so a PARTIAL offload can leave un-promoted unlabelled rows that the all-or-nothing selection then drops, lowering coverage. The direction of failure is a refusal to score rather than a wrong number, and it is visible in the coverage line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants