Charging: Check whether your phone's charge limit actually works - #89
Merged
Conversation
Amply's contribution wizard captures which setting key changes across OEM modes, a mapping. Whether the charging hardware obeys that setting is a different measurement, and until now an entirely manual one: adb, settings put in both directions, dumpsys battery sampled by hand, negotiated over days of email. Two of the eight qualified devices came from contributors willing to run that protocol repeatedly on request. This lands the core of an in-app version. EnforcementVerdictEngine's KDoc already specified it and said it was deliberately not implemented: write a cap below the current level and watch charging cut, raise it and watch it resume, cut again, a sequence no thermal pause can imitate. QualificationRunEngine is pure and JVM-testable, and emits commands rather than only reacting, since it drives the writes. Two shapes fall out of the adapter's own supportedPolicies: a variable-cap run (LineageOS 70-95, Samsung One UI 8 80/85/90/95) caps below the current level and releases by raising the cap, so the battery is never left uncapped; a fixed-cap run charges up to the cap first and releases with Unrestricted. Three properties are load-bearing: A phase timeout is INCONCLUSIVE, never REFUTED. Refutation stays reachable only from an observed climb past the cap, the same signal and the same OVERSHOOT_ALLOWANCE the passive engine uses, so a cold room or a weak charger can never permanently disable control on a working device. A run may not refute a mapping it guessed. On a candidate adapter the commanded value is an assumption: a One UI 6/7 device whose protect_battery means "cap at 85" charges straight past a commanded 80 while enforcing perfectly, and recording that as a refutation would lock out a device that works. It reports CAP_MISMATCH with the observed hold level instead. Flow is measured by charge counter, not percent, against thresholds expressed as a fraction of implied full capacity. MagicOS reports a synthetic level 100 with ~278 mAh of headroom, and a milli-reporting ROM scales counter and implied capacity together, so the ratio survives it. The positive verdict lives in its own record rather than as a constant in EnforcementVerdict. Adding one there would be fail-open: every field has a default, so a record that lost algorithmVersion decodes as version 0, skips the version-1 migration branch, and would deserialize into a claim of enforcement on a build that never earned one. Bumping ALGORITHM_VERSION would be worse, silently un-refuting every already-refuted device. The new record's fail-closed guard is an explicit protocolVersion, because there is no safe default for a one-constant positive enum. EnforcementStatus gains SELF_QUALIFIED rather than reusing CONFIRMED, which means a maintainer physically qualified the device and earned a ledger row. Refutation still outranks it. applyForQualification is ungated for the same reason restorePersistent is, but requires a token matching a live run record, so the path does not exist without a run in flight. Its writes are non-persistent, leaving the user's protective baseline and the reconnect gesture's arming basis untouched. The restore is registered as a FullChargeStore recovery target before the first write, so process death and reboot are covered by shipped, tested recovery rather than new machinery. Behind ENABLE_QUALIFICATION_RUN, debug and beta only.
A cross-model review of the previous commit found nine defects, several of them able to produce exactly the two outcomes the feature exists to avoid: claiming a cap works when it does not, and permanently disabling a device whose cap does work. This addresses all of them. The measurement is rebuilt around a within-run control. A new BASELINE phase lifts the cap and measures how fast charge actually goes into this battery from this charger, and every later phase is judged against that number: a cut requires the rate to fall tenfold below it, a resume requires a third of it back. The previous design confirmed a cut from elapsed time without a fixed-size rise, which meant a phone charging steadily at 100 mA under a cap that does nothing sat under the bar forever and passed. That bar was justified in the code as "an order of magnitude below any real charge current"; the reasoning was backwards, and no absolute threshold can be right for an unknown battery, charger and unit convention at once. A baseline too slow to measure against now ends NO_BASELINE rather than proceeding. Refutation is terminal, so it now needs three things instead of one: the level must exceed where the phase's window opened, the measured rate must agree that charge is going in, and the commanded write must have had time to land. Previously a single point of gauge drift refuted — and on a variable-cap run, which deliberately starts above its cap, the first such point always did. The near-full guard is also checked before the climb, so a battery that simply tops off is inconclusive rather than refuting. The runner no longer guesses. It refuses to start when the current policy is unreadable, rather than substituting the adapter's protective default and then persistently restoring that guess over an unrecognized native mode. It refuses while a charge rule owns the policy, instead of suspending the rule's cohort and discarding the baseline that rule still owed back. On a native change or a full-charge session starting mid-run it deliberately does not restore, because the newer choice is the user's. The shared recovery slot gets an owner-scoped clear (clearPendingRecoveryTargetIfOwnedBy): a widget or tile write can store a newer target while a run is finishing, and clearing unconditionally dropped that obligation with nothing left to repay it. A pass now licenses only the policies the run exercised, via AdapterSupport.licensedPolicies, enforced at the write path as well as the display. The evidence record's own KDoc promised this; the tier ignored it and unlocked the whole adapter. The evidence store gains a credibility check. protocolVersion alone was not fail-closed, because the outcome enum has exactly one constant and it is the positive one: a record carrying only a matching build and protocol version decoded as a pass with no adapter, cap, signal or policies. runActive is stamped onto each battery tick at capture time rather than read when the passive recorder gets round to it, so ticks taken during a run cannot be evaluated after it finished and combine with the restored cap into a terminal refutation of charging the run itself commanded. Two smaller fixes: the tick loop merges into the stored record inside one transaction, so a concurrent cancel is not overwritten and lost; and the run start nudges the charge service, which nothing did before — a run's record existed but no ticks ever reached it. Corrects the previous commit message and privileged-access.md on one point: avoiding a positive constant in EnforcementVerdict was justified there as strictly fail-open, which was overstated. A record missing algorithmVersion decodes as 0 and is scoped out anyway. The separation stands on the two records describing different things.
A rule that activates while a guided run is in flight captures the run's temporary policy as the baseline it owes the user back, and writes that back when it stops matching. The user's real charge setting is then gone with nothing left that remembers it, and the run reads the rule's write as CONFIGURATION_DRIFT and deliberately does not restore either. ruleOwnsPolicy() was only consulted at eligibility time, which covers a rule that is already active when a run starts but not one that starts matching mid-run. The guard therefore sits in RuleApplier.evaluate, the single entry point every evaluation goes through: guarding ChargeSessionService.evaluateBattery instead would miss ACTION_EVALUATE_RULES, which calls evaluateRules straight from the command path, so a Bluetooth connection or a rule edit could still activate.
licensedPolicies() returned null both when there was no evidence at all and when evidence existed but none of its exercised policy ids parsed. Null means "no restriction" downstream, so the second case handed the SELF_QUALIFIED tier the adapter's entire policy list — the opposite of what a pass claims, and reachable without any protocol-version bump: an app update that changes a stable-id format is enough. The two cases are now distinct. Null is only "no evidence"; evidence that resolves to nothing is an empty list, and resolveEnforcement drops the tier on an empty licence and falls through to the opt-in or candidate branch instead of widening.
…p of the policy A second review of the guided run found several ways it could still reach one of the two outcomes it exists to prevent: claiming a cap works when it does not, or permanently disabling a device whose cap does work. The measurement window no longer starts before there is anything to measure. enterPhase used to schedule the window at write time and take its starting readings from that same sample, so every rate divided charge accumulated over (settle + window) by (window). A working cap that takes a minute to engage gains a point during the settle period, and five minutes later the stale starting percent plus that accumulation reads as charging past the cap — a terminal refutation of a device that works. The scheduled opening and the anchor are now separate: the window opens on the first sample at or after its eligible time, and takes its readings and its clock from that sample. Rates, hold confirmation and phase budgets all measure from the anchor, so a late first sample cannot make an active flow look cut by counting unobserved time, and no phase completes, refutes or times out before its window anchors. That eligible time is derived from an apply-ack the runner persists after a write succeeds, not from when the engine emitted the command: the write in between can take seconds, and the configured-state drift check keys on the same instant for the same reason — a readback taken while the write is still in flight legitimately reports the previous phase's policy, and aborting there ends the run on CONFIGURATION_DRIFT, which deliberately does not restore. Samples older than the phase's ack, or than the run itself, move no state at all. Positioning is split from measurement. A fixed-cap run gets its CHARGE_UP phase back, so BASELINE is a bounded ten-minute control in both shapes rather than doubling as a charge-up up to twenty-four times longer. On top of the rate floor, leaving BASELINE now requires MIN_BASELINE_UPDATES observed changes of the accumulation signal: a device whose level or batched charge counter updates once every twenty minutes otherwise stages a textbook cut -> resume -> cut out of nothing but when it reported, with the cap doing nothing. Three updates inside ten minutes put the reporting period under five, which is what makes a twelve-minute quiet window evidence. Anything coarser ends SIGNAL_TOO_COARSE. Each tick's readings now come from one battery snapshot taken on the runner's own worker and timestamped there, so nowMillis, percent and chargeCounter describe the same instant. The tick from the watcher only signals "evaluate now": BATTERY_PROPERTY_CHARGE_COUNTER is a live property rather than a broadcast extra, so building it under the service's dispatch lock would both pair a broadcast-time level with a processing-time counter and put a Binder call somewhere the service keeps free for policy recovery. The closing sample's readings are also passed into merge() explicitly, or the phase log records blanks now that the next phase's anchors start empty. Run start moves onto the charge service's command queue (ACTION_QUALIFICATION_START). Run start and full-charge session start both claim the charge policy, and that queue is the one place they are serialized against each other, so the two can no longer each observe it free. The session side returns from the whole ACTION_START branch while a run is live rather than only skipping beginOrResume: falling through reaches the pending-recovery arm, which would find the recovery target the run registered before its first write and start repaying it while the run keeps writing. Because a session can no longer take ownership mid-run, SESSION_STARTED goes back to restoring the baseline on abort. The terminal path claims the record for finalization in one transaction and decides the outcome from what that claim saw. Finalizing restores the policy and only then records evidence, which is long enough for a cancel to land in the middle; previously it read the record before evaluation and could persist a pass and clear a record the user had cancelled. A cancel that wins the claim downgrades the outcome, one that arrives after it does not commit, and a downgrade never turns an abort into a pass. runActiveNow starts true and is only resolved by startupRepair, which is serialized with run start. The two errors are not symmetric: a wrong true costs the passive recorder a few seconds of observation, a wrong false lets it read charging the run itself commanded as charging past a cap. Whether a run's commanded values are a guess now travels on RunEligibility.Eligible instead of being a literal at the start call site, so the cap-mismatch protection that stops a guessed mapping from producing a terminal refutation has one source. No path produces a candidate run yet; candidate selection is a follow-up, and this is the plumbing that keeps it from re-introducing the literal.
The run's algorithm changed materially after PROTOCOL_VERSION 1: the within-run baseline control, the coarse-signal refusal and the rule that a pass licensing nothing is no pass all landed after it. A stored version-1 pass nevertheless matched QualificationEvidenceStore.scope(), so a result the current algorithm would never produce kept telling the user an inert cap protects their battery. PROTOCOL_VERSION is now 2, which drops those records rather than migrating them: there is no safe reinterpretation of a positive verdict measured by an algorithm that could not tell a weak supply from a cap. Fixes review finding F1.
…ching it claimForFinalization() persisted finalizing = true and nothing ever gave it back. A process death between the claim and the clear at the end of finalization — a window that spans a policy write, slow on a Shizuku adapter — or any throw inside finalize() made the run record permanent: startup repair and every later terminal tick found it already claimed and returned. The run then read as running forever, so charge rules never evaluated again, full-charge sessions were refused, passive enforcement stayed suppressed, the foreground service was kept alive, and the policy the run owed the user was never restored. Only clearing app data got out. claimForFinalization() takes an optional runId that claims a record even when it is already claimed, which startup repair passes for a record left behind by another process — the existing provenance filter is what keeps it from stealing a finalization still in flight here. finish() now wraps finalize() and, on a throw, transactionally clears the flag when the stored runId still matches, so the release can neither resurrect a record that was already cleared nor unclaim a newer run. Fixes review finding F2.
Every path inside the baseline phase's BASELINE_WINDOW_MILLIS check returns, so the PHASE_BUDGET_MILLIS branch below it could never run. It implied a 25-minute escape the phase does not have; the bounded ten minute window is itself the budget, and a device that never produces a usable control ends NO_BASELINE (or SIGNAL_TOO_COARSE) there. The KDoc now says so. Fixes review finding F3.
…n and process death finish() only released its finalization claim for an ordinary exception. A cancellation - the production path is the charge service's own lifecycle scope, which onDestroy cancels across a window that spans the restore write - left the record claimed and finalizing forever: its provenance still matches this process, so startup repair will not reclaim it, and every ordinary terminal attempt is refused. The release now sits in a finally and runs NonCancellable; on the success path the run-id guard finds nothing to match. The tick loop also trusted that startup repair had already closed out a record left by a dead process. Repair can fail (a store write, the pending recovery target), and since it now releases the claim and returns, the foreign record stays stored and unclaimed - the engine would then measure anchors, a hold clock and a baseline rate from observations this process never made, and could end the run Passed or Refuted instead of aborting PROCESS_DEATH. onTick refuses any record this process does not own and closes it out, so a failed repair simply retries on the next tick. Fixes review findings F4, F5.
The baseline phase ends at BASELINE_WINDOW_MILLIS (10 minutes), not the general 25 minute phase budget, so its bar filled to about 40% and then jumped - in the one phase the user is explicitly asked to wait through. Fixes review finding F6.
…e back A finalization that fails on a store write fails its release the same way: both are DataStore writes, and NonCancellable only keeps the release from being cancelled, not from throwing. The record then stays claimed with this process's provenance, so startup repair skips it (not foreign) and every ordinary terminal path refuses it (already claimed) — the app reads a run as live for the rest of the process's life, with rules suspended, no session, no further run, and the baseline the run owes never restored. onTick now treats any record it finds still claimed as close-out-only and aborts it with the new FINALIZATION_INTERRUPTED reason, which is the retry that store recovery otherwise has no way of reaching. The forced reclaim is only safe under serialization, so finish() runs claim-through-release under a finalizationMutex: it is reachable from the tick consumer and from the charge service's scope at once, and without the lock a tick could reclaim a record a live finalization is still working through, with both restoring the baseline and writing evidence. The release itself is wrapped so a failing release cannot replace the finalization's exception with its own. Fixes review finding F7.
… aborting it Finalizing a run is four durable steps - decide the terminal, write the evidence, publish the result, clear the record - but the only thing persisted was the boolean "a finalization started". A store write failing between the evidence write and the clear left the next tick with nothing but that boolean, so it substituted Aborted(FINALIZATION_INTERRUPTED): the user was told nothing was recorded while the pass that licenses charge control, or the refutation that withholds it for good, was already on disk. A failure before the evidence write threw away a terminal that had already been decided correctly. The record now carries a FinalizationIntent, written in the same transaction as the claim and alongside the closing measurement, so recovery replays what the run decided rather than inventing a substitute. The claim resolves the cancelled / write-failed downgrade itself - the stored outcome can never disagree with the one that was applied - and never overwrites an intent that is already there. Releasing a failed claim keeps the intent: the release means the attempt did not finish, not that the outcome was never decided. Every close-out path (the claim guard, the foreign-provenance guard, the startup repair) replays that outcome and falls back to an abort only for a record claimed by a build that wrote none, which is also why finalizing is kept as its own field rather than derived from the intent. A Passed or Refuted decided before a process death therefore now completes in the next process instead of becoming Aborted(PROCESS_DEATH). Dropping a refutation is the dangerous direction: it leaves the device holding control it demonstrably does not honour. The replay needs no bookkeeping to be idempotent - the restore is a no-op write, the qualification evidence store overwrites unconditionally with byte-identical content (both evidence records are stamped with the intent's decision time, not a fresh clock read), the enforcement store refuses a duplicate for the same scope, and republishing the result flow is harmless. Fixes review finding F8.
…t measured it A finalization intent can be persisted and then replayed by a later process, and that process may belong to an app update. Reading PROTOCOL_VERSION and ALGORITHM_VERSION from today's constants at write time therefore presented a measurement made under a superseded protocol as one the current protocol produced: the qualification store's scoping accepted it and it licensed charge control, and a refutation likewise skipped the enforcement store's per-version migration. Both stamps now come from the run record. protocolVersion was already there; enforcementAlgorithmVersion is new and set from the constant at run start. Its zero fallback is only reachable for a record written before the field existed, and such a record carries no finalization intent, so it can never be a cross-version replay - it closes out as FINALIZATION_INTERRUPTED or PROCESS_DEATH instead. The replay also covers the composition the measurement is persisted with the claim for: a replayed finalization now has a test that it publishes the merged record, phase-log row included, not just the terminal. Fixes review finding F9.
…wes it A replayed finalization could revert an explicit persistent choice the user made in between. The sequence: finalization restores the baseline and clears its recovery target, its record clear then fails while the claim release succeeds, and before the next tick the widget's fixed-limit/unrestricted button writes a policy and clears its own recovery target. The replay took the close-out-only path and wrote record.baseline again, so the user's choice was silently reverted with nothing left owed to bring it back - the close-out bypasses the engine's CONFIGURATION_DRIFT re-evaluation that used to catch this. The restore now goes through ChargingRepository.restoreQualification- BaselineIfOwned, which writes only while the single recovery slot is still owned by this run. Ownership is exact in both directions: an attempt that never reached the restore still owns the target, a failed restore leaves it owned, a completed restore or any newer producer does not. The check runs inside operationMutex, the same lock as the write - setPersistentPolicy registers its recovery record before entering that mutex, so either the replay writes first and the user's write lands on top, or the replay sees the newer owner and skips. Superseded counts as restore-complete: the terminal is still published and its evidence still written, only the policy write is skipped. QualificationRestoreOutcome is a separate type rather than an ApplyResult flag so "did not write, no longer ours" cannot collapse into "wrote and failed", which would strand an owed policy. Fixes review finding F10.
…easured it The zero fallback for a run record's enforcementAlgorithmVersion took the current EnforcementVerdictEngine.ALGORITHM_VERSION, on the premise that a record without the field can carry no finalization intent and so can never be replayed across an app update. That premise was wrong: the intent field shipped one build before the version field, so a record written in that window decodes with a replayable refutation and a zero version. After the next algorithm bump the replay would stamp that measurement with a version the newer algorithm never produced, imposing a refutation on its behalf. The fallback is now a literal 2, the version every build in that window measured at, so the evidence keeps its provenance and the enforcement evidence store decides for itself how a version-2 verdict migrates. Fixes review finding F11.
Reopening the app dispatches ACTION_CHECK, which resolves to boot recovery whenever a pending recovery target exists. During a qualification run that target is the run's own, registered before its first policy write, and the recovery flow clears it unconditionally. The run then kept commanding experimental policies while nothing was owed any more, and its finalization found no owner and skipped the restore, leaving the device on the experimental cap with the user's configured policy unrecoverable. startRecovery() now reads the live run and the pending target first and returns without recovering when the target belongs to that run, the same ownership rule ACTION_START already applies to a full-charge start. It is centralised there so ACTION_CHECK, ACTION_RECOVER, the sticky restart and the post-start branch are all covered once. A target owned by anything else is still recovered normally while a run exists. Nothing is stranded by refusing: the run record keeps the qualification watcher enabled, which holds the foreground service up, so the run's own close-out still restores. Fixes review finding F12.
The ownership guard that refuses boot recovery of a recovery target a live run owns relied on the run's own close-out performing the restore instead. That holds only while the run record exists, and finalize() clears the record whether or not the restore succeeded. The stranding sequence: a process death or reboot mid-run, BootReceiver dispatches ACTION_RECOVER, the guard refuses it, startup repair then closes the record out and its single baseline write fails — routine at boot, where the backend or provider is often not ready yet. The recovery target is correctly left behind, but the record is cleared, so QualificationWatcher goes quiet and the next continueGestureOrStop() stops the service; that function decides from the session, the gesture and the watchers and never looks at a pending recovery target. Nothing re-dispatched recovery, and the device could sit on the run's experimental policy — its deliberately less-protective release policy included — until the next foreground launch or reboot. finalize() now re-dispatches ACTION_RECOVER when its restore failed, after clearing the record: with the record gone the guard no longer refuses, so BootRecoveryFlow's bounded rewrite loop, which exists for exactly this kind of failure, picks the target up. Keyed on the same restored flag the target is kept by, so the two paths where nothing is owed (a CONFIGURATION_DRIFT supersession, and a target that is no longer this run's) cannot trigger it. It cannot loop: recovery never calls finalize(), and the run record a second close-out would need is already cleared. The dispatch is fire-and-forget and takes no lock, so the finalization's lock order is unchanged. Fixes review finding F13.
…g a dispatch A qualification close-out whose baseline write fails leaves the recovery target in place and dispatches ACTION_RECOVER, but clearing the run record is also what turns QualificationWatcher off, so the dispatch races the service's own stop decision. A stop decision that wins calls stopMonitoring(), which discards already-queued work and stopSelf()s; START_STICKY then redelivers a null intent, not the ACTION_RECOVER that was in flight. The device would sit on the run's experimental policy, target still owed, until the next foreground launch or reboot. The stop decisions now look at the obligation itself: continueGestureOrStop and evaluateBattery start recovery when a pending target exists that no live run owns, instead of stopping. The dispatch stays — it covers the service already having stopped or never having run, which a stop-decision check cannot. The recovery launch is split out of startRecovery() into beginRecovery() so these callers skip the ownership guard they have already evaluated (its refusal branch calls back into continueGestureOrStop, which would be mutual recursion). continueGestureOrStop's check is gated on no recovery being in flight, because that site is also the recovery job's own tail and BootRecoveryFlow deliberately keeps the pending target when a re-write fails, so the next service start can retry it. Ungated, a persistently failing restore would restart itself there forever at the flow's 25s/75s cadence with the service never stopping. evaluateBattery needs no such gate (it returns early while a recovery job is active) and restoreAndContinue's post-failure stop is left untouched for the same anti-loop reason. Both properties are covered: the race with an already-running instance whose hand-off intent is never delivered, and the failing-re-write recovery that must stop through its tail rather than restart itself. Fixes review finding F14.
The pre-check refused a run with "your battery is too low" and a disabled
Start button, and nothing on screen said how low, how high it had to be,
whether the phone was even charging, or how long the wait was. On a Pixel 6
running LineageOS the real threshold is 73% (lowest cap 70 plus the
variable-cap undershoot) while the battery sat at 64%.
The refusal now carries the level a run needs. It is found by scanning
resolvePlan over every level below the near-full cutoff rather than by
re-deriving the undershoot/headroom arithmetic: the eligibility rule exists
once so the pre-check list, the entry point and the runner agree by
construction, and a second copy of the maths would be a fourth
approximation of it. It is set only for BATTERY_LEVEL — a too-full battery
has to discharge, where a "needed" level would read as a target to charge
towards.
Alongside it, a pure estimate of the time to that level, extrapolated from
the reported charge counter and current. Direction comes from
BATTERY_STATUS_CHARGING plus a plug, never from the current's sign, which
is OEM-defined (the reasoning StatsPowerCalculator.chargeMilliwatts
documents). Every gate returns null rather than a guess: no charge, a
missing input, a level outside 1..99, a target at or below the level, or an
implausible result. It renders in round buckets ("about 40 minutes", "about
an hour", "over an hour") because an instantaneous-rate extrapolation
across a charge curve that flattens near the top cannot support "43
minutes". When the phone is not charging the block says so plainly instead
of estimating, which alone answers the most common cause of the block.
The block is live: while the pre-check step is on screen the figures and
eligibility are both re-resolved on the battery readout cadence, so a user
who reaches the threshold while sitting there sees Start enable instead of
having to leave and come back.
Not addressed here, deliberately: a device whose cap actually holds can
still be stuck below the threshold by its own limit. This makes that
situation legible; what to do about it is a separate decision.
…observed Two ways the block made a claim it could not back. A charge counter reported in milli- rather than micro-units, with a correctly scaled current, produced a confident short estimate: level 50 with a counter of 2 000 implies a 4 mAh pack, so 20 remaining points come out as 0.03 minutes and render as "About 10 minutes" for what is really a long wait. The two are independent HAL properties, so one being mis-scaled while the other is not is reachable. The old guards only judged the final figure, and the 24-hour ceiling only rejects a lie in the long direction. The implied capacity is now judged on its own against QualificationProtocol.MIN_PLAUSIBLE_FULL_MICROAMP_HOURS, which is the same decision QualificationRunEngine.resolveSignal already makes about the same input when it decides whether the counter is a usable signal at all. "Not charging right now" was printed for states where charging is unknown: a plugged charger with BATTERY_STATUS_UNKNOWN (a valid platform status), an unrecognised status value, or only the positive half of the plugged/status pair present. That is the line a waiting user is most likely to act on. The check is now a real tri-state: true only for a plug plus BATTERY_STATUS_CHARGING, false for an observed negative on either half (nothing plugged in, or discharging/not-charging/full), null otherwise. The screen already omits the line for null, so an unknown state now says nothing instead of asserting. Fixes review findings F17, F18.
The pre-check figures were driven by a collector launched eagerly in the ViewModel's init, keyed on the requested step. Leaving the screen by the top arrow changes the nav destination, not the step, and the state was collected at the composition root off an activity-scoped ViewModel, so a battery read plus a full eligibility resolution — store reads, and a Shizuku binder round trip on some adapters — kept running every three seconds for the Activity's lifetime, including while the app was in the background. The same collector was the only subscriber that ever filled the pre-check figures, and it unsubscribed when the step became RUNNING. The running screen then read the last value it happened to leave behind, showing a percent and charge counter frozen at the last pre-check reading for the thirty to ninety minutes a run takes, while looking live. Both come from the same shape, so both are fixed by the same change. The figures are now a cold flow the UI state is built from (liveFigures), so they exist only while the state has a subscriber; they are resolved for the running step as well as the pre-check, and the run progress takes the readout as a parameter instead of reading a cached one. The polling is keyed on the *resolved* step, so a run picked up from another process polls too. MainActivity collects the state inside the qualification destination with collectAsStateWithLifecycle rather than at the root: the step is not a proxy for the screen being visible, and only the collector's lifetime can stop the poll. A non-polling step emits one empty figure set, which is what clears the previous step's block, and a polling step emits one before its first reading so a step change is never held behind a battery read and an eligibility resolution. Fixes review findings F15, F16.
d4rken
added
enhancement
…ragraphs Every step rendered through two primitives, a title and a paragraph, appended as flat list items, so the running step read as two paragraphs, an unlabelled progress bar, and a run-on reading line. Nothing grouped, nothing labelled. The wizard now uses two shared blocks built on the app's existing card design system: QualificationCard (AmplyCard plus the standard header, so it matches the dashboard rather than inventing a second card style) and LabelledValue, a label/value row that replaces the run-on reading lines. The running step gets a progress card: a "Step N of M · <phase>" caption above the bar, which on its own fills once per phase and restarts, reading as no progress at all. The position comes from a pure runStep(), unit-tested for both run shapes, because a fixed-cap run has a charge-up phase ahead of the baseline and therefore one step more. The raw charge counter is gone from the UI, and from RunProgressUi with it. It is the protocol's internal flow signal in microamp-hours, actionable in no unit for a user; it stays in the contribution report, where a maintainer reads it. In its place the running card shows the charging state, derived by the same tri-state chargingOrNull() the pre-check uses, and skipped when unobserved. The pre-check block becomes the same card with LabelledValue rows, showing the same lines under the same conditions. The result verdict and the report preview move into cards, with their explanatory prose staying outside them.
…t a cut Setting the charge limit below the current battery level makes some builds report the phone as entirely unplugged: observed on a Pixel 6 running LineageOS BP4A.251205.006, where the cable stayed connected (adb live over it) while dumpsys reported "USB powered: false" with the limit active, and raising the cap resumed charging instantly. A VARIABLE_CAP run caps below the current level by design, so it aborted on its own first cut and told the user their charger had come out. A lost plug signal within PLUG_MASK_WINDOW_MILLIS of a cut's acknowledged write now ends the run as Inconclusive(PLUG_SIGNAL_LOST_AT_CUT) instead of Aborted(UNPLUGGED). Telling a cap-induced plug loss from a real unplug was only ever needed to reach a pass safely; here both readings end the run with no verdict, so an outcome that names both is true without having to distinguish them. The window is anchored on the acknowledgement because that is when our own cut could have masked the plug, and it decides wording only - it authorizes no pass, no hold, and no measurement decision. Outside a cut phase, outside the window, or with no acknowledged write, the ordinary unplug abort is unchanged, as is abort precedence. Inconclusive rather than a new AbortReason: a run that could not measure stores nothing and stays repeatable, which is exactly this situation. The user's configured policy is restored on this terminal like any other.
plugSignalLostAtCut() only checked the upper bound, so any negative elapsed satisfied it. A backwards wall-clock correction during an acknowledged cut - NTP, a time or timezone change, a user edit - stamps a sample before commandAckedAt, and the run then published Inconclusive(PLUG_SIGNAL_LOST_AT_CUT) about a moment that lies outside the window entirely. Nothing durable was wrong (this branch reaches no evidence and no verdict), but the statement was false, in the one change whose purpose is to stop the run saying something untrue. The engine's isStale() guard cannot catch it because abort handling runs first. The elapsed delta is now required to be >= 0 as well. The phase check and the commandAckedAt > 0 guard are unchanged, abortReasonFor and its ordering are untouched, and the window still decides wording only. Adds an engine case at commandAckedAt - 1 closing the lower boundary (the existing cases cover the inclusive upper bound and past-window), and pins the PLUG_SIGNAL_LOST_AT_CUT constant as stored text in StoredRecordFormatTest, since it travels on the replayed finalization intent. Fixes review finding F19.
The plug-signal-lost body claimed two things the run never saw. It said charging stopped, which is an inference: at that point the engine has only `readout.onCharger == false`, with no charging-rate or charging-status observation behind it. And it said the phone reported the charger as disconnected, which `BatteryReadout.onCharger` cannot distinguish from a phone that simply stopped reporting `plugged` at all. The message now claims only the observation: the phone stopped reporting the charger as connected while the lower limit was set. Both readings are still named (some builds report it that way when the limit engages, and a slipped cable looks the same), the cable-check retry stays, and so does the note that sending the result helps.
`finalize()` already knew whether the user's own charge setting made it back: a failed restore leaves the recovery target in place and hands the write to boot recovery. The published result did not carry that, so every result body that mentioned the setting claimed it was back regardless. `QualificationResult` now carries `restored` (presentation only, like the result itself; nothing persisted changes), the ViewModel threads it into the UI state, and the result step renders the restore statement as its own line with two variants: the existing "has been put back" wording when it landed, and an honest "couldn't put it back yet, still trying, you can set it yourself" when it did not. The eight abort bodies lose the sentence they used to carry so it is stated in exactly one place. The line is skipped for SERVICE_UNAVAILABLE alone: that abort happens before the run's service ever starts, its own copy already says nothing was changed, and a restore warning there would alarm about a setting that was never taken. The UI state defaults `restored` to false, so a state without a result cannot render the claim.
The phase sentences are written as finished facts ("The limit is set to
70%", "The limit is now above 70%"), but the phase is persisted before its
write is dispatched and `commandAckedAtWallMillis` is only stamped when the
adapter comes back. On a slow or failed write, that gap is the whole time the
user is looking at the screen, and the screen asserts a change that has not
happened.
`RunProgressUi` now carries the decoded commanded policy and whether its write
was acknowledged, both already on the run record, and a pure `runMessage()`
picks the sentence: unacknowledged renders one of two new pending strings
chosen by the commanded policy ("Setting the limit to X%…" for a cap,
"Removing the limit…" for Unrestricted), acknowledged falls through to the
phase sentence unchanged. Preflight commands nothing and keeps its sentence.
One generic pair rather than a pending variant per phase: every phase either
sets a limit or takes one off, so five more strings would say the same two
things.
…what it owes The result screen decided its restore line from a behavioural flag that means "no recovery obligation remains". That is equally true of a restore that was written and landed, of one that was skipped because a newer user choice already holds the slot, and of the drift abort that deliberately does not write so a newer choice is not clobbered. Only the first put anything back, yet all three rendered "Your charge setting has been put back". The result now carries a QualificationRestorePresentation (APPLIED / PENDING / OMIT) alongside the unchanged local flag: a successful write is APPLIED, a failed or throwing one is PENDING, and a superseded restore, the drift short-circuit and a run refused its service are OMIT, which renders no restore line at all. The restore itself, the owner-scoped recovery-target clearing and the failed-restore hand-off to boot recovery are untouched. The pending copy also promised that Amply keeps trying. The result is a one-time close-out snapshot and nothing on that screen observes the recovery target or boot recovery afterwards, so the assurance can be false while it is on screen - and a user who believes it may leave a setting in place that is less protective than their own. It now states what happened and asks the user to check the setting. Fixes review findings F20, F21.
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
On devices where Amply cannot confirm that a charge limit actually works, it can now offer to find
out. The check drives the limit itself: it caps below the current level and waits for charging to
stop, raises the cap and waits for charging to resume, then caps once more. Only a limit the charging
hardware really honours behaves that way on command, so the result settles a question Amply cannot
answer by watching alone (a battery resting below a limit looks the same as one paused by heat or a
weak charger).
The check is offered where it is useful and withheld where it is not: a device whose limit a
maintainer already verified has nothing to prove, and a build already known to charge past its limit
is not asked again. A run reports a pass, a refusal, or an inconclusive result, and produces a report
the user can send in to help get their build supported.
Before a run starts, the pre-check now shows the figures behind its own verdict rather than a bare
verdict: the current battery level, the level a run needs, whether the phone is charging, and a rough
time estimate. Those figures update live, so a user waiting to reach the required level sees them move
and sees the start button become available, instead of having to leave the screen and come back.
Throughout, the user's own charge setting is treated as owed work. It comes back when the check ends,
when the user stops it, when the charger is unplugged, and when the phone restarts partway through,
including when the app is killed between deciding the result and writing it down.
This feature is available in debug and beta builds only. It is switched off in release builds, so no
released user can reach it.
Technical Context
around that asymmetry. Nothing passively observable distinguishes a cap hold from a thermal or
supply-side pause, so a pass is scoped to the exact build that produced it and a refutation is
terminal for that build. The two catastrophic outcomes the design exists to prevent are a pass
recorded for a limit that does not work, and a refusal recorded against hardware that does.
evidence, publish the result, clear the record) with only "a finalization started" persisted. A
failure partway left recovery unable to tell how far it had got, so it substituted an abort and
contradicted evidence already on disk. Finalization is now a replayable state machine: the decided
outcome and the measurement it came from are written with the claim, in one transaction, and every
recovery path replays that instead of inventing a result. Replay is idempotent by construction
rather than by a ledger, which is why evidence carries the protocol and algorithm versions that
measured it rather than the running build's.
same lock as the write. Without that, a replayed restore could overwrite a persistent policy the
user chose in between, with nothing left owed to bring it back. Boot recovery correspondingly
refuses a recovery target a live run owns, and a close-out whose restore fails hands the work back
to boot recovery rather than dropping it.
oversights. An outstanding restore keeps the service alive at two stop decisions but not the third,
and one of those two is additionally gated on no recovery being in flight. Both exclusions exist
because that stop site is also the recovery job's own tail, and the recovery flow deliberately
keeps its target when a re-write fails so the next service start retries it. Removing either turns
a persistently failing restore into a retry loop.
required level is derived by scanning the real eligibility rule rather than restating its
arithmetic, so the number shown cannot drift from the number that gates the run; the time estimate
returns nothing rather than a guess when an input is missing or the implied battery capacity is
physically impossible, and it is rendered in coarse buckets because it extrapolates an
instantaneous rate across a non-linear curve; and the battery polling behind it is scoped to the
screen actually being visible, not to the wizard step, because that poll costs a privileged round
trip on some adapters. Worth close review: the claim-and-replay path in the run store and runner,
and the stop decisions in the charge service. The measurement engine is unchanged since its own
review and is not part of this change.