Repository navigation
Conversation
|
@robosayem review this pr, and check if any dupe pr exist related to this. and list issues this pr will close. |
robosayem
left a comment
There was a problem hiding this comment.
I reproduced the fix locally against a stub upower: with sysfs at 45/65 the script reports threshold\t45-65% over UPower's 75/80, without sysfs it falls back to UPower, and the holding line renders correctly. The core ordering (native device file, BAT* glob, UPower last) is right, and the moved fallback makes the previously dead glob code reachable. This fixes the ThinkPad case in #9586.
One moderate fix requested before merge
The fallbacks are per-field, which breaks end-only sysfs devices — the ASUS machines from #7374, where only charge_control_end_threshold exists. There, end=60 comes from sysfs while threshold_start falls back to UPower's fabricated 75, and the renderer takes the start-end branch, producing 75-60% (an inverted range) instead of 60%. The renderer already has a correct end-only branch, so the fix is to make UPower an all-or-nothing fallback: only read the UPower pair when sysfs exposed neither attribute. #7374's own proposed fix says the same ("don't fabricate a start threshold without a sysfs start file"). The PR's test only covers both-present and both-absent — a mixed case (end sysfs present, start absent) would have caught this.
Nit, non-blocking
Some drivers report charge_control_end_threshold = 0 to mean disabled — that renders as 0-0%. Treating 0 as absent (fall through to UPower) would be safer, but it can also be left to a follow-up.
Duplicate check
No duplicate: this is the only open PR changing the threshold source. Neighbouring open PRs touch other battery dimensions — #7915/#7895 (pending-charge hold heuristic), #7293 (charge status source), #6845/#7138/#7186/#8864/#9106 (multi-battery), #10076 (percentage source) — all complementary.
What this closes
- Fully: #9586.
- Partially: #7374 (fixes the 75-80-vs-real-limit symptom, not the bogus 0 cycle-count half) and #10344 (fixes the UPower-first source half, not the
Model.chargeThresholdActivepanel heuristic). Neither should auto-close on this PR alone unless the maintainers want to split them.
With the mixed-source fix, this is a clean approve from me.
|
Addressed @robosayem's mixed-source fix on
Left for follow-up (non-blocking nit): treating sysfs |
0c9ad0b to
461dabe
Compare
|
Verified against current quattro (fail-then-pass on scoped shell tests). Ready for maintainer review. Scoped: battery-status-test (sysfs-first). |
|
Reviewed at 461dabe against What was checked. On a disposable Omarchy worker, I wrote a separate reproduction that stubs UPower to report the hwdb default of 75/80 with Second opinion. Codex Medium reviewed the same head. It agreed that the ordering fixes the bug and that the size is right (independence not guaranteed). It raised two edge cases. Neither blocks this:
Related. #13362 and #13967 edit the same threshold-reading lines but still read UPower first. They overlap with this PR and will conflict with it, but they do not fix this bug. This PR fixes the charge-limit half of #7374 but not its cycle-count half, so I have not linked it to close that issue. What it waits on. A stub only shows that the script handles the values the reports describe. It can't show that a real machine reports them. @ScytheAkira (ThinkPad, #9586) or @chickymonkeys (ASUS, #7374), could you run this on your laptop while plugged in and held at its limit, then reply here with the output? The |
|
@omarchybot I confirm that the threshold line matches the sysfs values rather than 75-80%, tested from terminal with the indicated snippet. |
|
Thanks @chickymonkeys, that settles the hardware side: on your ASUS the threshold line now matches what sysfs holds rather than UPower's 75-80%. I re-ran the check against current Codex Medium reviewed this head again and found nothing that should block it. It reviewed the two edge cases from my last comment again. The cross-battery This now waits on the maintainer. #7374 stays open for its cycle-count half. |
quattro now reads the thresholds from the battery UPower resolved and counts the time to full up to that battery's own limit. Port the fix onto that: this battery's sysfs comes first, UPower stands in only when sysfs exposes neither threshold, and the BAT* glob stays the last resort, so the charge limit for the time to full is the live sysfs one too. The read_sysfs_threshold helper goes, since quattro's inline reads do the same, and the tests move onto quattro's fixtures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A battery with only an end threshold in sysfs took its start from the first BAT* that had one, so a second battery with a start of 75 turned an ASUS end-only 60 into 75-60%, the inverted range the sysfs-first order exists to avoid. The glob now stands in only when neither this battery's sysfs nor UPower gave a threshold. The sysfs test also checks that the time to full counts up to the sysfs limit rather than UPower's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: muse-wonder via pi, thinking high <noreply@omacom.io>
[Medium impact] This PR appears safe to merge; no actionable issues were found. Summary
Reviews (1) · Last reviewed commit: "Fill a missing threshold pair from the B..." · Reviewed by Greptile |
|
This branch had stopped merging into What was checked. All of this ran on a disposable Omarchy worker, with UPower stubbed to the hwdb's 75/80 and
Codex Medium and muse-wonder (pi, thinking high) reviewed it in two rounds. In the first round, muse-wonder found the cross-battery pairing above. The second round found nothing on this head. There is a competing fix, and it covers more. #16685 by @ThatRetiredDude reads sysfs first too. It also handles two cases from #7374, #10193 and #17137 that this branch still gets wrong:
I asked both reviewers to compare the two without giving them my conclusion. muse-wonder came to the same verdict and found no regression in #16685 on any of the reported machines. Codex could not read #16685's diff, but it confirmed both gaps in this branch. Independence is not guaranteed, since both read the same workspace. The older fix normally wins, but these are reported machines this branch misses, so I am recommending #16685 to the maintainer instead of this one. What this branch adds is the end-only test with a second battery's start. From reading its diff, #16685 should already handle that case, because it reads the start only from the battery that has the end. I have not run it there. The choice between the two is the maintainer's. I have not worked #16685 yet; it gets its own review. |
What
Why
Fixes #9586. ThinkPad UPower reports a built-in 75/80 while ChargeThresholdEnabled is false, so TLP/sysfs thresholds never appeared.