Make the idle→LOW_POWER throttle reachable so #477 can be validated - #1035
Open
ryanbr wants to merge 2 commits into
Open
Make the idle→LOW_POWER throttle reachable so #477 can be validated#1035ryanbr wants to merge 2 commits into
ryanbr wants to merge 2 commits into
Conversation
The risky half of #477 shipped dormant and stayed that way: AppViewModel passed a hard-coded 0 for idleThrottleBatteryPct, and 0 disables the gate, so no user or tester could arm it however they configured the app. Its own validation plan needs it running on a real strap, which nobody could do. Reads a pref instead. Default 0, so behaviour is unchanged for anyone who has not deliberately set it - and setConnectionPriorityManagement already restores BALANCED when the value returns to 0, so it is reversible without a reconnect. No Settings control on purpose. The throttle has two preconditions that would be invisible in a Settings row: it needs Fast history sync on as well, because refreshConnectionPriority early-returns without connection-priority management, and it keys on the STRAP's battery, so a healthy strap never trips it however low the phone is. Shipping a row that silently does nothing in both those cases would generate confusion rather than the field reports #477 wants. The value is clamped to 0 or 10..30 on read, failing CLOSED. With no UI the only way to set it is out-of-band on a debug build, which is exactly where a typo'd 95 would arm the throttle at essentially all times instead of at the low-battery edge; a link that keeps dropping is the worse failure. Does NOT address #1005, and should not be read as doing so: that is a phone battery report and this keys on the strap's. The lever a phone-drain user can already reach is the Power saving master, which stretches offload cadence from the phone's battery.
Re-reading my own change: AppViewModel.setIdleThrottleBatteryPct had no callers, and the reason I gave for it - 'so validating #477 does not require a relaunch' - was simply false, because nothing invokes it. Validation still means setting the pref out-of-band and relaunching, which applyPowerSaving already handles at launch. Speculative generality in a change whose whole brief was minimal. The NoopPrefs setter stays: it is where the WRITE clamp lives, so a later control has a correct path instead of reaching for edit().putInt() and bypassing it. 61 of the 69 keys in that file carry the same getter/setter pair, so it is also the local convention. Leaves seven executable lines: the key, a clamped getter, a clamped setter, the pure clamp, and the one call site that no longer passes a hard-coded 0.
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.
The minimal wiring discussed on #1005. Three files, no UI, no new strings.
Why
#477's risky half — drop an idle link to
LOW_POWER, the long interval that is the real all-day saving — shipped dormant and stayed that way.AppViewModel.applyPowerSavingpassed a hard-coded0, and0disables the gate, so nobody could arm it however they configured the app. Its own validation plan says it "must be validated on a real strap"; that was impossible.Same catch-22 #1027 broke for the offload: the thing could not be validated because it could not be reached.
What changes
A pref, clamped, read where the
0used to be. Default0, so behaviour is byte-identical for anyone who does not deliberately set it.setConnectionPriorityManagementalready restores BALANCED when the value returns to 0 — that edge was handled in #533 — so it is reversible without waiting for a reconnect.No Settings control, deliberately
The throttle has two preconditions that would be invisible in a Settings row:
refreshConnectionPriority()early-returns without connection-priority management.A row that silently does nothing in either case generates "this setting is broken" reports rather than the field data #477 wants. A Test Centre control is a small follow-up if validators would rather not use adb; it would call
NoopPrefs.setIdleThrottleBatteryPct(which applies the write clamp) and thenapplyPowerSaving(), with no BLE-layer change. I first added a passthrough onAppViewModelfor that and removed it on re-review — nothing called it, so the relaunch it claimed to avoid was not actually avoided.Fails closed
Clamped to
0or10..30on read. With no UI the only way to set it is out-of-band on a debug build, which is exactly where a typo'd95would arm the throttle at essentially all times rather than at the low-battery edge. Anything out of range reads as OFF, because a link that keeps dropping is the worse failure. Five tests cover it.What this does NOT do
It does not address #1005, and should not be read as doing so. That is a phone-battery report; this keys on the strap's. The lever a phone-drain user can already reach is the Power saving master, which stretches offload cadence from the phone's battery — a point I missed on that issue and have corrected in #1034.