Repository navigation
alarm: attribute the readback to a strap before judging it - #1707
Merged
Merged
Conversation
Reported as #1706. The debug export read: Model: WHOOP 5.0 / MG Last arm: sent 2026-08-26 06:30 Strap reports: 2045-06-10 14:03⚠️ MISMATCH — strap didn't accept the time None of the thirteen alarm.* preferences is device-scoped, and on Android the readback is written only on the WHOOP 4.0 path. So on an install with a 4.0 and an active 5.0, that verdict compared an arm sent to one strap against a readback from another, then named a strap that was never asked. Not cosmetic on Apple: the same comparison drives alarm.rejectStreak, which raises a warning in SmartAlarmView at two. A cross-strap reading could climb that streak indefinitely. Both halves now record the strap they came from, and a shared AlarmReadback returns MATCHES, MISMATCH, DIFFERENT_STRAP or UNATTRIBUTED. Only a proven same-strap disagreement means refusal. DIFFERENT_STRAP and UNATTRIBUTED deliberately do nothing to the streak rather than resetting it: advancing would warn about a strap that was never asked, clearing would hide a real refusal, and neither reading is evidence either way. The raw readback frame is persisted beside the epoch now. That is what separates a genuinely stored stale alarm from a misdecode of a fixed response field, and the capture built for exactly that question had already rolled out of the live log before the export was taken, which is why the 2045 value is still unexplained. Nothing here decides it; the next export will carry the bytes. Also states what the clock line measures. It reads RECORD timestamps and sat two lines above the 2045 readback, which reads as the two contradicting each other. The verdict lives in a package on the Swift side so both twins are covered by default CI without app-build. Verified: 4,722 Android tests, 0 failures with --rerun-tasks --no-build-cache; the Swift verdict compiled standalone and agreed with the Kotlin case for case.
…streak Re-review of the attribution change, three defects in it. The router's deviceId was a separate assignment after family, so a future connection path that set the family and forgot the id would carry the PREVIOUS connection's strap forward — a confident wrong attribution, which is the exact mistake this work exists to prevent. Cleared in family's didSet, already documented as the per-session reset hook, so a forgotten assignment now attributes nothing instead of lying. All three call sites set family then deviceId, in that order. The streak change stranded upgraders. Values standing in alarm.rejectStreak were built by the cross-strap comparison being replaced, and since only a proven match clears the streak now, an install that upgraded mid-streak would hold SmartAlarmView's warning up forever. The first unattributed readback — which can only be pre-upgrade data — discards it once. That fix then made the comment above it false: it still said an unattributed reading leaves the streak untouched, two lines above code clearing it. Rewritten. Also takes the deviceId local directly rather than re-reading from UserDefaults what it wrote three lines earlier. Kotlin is untouched by this pass. The Swift verdict still compiles standalone and behaves identically.
Owner
Author
|
Fourth review pass. Two findings, both reported rather than changed, with the reasoning for leaving them. Why 2045 was displayed at allThere is already a plausibility gate on the readback epoch, and its own doc states the intent:
But the window it actually enforces is A near-now alarm is within a day or two. An asymmetric window — generous into the past, since a genuinely stale stored alarm is real and diagnostic, tight into the future, since an armed alarm cannot be years ahead — would have rejected this outright and produced the raw-hex line the gate says it wants. Deliberately not tightening it here. That would suppress the symptom before anyone knows the cause. The raw frame this PR starts persisting is exactly what determines whether 2045 was a misdecode of a fixed response field or a genuinely stored value, and narrowing the gate now would stop that evidence from ever being captured. If it turns out to be a misdecode, the fix belongs in the decode, not the gate. Worth its own issue once an export carries the bytes. An attribution asymmetry between the platformsThe two sides now guarantee different things, and it is worth knowing which is which:
Narrow, and strictly better than the unscoped comparison it replaces, but it is a real race and the Swift side does not have it. Not fixing it in this PR because the Android readback is 4.0-only and the window is milliseconds wide; noting it so the asymmetry is a known one rather than an accident. |
7 of 12 tasks
ryanbr
pushed a commit
that referenced
this pull request
Aug 28, 2026
…are actually uses (#1713) Contributed by @Zebsi235, with three arm/readback captures from a WHOOP 4.0 on fw 41.17.6.0. The GET readback is 11 bytes and carries a stored flag the 9-byte SET payload does not, so its epoch sits at offset 2: SET (9): [form 01][u32 LE epoch][00 00 00 00] GET (11): [form 01][stored flag][u32 LE epoch][00 00][04 00 20] Decoding it at the SET-mirror offset returned the epoch's low three bytes shifted up a byte plus the flag — wrong by roughly 256x and free to land anywhere in u32 range. In all three captures it landed on a 2045 date INSIDE the 2017..2100 plausibility window, so the gate accepted it and a strap whose alarm register was working perfectly was reported as refusing the time. Every number re-derived from the raw payloads before merging. The u32-LE at offset 2 is the sent epoch byte-exact in all three; the u32-LE at offset 1 is exactly what the app logged in all three; the misreads step by 22,118,400, which is 86400 x 256 — one day of arm, shifted a byte. Three independent epochs, no misses. Why it hid: the only fixture in the tree, the #34 field report 01 00 00 00 00 00 00 00 04 00 20, is this same shape with the flag 00 and the epoch zeroed — and a zeroed epoch region decodes identically at offsets 1 and 2. The existing test could not have caught this, and did not. The decode tries the 11-byte shape first and, on an implausible epoch, returns nil to the raw-hex line rather than falling through to the mirror offsets: those are provably wrong for this shape, so trying them could resurrect the misread. readbackReportsNoAlarm takes the same ordering, so both functions read the same shapes in the same order their docs promise, and the length gate keeps the 9-byte #535 capture structurally unreachable from the new branch. Kotlin twin is logically identical; four new tests per platform, all three captures pinned on both, no existing test touched. This also changes what earlier reports meant: a pre-fix reported epoch from a 4.0 readback of this shape cannot be trusted whatever it looked like, sane-looking values included, because an offset-1 read that fell outside the window dropped through to a bare-u32 read of different bytes. #1707 stopped the cross-strap comparison from accusing the wrong device; this stops the same-strap one from being wrong as well. Left alone deliberately, and correctly: the 82-year plausibility window, since on firmwares answering with some other shape the wide window plus raw-hex fallback is still what gets the bytes captured. And the 2045 value from the OTHER install in #1706 is not explained here — its low byte is 0xD4, which cannot be this shape's flag, so that one still needs its own capture.
ryanbr
added a commit
that referenced
this pull request
Sep 28, 2026
The readback frame line was emitted only when the frame existed, so a readback carrying an epoch and no frame printed nothing at all, which reads as "the frame was checked and was fine". The two causes are indistinguishable from outside: #1707 began banking the frame on 2026-08-28, so any readback taken before that has an epoch and no frame, and a write that failed looks identical. A 2045 readback in a 2026-09-28 capture cost a trip through git history to establish that the record simply predated the capture. The line now always appears and names both possibilities, the way the strap-clock verdict above it already says why it abstains rather than printing no line. Since the writer stores at least "empty" for any frame it decodes, this branch is reachable in practice only for records banked before that capture existed, which is the intent. Also aligns the Apple twin's blank handling with Kotlin's isNotBlank. A whitespace-only value printed a frame line carrying no bytes there; it counts as not stored on both platforms now. Four Robolectric tests. Mutation-checked: removing the branch turns exactly the two new-behaviour tests red and leaves both controls green.
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
1 participant
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.
Addresses #1706.
What was wrong
None of the thirteen
alarm.*preferences is device-scoped, and on Androidalarm.lastReportedEpochis written only on the WHOOP 4.0 path (WhoopBleClient.kt, guarded byconnectedFamily == DeviceFamily.WHOOP4). So on an install with a 4.0 and an active 5.0, that verdict compared an arm sent to one strap against a readback from another — and then named a strap that was never asked.It is not cosmetic on Apple. The same comparison drives
alarm.rejectStreak, which raises a warning inSmartAlarmViewat ≥2. A cross-strap reading could climb that streak indefinitely. I did not know this when filing; it turned up while implementing, and it is the reason this is a bug rather than a wording fix.The fix
Both halves now record the strap they came from (
alarm.lastArmDeviceId,alarm.lastReportedDeviceId), and a sharedAlarmReadbackreturns one of:MATCHESMISMATCHDIFFERENT_STRAPUNATTRIBUTEDDIFFERENT_STRAPandUNATTRIBUTEDdeliberately leave the reject streak untouched rather than resetting it. Advancing it would warn about a strap that was never asked; clearing it would hide a real refusal. Neither reading is evidence either way, so neither is acted on.The 2045 value is still unexplained, on purpose
The raw readback frame is now persisted beside the epoch (
alarm.lastReportedRaw) and printed asReadback frame:. That is what separates a genuinely stored stale alarm from a misdecode of a fixed response field — the distinction the decode's own comment names — and the capture built for it had already rolled out of the live log before the export was taken.Nothing here decides what 2045 was. The next export will carry the bytes that can.
Also
Strap clock: OKnow readsStrap clock: OK (from record timestamps, not the alarm readback), byte-identical on both platforms. It measures record timestamps and sat two lines above the 2045 line, which reads as the two contradicting each other.Verified
The verdict lives in
Packages/StrandAnalyticson the Swift side, so both twins are covered by default CI without app-build.--rerun-tasks --no-build-cache;AlarmReadbackTest7/7.sqlite3.h, unavailable in WSL, so it runs here in CI).Two things I got wrong while writing this, noted since the diff does not show them: I first used
activeDeviceIdonBLEManagerandstate.activeDeviceIdonFrameRouter, neither of which exists —LiveStatecarries no device identity at all.FrameRouteralready had the right idiom (per-connection properties set byBLEManager, likefamily), so that is used instead of changing itsinit, leaving all ten test construction sites untouched.