Repository navigation
Conversation
BenBE
added
enhancement
|
.oO(Nice, quite a stunt, to cover all of the BSD's with this. :) |
1dc77c5 to
4d3bece
Compare
98680b3 to
422d5f3
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a unified Suggested reviewers: Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · up to Battery displays can show contradictory capacity values on macOS and falsely report AC power under PCP. These telemetry regressions should be corrected before merge. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. One struct gathers charge and power, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-485:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease the proplib objects on every exit path.
prop_dictionary_recv_ioctl()returns a dictionary owned by the caller,prop_dictionary_iterator()andprop_array_iterator()create iterator objects that must be released explicitly, and iterators retain their underlying collections. Currently,dict,devIter, and eachfieldsIterare never released, causing a memory leak on every call to this function. With repeated battery polling, this accumulates over time.All three objects must be released:
dictviaprop_object_release()(owned by caller fromprop_dictionary_recv_ioctl())devIterviaprop_object_iterator_release()(created byprop_dictionary_iterator())fieldsIterviaprop_object_iterator_release()(created byprop_array_iterator())Possible fix
void Platform_getBattery(BatteryInfo* info) { - prop_dictionary_t dict, fields, props; + prop_dictionary_t dict = NULL, fields, props; prop_object_t device, class; + prop_object_iterator_t devIter = NULL; + prop_object_iterator_t fieldsIter = NULL; @@ - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) goto error; @@ - prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); + fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) goto error; @@ while ((fields = prop_object_iterator_next(fieldsIter)) != NULL) { ... } + + prop_object_iterator_release(fieldsIter); + fieldsIter = NULL; } + + prop_object_iterator_release(devIter); + devIter = NULL; + prop_object_release(dict); + dict = NULL; error: + if (fieldsIter != NULL) + prop_object_iterator_release(fieldsIter); + if (devIter != NULL) + prop_object_iterator_release(devIter); + if (dict != NULL) + prop_object_release(dict); if (fd != -1) close(fd); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbsd/Platform.c` around lines 471 - 485, The code leaks proplib objects: the dictionary returned by prop_dictionary_recv_ioctl (dict) and the iterators created by prop_dictionary_iterator (devIter) and prop_array_iterator (fieldsIter) must be released on every exit path; update the function so that before jumping to the error/exit label or returning you call prop_object_release(dict) when dict is non-NULL and prop_object_iterator_release(devIter) and prop_object_iterator_release(fieldsIter) when those iterators are non-NULL (also release any fieldsIter created inside the loop before continuing), ensuring you don't release objects twice and that fieldsArray/device handling remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 116-141: The compact-mode token concatenation happens because the
AC/bat labels written by xSnprintf (the calls that print "%s" with "AC"/"AC+bat"
and the "bat" literal) lack a trailing separator; change those format strings to
include a space (e.g., "%s " and "bat ") so tokens don't get glued, and when
printing power in the xSnprintf call that uses info.powerCurr (inside the
isCharging || isDischarging branch) normalize the sign by printing the absolute
value (use fabs(info.powerCurr) or equivalent) so discharging shows positive
watts consistent with text mode; update the xSnprintf invocations that append to
buf/len accordingly.
In `@darwin/Platform.c`:
- Around line 734-761: The code is incorrectly assigning raw percentage values
(cap_current/cap_max) into Wh fields info->energyCurr and info->energyFull;
remove the two assignments so only info->percent = 100.0 * cap_current / cap_max
is kept and leave info->energyCurr and info->energyFull as NaN (do not populate
them from cap_current/cap_max). Update the block that checks cap_max > 0.0 (the
lines that set info->energyCurr = cap_current; and info->energyFull = cap_max;)
to remove those assignments and keep only the percent calculation.
In `@linux/Platform.c`:
- Around line 1095-1104: Reverse the probe order so sysfs is tried before
procfs: when Platform_Battery_method is BAT_SYS call
Platform_Battery_getSysData(&Platform_Battery_cache) first and if
isNonnegative(Platform_Battery_cache.percent) leave method as BAT_SYS; if that
fails set Platform_Battery_method = BAT_PROC and call
Platform_Battery_getProcData(&Platform_Battery_cache) as a fallback and only
then set Platform_Battery_method = BAT_ERR if percent is still not nonnegative.
Use the existing symbols Platform_Battery_method, Platform_Battery_getSysData,
Platform_Battery_getProcData, Platform_Battery_cache, BAT_SYS, BAT_PROC, BAT_ERR
and isNonnegative to implement this change.
- Around line 841-844: Platform_Battery_getProcData currently replaces a valid
procfs percent with NAN whenever procAcpiCheck() fails; change it to always read
the procfs battery percentage and only set percent to NAN if
Platform_Battery_getProcBatInfo() itself indicates failure. Concretely, call
Platform_Battery_getProcBatInfo() unconditionally to populate info->percent,
assign info->ac = procAcpiCheck() but leave info->ac as AC_ERROR if adapter
detection failed, and do not overwrite a valid percent with NAN based solely on
procAcpiCheck() failing; only set percent to NAN when
Platform_Battery_getProcBatInfo() reports an error.
In `@openbsd/Platform.c`:
- Around line 391-457: The code only calls findDevice("acpibat0", ...) which
collects battery metrics from a single pack; change the logic to iterate over
all acpibat devices (e.g., for i = 0; findDevice(name, mib, &snsrdev, &sdlen);
++i) using a formatted name like "acpibat%d" and accumulate totalFull,
totalRemain and totalPower per-device (the blocks that read SENSOR_WATTHOUR,
SENSOR_INTEGER, SENSOR_WATTS and update batteryFull, batteryRemain,
batteryState, batteryPower) into the existing totals; keep the final
percent/energy/power calculations using the aggregated totals (referencing
findDevice, totalFull, totalRemain, totalPower, and the sysctl queries for
SENSOR_WATTHOUR/SENSOR_WATTS/SENSOR_INTEGER).
In `@pcp/Platform.c`:
- Around line 880-883: The AC state logic is inverted: instead of setting
info->ac = AC_PRESENT when count < 1, set AC_PRESENT when there is at least one
battery (count >= 1) and then override to AC_ABSENT if any battery is
discharging (power < 0). Update the block that currently checks count and
assigns info->ac so the flow is: if count < 1 set a fallback/ERROR state (or
return appropriately), otherwise set info->ac = AC_PRESENT, iterate battery
instances to check power and if any power < 0 set info->ac = AC_ABSENT; apply
the same fix to the analogous block around the second occurrence (the block
referenced at lines ~912-914). Use the existing symbols info->ac, AC_PRESENT,
AC_ABSENT, count and the battery power checks to locate and change the logic.
- Around line 892-893: The code uses batteryEnergyFull[i].d directly as the
CLAMP upper bound which can be negative and cause info->energyCurr to go
negative; compute a non-negative full value first (e.g., double full =
isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0) and then use
CLAMP(batteryEnergyCurr[i].d, 0, full) to update info->energyCurr and add full
(not the raw batteryEnergyFull) to info->energyFull so both updates guard
against negative full-capacity samples (references: info->energyCurr,
info->energyFull, batteryEnergyCurr, batteryEnergyFull, CLAMP, isNonnegative).
---
Outside diff comments:
In `@netbsd/Platform.c`:
- Around line 471-485: The code leaks proplib objects: the dictionary returned
by prop_dictionary_recv_ioctl (dict) and the iterators created by
prop_dictionary_iterator (devIter) and prop_array_iterator (fieldsIter) must be
released on every exit path; update the function so that before jumping to the
error/exit label or returning you call prop_object_release(dict) when dict is
non-NULL and prop_object_iterator_release(devIter) and
prop_object_iterator_release(fieldsIter) when those iterators are non-NULL (also
release any fieldsIter created inside the loop before continuing), ensuring you
don't release objects twice and that fieldsArray/device handling remains
unchanged.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02679205-318c-40a3-9203-a042a4323268
📒 Files selected for processing (21)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdarwin/Platform.hdragonflybsd/Platform.cdragonflybsd/Platform.hfreebsd/Platform.cfreebsd/Platform.hlinux/Platform.clinux/Platform.hnetbsd/Platform.cnetbsd/Platform.hopenbsd/Platform.copenbsd/Platform.hpcp/Metric.hpcp/Platform.cpcp/Platform.hsolaris/Platform.csolaris/Platform.hunsupported/Platform.cunsupported/Platform.h
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 83-88: The block that appends an extra separator (the if (ret &&
len > 2) { *buf++ = ','; *buf++ = ' '; *buf = 0; len -= 2; }) causes a double
comma in TEXT_METERMODE output because the AC prefix written earlier already
includes ", ". Remove this entire conditional block (references: variables ret,
len, buf and the TEXT_METERMODE output path) so no additional ", " is appended;
no other changes are needed to the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5eef6dbc-1164-41ed-9430-663fb5dacd29
📒 Files selected for processing (1)
BatteryMeter.c
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
BatteryMeter.c (1)
137-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize discharging watts sign in compact mode.
Line 140 prints raw
info.powerCurr, so discharging displays negative watts, while text mode (Line 93) shows positive watts for discharging. Keep both modes consistent.Suggested patch
if (isCharging || isDischarging) { ret = xSnprintf( buf, len, "%.1fW @ %.1f/%.1fWh", - info.powerCurr, info.energyCurr, info.energyFull + isDischarging ? -info.powerCurr : info.powerCurr, + info.energyCurr, info.energyFull ); buf += ret; len -= ret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BatteryMeter.c` around lines 137 - 141, Compact-mode watt value uses raw info.powerCurr causing negative watts during discharging; update the xSnprintf call inside the if (isCharging || isDischarging) block to pass a normalized (absolute) watt value so discharging shows positive watts like text mode — e.g. replace the first format argument info.powerCurr with a normalized expression (use -info.powerCurr when isDischarging, or fabsf(info.powerCurr)) while keeping the rest of the arguments (info.energyCurr, info.energyFull) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 49-51: The percent-only fallback in BatteryMeter.c (when
haveEnergy is false) currently writes only "%.1f%%" to this->txtBuffer and
returns, losing AC/battery context; modify the early-return branch where
haveEnergy is checked to include info.ac in the formatted string (use info.ac to
choose an "AC"/"Battery" marker or a suffix/prefix) so that the call that writes
to this->txtBuffer includes both the percent (info.percent) and the AC state
(info.ac) before returning.
---
Duplicate comments:
In `@BatteryMeter.c`:
- Around line 137-141: Compact-mode watt value uses raw info.powerCurr causing
negative watts during discharging; update the xSnprintf call inside the if
(isCharging || isDischarging) block to pass a normalized (absolute) watt value
so discharging shows positive watts like text mode — e.g. replace the first
format argument info.powerCurr with a normalized expression (use -info.powerCurr
when isDischarging, or fabsf(info.powerCurr)) while keeping the rest of the
arguments (info.energyCurr, info.energyFull) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac465c8f-a119-478f-8226-175dae17a47b
📒 Files selected for processing (1)
BatteryMeter.c
In compact mode, better to include the sign (negative for discharging, positive for charging). |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-485:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease proplib objects to prevent memory leak.
dict,devIter, and eachfieldsItermust be released viaprop_object_release()when no longer needed. NetBSD proplib objects are reference-counted and require explicit release—failure to do so leaks memory on every battery refresh cycle in long-running sessions. Release on both success and error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a0ac52a-cfb8-45fa-b14d-6af829fe988e
📒 Files selected for processing (12)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdragonflybsd/Platform.cfreebsd/Platform.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Metric.hpcp/Platform.csolaris/Platform.cunsupported/Platform.c
f8fe874 to
55a56a0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
linux/Platform.c (5)
844-847: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard the procfs percentage when AC detection fails.
Line 846 still forces
percent = NANwheneverprocAcpiCheck()returnsAC_ERROR./proc/acpi/battery/*can hold valid capacity data while/proc/acpi/ac_adapter/*/stateis unreadable. In that casePlatform_getBattery()abandons procfs for no reason. Read the percentage unconditionally and leaveinfo->ac = AC_ERROR.Proposed fix
static void Platform_Battery_getProcData(BatteryInfo* info) { info->ac = procAcpiCheck(); - info->percent = AC_ERROR != info->ac ? Platform_Battery_getProcBatInfo() : NAN; + info->percent = Platform_Battery_getProcBatInfo(); }
1037-1048: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the power sign: unsigned multiplication plus unconditional negation.
Two defects combine here:
- Line 1038 multiplies
batteryCurrent(int64_t) bybatteryVoltage(uint64_t). The signed operand converts to unsigned, so a negativeCURRENT_NOWproduces a huge positivebatteryPower.- Line 1044 then negates the result whenever STATUS is
Discharging. Drivers that already report signedCURRENT_NOWget their sign inverted, whilePOWER_NOWis reported unsigned and does need the STATUS sign.Derive the magnitude first, then apply the STATUS sign once. This keeps the contract in
BatteryMeter.h(negative = discharging) valid for both driver styles.Proposed fix
if (!haveBatteryPower && haveBatteryCurrent && haveBatteryVoltage) { - batteryPower = (batteryCurrent * batteryVoltage) / 1000000; + batteryPower = ((int64_t)(llabs(batteryCurrent) * batteryVoltage)) / 1000000; haveBatteryPower = true; } if (haveBatteryPower) { + if (batteryPower < 0) + batteryPower = -batteryPower; if (batteryIsDischarging) batteryPower = -batteryPower;
1049-1063: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAggregate every AC supply instead of keeping the first result.
Line 1050 returns early once
info->acdiffers fromAC_ERROR, soreaddir()order decides the outcome on systems with several mains supplies. An offline entry visited first pinsinfo->actoAC_ABSENTeven when another supply is online. Line 1056 is also dead: the guard above guaranteesinfo->acis alreadyAC_ERROR.Let
AC_PRESENTwin overAC_ABSENT.Proposed fix
} else if (type == AC) { - if (info->ac != AC_ERROR) - goto next; - char buffer[2]; ssize_t r = Compat_readfileat(entryFd, "online", buffer, sizeof(buffer)); - if (r < 1) { - info->ac = AC_ERROR; + if (r < 1) goto next; - } - - if (buffer[0] == '0') - info->ac = AC_ABSENT; - else if (buffer[0] == '1') + + if (buffer[0] == '1') info->ac = AC_PRESENT; + else if (buffer[0] == '0' && info->ac != AC_PRESENT) + info->ac = AC_ABSENT; }
1072-1076: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCapacity-only batteries still report NAN percent.
A power supply may expose
POWER_SUPPLY_CAPACITYwithout anyENERGY_*orCHARGE_*attribute. That is a valid kernel configuration.totalFullthen stays 0,info->percentstays NAN, andPlatform_getBattery()promotes the method toBAT_ERRalthough a usable percentage was parsed. PublishbatteryLevelas a fallback when the aggregated energy totals are unavailable.This needs an accumulator for the parsed capacity values (for example
levelSumandlevelCountfilled inside theBATbranch), then:Proposed fix
if (totalFull > 0) { info->percent = ((double) totalRemain * 100.0) / (double) totalFull; info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0; + } else if (levelCount > 0) { + /* no energy/charge attributes exposed: fall back to reported capacity */ + info->percent = (double) levelSum / (double) levelCount; }
1099-1108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProbe sysfs before procfs.
Platform_Battery_getProcData()fills onlyacandpercent. It never setspowerCurr,energyCurr, orenergyFull. With the current BAT_PROC-first order, every host that exposes both interfaces keeps the new rate, capacity, and time-estimate fields at NAN, andBatteryMeterfalls back to the percent-only output. Try sysfs first and keep procfs as the fallback for legacy setups.Proposed fix
- if (Platform_Battery_method == BAT_PROC) { - Platform_Battery_getProcData(&Platform_Battery_cache); - if (!isNonnegative(Platform_Battery_cache.percent)) - Platform_Battery_method = BAT_SYS; - } if (Platform_Battery_method == BAT_SYS) { Platform_Battery_getSysData(&Platform_Battery_cache); if (!isNonnegative(Platform_Battery_cache.percent)) + Platform_Battery_method = BAT_PROC; + } + if (Platform_Battery_method == BAT_PROC) { + Platform_Battery_getProcData(&Platform_Battery_cache); + if (!isNonnegative(Platform_Battery_cache.percent)) Platform_Battery_method = BAT_ERR; }The initial value of
Platform_Battery_methodmust becomeBAT_SYSfor this order to take effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e830471-e1d8-4d47-a04e-bdd83ebfab6a
📒 Files selected for processing (1)
linux/Platform.c
55a56a0 to
98e6748
Compare
|
Tested the head of this branch ( Machine: MacBookPro12,1, macOS 12.7.6, Intel, Apple clang 14.0.0 (Command Line Tools 14.2), Build: Runtime, one-column header, discharging on battery: Numbers cross-checked against the registry sampled at the same moment 1. Amperage sign, answering your question at
|
| min | max | |
|---|---|---|
| Voltage | 11022 mV | 11319 mV |
| energyFull as computed | 70.31 Wh | 72.20 Wh |
That is a 1.9 Wh (2.7%) swing in the number presented to the user as the pack's full capacity, with
no physical change in the pack. The sag is largest under CPU load, so the displayed "full capacity"
shrinks exactly when the machine gets busy.
The time estimate is not affected: energyCurr carries the same voltage factor, so voltage cancels
in energyCurr / powerCurr. It is only the two absolute Wh figures that move.
If you want a stable denominator, the fix is a nominal pack voltage instead of the live one. This
pack reports its cells individually, BatteryData.CellVoltage = (3646, 3727, 3727), so cell count
is available and a nominal 3.8 V per cell would give a constant. That does mean hardcoding a
chemistry constant, so it may not be worth it. Documenting the approximation in the existing comment
would also be a defensible answer. Your call, and either way the current code is not wrong, just
noisy.
5. At 80 columns the ETA is the first thing lost
TEXT mode, one column header, discharging. The full line is 85 characters, 74 of them after the
Battery: caption. Rendered at three widths:
cols=80 Battery: Using bat, discharging at 7.3W, 50.0/71.8Wh (73.0%), time remaining
cols=100 Battery: Using bat, discharging at 5.9W, 49.9/71.8Wh (73.0%), time remaining: 8h28m
cols=120 Battery: Using bat, discharging at 5.9W, 49.9/71.8Wh (73.0%), time remaining: 8h28m
At the default 80 column terminal the new number that is hardest to get anywhere else is precisely
the one that gets cut. ETA 8h28m or 8h28m left in place of time remaining: 8h28m would fit at
80 with room to spare.
Everything above is the discharge path. I will follow up with the charging side (sign of
Amperage with the adapter connected, the AC+bat label, and the 95% cutoff in the time-to-full
estimate) once I can measure it on the same machine.
|
Follow-up with the charging path measured on the same machine (MacBookPro12,1, macOS 12.7.6, branch Amperage sign with the adapter connected. Positive, as the PR assumes: Together with the discharging samples in my previous comment (-413 to -2289 mA with no adapter), Meter output while charging, TEXT mode at 110 columns: Cross-checked against the registry at that instant ( The 95% cutoff is visible to the user. At that same moment macOS reported One correction to my previous comment: the |
|
Thank you very much for the very detailed report. The display format is actually still up for debate, as even as it is now, it packs very many information in a quite compact space, while still trying to stay comprehensible for a casual observer. I somewhat thought about the ETA label earlier, but it somehow didn't feel proper, while working on that part of the code. But as mentioned: I'm very much open for ideas on how to condense the text information better. Which brings me to the 95% cutoff: That one has a bit of reasoning behind it, that's not directly documented in the code itself (apart from the calculations that randomly take the 0.95 factor. But to elaborate on the reasoning there are mostly two reasons: The first one is that the charging isn't done in a linear fashion and actually slows down the more charge is already put into the battery. This causes the charging time to always be an underestimate, and might cause the remaining charging time to "increase". The second one is related to the way batteries can be damaged if always kept at full 100% charge. That's why when looking at the charge of your battery even with AC connected, you will often see only like 97-98% charge. If the cutoff was at full 100%, this would cause the battery to show ridiculous ETA times for charging when in fact that BMS mostly keeps the charge at a near-constant level without over-charging the battery. One test that might still be worth performing is putting the system under load with a charger that's too weak for the load. With Thinkpads you can usually operate (and charge) the system with a 65W charger, even when the system under load might take like 80W+. This will cause the battery to discharge despite AC being connected and should be reported by the meter accordingly. Finally, regarding the voltage level: calculating from cell voltage to nominal voltage requires knowledge of the pack topology, which is an entirely different can of worms. Glad to integrate patches for this, but not as part of this first set of patches. This PR is complex enough as it is. And given that most BMS can't even make up their mind about how much capacity their pack actually has, doesn't make this value very reliable either. On my notebook the shown pack capacity changes roughly based on phase of the moon, zodiac sign, number of coffee mugs emptied since last kernel update, Wifi signal strength, and the approximate payout of the retirement plan … |
|
I tried the weak-charger test. It does not reproduce on this machine, so here is the negative result with numbers, plus two things the attempt turned up. The undersized-charger case is not reachable hereFour spinners on a 4-thread i5 (MacBookPro12,1, macOS 12.7.6), The charge current does not even dip. A dual core i5 at full tilt is nowhere near 60W, so the adapter never runs short and the battery never discharges. Reproducing your Thinkpad case needs an adapter weaker than the load, and I do not have one for this machine. For what it is worth the code path looks right by reading: Reading it did raise one thing. The percent and the Wh pair on the same line come from different subsystems
The gap widens from 3.6 to 4.9 points across the run, so it is not a fixed offset a user could learn to discount. Cross-checked against The last row is the one I would fix. The meter prints while A correction to my earlier comment. I wrote that every field matched after cross-checking. Each field does match its own source, but I never checked the percent against the Wh pair, and it did not match even then: 53.70/75.76 is 70.9% where the line displayed 74.0%. The discrepancy was sitting in my own data and I missed it. Your non-linear charging point is right, and it starts earlier than 95%This is the part of your reply I could measure, and it supports the design:
The ETA grows by nine minutes while the battery is filling, exactly the effect you described. After that the current holds near 15W and the estimate falls monotonically to 0h05m. So the tail is real, but on this pack it is the 79 to 84 percent band, where the BMS ramps from 30W down to 15W, and the 95% cutoff does not cover it. Whatever the label ends up saying, a user watching this meter between 80 and 85 percent sees the remaining time going up. One more data point for the nominal voltage question
Observation window was 14:31 to 15:14 local; I did not capture the transition into |
|
Addendum, because every number above came from the charging path and I do not want to leave the impression that this is charge-specific. Same machine a few hours later, on battery with no adapter: 68.9/75.7 is 91.0%. At that instant Two things follow. The divergence is in every branch of the meter rather than only the charging one. And it tracks charge level rather than direction: this discharging sample at raw 91.0% sits on the same curve as the charging samples at raw 90.2% (shown 95.0%) and raw 91.4% (shown 96.0%). So whatever you settle on for the ETA and the label, the percent looks like one change in |
98e6748 to
f7a23a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 31778376-ebed-41bd-b32f-478790913681
📒 Files selected for processing (2)
darwin/Platform.cpcp/Platform.c
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if (cap_max > 0.0) | ||
| *percent = 100.0 * cap_current / cap_max; | ||
| if (cap_max > 0.0) { | ||
| info->percent = 100.0 * cap_current / cap_max; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one authoritative source for percentage and energy values.
Line 733 derives info->percent from IOPS capacity values, while lines 766-767 derive Wh values from AppleSmartBattery raw capacity values. These sources can diverge, so expanded output can show a percentage that conflicts with energyCurr / energyFull.
Use one source for all three values when it is available, and use the other source only as a complete fallback. PR objectives confirm this divergence in hardware testing.
f7a23a1 to
8e6e6a3
Compare
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini Co-authored-by: Nathan Scott <nathans@redhat.com>
Assisted-by: GPT 5.4
Assisted-by: GPT 5.4 DAssisted-by: Raptor mini
3e7be9c to
20cb034
Compare
Assisted-by: GPT 5.4 Assisted-by: GPT 5.6 Luna Assisted-by: Raptor mini
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini
Assisted-by: CodeRabbit Assisted-by: GPT 5.4
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini Co-authored-by: Nathan Scott <nathans@redhat.com>
Assisted-by: CodeRabbit Assisted-by: GPT 5.4 Assisted-by: Raptor mini
Assisted-by: GPT 5.6 Luna
20cb034 to
d56a6df
Compare
|
@coderabbitai Are there any major code issues in this PR left, that need urgent addressing; like actual bugs, instabilities, crashes or other important oversights? Only issues that are newly introduced by code or changes from this PR. Please mention (and reference) still-open issues noted earlier in this PR, that fit this filter. @natoscott Can you check the marked issue regarding PCP and provide a patch for it if necessary? @ravi-arnan Can you please give this updated PR a test-spin on Linux, too? Didn't update the Darwin code regarding the percentage source yet; plan is to pull in those changes in a separate PR, as this one is already complex enough. If you base a PR addressing the percentage issue on this PR, just drop me a note and this might get pulled early. Also maybe a quick note regarding the 95%: fully writing 95% all the times in the UI makes things unnecessarily cluttered without much benefit; a note in the manpage is likely best. Suggestions for wording in the docs much appreciated. @christianhorn Any chance you can test-drive this on some *BSD? Feedback regarding accuracy and (sign) correctness of the values much appreciated. If nothing major comes up I'll likely merge this in about 2 weeks from now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git status --short
gh pr view 1967 --repo htop-dev/htop --json headRefOid,baseRefOid,url
gh api graphql -f query='query { repository(owner:"htop-dev",name:"htop") { pullRequest(number:1967) { reviewThreads(first:100) { nodes { isResolved isOutdated path line comments(first:20) { nodes { author { login } body url } } } pageInfo { hasNextPage endCursor } } } } }'
gh pr diff 1967 --repo htop-dev/htop --name-only
ast-grep outline BatteryMeter.c --items all
for f in linux/Platform.c pcp/Platform.c darwin/Platform.c freebsd/Platform.c dragonflybsd/Platform.c netbsd/Platform.c openbsd/Platform.c; do
ast-grep outline "$f" --match 'Battery' --items all
done
fd -i 'htop.*\.(1|in)$|man' . --max-depth 3Length of output: 119973 🏁 Script executed: #!/bin/bash
git rev-parse HEAD
nl -ba BatteryMeter.c | sed -n '23,180p'
nl -ba linux/Platform.c | sed -n '844,1145p'
nl -ba pcp/Platform.c | sed -n '869,940p'
nl -ba darwin/Platform.c | sed -n '683,787p'
nl -ba freebsd/Platform.c | sed -n '391,520p'
nl -ba dragonflybsd/Platform.c | sed -n '370,510p'
nl -ba netbsd/Platform.c | sed -n '449,660p'
nl -ba openbsd/Platform.c | sed -n '376,545p'
git diff d76b23eeb68515bd9e088a14c98e5c7d658f892f HEAD -- BatteryMeter.c pcp/Platform.c pcp/Metric.h
ast-grep outline pcp/Metric.c --match 'Metric_values|Metric_instanceCount|Metric_desc' --items all
rg -n -C3 'Battery|battery' htop.1.inLength of output: 56962 🏁 Script executed: #!/bin/bash
curl -fsSL https://raw.githubusercontent.com/performancecopilot/pcp/main/src/pmdas/denki/help | grep -n -A12 -B3 'bat.capacity\|bat.energy_now\|bat.power_now'
curl -fsSL https://raw.githubusercontent.com/performancecopilot/pcp/main/src/pmdas/denki/denki.c | grep -n -A10 -B5 'BAT_CAPACITY\|energy_full\|bat.capacity\|power_now\|energy_now'Length of output: 12049 🏁 Script executed: #!/bin/bash
ast-grep outline pcp/Metric.c --items all
ast-grep outline XUtils.c --match 'xSnprintf' --items all
nl -ba pcp/Metric.c | sed -n '15,200p'
rg -n -C4 'PCP_DENKI|Metric_enable|Metric_enabled|Metric_init' pcp
nl -ba XUtils.c | sed -n '279,320p'
rg -n -C2 'txtBuffer\[' Meter.h
git diff d76b23eeb68515bd9e088a14c98e5c7d658f892f HEAD -- linux/Platform.c netbsd/Platform.c openbsd/Platform.c freebsd/Platform.cLength of output: 45768 🏁 Script executed: #!/bin/bash
curl -fsSL https://raw.githubusercontent.com/openbsd/src/master/sys/dev/acpi/acpibat.c | grep -n -A12 -B8 'SENSOR_INTEGER\|sensors\[.*value\|BST_DISCHARGE\|BATT_DISCHARGE\|battery state'Length of output: 2810
I checked head Fix before merge
Open issue that can remain a follow-upThe Darwin percentage-source finding remains valid. It produces contradictory percentage and Wh output. The supplied hardware tests confirm this, but do not show a crash or instability. Given your explicit follow-up plan, I would not classify it as an urgent stability blocker. I used the retrieved project guidance to exclude the withdrawn Linux capacity-only fallback, Linux sign-removal, and PCP AC-state suggestions. Suggested manpage wording
Thanks for the guidance on keeping the 95% explanation in the manpage rather than repeating it in the UI. ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
|
Test-spun on Linux at The host battery ( Method: All four values match what the files imply (energy = chargevoltage/1e6, Caveat: the machine is on AC, so only the charging branch ran. I could not reach |
3 participants
This is the beginning of a series of commits to extend the BatteryMeter to provide additional information like battery capacity, charge rate and (dis)charge time estimates.
For the BSD parts of this PR I'll need some input from the people on these platforms regarding the actual interface to use to get the following values:
Pointers for sample code on each of the different platforms would be nice.