Repository navigation
fix: correct WHOOP 5 R-R interval units - #2046
Conversation
63da146 to
13c8923
Compare
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
Reviewed against the code and the wire spec rather than the description. The core claim holds and the
evidence is the right kind, but the blast radius of the exclusion is larger than the description
suggests and nothing tells the wearer.
The units claim is right
BLE's 0x2A37 R-R field is defined in 1/1024-second units, so if the type-40 and v18 words share that
encoding then reading them as milliseconds inflates every interval by 2.4 percent. The paired-capture
evidence is the right shape too: 131 native-live arrays matching 131 distinct standard-BLE arrays, and
94 historical arrays matching the raw words while matching none of the already-converted millisecond
arrays, is a discriminating test rather than one coincidence. Whoop5RR.milliseconds rounds to nearest
and 1024 ticks come out as 1000 ms.
What the exclusion actually does
rrIntervals pins one transport per window:
srcChannel = (SELECT MIN(srcChannel) ... WHERE srcChannel IN (5, 7) ...)
An unlabelled row has srcChannel IS NULL, so it never equals the pinned channel, and a window holding
only unlabelled rows returns ZERO beats rather than fewer. That propagates further than "excluded from
scoring" implies:
- no beats means no
avgHrvfor the day recomputeRecoveryopens withguard let hrvVal = daily.avgHrv ... else { return nil }, so the day
has no Charge either
Because the analyze pass re-scores a bounded recent window, the effect is a SEAM: the recent stretch
loses HRV and Charge while older banked days keep the scores they already had, computed from the very
rows now judged unusable. A wearer sees recent history hollow out while last month stays intact, which
reads as the app breaking rather than as a correction.
The reasoning for excluding is sound, and I want to be clear I agree with it: legacy rows genuinely mix
correctly-converted standard-BLE milliseconds with wrongly-scaled native words, and nothing on disk
distinguishes them, so scoring the mixture is worse than declining to. My concern is not the policy.
What is missing
Nothing in these 54 files touches UI or copy. There is no empty state, no note on the HRV card, nothing
in the strap log that a reader would find when their Charge history goes quiet. This project's own
conventions are strict about honest-nil being EXPLAINED rather than merely honest, and a silent
regression to "Calibrating" for a WHOOP 5 wearer is the case those conventions exist for.
A single line where the gap shows, saying earlier WHOOP 5 beats cannot be scored and a re-sync restores
them, would turn a mystery into a known state. That is what I would want before this lands.
Two smaller things
The last line of the description pairs a closing keyword with the issue number, which auto-closes that
issue on merge. Please reword it to a plain reference: issues here are closed deliberately, after the
fix is confirmed on hardware. (Quoting the line verbatim here would do the same thing from my side,
which is why it is described rather than shown.)
The reported "two failures in the unchanged RecoveryDriversTest" do not reproduce. On this PR's own
head it is 10 tests, 0 failures, and the identical caveat on your other PR did not reproduce either, on
main or on that head. Whatever that was on the older base is gone, so the Android checklist box can be
ticked and the caveat dropped. Worth mentioning because an unticked "unit tests pass" on a 54-file
protocol change reads as a real risk when it is not one.
Scope
This carries the units conversion, transport labelling, a read policy, cache invalidation and new
indexes. Each is defensible, but a units correction that changes every WHOOP 5 HRV number is the kind
of change worth landing alone, so that if something moves unexpectedly there is one candidate rather
than five. I am not asking for a split if you would rather not, only noting it.
Thanks @Trillient. The decode work here is careful, and the paired-capture method is exactly how this
should be argued.
…rtialBaseline The 260921-0740 log, read through the ledgers 18.16 shipped, showed the Charge flip's real mechanism. The 260906 abort gate stops a full pass at night N, and everything after it kept running AS a full pass. Two of those steps are window-wide: the stale eviction deletes every computed row in [oldestDay, newestDay] the pass did not reproduce, and persistComputedScores wide-deletes metricSeries + provenance over the same span. oldestDay is the whole 21-day window regardless of where the scan stopped, so a pass abandoned after night 1 deleted the other twenty computed days — four times in one morning (`ABANDONED after 1/21` x4). Today was then written nil (sub-seed baseline), and with Sep 12-20 gone the Today/widget anchor fell back to the oldest scored row OUTSIDE the window, immune to eviction: 18 August, Charge 36. The next foreground pass restored the window and today read 62. Every abandoned background pass repeated it. The 260902 light-pass guard describes this exact failure and keyed it on `lightPass`; the abort gate opened a second route the flag cannot see. Every post-scan gate now reads `partialBaseline` (light OR abandoned): persist window bounded to the scanned days, rest points and weekly stages skipped, eviction not run. A partial pass rewrites what it produced and nothing else. Nothing changes for a completed pass or a light pass. A row ryanbr#2115 preserves — a night whose R-R the WHOOP 5 unit policy (ryanbr#2046) withholds — has no raw to re-derive from; evicted once, it is gone. That is how Aug 19-Sep 11 lost their HRV/Charge: the fork ran v11.6.0 (ryanbr#2046, no ryanbr#2115) from Sep 11 to 19 and each full pass overwrote them with nil; the eviction then removed any survivors. Not relabelled here, by instruction; the window heals as it slides. Two ledgers, always-on, counts only: - `evict: removed N stale computed day(s)` — only when N > 0. A delete of computed history is never silent. - `legacyScores preserved=X hrvMissingWithSleep=Y of N` — per completed pass. ryanbr#2115 had no log line; this is the number that must only ever fall. - `chargePublish` gains `battery=`, for the widget report. Tests: AbandonedPassBaselineTests grows to 9 — the eviction predicate and the persist window, both directions. Full 2488-test macOS suite: only the two pre-existing failures. Verified: Strand (macOS) + NOOPiOS build. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…indow A WHOOP 5/MG window was pinned to the lowest scorable transport present anywhere in it, so one stray v18 history record outvoted a whole night of standard-BLE beats: the night then scored on the history alone and its HRV, respiratory rate and recovery came out thin or blank. The choice is now made per UTC hour, counted over the whole hour like the WHOOP 4 choice: the transport holding more beats wins, history on a tie. Two transports are still never merged inside an hour; native realtime (6) and unlabelled legacy rows stay unscored. No BLE change. Both platforms change together (WhoopStore.rrIntervals and WHOOP5_RR_INTERVALS_SQL), and Tools/mg_vs_watch.py follows the same rule. The per-window tests are replaced by one per-hour test with the same assertions on both sides (fuller wins, tie keeps history, choice precedes LIMIT, whole hour decides, quarantine, realtime). The two nightly-scoring tests that pinned the old behaviour now assert that a history record in another hour no longer discards the night's standard beats. Tests: WhoopStore 632/632, StrandTests IntelligenceRRSourceTests 5/5, Android Whoop5RRSqliteTest and SlidingStreamWindowTest pass, test_mg_vs_watch passes. Not yet checked on a real night. Refs ryanbr#2046 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What this PR does
WHOOP 5 type-40 and v18 R-R words are decoded as milliseconds even though paired captures on firmware 50.41.1.0 show the same 1/1024-second words as standard BLE 0x2A37. For example, 1024 ticks represent 1000 ms; treating them as milliseconds scales intervals by 2.4%.
Convert those layouts to rounded milliseconds in Swift and Kotlin and preserve raw ticks in the parsed output. Label historical, native-live and standard-BLE observations so scoring uses one transport per requested window: history when available, otherwise standard BLE. Keep the selected stream's beat order through exact-key collisions and invalidate cached scores when source provenance changes.
Existing unlabelled WHOOP 5 rows remain on disk but are excluded from scoring because their units and transport cannot be recovered reliably. Newly captured or re-offloaded labelled rows supply the corrected input. WHOOP 4 and other-device reads retain their existing policy. This does not improve sparse overnight coverage or change the HRV formula.
Raw CSV export retains legacy and all WHOOP transport rows using its existing quarantine exclusions; scoring selection does not filter the export. Additive R-R source indexes keep provenance-sensitive cache checks bounded by indexed queries. Regression coverage includes archived physical owners and the ambiguous canonical alias after re-pairing.
Type of change
How it was tested
swift testpassed for WhoopProtocol, WhoopStore and StrandAnalytics. Coverage includes conversion/packet bounds, legacy-owner policy, transport precedence, repeated equal beats, zero-insert source promotion and cache invalidation. Includes paired, redacted capture fixtures for firmware50.41.1.0: R-R count/word bytes retain captured values; identities and original timestamps are omitted, other fields are synthetic, and checksums are recomputed. Exhaustive 16-bit conversion oracles and synthetic boundary fixtures supplement these paired cases.RecoveryDriversTestexact floating-point half-tie assertions. Both failures reproduce on the unmodified base commitaff58ec3(expected-0.5, actual-0.4999999999999929).Checklist
android/— two rounding assertion failures described aboveStrandDesigntokens — no UI changesdocs/CONTRIBUTING.mdRelated issues
Relates to #1505.