On Fri Jun 07, 2024 at 12:20:41PM +0200, Omar Polo wrote:
> On 2024/06/07 11:20:29 +0200, Rafael Sadowski <raf...@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.
Thanks for your feedback!
>
> > [...]
> > ++#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.
Yes, that was my idea, I have no other way of checking whether this feature is
available. Good point I'll add a comment.
>
> > ++}
> > ++
> > ++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.
Yeah, I didn't see the point. if sysctl(2) fails I return -1 (default). If it
passed I return the value set by sysctl. I safe code and one more return
statement.
>
> > ++ return percentage;
> > ++}
>
>
>
> Thanks,
>
> Omar Polo
>
