Skip to content
146 changes: 127 additions & 19 deletions BatteryMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,142 @@ static const int BatteryMeter_attributes[] = {
};

static void BatteryMeter_updateValues(Meter* this) {
ACPresence isOnAC;
double percent;
BatteryInfo info = {
.ac = AC_ERROR,
.percent = NAN,
.powerCurr = NAN,
.energyCurr = NAN,
.energyFull = NAN,
};

Platform_getBattery(&percent, &isOnAC);
Platform_getBattery(&info);

if (!isNonnegative(percent)) {
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;
Comment on lines +38 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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;
Comment on lines +49 to +51

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

}

bool havePower = isfinite(info.powerCurr);
Comment on lines +46 to +54

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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)


/* stable: power unknown or |power| < 5 W – time estimate would be unreliable */
bool isDischarging = havePower && info.powerCurr <= -5.0;
bool isCharging = havePower && info.powerCurr >= 5.0;

/* time estimate in whole minutes; -1 means not available */
int timeMinutes = -1;
if (isDischarging && isPositive(info.energyCurr)) {
/* floor for discharge; powerCurr is negative, use negative scaling when dividing */
timeMinutes = (int)floor(info.energyCurr / info.powerCurr * -60.0);
} 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);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

char* buf = this->txtBuffer;
size_t len = sizeof(this->txtBuffer);
int ret = 0;

if (this->mode == TEXT_METERMODE) {
if (info.ac == AC_PRESENT) {
ret = xSnprintf(buf, len, "Using %s", isDischarging ? "AC+bat" : "AC");
buf += ret; len -= ret;
} else if (info.ac == AC_ABSENT) {
ret = xSnprintf(buf, len, "Using bat");
buf += ret; len -= ret;
}

if (ret && len > 2) {
*buf++ = ',';
*buf++ = ' ';
*buf = 0;
len -= 2;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if (isDischarging) {
ret = xSnprintf(
buf, len, "discharging at %.1fW, %.1f/%.1fWh (%.1f%%)",
-info.powerCurr, info.energyCurr, info.energyFull, info.percent
);
buf += ret; len -= ret;
if (timeMinutes >= 0) {
ret = xSnprintf(buf, len, ", time remaining: %dh%02dm", timeMinutes / 60, timeMinutes % 60);
buf += ret; len -= ret;
}
} else if (isCharging) {
ret = xSnprintf(
buf, len, "charging at %.1fW, %.1f/%.1fWh (%.1f%%)",
info.powerCurr, info.energyCurr, info.energyFull, info.percent
);
buf += ret; len -= ret;

if (timeMinutes >= 0) {
ret = xSnprintf(
buf, len, ", time to full: %dh%02dm",
timeMinutes / 60, timeMinutes % 60
);
buf += ret; len -= ret;
}
} else {
ret = xSnprintf(
buf, len, "stable at %.1f/%.1fWh (%.1f%%)",
info.energyCurr, info.energyFull, info.percent
);
buf += ret; len -= ret;
}
} else {
/* compact label for bar / graph modes */
if (info.ac == AC_PRESENT) {
ret = xSnprintf(buf, len, "%s", isDischarging ? "AC+bat" : "AC");
buf += ret; len -= ret;
} else if (info.ac == AC_ABSENT) {
ret = xSnprintf(buf, len, "bat");
buf += ret; len -= ret;
}

if (ret && len > 1) {
*buf++ = ' ';
*buf = 0;
len--;
}

if (isCharging || isDischarging) {
ret = xSnprintf(
buf, len, "%+.1fW @ %.1f/%.1fWh",
info.powerCurr, info.energyCurr, info.energyFull
);
buf += ret; len -= ret;

if (timeMinutes >= 0) {
ret = xSnprintf(
buf, len, ", %dh%02dm",
timeMinutes / 60, timeMinutes % 60
);
buf += ret; len -= ret;
}
} else {
ret = xSnprintf(
buf, len, "stable @ %.1f/%.1fWh",
info.energyCurr, info.energyFull
Comment thread
coderabbitai[bot] marked this conversation as resolved.
);
buf += ret; len -= ret;
}
}

xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%%s", percent, text);
// Simplify the pattern to always use "buf += ret; len -= ret;" for all cases
(void)ret;
(void)buf;
(void)len;
}

