Fix: Reconnect gesture stopped working after a mistimed replug - #41
Merged
Conversation
…d replug QuickFullChargeGesture latched an arming basis, bridged the unplugged gap with it, and then discarded it at every replug edge, re-deriving the basis from the instantaneous replug reading. A physically replugged phone cannot report a settled limit hold at that instant — it reads CHARGING while it tops back up — so a replug that missed the 2-10s window disarmed the gesture. The next unplug edge then had no basis to open a window with, and every retry was inert. The two mutable fields (armedBy + disconnectedAtMillis) are replaced by a single sealed state — Idle / Armed(basis) / AwaitingReconnect(basis, since) — so the basis is structurally tied to the window it belongs to. A too-fast replug now returns to Armed with the carried basis; a too-late replug still re-derives from the fresh reading, and every other transition keeps today's behaviour, including any-level revocation (which runs before the plug edges, so a revoked basis is never carried over). No new memory concept or timeout constant is needed: basis lifetime equals window lifetime, so staleness stays bounded. The two test inputs that reported NOT_CHARGING + charging state 4 at a replug instant are corrected to the realistic CHARGING reading — those unphysical inputs are why the defect shipped green.
…ersistent policy The dashboard's policy buttons wrote through ChargingRepository directly and only nudged ACTION_MONITOR, which re-evaluates whether to keep monitoring but never touches QuickFullChargeGesture — so a persistent-policy change could leave a latched arming basis or an open reconnect window in place. They now send ACTION_SET_PERSISTENT_POLICY, the same serialized command the widget's ∞80% / ∞100% buttons use, which resets the engine after the write. The service path is a strict superset of what the ViewModel did: it refuses when no backend can write (canApply), persists the recovery target before the risky write, cancels a running session without restoring, suppresses its own settings-observer trip, and force-writes via reapplyPersistent so a same-value write still re-triggers the HAL. On success it clears the pending interruption warning (InterruptionAssessor.onExplicitPolicyWrite → InterruptionStore .clearPending, what the ViewModel did inline) and cancels the recovery notification; on failure it posts one, which the ViewModel did not. Second stale-state path: a tick that finds the gesture disabled or unsupported while a watcher keeps the service alive now resets the engine too. Previously only stopMonitoring() reset it, so a disable/re-enable cycle on a service kept alive by the charge alarm resumed on pre-disable gesture state. ChargeSessionManager is no longer a dashboard dependency.
The KDoc claimed the arming basis lifetime equals the reconnect window lifetime, so staleness was "bounded structurally". It is not: the fast-replug branch returns Armed(basis), which carries no timestamp, and the steady-plugged branch never clears a non-Idle state, so a carried basis persists for the rest of the plugged period. Describe what the code does instead — a rejected sub-minReconnectMillis gap is a non-event, the basis falls back to the ordinary plugged-period latch and is retired only by reset(), a trigger, or the out-of-band check. Fixes review finding F2
A carried arming basis is deliberately not bounded by the reconnect window it came from: a rejected sub-2s replug is a non-event, so a deliberate retry minutes later still opens a fresh window and triggers. A carry deadline anchored to the original unplug would silently reinstate the reported bug, so the behaviour is now pinned by an explicit test rather than left implicit. Fixes review finding F3
The steady-plugged branch has never dropped a latched arming basis, so a limit removed in system settings left the gesture armed while the battery climbed past the expected limit range — a later unplug/replug could then start a full-charge session on evidence that no longer held. A readable percent outside the arming band now retires a limit-hold basis. The check runs before the plug edges, so it also cancels an already-open reconnect window. It is deliberately narrow: an any-level basis is percent-independent by design and is left alone, and an unreadable percent (< 0) retires nothing, so one failed sticky-broadcast read cannot disarm a healthy gesture. Fixes review finding F4
…ment set The lifetime sentence listed the out-of-band check, reset, and trigger consumption as the only ways a carried arming basis is retired. Reconnect window expiry, a replug that re-derives to nothing, and the any-level revocation retire it too, so the list was not exhaustive. Describe the retirement paths without asserting closure, keeping the load-bearing point: the basis carries no timestamp, so a retry is deliberately not bounded by the original reconnect window. Fixes review finding F5.
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.
What changed
The reconnect gesture could get stuck. If you unplugged and replugged slightly outside the 2–10 second window, the gesture silently lost its arming — and because a reconnect window only opens while the gesture is armed, every further attempt did nothing. It stayed dead until the charge limit settled back in on its own, which is why repeated tries felt like the feature was broken. Turning on "any charge level" hid the problem, so that looked like the only way to make the gesture work at all.
A mistimed attempt now keeps the arming, so retrying works.
Two further changes:
Technical Context
Root cause.
QuickFullChargeGesturelatched the arming basis but discarded it on every replug edge and re-derived it from the instantaneous reading. A physically replugged phone at 80% reportsBATTERY_STATUS_CHARGINGwhile it tops back up, so a limit-hold basis can never be re-derived at that instant. The unplug edge only opens a reconnect window when a basis is latched, which is what produced the dead-gesture state. The any-level basis needs onlyplugged && PROTECTIVE— both true at that same tick — which is why enabling it masked the bug.Why it was never caught.
QuickFullChargeGestureTestfed a settled-hold reading (NOT_CHARGING+ charging state 4) as the replug tick, which is physically impossible. The project'sdumpsys battery unplugrecipe freezes reported state and reproduces the same impossible reading, so the defect was invisible to both unit tests and simulated device runs, and surfaced only with a real cable.Approach.
armedBy+disconnectedAtMillisare replaced by a sealedState(Idle/Armed(basis)/AwaitingReconnect(basis, sinceMillis)). Every prior transition is preserved; the only behavioural change is that a replug shorter thanminReconnectMillisreturns toArmedwith the carried basis instead of re-deriving. No existing test assertion changed.Deliberate non-bound. The carried basis intentionally holds no timestamp. A bounded variant — expiring it 10 s after the original unplug — was considered and rejected: a human retry after a failed attempt is frequently slower than 10 s, so that bound would re-break the reported bug for exactly the people who hit it. The test
a retry long after the original unplug still triggerspins this so a future change cannot quietly reintroduce it.Out-of-band retirement. A
percentoutside 75–90 now retires aLIMIT_HOLDbasis. This addresses a pre-existing gap, not this bug — the steady-plugged branch has never dropped a latched basis, so a limit removed in system settings left the gesture armed as the battery climbed. It deliberately does not touch an any-level basis (percent-independent by design) and ignorespercent < 0, so a single failed sticky-broadcast read cannot disarm a healthy gesture. It runs before the plug edges, so it also cancels an already-open window.Dashboard policy path.
DashboardViewModel.applyPolicynow sendsACTION_SET_PERSISTENT_POLICYinstead of callingrepository.applyPersistentdirectly. Beyond resetting the gesture engine, this gains thecanApplyrefusal guard, a recovery target persisted before the write, settings-observer suppression,reapplyPersistent's forced re-write, and failure surfacing.ChargeSessionManagerbecame unused in the ViewModel and was dropped from its constructor.Review guidance. The state-machine rewrite is the risk surface. Worth checking transition equivalence against the previous
armedBy/disconnectedAtMillispair — in particular the any-level revocation guard (disconnectedAtMillis == nullbecamestate !is AwaitingReconnect) and thestatusOutputmapping.Verification. On a Pixel 8 holding at its 80% limit: after a rejected sub-2 s blip the notification stays on the armed copy (the previous build fell back to the idle copy), the retry then starts a session, and disconnecting restores the limit. Forcing level 95 confirmed the out-of-band retirement, and it re-armed on reset. Unit suite: 1574 tests green across both flavours.