Report what each offload burst actually achieved (#1007) - #1027
Merged
Conversation
#477 (CONNECTION_PRIORITY_HIGH for the burst) and #533 (LE 2M around it) both ship OFF, gated on a real-strap before/after that has never been done. The reason is visible in the code: nothing measures the thing they change. The onPhyUpdate comment already talks about reading "the negotiated PHY next to the offload's records/sec" - but no records/sec is computed anywhere, which is why #1007's figures had to be counted by hand out of a raw capture. exitBackfilling now logs the burst's frame count, elapsed and achieved rate beside its exit reason, so validating those levers is toggle-sync-compare rather than frame-counting in a capture. Unconditional, one line per offload rather than per frame: behind the test-mode gate a reporter would have to find Test Centre first, and that friction is part of why the levers are still unvalidated. Frames, not records: offloadFramesThisSession counts genuine offload frames (47/48/49/50), which is what the levers move and the same numerator #1007 used. A non-positive elapsed reports the count with NO rate rather than a fabricated one. Locale-fixed so a decimal comma can't follow the phone language into a pasted log. Diagnostic only - no BLE op, no timing, no default changed. Deliberately not flipping either lever's default: "a shorter interval is strictly safer" is a plausible argument, not a measurement, and this is the change that lets it become one.
exitBackfilling has exactly three call sites - HISTORY_COMPLETE, timeout and user-abort. A burst interrupted by a disconnect goes through reset() instead, which clears backfilling and zeroes the frame counter without passing through any of them, so it emitted no throughput line at all. That absence is ambiguous in the worst way for the question this measurement exists to answer: no offload at all, and an offload that was cut off, look identical in the log - and for battery they are opposite findings, since the interrupted one spent radio and banked nothing. Logged in reset() rather than by calling exitBackfilling there: that method also releases the connection priority and the PHY and records a sync outcome, none of which the teardown path does today. This stays diagnostic. Guarded on backfilling, so a completed burst that already logged on its way through exitBackfilling cannot log twice.
The field's doc said it was "only read inside exitBackfilling" - true when it was written, false one commit later when the interrupted-burst case added a read in reset(). Same class of error as #1023's field docs: an invariant stated at the point it happened to hold rather than one anything maintains. Now says what is actually true: stamped by enterBackfilling, never cleared, so it holds the previous burst's start between bursts - which is safe only because both readers pair it with `backfilling`, and that is false until enterBackfilling re-stamps. Deliberately makes NO threading claim. reset() is reached from handleDisconnect, which runs from the GATT callback rather than a handler.post, so "main-looper only" is not something this change verified - and asserting it while fixing an unverified assertion would repeat the mistake. It states only that the two readers are the paths that already own offloadFramesThisSession.
This was referenced Aug 2, 2026
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.
Diagnostic only, from #1007. No BLE op, no timing change, no default flipped.
The gap
History offload is the dominant battery cost — #1007 measured ~2.4 h of radio a day on a WHOOP 4.0 and ~41 min on a 5/MG, against the official app doing 24 h in 1–3 minutes.
Two levers that attack exactly that are already written and tested:
CONNECTION_PRIORITY_HIGHfor the bounded offload burst. HIGH is a shorter interval, so it cannot cause a supervision-timeout drop; it shortens the radio-on window.Both ship off, gated on a real-strap before/after. That validation has never happened, and the reason is visible in the code: nothing measures the thing they change.
onPhyUpdate's own comment already talks about a log showing "the negotiated PHY next to the offload's records/sec" — but no records/sec is computed anywhere. #1007's figures had to be counted by hand out of a raw capture (3193 frames / 90 s), which is not something a reporter can be asked to do twice.What changes
exitBackfillinglogs what the burst achieved, beside the exit reason it already classifies:Validating #477 or #533 becomes toggle → sync → compare two lines.
Unconditional, and one line per offload rather than per frame. Behind
TestDomain.CONNECTIONa reporter would have to find Test Centre first — that friction is part of why these levers are still unvalidated after two issues. One line per burst costs nothing.Frames, not records.
offloadFramesThisSessioncounts genuine offload frames (47/48/49/50) — what the levers actually move, and the same numerator #1007 measured, so the new line is directly comparable to the figures already in that issue.A non-positive elapsed reports the count with no rate, never a fabricated one.
Deliberately not included
Neither default is flipped. "A shorter interval is strictly safer" is a plausible argument, not a measurement — and this is BLE code with hardware-learned hazards (#85/#50, #241). This PR is what lets that argument become a measurement; flipping on the strength of the argument alone is the mistake.
Android only. The levers being validated have no CoreBluetooth equivalent — iOS cannot set connection priority at all, which #477 records as a deliberate platform divergence rather than a parity gap. Adding an unused metric to app-target Swift that CI does not compile would be surface without a purpose. Easy follow-up if the throughput figure is wanted on iOS as a general diagnostic.
Re-review: the first cut missed the interrupted burst
exitBackfillinghas exactly three call sites —HISTORY_COMPLETE,timeoutandaborted by user. A burst cut short by a disconnect goes throughreset()instead, which clearsbackfillingand zeroes the frame counter without passing through any of them. So the metric silently skipped it.That absence is ambiguous in exactly the wrong way for a battery question: "no offload happened" and "an offload was cut off" looked identical in the log, and they are opposite findings — the interrupted one spent radio and banked nothing.
Now logged from
reset()as(interrupted). Deliberately not by callingexitBackfillingthere: that method also releases the connection priority and the PHY and records a sync outcome, none of which the teardown path does today, so routing through it would turn a diagnostic into a behaviour change. Guarded onbackfilling, so a burst that already logged on its way throughexitBackfillingcannot log twice.The one remaining bypass is deliberate:
onBackfillTimeout's 5/MG zero-frame retry clearsbackfillingdirectly, but it already logsno history frames arrived — retrying request, so a0 frame(s)line beside it would only repeat what that says.Verification
Tools/doc_comment_lint.pyandTools/i18n_audit.py --ci mainclean.backfillStartedAtMsis its own field rather than a reuse oflastBackfillAtMs, which is theBackfillPolicyfloor and measures from the last kick.exitBackfillingearly-returns unlessbackfilling, which onlyenterBackfillingsets — and that is where the stamp is taken, so the elapsed cannot span two bursts.