Repository navigation
ble: bound the 2M PHY to the offload burst; rename the shared edge rule - #538
Merged
Merged
Conversation
Two follow-ups on #536/#537, found re-reviewing them after merge. A. The PHY was never released at offload END, unlike its twin lever. refreshConnectionPriority() runs at BOTH enterBackfilling and exitBackfilling, so the priority escalation is bounded to the burst. applyPreferredPhy() ran only at enterBackfilling - and a PHY PERSISTS once negotiated, so a link that went 2M for a sync stayed 2M for the WHOLE connection, including the overnight window. That is not what the toggle's copy promises (it says "while your strap hands over its stored history"), and it left 2M's range trade-off in force long after the transfer it was for. Release to 1M at exitBackfilling so the preference is bounded to the burst, symmetric with the priority lever, and the shipped copy becomes accurate rather than aspirational. Battery was NOT the reason: 2M costs LESS energy per byte, so leaving it on was arguably fine. The reason is that RANGE is 2M's unvalidated trade-off, and an experimental lever should confine its unknown to the window the user was told about. If field data later shows 2M is good everywhere, widen it deliberately. The guard lives at the call site, not inside releasePreferredPhy: that method cannot check the flag, because setFastLinkPhy's on->off edge calls it AFTER the flag is already false. Guarding at the call site keeps the default path at ZERO BLE ops. B. releasesConnectionPriority was a misnomer the moment #537 reused it for the PHY: one predicate, two levers, a name that claimed one. Renamed to releasesOnDisable and its doc now says it serves both. My own mess from the reuse. compileFullDebugKotlin + the full com.noop.ble suite green (PreferredPhyTest 6, ConnectionPriorityTest 11, GattCrashSafetyTest 7); i18n audit OK. Still not validated on hardware - both toggles remain default-off.
ryanbr
added a commit
that referenced
this pull request
Jul 17, 2026
Numbers are main's current 292/203 +1, matching the previous staging commit's pattern, so the .staging app upgrades cleanly over the last testing build (290) and stays above main's released high-water. Carries everything merged since the 9.0.1 release: - Workouts read HR under the recording strap, not a hardcoded id (#512 Kotlin, #513 Swift twin) - Oura 0x60/0x80 IBI decoders corrected to the ring's real byte layout (#511) - Whoop5Config enable_sig12 corrected to ASCII '1' (#522) - Sleep time edits save as one validated window (#525) - "Recompute this night" for deleted sleep windows (#526) - Stress marker values stay readable (#507) - Faster history sync + faster Bluetooth link, both EXPERIMENTAL and default-off (#536/#537/#538) - the whole point of this build is field data on those - i18n changelog-title fix + main-push audit (#514/#535) versionName stays 9.0.2 here for the staging identity only; the real 9.0.2 release still bumps main separately (and must be raised past 293 first).
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
…le (ryanbr#538) Two follow-ups on ryanbr#536/ryanbr#537, found re-reviewing them after merge. A. The PHY was never released at offload END, unlike its twin lever. refreshConnectionPriority() runs at BOTH enterBackfilling and exitBackfilling, so the priority escalation is bounded to the burst. applyPreferredPhy() ran only at enterBackfilling - and a PHY PERSISTS once negotiated, so a link that went 2M for a sync stayed 2M for the WHOLE connection, including the overnight window. That is not what the toggle's copy promises (it says "while your strap hands over its stored history"), and it left 2M's range trade-off in force long after the transfer it was for. Release to 1M at exitBackfilling so the preference is bounded to the burst, symmetric with the priority lever, and the shipped copy becomes accurate rather than aspirational. Battery was NOT the reason: 2M costs LESS energy per byte, so leaving it on was arguably fine. The reason is that RANGE is 2M's unvalidated trade-off, and an experimental lever should confine its unknown to the window the user was told about. If field data later shows 2M is good everywhere, widen it deliberately. The guard lives at the call site, not inside releasePreferredPhy: that method cannot check the flag, because setFastLinkPhy's on->off edge calls it AFTER the flag is already false. Guarding at the call site keeps the default path at ZERO BLE ops. B. releasesConnectionPriority was a misnomer the moment ryanbr#537 reused it for the PHY: one predicate, two levers, a name that claimed one. Renamed to releasesOnDisable and its doc now says it serves both. My own mess from the reuse. compileFullDebugKotlin + the full com.noop.ble suite green (PreferredPhyTest 6, ConnectionPriorityTest 11, GattCrashSafetyTest 7); i18n audit OK. Still not validated on hardware - both toggles remain default-off.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…le (ryanbr#538) Two follow-ups on ryanbr#536/ryanbr#537, found re-reviewing them after merge. A. The PHY was never released at offload END, unlike its twin lever. refreshConnectionPriority() runs at BOTH enterBackfilling and exitBackfilling, so the priority escalation is bounded to the burst. applyPreferredPhy() ran only at enterBackfilling - and a PHY PERSISTS once negotiated, so a link that went 2M for a sync stayed 2M for the WHOLE connection, including the overnight window. That is not what the toggle's copy promises (it says "while your strap hands over its stored history"), and it left 2M's range trade-off in force long after the transfer it was for. Release to 1M at exitBackfilling so the preference is bounded to the burst, symmetric with the priority lever, and the shipped copy becomes accurate rather than aspirational. Battery was NOT the reason: 2M costs LESS energy per byte, so leaving it on was arguably fine. The reason is that RANGE is 2M's unvalidated trade-off, and an experimental lever should confine its unknown to the window the user was told about. If field data later shows 2M is good everywhere, widen it deliberately. The guard lives at the call site, not inside releasePreferredPhy: that method cannot check the flag, because setFastLinkPhy's on->off edge calls it AFTER the flag is already false. Guarding at the call site keeps the default path at ZERO BLE ops. B. releasesConnectionPriority was a misnomer the moment ryanbr#537 reused it for the PHY: one predicate, two levers, a name that claimed one. Renamed to releasesOnDisable and its doc now says it serves both. My own mess from the reuse. compileFullDebugKotlin + the full com.noop.ble suite green (PreferredPhyTest 6, ConnectionPriorityTest 11, GattCrashSafetyTest 7); i18n audit OK. Still not validated on hardware - both toggles remain default-off.
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.
Two follow-ups on #536/#537, found re-reviewing them after merge.
A. The PHY was never released at offload end (asymmetric with its twin)
The priority escalation is bounded to the burst; the PHY wasn't. A PHY persists once negotiated, so a link that went 2M for a sync stayed 2M for the whole connection — including the overnight window.
That contradicts the toggle's own shipped copy, which says it applies "while your strap hands over its stored history".
Now released to 1M at
exitBackfilling, so the preference is bounded to the burst, symmetric with the priority lever, and the copy becomes accurate rather than aspirational.Battery was not the reason. 2M costs less energy per byte, so leaving it on was arguably fine — even a small win on the 1 Hz overnight stream. The reason is that range is 2M's unvalidated trade-off, and an experimental lever should confine its unknown to the window the user was told about. If field data later shows 2M is good everywhere, widen it deliberately rather than by omission.
Why the guard is at the call site:
releasePreferredPhy()can't checkfastLinkPhyEnableditself, becausesetFastLinkPhy's on->off edge calls it after the flag is already false. Guarding at the call site keeps the default path at zero BLE ops.B.
releasesConnectionPrioritywas a misnomerThe moment #537 reused it for the PHY (
setFastLinkPhy), it became one predicate serving two levers with a name claiming one — asserted from both test files too. Renamed toreleasesOnDisable, doc updated to say it serves both. My own mess from the reuse.Verification
compileFullDebugKotlin+ fullcom.noop.blesuite green —PreferredPhyTest6/6,ConnectionPriorityTest11/11,GattCrashSafetyTest7/7.Not in this PR
escalateForLiveHris still never settrue, so the resolver's live-HR branch stays unreachable in production. That's a YAGNI-vs-keep-#477's-capability call, not a defect — left for a deliberate decision rather than reversed unilaterally.