Repository navigation
charging: stop pack presence from overriding the strap gauge - #1945
Merged
Merged
Conversation
The doc on this flag said it came from BATTERY_LEVEL events. That is where its AUTHORITY comes from, but on a 5/MG it is also set by BATTERY_PACK_CONNECTED(21) and cleared by BATTERY_PACK_REMOVED(22), which the doc did not mention at all. 21 means a pack was attached, not that charging began, so on a depleted pack or a poor contact the flag reads true while nothing charges. That state is bounded, and the bound is the reason to document rather than split: it does not last until 22, because the next live BATTERY_LEVEL rewrites the flag from the strap's own gauge, roughly one battery cadence later. Splitting the state across both platforms, or making the throttle wait for a rising gauge, would cost every honest attach to close an edge that already closes itself. Two corrections to my own reading while writing this. The consequence is not one lever but three: idleThrottleActive gates the low-battery offload cadence, the GATT connection-priority throttle, AND the continuous-capture pause behind the user's own "Pause HRV capture" percentage. So a pack that never charges can keep background capture running at low battery after the user asked for it to stop. That is a larger surface than the offload cadence alone, and it is the part worth knowing. batteryPollDue reads the flag too, polling every tick rather than every other, which is harmless. The note lives on the flag, with a pointer from the throttle predicate on both platforms, because that predicate is where someone reasoning about a trend check would be reading and the reason not to add one is not visible from there. Comments only. Android suite green, doc-comment gate clean.
The bound is the next live BATTERY_LEVEL, but that rewrite is suppressed while an offload replays, so a long history sync holds a stale flag for its duration plus a cadence. Android gates it explicitly, iOS structurally.
The claim that a stale true lasts about one battery cadence is wrong on Android. Pack-info event 109 rewrites charging=true on pack PRESENCE every couple of minutes, which outruns the ~8 min BATTERY_LEVEL that would clear it, so a flat or badly seated pack holds the flag true until removal. iOS keeps the pack record log-only, so its bound is real. Also drops a doc link to lowPowerThrottleActive, which is an iOS-only symbol.
Event 109 says a pack is ATTACHED and how full it is, never that current is flowing, but it wrote charging=true on presence as an anti-staleness half for a missed attach edge. It repeats every couple of minutes, so it outran the ~8 min BATTERY_LEVEL that corrects the flag from the strap's own gauge: a flat or badly seated pack read as charging for its whole attachment instead of self-correcting, which also held off the three levers behind idleThrottleActive, the user's own capture pause included. It now publishes packSocPct alone, which is what the anti-staleness job wanted. BATTERY_PACK_CONNECTED(21) still lights the pill on the attach edge, so the latency win survives and the gauge gets the last word, which is what iOS already did. A missed attach edge now costs at most one battery cadence of lag and is then right. The rule both platforms follow: an EDGE may set the flag, a repeating presence signal may not. Pinned against the source, since the write lives in a GATT callback no JVM test can construct.
ryanbr
changed the title
This was referenced Sep 7, 2026
ryanbr
added a commit
that referenced
this pull request
Sep 7, 2026
Option 1 from #1948, and the investigation made it bigger than the issue said. ## What was happening The keep-alive polled battery commands the 5/MG send allowlist rejects before they leave the app, under a comment claiming the pack "rides the SAME cadence as the strap's own gauge". Android sent `GET_BATTERY_PACK_INFO`, which the allowlist admits only while a user-initiated probe is in flight: ``` 12:31:48 send(GET_BATTERY_PACK_INFO) skipped — no WHOOP 5/MG framing for this command yet 12:32:48 send(GET_BATTERY_PACK_INFO) skipped — no WHOOP 5/MG framing for this command yet ``` 40 of them in 40 minutes of one capture: one wasted call and one skip line per minute. **iOS was worse than #1948 recorded.** The allowlist has no clause for `.getBatteryLevel` either, so the whole block was dead rather than half of it: TWO sends and two skip lines per tick. The issue said "one refused command per platform"; that was wrong and is corrected there. A re-review then narrowed WHO pays it. `keepAliveMayRun` refuses an unbonded 5/MG outright, so the tick never fires there and only a BONDED 5/MG was paying. That correction strengthens the case rather than weakening it, and is why the comment now argues from co-residence: the same `didBond` that lets this tick run has already fired `enableLiveNotifications` a few lines above, which drives the `0x2A19` read a 5/MG's percent comes from. The read and the refused send were always on the same tick, so dropping the send cannot strand the reading. ## The change Android drops the pack poll from the WHOOP5 keep-alive branch. iOS makes that whole battery block WHOOP 4.0 only, which matches `refreshBattery`, already guarded `if selectedModel.deviceFamily == .whoop4` for the same command. Nothing is lost: - the strap's PERCENT comes from the `0x2A19` read both platforms already drive off this same keep-alive, on the same tick the removed send rode (iOS throttled to `whoop5BatteryReadMinIntervalSeconds`) - the PACK's charge comes from the pushed pack-info event (109), that flag's only writer since #1945 - Android's user-initiated probe path is untouched, and remains how the hardware question ("does a 5/MG answer 151 at all?") gets asked On iOS this leaves opcode 151 with no sender at all, because there is no iOS twin of that probe. That is not a loss: it restores exactly what `FrameRouter` already records for its own decoder, "nothing sent the command, so the decoder had no caller". Noted at the call site so the asymmetry is not rediscovered. The comments now say what actually happens instead of describing a cadence that never ran. ## Tests Both sends live in keep-alive bodies no unit test can drive, so both are pinned against the source. The Android guard is scoped to the WHOOP5 branch and also asserts the WHOOP4 poll beside it survives, since this is a 5/MG-only removal rather than a change to battery polling. Verified as a NEGATIVE CONTROL rather than assumed: re-adding the Android send fails the guard, restoring it passes. The Swift guard needed a fix a re-review caught before CI did: it asserted the block contains no `getBatteryPackInfo` without stripping comments, while the comment above it names that symbol to explain why it is not sent, so it would have failed on its own rationale. It judges the code only now, the rule the Kotlin twin already followed. The Swift guard normalises whitespace on both sides, because a Swift multiline literal strips indentation to its closing delimiter and so can never carry the source's own leading spaces — a raw comparison would have failed for a reason unrelated to the guard. Android suite 5521 green, doc-comment gate clean. The Swift guard DOES run on this PR: `app-build.yml` triggers on `Strand/**` and its macOS leg has a `Test Strand` step, so `StrandTests` executes in CI here. Worth flagging separately: that path filter lists `Strand/**`, `StrandiOS*/**`, `Packages/**` and `project.yml`, but NOT `StrandTests/**`. A PR touching only app-target tests therefore builds and runs nothing, and a broken test file would merge unverified to fail on the next PR that happens to touch `Strand/**`. This PR is covered because it edits `BLEManager.swift` too. ## Risk About as low as it gets: these are sends the allowlist already refused. The visible effect is less pointless traffic and quieter captures — an iOS Test Centre connection capture on a 5/MG was carrying roughly two wasted lines a minute.
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
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.
Started as option A from #1935, document the conflation rather than split the
state. Re-reviewing the doc before merging showed the argument that option A
rested on was wrong, so this now carries the fix as well.
What the re-review found
The doc claimed a spurious
charging = truewas bounded and self-healing: thenext
BATTERY_LEVELoverwrites it from the strap's own gauge, so at worst it iswrong for about one battery cadence. That is true on iOS. It was not true on
Android, because
BATTERY_LEVELwas not the only writer.The pushed pack-info event (109) also wrote
charging = true, keyed on packPRESENCE plus a plausible SoC.
displayableispresent && socPct in 0.0..100.0,so a flat pack qualifies, and the decoder's own hardware-confirmed note says the
strap volunteers 109 every couple of minutes while a pack is attached. That
outruns the ~8 min
BATTERY_LEVELthat would clear the flag. So the gauge nevergot the last word: a pack attached and not charging read as charging for its
whole attachment, not for one cadence.
That is not only a wrong pill.
idleThrottleActivereads the flag and gatesthree levers: the low-battery offload cadence, the GATT connection-priority
throttle, and the continuous-capture pause behind the user's own "Pause HRV
capture" percentage. A strap on a flat or badly seated pack kept background
capture running at low battery after the wearer asked for it to stop.
iOS was never affected. There the pack record is log-only and Test Centre gated,
so nothing but the gauge writes the flag.
The fix
109 now publishes
packSocPctalone. That is what the anti-staleness halfactually wanted, and the neighbouring doc already calls a non-null
packSocPctthe pack-attached signal.
BATTERY_PACK_CONNECTED(21)still sets the flag on the attach edge, so thelatency win that #1826 added survives, and
BATTERY_LEVELcan now correct it.A missed attach edge costs at most one battery cadence of lag and is then right,
rather than being fast and possibly wrong. Android ends up doing what iOS
already did.
The rule both platforms now state at the field: an EDGE may set this flag
(7, 21, 22), a repeating presence signal may not, or the gauge can no longer
correct it.
Tests
One new test, pinned against the source, because the write lives inline in a
GATT callback no JVM test can construct. It is anchored on the handler's own
info.displayable && soc != nullguard, which is unique in the file. Anchoringon the decode call or the event constant instead swallowed 400k characters and
passed on the pre-fix source, so the anchor is checked rather than assumed.
Verified as a negative control: re-adding
charging = trueto that one linefails the test, restoring it passes. Full Android suite 5504 green, doc-comment
gate clean.
Swift is doc-only, so CI is the check there.
Not included
The
chargingflag still means "attached or charging" on the 21 edge, which isthe original conflation #1935 raised. That stays documented rather than split:
21 leads
CHARGING_ON(7)by up to ~17 s in captures, and with the gauge able tocorrect the flag again, the window is genuinely one cadence.
One thing remains unmeasured, and it is noted in the code rather than glossed:
how often 109 repeats on a pack that is attached but flat. The fix does not
depend on it, since a repeating presence write is wrong regardless, but the
severity of what was there before does.