Extend BatteryMeter display#1967
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
📝 WalkthroughWalkthroughThis PR introduces a unified Suggested reviewers: Poem
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. 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
| if (Platform_Battery_method == BAT_PROC) { | ||
| Platform_Battery_getProcData(percent, isOnAC); | ||
| if (!isNonnegative(*percent)) | ||
| 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(percent, isOnAC); | ||
| if (!isNonnegative(*percent)) | ||
| Platform_Battery_getSysData(&Platform_Battery_cache); | ||
| if (!isNonnegative(Platform_Battery_cache.percent)) | ||
| Platform_Battery_method = BAT_ERR; | ||
| } |
There was a problem hiding this comment.
Prefer sysfs before procfs for the new battery telemetry.
Platform_Battery_getProcData() never fills powerCurr or energy*, so the current BAT_PROC-first flow leaves the new rate/capacity/time-estimate inputs permanently NAN on hosts that expose both procfs and sysfs. Sysfs needs to be the preferred source now, with procfs kept as a fallback for older setups.
🤖 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 `@linux/Platform.c` around lines 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.
| bool found = findDevice("acpibat0", mib, &snsrdev, &sdlen); | ||
|
|
||
| *percent = NAN; | ||
| if (found) { | ||
| bool haveTotalFull = false; | ||
| bool haveTotalRemain = false; | ||
| bool haveTotalPower = false; | ||
|
|
||
| int64_t totalFull = 0; | ||
| int64_t totalRemain = 0; | ||
| int64_t totalPower = 0; | ||
|
|
||
| /* See "sys/dev/acpi/acpibat.c" of OpenBSD source code for the indices | ||
| of the last field. */ | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 0; /* "last full capacity" */ | ||
| double last_full_capacity = 0; | ||
| bool haveBatteryFull = false; | ||
| int64_t batteryFull = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| last_full_capacity = s.value; | ||
| if (last_full_capacity > 0) { | ||
| batteryFull = s.value; | ||
|
|
||
| if (batteryFull > 0) | ||
| haveBatteryFull = true; | ||
|
|
||
| if (haveBatteryFull) { | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 3; /* "remaining capacity" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| double charge = s.value; | ||
| *percent = 100 * (charge / last_full_capacity); | ||
| if (charge >= last_full_capacity) { | ||
| *percent = 100; | ||
| int64_t batteryRemain = s.value; | ||
| if (batteryRemain >= 0) { | ||
| totalRemain += batteryRemain; | ||
| totalFull += batteryFull; | ||
| haveTotalRemain = true; | ||
| haveTotalFull = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (haveTotalRemain && haveTotalFull && totalFull > 0) { | ||
| info->percent = ((double) totalRemain * 100.0) / (double) totalFull; | ||
| if (totalRemain >= totalFull) | ||
| info->percent = 100; | ||
|
|
||
| info->energyCurr = (double) totalRemain / 1000000.0; | ||
| info->energyFull = (double) totalFull / 1000000.0; | ||
| } | ||
|
|
||
| mib[3] = SENSOR_INTEGER; | ||
| mib[4] = 0; /* "battery state" */ | ||
| int64_t batteryState = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| batteryState = s.value; | ||
|
|
||
| mib[3] = SENSOR_WATTS; | ||
| mib[4] = 0; /* "rate" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| int64_t batteryPower = s.value; | ||
| if (batteryState & 0x01) | ||
| batteryPower = -batteryPower; | ||
|
|
||
| totalPower += batteryPower; | ||
| haveTotalPower = true; | ||
| } | ||
|
|
||
| if (haveTotalPower) { | ||
| info->powerCurr = (double) totalPower / 1000000.0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't hardcode a single battery device.
Line 391 only queries acpibat0, so totalFull, totalRemain, and totalPower can never include any other battery device even though this code now derives aggregate battery totals from them. On multi-battery hardware, the reported percent, Wh, and time estimates will reflect only the first pack.
🤖 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 `@openbsd/Platform.c` around lines 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).
There was a problem hiding this comment.
Can you point to documentation where it describes how to best find the number of available devices and how they are named?
There was a problem hiding this comment.
@BenBE — here are the relevant references:
Official documentation:
acpibat(4)— describes the driver; battery status is accessible viasysctl(8)orsensorsd(8).sysctl(2)— documents theHW_SENSORSMIB layout: the third level is an array ofstruct sensordevstructures for each sensor device, and the third/fourth/fifth levels together address individualstruct sensorreadings. The third level is the numeric device index whosexnameis matched against the device name.
The enumeration protocol (already implemented in your findDevice()) is: iterate mib[2] from 0; ENXIO means no device at this slot (skip); ENOENT means no more devices (stop). Batteries are named acpibat0, acpibat1, etc.
Real-world confirmation:
A two-battery system exposes hw.sensors.acpibat0.* and hw.sensors.acpibat1.* as sibling sensordev entries with identical sensor indices.
Prior-art for the iteration pattern:
The distatus/battery library on OpenBSD does exactly this: it loops over consecutive MIB indices, calls sysctl for each sensordev, breaks on ENOENT, and checks whether xname has the prefix "acpibat".
Suggested adaptation for Platform_getBattery: instead of calling findDevice("acpibat0", ...) once, inline the sensordev scan and match any xname with strncmp(snsrdev.xname, "acpibat", 7) == 0 (or equivalent), accumulating totalFull/totalRemain/totalPower across all matching devices before computing the final percent/energy/power fields.
|
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
There was a problem hiding this comment.
Actionable comments posted: 6
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-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource leak: proplib objects never released.
dict,devIter, andfieldsIterare allocated but never freed. ThefieldsIteris created each loop iteration without release. Per proplib API, these requireprop_object_release()andprop_object_iterator_release().Proposed fix
void Platform_getBattery(BatteryInfo* info) { - prop_dictionary_t dict, fields, props; + prop_dictionary_t dict = NULL; + prop_dictionary_t fields, props; prop_object_t device, class; + prop_object_iterator_t devIter = NULL; // ... initialization unchanged ... if (prop_dictionary_recv_ioctl(fd, ENVSYS_GETDICTIONARY, &dict) != 0) - goto error; + goto cleanup; - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) - goto error; + goto cleanup; while ((device = prop_object_iterator_next(devIter)) != NULL) { prop_object_t fieldsArray = prop_dictionary_get_keysym(dict, device); if (fieldsArray == NULL) - goto error; + goto cleanup; prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) - goto error; + goto cleanup; // ... process fields ... + prop_object_iterator_release(fieldsIter); } -error: +cleanup: + if (devIter) + prop_object_iterator_release(devIter); + if (dict) + prop_object_release(dict); if (fd != -1) close(fd); }
♻️ Duplicate comments (2)
dragonflybsd/Platform.c (1)
479-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer sysctl percent as authoritative when aggregation is incomplete.
The aggregation loop may skip battery units on ioctl failure or missing voltage data. Overwriting
info->percentwith the partial aggregate can produce an incorrect percentage compared to the kernel-reportedhw.acpi.battery.lifevalue already stored at line 382. Only setinfo->percentfrom the aggregate when the sysctl failed (i.e.,info->percentis stillNAN).Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100.0; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0;freebsd/Platform.c (1)
507-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sysctl percent as fallback for incomplete aggregation.
Same issue as DragonFlyBSD: the loop can skip units when
ACPIIO_BATT_GET_BIXorACPIIO_BATT_GET_BSTfails. The kernel-reportedhw.acpi.battery.life(already stored at line 415) is more reliable than a partial aggregate. Only overwriteinfo->percentwhen the sysctl value is unavailable.Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100.0; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0881be70-ec0a-435f-9e26-53a032962ece
📒 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
| ACPresence ac; | ||
|
|
||
| double percent; /* [0..100], NAN if unknown */ | ||
| double powerCurr; /* instantaneous power in W, NAN if unknown */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the powerCurr sign contract.
BatteryMeter_updateValues() derives charging vs. discharging entirely from this field's sign, but the header only documents the unit. Please state the convention here explicitly, e.g. positive while charging and negative while discharging, so platform collectors implement the same contract.
| if (!haveBatteryPower && haveBatteryCurrent && haveBatteryVoltage) { | ||
| batteryPower = (batteryCurrent * batteryVoltage) / 1000000; | ||
| haveBatteryPower = true; | ||
| } | ||
|
|
||
| if (!now && full && isNonnegative(capacityLevel)) | ||
| totalRemain += capacityLevel * fullCharge; | ||
| if (haveBatteryPower) { | ||
| if (batteryIsDischarging) | ||
| batteryPower = -batteryPower; | ||
|
|
||
| totalPower += batteryPower; | ||
| havePower = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Linux kernel documentation and driver conventions for CURRENT_NOW signedness
rg -n -C3 "CURRENT_NOW" --type=c | head -80
# Search for any existing handling of signed current in power_supply drivers
ast-grep --pattern 'POWER_SUPPLY_PROP_CURRENT_NOW'Repository: htop-dev/htop
Length of output: 399
🏁 Script executed:
#!/bin/bash
# Get the full context of the battery power calculation
echo "=== Checking variable declarations and initialization ==="
rg -n "batteryCurrent|batteryPower|batteryIsDischarging|batteryVoltage" linux/Platform.c | head -40
echo -e "\n=== Full battery reading section ==="
sed -n '950,1050p' linux/Platform.cRepository: htop-dev/htop
Length of output: 4456
🏁 Script executed:
#!/bin/bash
# Check how val is parsed - does it handle negative numbers?
echo "=== Checking val parsing context ==="
sed -n '900,950p' linux/Platform.c
# Look for any comments about battery or current in the file
echo -e "\n=== Comments about battery/power handling ==="
rg -n "battery|CURRENT|current" linux/Platform.c | grep -i "comment\|negative\|discharge" | head -20Repository: htop-dev/htop
Length of output: 1742
🌐 Web query:
Linux kernel POWER_SUPPLY_PROP_CURRENT_NOW sysfs convention signedness negative discharge
💡 Result:
In the Linux kernel power supply class, the sysfs attribute POWER_SUPPLY_PROP_CURRENT_NOW follows a specific convention regarding signedness to distinguish between charging and discharging states [1][2]. According to the official kernel documentation, the convention is as follows: 1. Signedness: The value is treated as a signed integer [1][2]. 2. Negative Values: Negative values are used to represent battery discharging (i.e., current flowing out of the battery to power a load) [1][2]. 3. Positive Values: Positive values are used to represent battery charging (i.e., current flowing into the battery) [1][2]. This convention allows user-space applications to determine the direction of current flow for battery-backed devices by checking the sign of the integer reported in the current_now sysfs file [1][2]. For other types of power supplies, such as USB IBUS, positive values are typically used to report the current being supplied [1][2]. Drivers are responsible for converting their raw hardware readings into these standardized units (microamperes) and applying the correct sign based on the operational state [3][4].
Citations:
- 1: https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-power
- 2: https://kernel.googlesource.com/pub/scm/linux/kernel/git/rzhang/linux/+/bb48a59135926ece9b1361e8b96b33fc658830bc/Documentation/ABI/testing/sysfs-class-power
- 3: https://kernel.org/doc/html/v5.18/power/power_supply_class.html
- 4: https://unix.stackexchange.com/questions/726822/understanding-power-supply-files
🏁 Script executed:
#!/bin/bash
# Check how totalPower is used and any sign convention documentation
echo "=== Usage of totalPower after calculation ==="
sed -n '1040,1100p' linux/Platform.c
echo -e "\n=== Look for any function comments about sign convention ==="
sed -n '850,920p' linux/Platform.c | grep -A5 "^[[:space:]]*//.*power\|^[[:space:]]*/*.*power"
# Check if there's any test or usage that shows expected sign convention
echo -e "\n=== Search for power-related struct/API that shows sign expectation ==="
rg -n "totalPower|batteryPower" linux/Platform.c | tail -20Repository: htop-dev/htop
Length of output: 2333
Remove the STATUS-based negation; CURRENT_NOW is already signed per Linux kernel convention.
The Linux power supply API reports CURRENT_NOW as a signed value where negative indicates discharge and positive indicates charge. Since batteryCurrent is read directly from CURRENT_NOW (line 1034), the power calculation already has the correct sign. The subsequent negation at line 1040 based on batteryIsDischarging inverts this, causing discharge to report positive power.
Remove the sign negation or check whether batteryCurrent is already negative before applying the STATUS-based correction.
368e9fb to
876564f
Compare
There was a problem hiding this comment.
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-476:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProplib iterator not released before return.
devIteris created at line 474 but never released. When jumping toerror:or reaching the end of iteration,prop_object_iterator_release(devIter)should be called. Similarly forfieldsItercreated at line 483.Proposed fix
+ prop_object_iterator_t devIter = NULL; + prop_object_iterator_t fieldsIter = NULL; + int fd = open(_PATH_SYSMON, O_RDONLY); if (fd == -1) goto error; if (prop_dictionary_recv_ioctl(fd, ENVSYS_GETDICTIONARY, &dict) != 0) goto error; - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) goto error; while ((device = prop_object_iterator_next(devIter)) != NULL) { // ... - prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); + fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) goto error; // ... loop body ... + prop_object_iterator_release(fieldsIter); + fieldsIter = NULL; } + prop_object_iterator_release(devIter); + devIter = NULL; // ... percent/power calculations ... error: + if (fieldsIter) + prop_object_iterator_release(fieldsIter); + if (devIter) + prop_object_iterator_release(devIter); + if (dict) + prop_object_release(dict); if (fd != -1) close(fd); }Also applies to: 637-640
♻️ Duplicate comments (2)
pcp/Platform.c (1)
891-894:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
energyCurrfrom invalid full-capacity samples.Line 892 uses
batteryEnergyFull[i].ddirectly as the CLAMP upper bound. If that value is negative,energyCurraccumulates an incorrect contribution while line 893 correctly excludes it fromenergyFull. Compute the sanitized full value once and use it consistently:Proposed fix
info->energyCurr = 0.0; info->energyFull = 0.0; for (i = 0; i < count; i++) { - info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d); - info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0; + double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0.0; + info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0.0, full); + info->energyFull += full; }linux/Platform.c (1)
841-844:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProcfs percent discarded when AC detection fails.
Line 843 sets
info->percent = NANwhenprocAcpiCheck()returnsAC_ERROR, even though/proc/acpi/battery/*may still contain valid capacity data. The AC adapter state and battery capacity are independent data sources.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(); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9b26473-03af-404a-85f4-d4af978bc086
📒 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
BatteryMeter.h (1)
21-29: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the
powerCurrsign convention.Still undocumented: only the unit is specified, not the sign contract.
BatteryMeter_updateValues()derives charging/discharging purely frompowerCurr's sign (negative = discharging, positive = charging per BatteryMeter.c logic), so every platform collector must implement this convention identically. State it explicitly in the comment.Suggested doc fix
- double powerCurr; /* instantaneous power in W, NAN if unknown */ + double powerCurr; /* instantaneous power in W, NAN if unknown; + positive while charging, negative while discharging */BatteryMeter.c (3)
38-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp
percentto the documented[0..100]range.
isNonnegativeonly rejects negative/NAN values; anything above 100 flows straight intothis->values[0]even though the meter'stotalis fixed at 100 and the header documents[0..100].Suggested fix
this->values[0] = info.percent; + this->values[0] = CLAMP(this->values[0], 0.0, 100.0);
46-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPercent-only fallback drops AC state and available power telemetry.
When
!haveEnergy, the function returns showing only%.1f%%, discarding bothinfo.accontext and any usableinfo.powerCurr(per the stack context, Darwin can supply power without full/current energy). This makes the new charge/discharge-rate work invisible on platforms that only expose power, and hides AC/battery context users previously had.Suggested fix
- if (!haveEnergy) { - xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent); - return; - } + bool havePowerOnly = !haveEnergy && isfinite(info.powerCurr); + if (!haveEnergy) { + const char* src = + (info.ac == AC_PRESENT) ? " (AC)" : + (info.ac == AC_ABSENT) ? " (bat)" : ""; + if (havePowerOnly) + xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%% (%+.1fW)%s", info.percent, info.powerCurr, src); + else + xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%%s", info.percent, src); + return; + }
61-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCharging ETA still targets 95%, but the label says "time to full" (also applies to 107-113).
timeMinutesfor charging is computed against0.95 * info.energyFull, while the corresponding display at lines 107-113 reads "time to full". A prior review flagged this exact mismatch and it's marked as addressed in an earlier commit, but the code here still reproduces the same pattern — please confirm whether this reflects an intentional design (e.g., treating 95% as practically "full" due to trickle-charge tail) that simply needs a comment, or whether the fix regressed.Suggested fix (if unintentional)
- } else if (isCharging && 0.95 * info.energyFull > info.energyCurr) { - /* ceil for charge */ - timeMinutes = (int)ceil((0.95 * info.energyFull - info.energyCurr) / info.powerCurr * 60.0); + } else if (isCharging && info.energyFull > info.energyCurr) { + /* ceil for charge */ + timeMinutes = (int)ceil((info.energyFull - info.energyCurr) / info.powerCurr * 60.0);dragonflybsd/Platform.c (1)
479-486: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't overwrite the system percentage with a partial energy aggregate.
info->percentwas already set fromhw.acpi.battery.life(line 382), a whole-system value. This block recomputes it fromtotalRemain/totalFull, which the loop populates only for units that pass everyioctl/voltage guard. On a multi-battery host where a subset fails, a valid system percentage gets replaced by a partial one. Guard the assignment onisnan(info->percent); keep the energy assignments unconditional.Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100.0; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0; }pcp/Platform.c (2)
876-877: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPartial Denki telemetry still dropped.
Unchanged from the prior review: this early return discards
info->powerCurrwheneverdenki.bat.energy_nowis unavailable, even ifdenki.bat.power_nowexists. TheBatteryInfocontract tolerates unknown energy fields, so a Denki install exposing only power should still surface it.
892-893: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
energyCurrclamp still uses unguardedbatteryEnergyFull[i].d.Unchanged from the prior review: a negative
batteryEnergyFull[i].dsample letsCLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d)go negative. Clamp against a non-negativefullfirst, as previously proposed.🐛 Proposed fix
info->energyCurr = 0.0; info->energyFull = 0.0; for (i = 0; i < count; i++) { - info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d); - info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0; + double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0.0; + info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0.0, full); + info->energyFull += full; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07de2fe8-b13a-4184-af4d-b61f5adbd424
📒 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
| if (info->energyFull > 0) { | ||
| info->percent = CLAMP((info->energyCurr / info->energyFull) * 100.0, 0.0, 100.0); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Omit braces around trivial single-statement if bodies.
Both blocks wrap a single trivial assignment; the styleguide requires omitting braces in this case.
As per coding guidelines: "Omit braces around simple single statements (return, break, continue, trivial assignments)."
♻️ Proposed fix
if (info->energyFull > 0) {
- info->percent = CLAMP((info->energyCurr / info->energyFull) * 100.0, 0.0, 100.0);
- }
+ info->percent = CLAMP((info->energyCurr / info->energyFull) * 100.0, 0.0, 100.0);
@@
- if (info->powerCurr < 0) {
- info->ac = AC_ABSENT;
- }
+ if (info->powerCurr < 0)
+ info->ac = AC_ABSENT;Also applies to: 912-914
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43d72b57-9ded-4c8a-bf00-53ae6fe836bb
📒 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
| int life; | ||
| size_t life_len = sizeof(life); | ||
| if (sysctlbyname("hw.acpi.battery.life", &life, &life_len, NULL, 0) == -1) | ||
| *percent = NAN; | ||
| else | ||
| *percent = life; | ||
| if (sysctlbyname("hw.acpi.battery.life", &life, &life_len, NULL, 0) != -1) | ||
| info->percent = life; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does DragonFly BSD hw.acpi.battery.life sysctl return -1 when battery capacity is unknown?
💡 Result:
In DragonFly BSD, the sysctl hw.acpi.battery.life typically returns -1 when the battery capacity or status is unknown, unavailable, or when the battery is not detected [1]. The sysctl implementation in DragonFly BSD shares much of its codebase with FreeBSD's ACPI battery driver [2][3]. When the system's ACPI subsystem cannot retrieve a valid battery capacity—for example, if the battery is not plugged in, or the hardware fails to report information—it commonly initializes or resets these values to -1 to indicate an error or an unknown state [1]. Users monitoring battery status through system tools may observe this value when the kernel fails to get a successful reading from the ACPI battery interface [1][4].
Citations:
- 1: https://forums.freebsd.org/threads/battery-life-indicator.6568/
- 2: https://lists.dragonflybsd.org/pipermail/commits/2014-September/857187.html
- 3: https://www.leidinger.net/FreeBSD/dox/dev_acpica/html/df/da3/acpi__battery_8c.html
- 4: https://lists.dragonflybsd.org/pipermail/users/2021-April/451723.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and nearby consumers
git ls-files 'dragonflybsd/Platform.c' '*/Platform.c' '*/Machine.c' '*/Process*.c' '*/Battery*' '*/*.c' | sed -n '1,120p'
printf '\n--- dragonflybsd/Platform.c around the battery code ---\n'
sed -n '360,520p' dragonflybsd/Platform.c
printf '\n--- search for isNonnegative and percent fallback logic ---\n'
rg -n "isNonnegative|isnan\\(info->percent\\)|info->percent = life|battery.life|ACPIIO_BATT" -S .Repository: htop-dev/htop
Length of output: 8797
Guard hw.acpi.battery.life against -1. sysctlbyname() can succeed with an unknown battery value, but -1 is not a usable percent here: it blocks the ACPI energy fallback and later shows up as N/A via isNonnegative(). Only assign values in the 0–100 range.
| if (energyCount > 0) { | ||
| pmAtomValue* batteryEnergyCurr = xCalloc(energyCount, sizeof(pmAtomValue)); | ||
| pmAtomValue* batteryEnergyFull = xCalloc(energyCount, sizeof(pmAtomValue)); | ||
| if (Metric_values(PCP_DENKI_ENERGY_NOW, batteryEnergyCurr, energyCount, PM_TYPE_DOUBLE) && | ||
| Metric_values(PCP_DENKI_ENERGY_FULL, batteryEnergyFull, energyCount, PM_TYPE_DOUBLE)) { | ||
| info->energyCurr = 0.0; | ||
| info->energyFull = 0.0; | ||
| for (int i = 0; i < energyCount; i++) { | ||
| double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0.0; | ||
| info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0.0, full); | ||
| info->energyFull += full; | ||
| } | ||
|
|
||
| if (info->energyFull > 0) { | ||
| info->percent = CLAMP((info->energyCurr / info->energyFull) * 100.0, 0.0, 100.0); | ||
| } | ||
| } | ||
| free(batteryEnergyCurr); | ||
| free(batteryEnergyFull); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and nearby context.
for f in pcp/Metric.h pcp/Platform.c; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== pcp/Metric.h (around lines 100-120) ====="
sed -n '100,120p' pcp/Metric.h
echo "===== pcp/Platform.c (around mappings near lines 230-245) ====="
sed -n '230,245p' pcp/Platform.c
echo "===== pcp/Platform.c (around battery aggregation near lines 880-920) ====="
sed -n '880,920p' pcp/Platform.c
# Search for denki references and any documentation/comments about capacity semantics.
echo "===== grep denki ====="
rg -n "denki\.bat\.(power_now|energy_now|energy_full|capacity)|pmda-denki|fillstate|percent" -S .Repository: htop-dev/htop
Length of output: 41367
🌐 Web query:
pmda-denki denki.bat.capacity fillstate percent energy_now documentation
💡 Result:
pmda-denki is a Performance Metrics Domain Agent (PMDA) for the Performance Co-Pilot (PCP) suite, designed to collect and expose electrical power consumption and battery metrics on Linux systems [1][2]. Regarding the specific battery metrics mentioned, the following details are documented: denki.bat.capacity This metric reports the battery charge level as a percentage [3]. denki.bat.energy_now This metric shows the current battery charge [3]. It is often used to represent the raw energy level (frequently in Watt-hours) read from the system filesystem [4]. Related Metrics While you mentioned "fillstate," the official documentation and handbook identify the following core metrics for battery management: - denki.bat.power_now: Represents the current consumption or charge rate of the battery, typically updated by firmware [3][4]. - denki.bat.energy_now_rate: This is a computed metric calculated by pmda-denki itself, representing the rate of charging or discharging in 60-second intervals [3][4]. pmda-denki is generally installed and managed via the PCP infrastructure (e.g., using the Install/Remove scripts located in $PCP_PMDAS_DIR/denki) [1][2]. It is designed to work with system-provided power data, such as Intel's RAPL (Running Average Power Limit) interface for CPU power and standard battery sysfs interfaces for battery data [1][5]. For further details, the authoritative documentation can be found in the pmda-denki handbook [3][6].
Citations:
- 1: https://man7.org/linux/man-pages/man1/pmdadenki.1.html
- 2: https://denki.fluxcoil.net/
- 3: https://fluxcoil.net/files/pmda-denki-handbook-publish/denki.html
- 4: https://blog.fluxcoil.net/posts/2021/07/measuring-power-consumption-on-linux2/
- 5: https://blog.fluxcoil.net/posts/2021/06/measuring-power-consumption-on-linux/
- 6: https://fluxcoil.net/wiki/software/performance_co-pilot/pmda-denki
denki.bat.capacity is a percentage, not an energy-capacity metric
pcp/Metric.h and pcp/Platform.c map PCP_DENKI_ENERGY_FULL to denki.bat.capacity, then pcp/Platform.c#L891-L909 treats it as Wh and derives energyFull/percent from mixed units. Rework this to use a real full-capacity metric, or leave energyFull/ETA unset when only fill-state is available.
📍 Affects 2 files
pcp/Platform.c#L891-L909(this comment)pcp/Metric.h#L110-L112pcp/Platform.c#L237-L239
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
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: 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
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
freebsd/Platform.c (1)
495-498: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOverwrite clobbers the authoritative
hw.acpi.battery.lifepercentage.
info->percentis set fromhw.acpi.battery.lifeat Line 403 (overall battery state). Lines 496-498 unconditionally overwrite it with the aggregated subset, which is populated only whenACPIIO_BATT_GET_BIX/_BSTsucceed and voltage data is present. On multi-battery or partial-read systems the aggregate reflects only the reporting units, degrading a correct reading. The dragonflybsd sibling guards this overwrite withisnan(info->percent); freebsd is missing the same guard.Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0; }linux/Platform.c (1)
844-846: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't discard a valid procfs percent when AC detection fails.
Line 846 sets
info->percent = NANwheneverprocAcpiCheck()returnsAC_ERROR, causingPlatform_getBattery()to abandon procfs even when/proc/acpi/battery/*still holds valid capacity data. Read the battery percentage unconditionally and leaveac = AC_ERRORonly for the adapter state.Suggested 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(); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 200d9016-31d0-41d3-82d5-28c60d392b9a
📒 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
| if (!haveBatteryPower && haveBatteryCurrent && haveBatteryVoltage) { | ||
| batteryPower = (batteryCurrent * batteryVoltage) / 1000000; | ||
| haveBatteryPower = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Linux power_supply CURRENT_NOW sysfs negative value discharge sign convention
💡 Result:
According to the Linux kernel ABI documentation for the power supply class, the sign convention for current readings (such as current_now) is explicitly defined: negative values indicate that the battery is discharging, while positive values indicate that the battery is charging [1][2]. Specifically, for current_now, the documentation states that "negative values are used for discharging batteries, positive values for charging batteries and for USB IBUS current" [1]. While this is the established standard, it is important to note that historically, there has been inconsistency across different kernel drivers [2]. Some drivers may incorrectly report positive values for discharging or handle current direction differently due to hardware-specific limitations or legacy implementation choices [2][3]. The kernel community has actively worked to standardize this behavior to ensure that userspace applications can rely on a consistent sign convention [2][4]. If you encounter a driver that does not follow this convention, it is considered a driver-level issue or limitation rather than an ambiguity in the ABI specification itself [2][4]. Developers are encouraged to ensure drivers adhere to the "negative = discharging" convention to maintain consistency across the ecosystem [2][4].
Citations:
- 1: https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-power
- 2: https://lists.openwall.net/linux-kernel/2020/08/27/830
- 3: sysfs current_now doesn't report negative values during discharge system76/ec#468
- 4: https://www.spinics.net/lists/linux-acpi/msg132899.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant code and type declarations.
grep -nR "batteryCurrent\|batteryVoltage\|batteryPower" linux/Platform.c | head -n 40
echo
echo "--- linux/Platform.c around the reported lines ---"
sed -n '1010,1055p' linux/Platform.c
echo
echo "--- broader declarations in linux/Platform.c ---"
grep -nR "int64_t batteryCurrent\|uint64_t batteryVoltage\|batteryCurrent =" linux/Platform.c | head -n 40Repository: htop-dev/htop
Length of output: 2834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Linux power-supply current parsing path and confirm the type flow.
rg -n "CURRENT_NOW|current_now|batteryCurrent|batteryVoltage" linux/Platform.c linux -g '!**/build/**' -g '!**/dist/**'
echo
echo "--- linux/Platform.c surrounding battery power computation ---"
sed -n '990,1050p' linux/Platform.cRepository: htop-dev/htop
Length of output: 3792
Cast batteryVoltage to int64_t before multiplying. batteryCurrent may be negative for discharge, but mixing it with the unsigned voltage forces unsigned arithmetic and mangles the computed power. The current fix is just the cast on the right-hand operand.
| if (totalFull > 0) { | ||
| info->percent = ((double) totalRemain * 100.0) / (double) totalFull; | ||
| info->energyCurr = (double) totalRemain / 1000000.0; | ||
| info->energyFull = (double) totalFull / 1000000.0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Capacity-only batteries still yield NAN percent.
When a sysfs battery exposes POWER_SUPPLY_CAPACITY but neither ENERGY_* nor CHARGE_* fulls (a valid kernel configuration), totalFull stays 0 and info->percent remains NAN, so Platform_getBattery() switches the method to BAT_ERR despite a usable percentage. Add a fallback that publishes batteryLevel directly when aggregated totals are unavailable.
| bool found = findDevice("acpibat0", mib, &snsrdev, &sdlen); | ||
|
|
||
| *percent = NAN; | ||
| if (found) { | ||
| bool haveTotalFull = false; | ||
| bool haveTotalRemain = false; | ||
| bool haveTotalPower = false; | ||
|
|
||
| int64_t totalFull = 0; | ||
| int64_t totalRemain = 0; | ||
| int64_t totalPower = 0; | ||
|
|
||
| /* See "sys/dev/acpi/acpibat.c" of OpenBSD source code for the indices | ||
| of the last field. */ | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 0; /* "last full capacity" */ | ||
| double last_full_capacity = 0; | ||
| bool haveBatteryFull = false; | ||
| int64_t batteryFull = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| last_full_capacity = s.value; | ||
| if (last_full_capacity > 0) { | ||
| batteryFull = s.value; | ||
|
|
||
| if (batteryFull > 0) | ||
| haveBatteryFull = true; | ||
|
|
||
| if (haveBatteryFull) { | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 3; /* "remaining capacity" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| double charge = s.value; | ||
| *percent = 100 * (charge / last_full_capacity); | ||
| if (charge >= last_full_capacity) { | ||
| *percent = 100; | ||
| int64_t batteryRemain = s.value; | ||
| if (batteryRemain >= 0) { | ||
| totalRemain += batteryRemain; | ||
| totalFull += batteryFull; | ||
| haveTotalRemain = true; | ||
| haveTotalFull = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (haveTotalRemain && haveTotalFull && totalFull > 0) { | ||
| info->percent = ((double) totalRemain * 100.0) / (double) totalFull; | ||
| if (totalRemain >= totalFull) | ||
| info->percent = 100; | ||
|
|
||
| info->energyCurr = (double) totalRemain / 1000000.0; | ||
| info->energyFull = (double) totalFull / 1000000.0; | ||
| } | ||
|
|
||
| mib[3] = SENSOR_INTEGER; | ||
| mib[4] = 0; /* "battery state" */ | ||
| int64_t batteryState = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| batteryState = s.value; | ||
|
|
||
| mib[3] = SENSOR_WATTS; | ||
| mib[4] = 0; /* "rate" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| int64_t batteryPower = s.value; | ||
| if (batteryState & 0x01) | ||
| batteryPower = -batteryPower; | ||
|
|
||
| totalPower += batteryPower; | ||
| haveTotalPower = true; | ||
| } | ||
|
|
||
| if (haveTotalPower) { | ||
| info->powerCurr = (double) totalPower / 1000000.0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Aggregation only ever sees acpibat0.
The block builds totalFull/totalRemain/totalPower as if summing multiple packs, but findDevice("acpibat0", ...) restricts it to the first battery. On multi-battery hardware percent, Wh, and time estimates reflect only that pack. Enumerate all acpibat* sensordevs (iterate mib[2], match xname prefix "acpibat") and accumulate across matches, as discussed on the prior review thread.
2 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.