From: Omar Polo <op@omarpolo.com>
Subject: Re: Control battery charging support in x11/kde-plasma/powerdevil
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: ports <ports@openbsd.org>
Date: Fri, 07 Jun 2024 12:20:41 +0200

On 2024/06/07 11:20:29 +0200, Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> Please find below a simple patch to add control battery charging support
> in powerdevil aka KDE Plamsa - systemsettings via sysctl(2).
> 
> I would be happy if someone would take a look at the code.

never used plasma so haven't tested, but fwiw the sysctl(2) bits looks
fine to me.

> [...]
> ++#if defined(__OpenBSD__)
> ++static bool isThresholdSupported()
> ++{
> ++    int mode;
> ++    size_t len = sizeof(mode);
> ++    int mib[] = {CTL_HW, HW_BATTERY, HW_BATTERY_CHARGEMODE};
> ++    return (sysctl(mib, 3, &mode, &len, NULL, 0) != -1);

I was a bit surprised by this initially.  However, you're just testing
whether the hw.battery node is available, so this seems fine to me.
Maybe add a comment just in case?  Just to save a few seconds to the
next one glancing at this.

> ++}
> ++
> ++static int getBatteryCharge(const int type)
> ++{
> ++    int percentage = -1;
> ++    size_t len = sizeof(percentage);
> ++    int mib[] = {CTL_HW, HW_BATTERY, type};
> ++    sysctl(mib, 3, &percentage, &len, NULL, 0);

maybe consider checking whether sysctl returns successfully here or not.
If we call this, it means that thet hw.battery node exists, so this
shouldn't be failing, but still it doesn't cost much to add an if.

> ++    return percentage;
> ++}



Thanks,

Omar Polo