Extend BatteryMeter display#1967
Conversation
BenBE
added
enhancement
|
.oO(Nice, quite a stunt, to cover all of the BSD's with this. :) |
BenBE
force-pushed
the
battery-extension
branch
from
1dc77c5 to
4d3bece
Compare
BenBE
force-pushed
the
battery-extension
branch
2 times, most recently
from
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:
WalkthroughThis PR consolidates battery outputs into a BatteryInfo struct (ac, percent, powerCurr W, energyCurr Wh, energyFull Wh). BatteryMeter now calls Platform_getBattery(&info), validates percent, detects haveEnergy/havePower, classifies charging/discharging/stable, computes whole-minute remaining/to-full estimates, and formats both TEXT_METERMODE and compact displays. Platform implementations (Darwin, FreeBSD, DragonFlyBSD, Linux, NetBSD, OpenBSD, PCP, Solaris, Unsupported) were updated to populate BatteryInfo; PCP also adds Denki metrics. Poem
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
| static void Platform_Battery_getProcData(BatteryInfo* info) { | ||
| info->ac = procAcpiCheck(); | ||
| info->percent = AC_ERROR != info->ac ? Platform_Battery_getProcBatInfo() : NAN; | ||
| } |
There was a problem hiding this comment.
Do not discard procfs percent when AC detection fails.
Line 843 turns any AC-state lookup failure into percent = NAN, which makes Platform_getBattery() abandon procfs even if /proc/acpi/battery/* still contains valid capacity data. Keep the battery percentage and leave info->ac = AC_ERROR when the adapter state is unknown.
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();
}🤖 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 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.
| 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.
| info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d); | ||
| info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0; |
There was a problem hiding this comment.
Guard energyCurr from going negative on invalid full-capacity samples.
Line 892 uses batteryEnergyFull[i].d directly as the clamp upper bound. If that value is negative, energyCurr can become negative. Clamp against a non-negative full first.
Proposed patch
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;
}🤖 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 `@pcp/Platform.c` around lines 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).
|
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
| if (!haveEnergy) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent); | ||
| return; |
There was a problem hiding this comment.
Preserve AC source in the percent-only fallback.
At Line 49-Line 51, the early return prints only %.1f%%. When energy metrics are unavailable, users lose AC/battery context even though info.ac is already known.
Suggested patch
/* Without energy data there is nothing useful to show beyond the percent. */
if (!haveEnergy) {
- xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent);
+ const char* src =
+ (info.ac == AC_PRESENT) ? " (AC)" :
+ (info.ac == AC_ABSENT) ? " (bat)" : "";
+ xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%%s", info.percent, src);
return;
}🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 49-49: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🤖 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 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.
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
| 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; |
There was a problem hiding this comment.
Keep the sysctl percentage as the fallback/authority.
This overwrite is unsafe because the aggregation above is allowed to skip units when ACPIIO_BATT_GET_BIX/_BST fails or voltage data is missing. On multi-battery systems, info->percent can therefore become the percentage of only the reporting subset instead of the overall battery state that hw.acpi.battery.life already exposed.
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;| if (String_eq(field, "CAPACITY")) { | ||
| capacityLevel = val / 100.0; | ||
| batteryLevel = val; | ||
| haveBatteryLevel = true; | ||
| continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Linux power_supply sysfs battery interface: can a battery device expose POWER_SUPPLY_CAPACITY while omitting POWER_SUPPLY_ENERGY_* and POWER_SUPPLY_CHARGE_* attributes?
💡 Result:
Yes, a Linux battery device can expose the POWER_SUPPLY_CAPACITY attribute while omitting POWER_SUPPLY_ENERGY_* and POWER_SUPPLY_CHARGE_* attributes [1][2]. The Linux power supply class is designed to export properties that are directly measurable by the specific hardware available [1][2]. There is no requirement for a driver to implement a mandatory set of attributes beyond what the hardware can actually provide [1][2]. If a monitoring chip or firmware does not provide energy or charge data, those attributes should simply be omitted from the sysfs interface [1][2]. The kernel documentation explicitly states that while the defined attributes are intended to be universally applicable, specific monitoring hardware may not be able to provide them all, and any of them may be skipped [1][2]. Consequently, it is common and acceptable for a device to expose only a subset of properties, such as POWER_SUPPLY_CAPACITY, if that is all the underlying hardware supports [3][1]. Drivers are discouraged from attempting to infer or calculate missing properties using heuristics or mathematical models, as such functionality is considered outside the scope of a kernel battery driver [1][2].
Citations:
- 1: https://www.kernel.org/doc/html/latest/power/power_supply_class.html
- 2: https://docs.kernel.org/6.2/power/power_supply_class.html
- 3: XP-Pen Deco LW support tracking issue DIGImend/digimend-kernel-drivers#635
🏁 Script executed:
cd linux && wc -l Platform.cRepository: htop-dev/htop
Length of output: 73
🏁 Script executed:
sed -n '940,960p' linux/Platform.cRepository: htop-dev/htop
Length of output: 838
🏁 Script executed:
sed -n '1058,1082p' linux/Platform.cRepository: htop-dev/htop
Length of output: 704
🏁 Script executed:
grep -n "haveBatteryLevel" linux/Platform.cRepository: htop-dev/htop
Length of output: 183
🏁 Script executed:
grep -n "batteryLevel" linux/Platform.cRepository: htop-dev/htop
Length of output: 300
🏁 Script executed:
sed -n '1000,1020p' linux/Platform.cRepository: htop-dev/htop
Length of output: 948
🏁 Script executed:
sed -n '1015,1075p' linux/Platform.cRepository: htop-dev/htop
Length of output: 2027
Add fallback to use POWER_SUPPLY_CAPACITY percentage directly when ENERGY_ and CHARGE_ attributes are unavailable.**
The current implementation parses POWER_SUPPLY_CAPACITY but only publishes info->percent from aggregated totalFull/totalRemain. On devices exposing only capacity without energy or charge attributes—a valid scenario per Linux kernel design—totalFull remains zero and info->percent is left uninitialized, returning NAN. This causes Platform_getBattery() to incorrectly disable sysfs even though a valid battery percentage is available.
Add a fallback after the totalFull check (around line 1072) to directly populate info->percent from batteryLevel when it has been successfully parsed but aggregated totals remain unavailable.
Also applies to lines 1068-1072.
| if (info->ac != AC_ERROR) | ||
| goto next; | ||
|
|
||
| char buffer[2]; | ||
| ssize_t r = Compat_readfileat(entryFd, "online", buffer, sizeof(buffer)); | ||
| if (r < 1) { | ||
| *isOnAC = AC_ERROR; | ||
| info->ac = AC_ERROR; | ||
| goto next; | ||
| } | ||
|
|
||
| if (buffer[0] == '0') | ||
| *isOnAC = AC_ABSENT; | ||
| info->ac = AC_ABSENT; | ||
| else if (buffer[0] == '1') | ||
| *isOnAC = AC_PRESENT; | ||
| info->ac = AC_PRESENT; |
There was a problem hiding this comment.
Aggregate all AC supplies before deciding info->ac.
This currently stops after the first readable AC entry, so readdir() order decides the result on systems with multiple mains supplies. If one source is offline and another is online, info->ac can become AC_ABSENT purely because the offline entry was visited first.
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;
}
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
| if (!isNonnegative(info.percent)) { | ||
| this->values[0] = NAN; | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | ||
| return; | ||
| } | ||
|
|
||
| this->values[0] = percent; | ||
|
|
||
| const char* text; | ||
| switch (isOnAC) { | ||
| case AC_PRESENT: | ||
| text = this->mode == TEXT_METERMODE ? " (Running on A/C)" : "(A/C)"; | ||
| break; | ||
| case AC_ABSENT: | ||
| text = this->mode == TEXT_METERMODE ? " (Running on battery)" : "(bat)"; | ||
| break; | ||
| case AC_ERROR: | ||
| default: | ||
| text = ""; | ||
| break; | ||
| this->values[0] = info.percent; |
There was a problem hiding this comment.
Enforce the upper bound on percent here too.
The BatteryInfo contract says percent is [0..100], but this branch only rejects negative/unknown values. Anything above 100 is written straight into this->values[0] even though the meter total is fixed at 100.
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 39-39: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🪛 Cppcheck (2.20.0)
[style] 39-39: The function 'Vector_quickSort' is never used.
(unusedFunction)
| bool haveEnergy = isNonnegative(info.energyCurr) && isNonnegative(info.energyFull); | ||
|
|
||
| /* Without energy data there is nothing useful to show beyond the percent. */ | ||
| if (!haveEnergy) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent); | ||
| return; | ||
| } | ||
|
|
||
| bool havePower = isfinite(info.powerCurr); |
There was a problem hiding this comment.
Do not discard powerCurr when energy is unavailable.
This fallback reduces the output to percent-only even if rate telemetry is present. Per the supplied stack context, Darwin currently provides powerCurr, so this branch makes the new charge/discharge-rate work invisible there. Compute havePower before the early return and show a watt-based fallback when energyCurr/energyFull are missing.
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 49-49: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🪛 Cppcheck (2.20.0)
[style] 53-53: The function 'String_startsWith' is never used.
(unusedFunction)
| } 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); |
There was a problem hiding this comment.
Match the charge ETA label to the 95% cutoff.
The ETA is calculated only until 0.95 * info.energyFull, but the string says time to full. That understates the remaining charge time near the end of the cycle. Either compute against energyFull or relabel the output.
Also applies to: 107-110
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 66-66: The function 'HeaderLayout_getName' is never used.
(unusedFunction)
| 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.
| if (Metric_desc(PCP_DENKI_ENERGY_NOW) == NULL) | ||
| return; |
There was a problem hiding this comment.
Allow partial Denki battery telemetry.
This early return drops info->powerCurr whenever denki.bat.energy_now is unavailable, even if denki.bat.power_now exists. The unified BatteryInfo contract allows unknown energy fields, and BatteryMeter can still render charge/discharge power, so this suppresses valid PCP battery data on partial Denki installations.
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.
What changed
Cleanliness and quality
Public API & impact
Commit structure & scope
Review notes / outstanding items
Verdict: Focused and consistent cross‑platform enhancement that centralizes battery telemetry. Technically clean but broad in scope — requires careful platform‑specific review and coordinated updates for callers due to the public API change.