Repository navigation
Conversation
|
Reviewed at 92f86b0. Claude Opus 5.5 reviewed it, and Codex Medium gave a second opinion. The tests ran on a disposable Omarchy VM. What ran: A regression on machines with two packs. Threshold detection is skipped when the script cannot see a hold. Smaller things in the script:
Proportionality. The bug is one wrong denominator on one machine. This change hands percentage, size, state and time for every laptop to sysfs. It also moves both consumers off UPower and adds a 3-second poll that runs while the panel is closed. That poll is cheap (about 14 ms per run on the VM), but it adds up next to the service's own poll. A fix kept to the latched Also: the branch conflicts with Nothing was pushed. This is waiting on the author: the two-pack regression and the conflict need resolving, and ideally the change should be narrowed to the latched value. |
|
Confirming this on a MacBookPro13,3 (15" 2016, Omarchy 4.0.4, upower 1.91.3, battery After boot UPower had latched
|
|
Re-reviewed at 92f86b0 after @andersryanc's report. The head has not moved, so this covers only what that report changes. Claude Opus 5.5 read it against the source, and Codex Medium gave a second opinion. Codex agreed with everything below, though its independence is not guaranteed. Nothing new was run on a worker. The tests from the last review still stand at this head. The bug is real on a second machine. A MacBookPro13,3 latched The "Holding" state comes from the eight-hour rule. On The report favours the narrower fix. Both latched readings are 9 to 14 times the design capacity, so a guard that uses sysfs only when UPower's Nothing else has changed. The two-pack regression, the threshold detection that is skipped, the smaller defects in This is waiting on the author to resolve the regression and the conflict, and ideally to narrow the change to the latched value. |
92f86b0 to
975445c
Compare
|
Reworked after the review, rebased on current
The three battery suites, @andersryanc, if you get a chance, |
[Medium impact] The changes affect battery status across the shell’s warning and power-panel workflows. The PR still needs the existing multiple-battery issue fixed before merging. Findings
SummaryAdds recovery when battery registers cannot be read and remembers refresh requests that arrive while the helper runs.
Reviews (7) · Last reviewed commit: "Fall back to the design capacity when la..." · Reviewed by Greptile |
|
Re-reviewed at 11d2d3c, after the rework. Claude Opus 5.5 reviewed it, and Codex Medium and muse-spark-1.3 (thinking high) each gave a second opinion. Tests ran on a disposable Omarchy VM. The rework answers the last review. The two-pack regression, the skipped threshold detection, the per-panel poll and the conflict with What ran: One fix pushed (11d2d3c). The status script prints nothing when it finds no battery for a moment, which Still open, for the maintainer: with this PR alone, a latched MacBook charging below 99% still shows Threshold and Holding. That's because Noted, not changed: on a two-pack machine where one pack latches, the correction uses the first pack's percentage rather than an aggregate. That case hasn't been reported anywhere, and a sound two-pack machine is unchanged from What it waits on: a VM has no Smart Battery, so nothing here reproduces the latch itself. The fixtures prove the override wins, not that it fixes real hardware. @andersryanc, if you can, please test this exact commit on the MacBookPro13,3 while UPower is latched. Check out |
|
|
||
| function batteryPercentage(device, kernelLevel) { | ||
| if (!device || !device.isPresent) return -1 | ||
| if (typeof kernelLevel === "number" && kernelLevel >= 0) return kernelLevel |
There was a problem hiding this comment.
One pack replaces total charge
kernelLevel comes from the first physical battery found by omarchy-battery-status, but this return replaces the whole machine's UPower.displayDevice percentage. On a two-battery laptop where that first pack has a latched reading, the bar and low-battery warning follow only that pack, ignoring the other pack's remaining charge. Combine readings across packs, or restrict this override to machines with one battery.
|
Re-reviewed at 4a12795, after the merge of The merge is correct. The only conflict was in What ran: at the merged head, One fix pushed (4e726a2, comment reworded in 4a12795). Greptile's second comment holds. The low-battery timer fired as soon as the shell started, before the status script answered. So on a latched MacBook running on battery below about 90%, UPower's latched tenth triggered the low-battery warning and the For the maintainer: the two-pack case (Greptile's first comment). It holds as stated: the correction uses the first pack's percentage in place of UPower's total. On a two-pack laptop where the first pack latches and the packs are far apart, that can hide a low-battery warning that Still waiting on: a VM has no Smart Battery, so the latch itself has not been reproduced, only the override. @andersryanc, if you can, please test |
UPower can latch energy-full on a garbage Smart Battery reading (a MacBook Air: 484 Wh on a 54 Wh pack; a MacBook Pro: 733 Wh on 77 Wh) and keep it until the daemon restarts. The bar then shows a tenth of the charge, the panel a pack size in the hundreds of watt-hours and a time to full in days, and the low-battery warning fires on a full pack. A full charge beyond twice the design capacity cannot be real, so omarchy-battery-status reads this battery's own sysfs registers for its energy, full charge, percentage and size in that case, and names the kernel as its source. The battery service runs the script once at startup, again when UPower's full charge moves by more than a factor of two, and while a latch holds, as UPower's percentage moves; its kernel percentage stands in for the display device's in the low-battery check and the power panel's bar icon. On every other machine nothing changes: aggregation, state and time stay with UPower, and the script runs once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the kernel figure The status script prints nothing when it finds no battery for a moment, which happens around AC plug events. The service took that as a sound answer: the kernel percentage went to -1 while the latched capacity stayed recorded, so no later change ever asked again and the bar and low-battery check fell back to UPower's latched tenth until the shell restarted. An empty run now keeps the last kernel percentage and clears the recorded capacity, so UPower's next change runs the script again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
The warning timer fired on start, before the status script answered, so a latched MacBook on battery checked UPower's tenth of the real charge and ran the low-battery warning and battery-low hook at every shell start. The startup read already calls checkBattery() when it finishes; the timer now only repeats, so a genuine warning waits at most one 30-second tick if the read never answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: muse-wonder thinking high <noreply@muse.dev>
A power-source change, an empty status run or a helper that never answers can still check before the kernel correction lands; the comment no longer claims otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com> Co-Authored-By: muse-wonder thinking high <noreply@muse.dev>
…readable one isn't 0%
4a12795 to
5793ac4
Compare
…, and rerun a status read a change overtook Past a latched energy-full, a register pair that doesn't answer used to leave no source line, so the shell took -1 (UPower's latched figure) and never read again. Use UPower's energy over the design capacity there (source design); with no energy either, print source unavailable, which the shell treats as no reading: it keeps the last level and retries. A UPower change that arrives while the status script runs now reruns it when the run exits instead of being dropped.
What
UPower can latch
energy-fullafter a Smart Battery blip (MacBook Air: 484 Wh on a 54 Wh pack; the MacBookPro13,3 in the thread: 733 Wh on 77 Wh) and keep it until the daemon restarts. The bar then shows a tenth of the charge, the panel a pack size in the hundreds of Wh and a time to full in days, and the low-battery warning fires on a full pack.Reworked after review into a guard rather than a replacement:
omarchy-battery-statusreads this battery's own sysfs registers for energy, full charge, percentage and size only when UPower'senergy-fullis more than twiceenergy-full-design, and prints asource kernelline to say so. Its existing time formula then counts to the real full charge. On every other machine the output is unchanged.energyCapacitymoves by more than a factor of two (a latch appearing or clearing), and while a latch holds, as UPower's percentage moves a tenth of a point. No timer, andshell-idle-spawn-test.shholds a sound machine to the one run. ItskernelPercentageis -1 unless the script named the kernel, so aggregation across packs, charge state and threshold detection stay with UPower.chargeThresholdActive()is left alone; Keep charging visible with long time-to-full estimates #14141 owns that symptom.Test plan
bash test/shell.d/battery-status-test.sh(new: latched charge- and energy-counting packs, a latch with no registers, a plausibleenergy-full; the latched assertions fail onquattro)bash test/shell.d/battery-test.shbash test/shell.d/power-test.sh./test/cli./test/shellRelated #9668