fix(protocol): correct GET_DATA_RANGE pagesBehind field offsets - #818
Merged
Merged
Conversation
…br#689/ryanbr#815) The write/read/ring-capacity triplet was read 2 bytes early: cmdOff+10/14/22 instead of cmdOff+12/16/24. That meant the ring-capacity field always landed on padding (0), so the plausibility guard rejected every real frame and pagesBehind never fired on hardware. Confirmed the corrected offsets against four real captures across both WHOOP 4.0 (ryanbr#791) and 5.0/MG (ryanbr#815) — ring capacity is 131072 in every one, and the backlog math now matches by hand calculation in each case. Swift and Kotlin updated in lockstep per the parity contract; Kotlin unit tests weren't run locally (no JDK in this environment) but the change mirrors the Swift edit byte-for-byte, including the new real-capture fixtures.
ryanbr
approved these changes
Jul 26, 2026
ryanbr
left a comment
ryanbr
left a comment
Owner
There was a problem hiding this comment.
Verified independently against the four fixtures — the fix is right, and the old code was worse than "unconfirmed".
Decoding the captures both ways:
new +12/16/24 |
old +10/14/22 |
||||
|---|---|---|---|---|---|
| W | U | T | W | T | |
| #791 4.0 | 67491 | 67408 | 131072 | 128122881 | 0 |
| #815 5/MG | 100790 | 100788 | 131072 | 2310406145 | 0 |
| #815 5/MG | 100837 | 100829 | 131072 | 2313486337 | 0 |
| #815 5/MG | 112968 | 112474 | 131072 | 3108503553 | 0 |
T = 131072 is 2¹⁷ on every frame across both families, W/U are plausible page counters, and pagesBehind comes out 83 / 2 / 8 / 494 exactly. The old read gives ring capacity 0 and pointers in the billions, so your "always landed on padding" is confirmed — the plausibility guard rejected 100% of real frames and the feature has never once fired.
Renaming whoop5CmdOff10 is the right call too. A synthetic-only test named as though it were hardware-validated is how the wrong offsets survived; the doc comment saying "not yet confirmed" was accurate and everyone read past it.
Kotlin twin is byte-for-byte: same payloadOffset + 11/15/23, same four fixtures, same expected values. It can't run here either — no JDK, and android.yml is disabled, so nothing in CI executes Kotlin tests at all. Worth knowing that "should pass under android.yml" won't happen automatically. All 9 Swift checks green including test (WhoopProtocol).
One housekeeping note: the PR body carries a Claude attribution line and session link. I'll drop those from the squash message when merging — the maintainer keeps commit history free of them.
Nothing blocking. Wraparound staying unverified is fine given it's diagnostic-only, and reading T from the frame rather than hardcoding 131072 is the right instinct even though it's been constant so far.
ryanbr
added a commit
that referenced
this pull request
Jul 26, 2026
…eeds (#821) Follow-up to #818, found reviewing it. Both platforms logged the ring backlog only on success, so a rejected decode produced no line at all — a strap log was indistinguishable from one where the strap never answered a GET_DATA_RANGE. That silence is why #818's bug lasted. The offsets were two bytes early, ring capacity always read 0, the `t > 0` plausibility guard rejected 100% of real frames, and nothing anywhere said so. The doc comment's "RE'd, unconfirmed" got read as "probably fine" because no evidence could contradict it — none could exist. The file already applied the right principle three lines up: the raw-frame dump is unconditional, explicitly "even if decode returns nil, so the field offsets are inspectable from a strap log". That reasoning simply was not carried across to the field parsed FROM that frame. Both branches now log, and the failure line points at the raw frame above it as the input to inspect. If a firmware revision shifts these fields again, the rejection is visible rather than silent. Verified: the failure strings AND the pre-existing success strings are byte-identical across platforms — reconstructed from the concatenated literals rather than read, since the source wrapping differs and a first extraction attempt reported a false mismatch. The handler is gated to GET_DATA_RANGE on both families (frame[6] on 4.0, frame[8]==0x24 && frame[10] on 5/MG), and that command is sent once per connect handshake, so this adds at most one line per connection. Diagnostic only — sync and backfill untouched either way. Not compiled: app-target code on both platforms and neither android.yml nor app-build.yml runs, though the 7bed0b4 staging build was green on all four platforms an hour ago.
ryanbr
added a commit
that referenced
this pull request
Jul 26, 2026
… for the export shape Two claims re-checked against the tree rather than left as inherited assertions. "There is no GET_SPO2-style opcode" was an overclaim. What we can actually say is that our CommandNumber catalogue has 80 commands and none reads oxygen, and that independent RE reports none. That is not proof about the strap's command space: §6 is explicitly a "safe subset", and the 98-vs-87 battery dispute (#818, #592) shows the map is incomplete. Restated as "nobody has found one", which still supports the useful conclusion — hunting a missing opcode is the wrong lead. The per-cycle export shape came from #807 and I had passed it through unverified. Now confirmed from our own code and cited: bloodOxygenPct is read on the physiological-cycles row beside recovery_score_pct and day_strain, keyed on cycleStart/cycleEnd (WhoopExportImporter.swift:272). So "one value per recovery cycle" is ours to assert, not a claim borrowed on trust.
ryanbr
pushed a commit
that referenced
this pull request
Jul 26, 2026
Closes #245 — the Liquid Today header lost the sync indicator the classic header had, so on iOS there was no sign the strap was mid-offload. This restores it, and the syncing state carries the chunk count, which covers the "current progress of Syncing" the reporter described losing. Extracts the decision into a testable `SyncChipState` enum with six tests. The extraction is behaviour-preserving: `resolve` keeps the original branch order exactly — backfilling -> lastSyncedAt -> historySyncExperimental -> nothing — and `shortAgo` moved verbatim. The classic chip keeps its `StrandPalette` tints while Liquid gets the dark-hero idiom, so only the chrome differs. Shared decision, separate styling, which is the right split. Android parity verified rather than assumed: `SyncStatusChip` (TodayScreen.kt:2128) uses the same branch order, the same strings, and `shortSyncAgo` matches `shortAgo` boundary for boundary — <60 "now", <3600 "Nm", <86400 "Nh", else "Nd". Android has no Liquid/classic split, so there is genuinely nothing to mirror. TWO REVIEW ASKS WITHDRAWN — BOTH WERE MINE, NOT THIS PR'S DEBT Localizing the chip strings. I flagged it as something to settle before leaving draft. It is pre-existing: TodayScreen.kt:2142 already ships "live" as a bare literal on main, and shortSyncAgo already produces "now"/"Nm"/"Nh"/"Nd". This PR moves strings that were already unlocalized into a testable type; it does not introduce them. Worth fixing across both platforms as its own change, not as a condition on a refactor that improves matters. Note the i18n gate cannot see this either way: the chip renders Text(variable), and the audit only reads literals. A green check here means less than it looks. Extracting Android's SyncChipState. Pure test parity — Android's logic is correct and identical, it just lives inline in a @composable where it cannot be unit-tested. Worth doing, not this contributor's job. FOLLOW-UP WORTH KNOWING ABOUT jbergler asked on the PR whether #818's now-working pagesBehind could drive a percentage. Pages are not chunks — a page is a ring-buffer slot (W - U against a 131072 capacity), a chunk is a HISTORY_END boundary, and nothing relates them. But pages are the better denominator if sampled twice, and today GET_DATA_RANGE is sent once, from the connect handshake (BLEManager.swift:4080). Mid-offload re-polling would be needed, and whether that perturbs a firmware-paced ~10 records/s offload is a hardware question. The free intermediate is "Syncing — N pages behind" from the single sample we already take.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DataRange.pagesBehind(Swift) /DataRange.pagesBehind(Kotlin) read the write/read/ring-capacity triplet 2 bytes early (cmdOff+10/14/22instead ofcmdOff+12/16/24). The ring-capacity field always landed on padding (0), so the plausibility guard rejected every real frame —pagesBehindnever actually fired on hardware, despite the doc comment implying it just hadn't been checked yet.131072in every one, and pagesBehind computes cleanly (83, 2, 8, 494 pages).whoop5CmdOff10test (misleading — implied hardware validation that didn't exist) for real fixtures on both platforms.Int?/Long?return, wraparound branch still unverified against real hardware (no capture has crossed it) but left as-is since it's low-risk (UI-diagnostic only).Test plan
swift test— fullWhoopProtocolpackage suite (331 tests) passes, including newtestPagesBehind_realCaptures.DataRangeScanTest— mirrored byte-for-byte from the Swift change (same offsets, same fixtures, same expected values) but not run locally (no JDK in this environment). Should pass underandroid.yml/./gradlew testFullDebugUnitTest --tests "com.noop.ble.DataRangeScanTest".Packages/WhoopProtocol(pure) and its Kotlin twin.