const MeterClass BatteryMeter_class = {
Expand Down
9 changes: 9 additions & 0 deletions BatteryMeter.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ typedef enum ACPresence_ {
AC_ERROR
} ACPresence;

typedef struct BatteryInfo_ {
ACPresence ac;

double percent; /* [0..100], NAN if unknown */
double powerCurr; /* instantaneous power in W, NAN if unknown */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

double energyCurr; /* Wh, NAN if unknown */
double energyFull; /* Wh, NAN if unknown */
} BatteryInfo;

extern const MeterClass BatteryMeter_class;

#endif
68 changes: 61 additions & 7 deletions darwin/Platform.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ in the source distribution for its full text.
#include "SysArchMeter.h"
#include "TasksMeter.h"
#include "UptimeMeter.h"
#include "XUtils.h"
#include "darwin/DarwinMachine.h"
#include "darwin/PlatformHelpers.h"
#include "generic/fdstat_sysctl.h"
Expand Down Expand Up @@ -677,9 +678,14 @@ bool Platform_getNetworkIO(NetworkIOData* data) {
return true;
}

void Platform_getBattery(double* percent, ACPresence* isOnAC) {
*percent = NAN;
*isOnAC = AC_ERROR;
void Platform_getBattery(BatteryInfo* info) {
*info = (BatteryInfo) {
.ac = AC_ERROR,
.percent = NAN,
.powerCurr = NAN,
.energyCurr = NAN,
.energyFull = NAN,
};

CFArrayRef list = NULL;

Expand Down Expand Up @@ -710,8 +716,8 @@ void Platform_getBattery(double* percent, ACPresence* isOnAC) {
/* Determine the AC state */
CFStringRef power_state = CFDictionaryGetValue(power_source, CFSTR(kIOPSPowerSourceStateKey));

if (*isOnAC != AC_PRESENT)
*isOnAC = (kCFCompareEqualTo == CFStringCompare(power_state, CFSTR(kIOPSACPowerValue), 0)) ? AC_PRESENT : AC_ABSENT;
if (info->ac != AC_PRESENT)
info->ac = (kCFCompareEqualTo == CFStringCompare(power_state, CFSTR(kIOPSACPowerValue), 0)) ? AC_PRESENT : AC_ABSENT;

/* Get the percentage remaining */
double tmp;
Expand All @@ -721,8 +727,56 @@ void Platform_getBattery(double* percent, ACPresence* isOnAC) {
cap_max += tmp;
}

if (cap_max > 0.0)
*percent = 100.0 * cap_current / cap_max;
if (cap_max > 0.0) {
info->percent = 100.0 * cap_current / cap_max;
}

io_service_t batt = IOServiceGetMatchingService(iokit_port, IOServiceMatching("AppleSmartBattery"));
if (batt) {
CFNumberRef ampRef = IORegistryEntryCreateCFProperty(batt, CFSTR("Amperage"), kCFAllocatorDefault, 0);
CFNumberRef voltRef = IORegistryEntryCreateCFProperty(batt, CFSTR("Voltage"), kCFAllocatorDefault, 0);
CFNumberRef currCapRef = IORegistryEntryCreateCFProperty(batt, CFSTR("AppleRawCurrentCapacity"), kCFAllocatorDefault, 0);
CFNumberRef maxCapRef = IORegistryEntryCreateCFProperty(batt, CFSTR("AppleRawMaxCapacity"), kCFAllocatorDefault, 0);

if (ampRef && voltRef) {
double ampMA = 0.0;
CFNumberGetValue(ampRef, kCFNumberDoubleType, &ampMA);

double voltMV = 0.0;
CFNumberGetValue(voltRef, kCFNumberDoubleType, &voltMV);

// Follows the Smart Battery System (SBS) Standard
info->powerCurr = ampMA * voltMV / 1e6;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if (currCapRef && maxCapRef && voltRef) {
double currMAh = 0.0;
CFNumberGetValue(currCapRef, kCFNumberDoubleType, &currMAh);

double maxMAh = 0.0;
CFNumberGetValue(maxCapRef, kCFNumberDoubleType, &maxMAh);

double voltMV = 0.0;
CFNumberGetValue(voltRef, kCFNumberDoubleType, &voltMV);

/* Approximate energy from charge and current battery voltage: mAh * mV / 1e6 = Wh. */
if (maxMAh > 0.0 && voltMV > 0.0) {
info->energyCurr = CLAMP(currMAh, 0.0, maxMAh) * voltMV / 1e6;
info->energyFull = maxMAh * voltMV / 1e6;
}
}

if (maxCapRef)
CFRelease(maxCapRef);
if (currCapRef)
CFRelease(currCapRef);
if (voltRef)
CFRelease(voltRef);
if (ampRef)
CFRelease(ampRef);

IOObjectRelease(batt);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

cleanup:
if (list)
Expand Down
2 changes: 1 addition & 1 deletion darwin/Platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ bool Platform_getDiskIO(DiskIOData* data);

bool Platform_getNetworkIO(NetworkIOData* data);

void Platform_getBattery(double* percent, ACPresence* isOnAC);
void Platform_getBattery(BatteryInfo* info);

static inline void Platform_getHostname(char* buffer, size_t size) {
Generic_hostname(buffer, size);
Expand Down
Loading
Loading