Repository navigation
ble: handle the pack-attach events on Apple too (#1826) - #1831
Conversation
Apple twin of @Zebsi235's Android change. BATTERY_PACK_CONNECTED(21) and BATTERY_PACK_REMOVED(22) are declared in the SHARED protocol schema, so both platforms already decode the names — neither was listening. On a 5/MG they fire on every attach and detach and lead the CHARGING_ON/OFF edges, so the charging pill responds when a pack goes on rather than waiting to catch a later edge. Measured on WHOOP MG fw 50.39.1.0 over two attach/detach cycles; a WHOOP 4.0 never sends them, so it is unaffected. NO REPLAY GUARD, AND THAT IS THE POINT. The Kotlin branch sits inside shouldApplyChargingFromBatteryEvent(replayedOffload) because Android's EVENT routing does see replayed offload events — the capture on the PR shows the strap re-sending these edges with byte-identical payloads, which would relight the pill minutes after the pack came off. This router is live-only: Backfiller holds no reference to it, so a replay never arrives here. That is the same reason the CHARGING_ON/OFF branch directly above carries no guard. Verified rather than taken from the comment. Copying the Android gate would have guarded against something that cannot happen here, and implied a hazard that does not exist on this path. Worth recording: `charging` is not only the pill on either platform. Apple feeds it to batteryPctAndCharging() for the low-battery offload interval, as Android feeds idleThrottleActive for connection priority and background capture. A strap on a pack now stops throttling sooner on both — the direction those gates intend, but behaviour outside the Live screen.
versionName/MARKETING_VERSION 11.1.0 -> 11.1.1, versionCode 418 -> 419, CURRENT_PROJECT_VERSION 299 -> 300. 418 was the 11.1.0 RELEASE, so 419 clears that high-water and the testing build can install over it. Carries everything merged since the release, none of which has reached a user yet: - the clock perf regression from #1822, where ~13 one-time DateFormatters became computed properties running an ICU pattern build and a Bundle lookup per access (#1830) - pack-attach events lighting the charging pill on a 5/MG, both platforms (#1826, #1831) - the Test Centre event census (#1825) - redaction for a strap serial that arrives as HEX inside a payload dump, both platforms (#1833). 11.1.0 can still print one through the alarm readback or the clock frame= dump, so this build is the first where "send me a strap log" is safe to ask again The 11.1.0 startup report is NOT known to be addressed by any of this. The clock fix is a real regression but was never established as its cause, and this build changes nothing about that question - it just stops shipping the regression.
|
The frames, from a capture taken just now. Both live — neither is a replayed offload record. WHOOP MG, hw How they decode, so the fixture can be checked rather than trusted:
Both 24 bytes, both matching the 5/MG geometry (inner record at The timestamps are real and deliberately not redacted. Zeroing those four bytes would invalidate the CRC, and a fixture with a hand-repaired CRC is precisely the synthetic frame you declined to write. They carry nothing else — no address, no serial, no ASCII of any kind. I checked that specifically, because it is not a foregone conclusion on this strap: the event-109 record in the same capture does carry the BT address and the pack serial in clear. 21 and 22 do not. One thing worth knowing about where these came from, since it explains why the census could not produce them. #1825 logs the decoded payload ( Same caveat as everything else I have sent on this strap: one device, one firmware. Nothing here says another strap or firmware frames these identically. I see this has merged already, so these are for the routing test whenever it is worth adding rather than anything blocking. |
Apple twin of @Zebsi235's Android change in #1826.
BATTERY_PACK_CONNECTED(21)andBATTERY_PACK_REMOVED(22)are declared in the shared protocolschema, so both platforms already decode the names. Neither was listening to them.
On a 5/MG they fire on every attach and detach and lead the
CHARGING_ON/CHARGING_OFFedges, sothe charging pill responds when a pack goes on rather than waiting to catch a later edge. Measured on
WHOOP MG fw 50.39.1.0 across two attach/detach cycles (capture on #1826). A WHOOP 4.0 never sends them.
Why there is no replay guard here
The Kotlin branch sits inside
shouldApplyChargingFromBatteryEvent(replayedOffload), and on Android thatgate is load-bearing: its capture shows the strap replaying these edges during the next historical
offload with byte-identical payloads, which would relight the pill minutes after the pack came off.
This router is live-only —
Backfillerholds no reference toFrameRouter, so a replayed event neverreaches this code. That is the same reason the
CHARGING_ON/CHARGING_OFFbranch immediately abovecarries no guard, and I verified it against the call graph rather than trusting the comment that says so.
Copying the Android gate would guard against something that cannot happen on this path, and would imply a
hazard that does not exist here. The asymmetry is deliberate and the diff says so.
It reaches the family it targets
Checked rather than assumed, because a handler that can never fire is the obvious way this goes wrong:
case "EVENT"is not family-gated, so 5/MG frames reach the branch.Whoop5EventTestsverifies puffin EVENT (type 48) decodes withtypeName == "EVENT"against realcaptured frames, so the routing path is real for this family.
"21": "BATTERY_PACK_CONNECTED"), andSchema.enumNameappends the raw value, so
hasPrefixmatchesBATTERY_PACK_CONNECTED(21).Scope note, on both platforms
chargingis not only the pill. Apple feeds it throughbatteryPctAndCharging()into the low-batteryoffload interval; Android feeds
idleThrottleActive, which gates GATT connection priority and pausingbackground capture. So a strap sitting on a pack now stops throttling sooner on both. That is the
direction those gates intend — "a charging strap never throttles" — but it is behaviour outside the Live
screen and worth stating rather than leaving to be discovered.
Tests — correcting my own first claim
I originally wrote that no test was feasible here. That was wrong:
StrandTestshas an end-to-end routerharness (
BatteryResultProvenanceDumpTestsbuilds aLiveState, drivesrouter.handle(frame:)andasserts on the resulting state), so a test that proves this branch flips
chargingis entirelywritable.
What is missing is a real captured 21/22 frame.
Whoop5EventTestsstates its own rule plainly — "allfixtures are real frames" — and I am not going to fabricate one with hand-computed CRCs to satisfy a
test, because a synthetic fixture proves the branch matches a string I invented rather than what the
strap sends.
@Zebsi235 — if your capture has the raw frame bytes for a
BATTERY_PACK_CONNECTED(21)and aBATTERY_PACK_REMOVED(22), those two hex strings would let this ship with a real routing test on bothplatforms. No rush, and the change stands without it.
Verification
Tools/doc_comment_lint.pyclean. App-target Swift, so the compile needs the app-build gate — not yetrun.
An honest caveat about what 21 asserts
BATTERY_PACK_CONNECTED(21)means a pack was attached.CHARGING_ON(7)means charging began. Thischange (and the merged Android one) treats the first as the second.
In the normal case that is purely a latency win — @Zebsi235's capture shows 7 trailing 21 by under a
second in one cycle and ~17s in another. The edge is a pack attached that never starts charging: a
depleted pack, a poor contact. Then 21 fires, 7 never does, and
chargingstays true untilBATTERY_PACK_REMOVED(22).Two consequences, and the second is the one that matters:
lowPowerThrottleActiveisthresholdPct > 0 && !charging && batteryPct <= thresholdPct, so a truechargingdisables the low-battery throttle. A strap at low battery on a dead pack would stopstretching its offload interval — the opposite of what that gate exists to do — for as long as the pack
stays attached.
I am not changing it here.
chargingis a single field serving both the pill and the throttle, soseparating "pack attached" from "actually charging" needs a second piece of state, and doing that on Apple
alone would put the platforms out of step on something the merged Android change already does.
Worth its own issue if it should be split. Flagged rather than left to be found in a battery report months
from now.