Repository navigation
fix: WHOOP 5 R-R words are milliseconds, not 1/1024-second ticks - #2195
Conversation
…icks Reverses the conversion introduced in #2046. That PR matched native and standard BLE raw words and assumed they shared the BLE spec's 1/1024-second unit. The assumption was reasonable — the spec is unambiguous — but wrong for this strap. Four independent estimators over 71,464 CRC-verified type-40 frames from 19 captures (firmware 50.41.1.0) now show the words are already milliseconds: elapsed-time slopes 1000.19–1000.76, phase-feasibility peak at 1003, longest self-consistent single-beat run 83 @1000 vs 49 @1024, HR-byte cross-check median 990. Transport parity (same raw words on native and standard BLE 0x2A37) means the strap sends millisecond values on both channels, non-compliant with the BLE spec. The old (ticks * 1000 + 512) / 1024 conversion systematically shrank every interval by ~2.3 %, which broke tachogram contiguity (max chain 126 → 1442 beats corrected), starved the RSA respiratory estimator, and biased RMSSD. Whoop5RR.milliseconds is now identity on both Swift and Kotlin. Oracle hashes, wire-case ms, and paired-capture tests updated to assert identity (raw == ms) instead of matching the 1/1024 conversion.
FramingTest and Whoop5HistoricalDecodeTest hardcoded the old 1/1024 converted values independently of the oracle fixtures. Update them to assert the raw millisecond values (603/587, 602/613) that match the identity conversion.
|
Reviewed at b393c9e. The evidence is strong and the epistemics are better than the evidence. The key sentence is this one: matching proves the channels carry the same encoding, but does not prove which encoding that is. That is precisely where #2046 went wrong, and it is a failure mode that looks like confirmation at the time. Four estimators that use the strap's own internal consistency rather than the spec, all landing near 1000 and not near 1024, is the right way to break the tie. The elapsed-time slope is the one I find hardest to argue with: summed words against wall clock does not care what the spec says. The external check earns its place too. MAE 10.2 to 8.899 and r 0.68 to 0.739 against 87 concurrent Garmin nights is an independent instrument agreeing, and it moves the right way by roughly the right amount. The fix is only half applied
That is not theoretical. I would rather this landed as one change than as a correct half. Either the standard parser needs a WHOOP-5 branch that skips the conversion, or, if there is a reason the native path is the only one whose R-R is ever stored for a 5, that reason belongs in the description because it is not visible from here. Smaller notesThe oracle staying as Reversing a merged decision on better evidence, and saying plainly what the earlier evidence did and did not show, is the right way to do this. 16 of 16 checks. The standard-path gap is the only thing between this and a merge from my side. |
WHOOP 5 sends milliseconds on the standard BLE 0x2A37 characteristic, non-compliant with the BLE spec's 1/1024-second unit. The parser was applying the spec conversion, creating a 2.3% systematic shrink vs the native channel (which already uses identity). Add rrRawTicks to the parser return value on both platforms. WHOOP-5-aware callers (BLEManager on iOS, WhoopBleClient on Android) now use raw ticks instead of spec-converted values, eliminating the channel disagreement. The parser itself remains spec-correct for all other devices.
|
Reviewed at 148d4bb. This is the half I was waiting for, and it lands the fix on both platforms rather than leaving the two channels 2.3% apart. 16 of 16 checks. The assertion that carries the PR is the one added to That is what turns the claim from an inference into a fact. Both channels carry byte-identical u16 words, so exactly one of the two conversions can be right, and the native channel's is the one the strap's own store agrees with. The earlier I checked the family read at both selection points, since both default to WHOOP 4 and a wrong read would silently reinstate the bug:
Both are fine. Flagging that I checked so it does not have to be re-derived later. One residual, not blocking this. The generic strap source still converts on both platforms:
Both read That last part is a hardware question I cannot answer from code, and you have the strap: does a WHOOP 5 appear in noop's "HR strap" scan list? That scan filters on an advertised 0x180D. The strap clearly exposes 0x180D once connected, but advertising it is a separate matter. One scan settles it, no capture needed. If it does appear, that path wants the same treatment and I will open something for it; if it does not, there is nothing there to fix. Minor. The doc line saying callers "should use The selection ternaries themselves are uncovered on both sides, while the parser is covered well. Both sites are BLE-bound, so that matches how the rest of this layer is tested and I am not treating it as a gap. Good to merge from my side. Thanks for splitting it this way, and for the paired-capture method, which is what made the whole thing decidable. |
|
Merged as a78e2a6. Thanks again for splitting it, and for the paired capture. Correcting one thing I wrote above, because it would send whoever picks up the residual in the wrong direction. I said the generic strap source "wants the same treatment", meaning the same raw-ticks selection. Having looked properly at what that path does, that is the wrong prescription.
That is duplicate data, which is a considerably worse outcome than R-R being 2.3% low, and it is the failure class already on record here. Converting the R-R correctly on that path would make the duplicate rows agree with each other, which is arguably worse still, since the duplication would then be harder to spot. If a 5.0 turns out to be reachable there, the fix is to keep it out of the generic list rather than to parse it better. The codebase already states that principle, in
There is no WHOOP exclusion in either So the open question is unchanged and still worth one scan when convenient: does a WHOOP 5 appear in noop's "HR strap" scan list? Only now a yes means something different from what I said earlier. A knock-on: if the answer is that a WHOOP 5 has no business on that path at all, then no caller of the parser legitimately needs the family, Nothing here needs anything from you beyond the scan. Recording it on the PR so the reasoning sits with the change rather than in my head. |
The 0x2A37 R-R word is 1/1024 s per the BLE spec, but a WHOOP 5/MG sends plain milliseconds, so the 1000/1024 conversion landed 2.3% short (a raw 512 became 500, which reads as a plausible value). Convert per family through a pure standardProfileRrMs; WHOOP 4 keeps the spec conversion. Recorded in docs/ALGORITHMS.md as decode arithmetic still to move to whoop-rs (no FFI export parses the standard HR measurement). Cherry-picked from upstream a78e2a6 (ryanbr/noop ryanbr#2195). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What this PR does
Reverses the 1/1024-second conversion introduced in #2046.
That PR matched native and standard BLE raw words and assumed they shared the BLE spec's 1/1024-second unit. The assumption was reasonable — the spec is unambiguous — but it turns out to be wrong for this strap.
Four independent estimators over 71,464 CRC-verified type-40 frames from 19 captures (firmware 50.41.1.0) show the words are already milliseconds:
Transport parity (same raw words on native type-40/v18 and standard BLE 0x2A37) means the strap sends millisecond values on both channels, non-compliant with the BLE spec.
Why #2046 looked right
The paired-capture evidence in #2046 was real: 131 native-live arrays genuinely matched 131 standard-BLE raw arrays. What we got wrong was the UNIT of those matching words. Matching proves the channels carry the same encoding, but does not prove which encoding that is. BLE 0x2A37 says 1/1024 s, so that is what we assumed — but the strap simply does not comply with that part of the spec.
The new evidence distinguishes the two hypotheses using the strap's own internal consistency rather than the BLE spec, which is why the four estimators above all converge near 1000 (ms) rather than 1024 (ticks).
Impact of the regression
The
(ticks * 1000 + 512) / 1024conversion systematically shrank every interval by ~2.3 %, which:What changes
Whoop5RR.millisecondsbecomes identity on both Swift and Kotlin. Oracle hashes, wire-case ms values, and paired-capture tests updated to assert identity (raw == ms) instead of matching the 1/1024 conversion. Two additional Android test files (FramingTest, Whoop5HistoricalDecodeTest) that independently hardcoded the old converted values are also updated. WHOOP 4 decoding is unchanged.Type of change
How it was tested
swift teston WhoopProtocol: 704 passed, 1 skipped, 0 failures — includes the paired-capture, oracle FNV hash, wire-case, and boundary tests from fix: correct WHOOP 5 R-R interval units #2046, all updated to the identity conversion.Checklist
android/StrandDesigntokens — no UI changesdocs/CONTRIBUTING.mdRelated issues
Reverses the conversion from #2046. Relates to #1505.