linux,darwin: introduce battery info API #4258

juanarbol · 2023-12-18T04:53:02Z

This is WIP with the intention of doing corrections if needed and may actually define the uv_battery_info_t struct and refine the implementation to handle all that the getBattery needs.

Refs: #4172

bnoordhuis

Thanks for the PR. Left some comments.

include/uv.h

src/unix/darwin-battery.c

bnoordhuis · 2023-12-22T11:57:06Z

src/unix/darwin-battery.c

+ goto out;
+
+ err = UV_EINVAL;
+ info->charging = pIOPSGetTimeRemainingEstimate();


You should assign this to a local variable first because implementation details like the -1 and -2 status codes shouldn't leak out to callers.

On it!

How are we going to handle -1 and -2 states? As long as the struct will store only positive numbers?

I've added further members to the structure, but I think the question remains: how to handle platform-specific values like -1, or BYTE(255) in Win32 case.

#4271

test/test-platform-output.c

bnoordhuis · 2024-01-12T11:16:59Z

@juanarbol is this ready for re-review? Some of the comments don't seem to be addressed.

juanarbol · 2024-01-15T12:56:24Z

@juanarbol is this ready for re-review? Some of the comments don't seem to be addressed.

Sorry, I will fix those today (or tomorrow)! Quite busy lately

I'm revamping this whole thing, I'm gonna need a bit of extra time.

juanarbol · 2024-01-17T04:32:55Z

I had to spin a Linux VM. I'm working on the Linux patch. This is WIP.

juanarbol · 2024-01-17T05:29:10Z

~~Some IOKit calls or "core foundation" are unavailable for the macOS machine, but I am not sure...~~
The build servers normally don't have batteries 😂

juanarbol · 2024-01-21T01:36:43Z

This is now ready for review, but I have a question about how this should behave.

For now, if there is no battery for both Linux and Darwin, it will return UV_EPERM, and nothing else will be done.

In the Linux implementation, if it fails to read any other "battery information" file, I will return UV_EPERM, but macOS will leave the struct with garbage in the struct members and will return 0.

What do reviewers think is a better behavior to adopt? I propose two. I'm open to modifying this to a better one or unifying this into a decided one (and make the CI pass, for sure)

include/uv.h

docs/src/misc.rst

test/test-platform-output.c

src/unix/battery.c

src/win/battery.c

test/test-platform-output.c

src/unix/battery.c

cjihrig · 2024-01-21T02:25:17Z

src/unix/battery.c

+ FILE* fp;
+
+ err = UV_EPERM;
+ fp = uv__open_file("/sys/class/power_supply/BAT0");


Would uv__slurp() help with all of this file reading logic?

Another thing: there can be multiple batteries. Linux's ACPI smart battery driver supports up to 4 (BAT0 to BAT3.)

I suppose you should try them all and merge the results? Either that or the documentation should make explicit that it only queries the first one.

src/unix/battery.c

docs/src/misc.rst

src/unix/battery.c

bnoordhuis · 2024-01-21T12:59:44Z

src/unix/battery.c

+#endif // defined(__APPLE__) || TARGET_OS_IPHONE
+
+// macOS won't need any of this
+#if defined(__LINUX__)


Here and elsewhere:

Suggested change

#if defined(__LINUX__)

#if defined(__linux__)

That said, it's probably better to move the platform-specific code into linux.c, darwin.c, etc., and have a UV_ENOSYS stub for platforms where it's unimplemented.

src/unix/battery.c

bnoordhuis · 2024-01-21T13:16:03Z

src/unix/battery.c

+ FILE* fp;
+
+ err = UV_EPERM;
+ fp = uv__open_file("/sys/class/power_supply/BAT0");


Another thing: there can be multiple batteries. Linux's ACPI smart battery driver supports up to 4 (BAT0 to BAT3.)

I suppose you should try them all and merge the results? Either that or the documentation should make explicit that it only queries the first one.

src/unix/battery.c

bnoordhuis · 2024-01-21T13:30:41Z

In the Linux implementation, if it fails to read any other "battery information" file, I will return UV_EPERM, but macOS will leave the struct with garbage in the struct members and will return 0.

If you return an error, it's okay to leave the struct fields untouched. If you return 0 (no error), they must be set to something sensible (like zero.)

juanarbol

Thanks for taking the time reviewing this, I will address all your suggestions as soon as I can :)

juanarbol · 2024-02-02T00:46:23Z

I am still working on this; this is not stale; I am just not having the best time w/out a Linux machine with a better attached to it.

juanarbol · 2024-02-22T21:17:28Z

I suppose you should try them all and merge the results? Either that or the documentation should make explicit that it only queries the first one.

I have the loop counting and reading all batteries inside this code (validated with my coworker's machine), but I'm not sure what do you mean with "merge" results, what if one of the three batteries is charging but the rest are not? I was considering using a uv_cpu_info_t array, likes uv_cpu_info(), return an array of batteries instead. But I think windows, nor darwin support multiple batteries.

That said, it's probably better to move the platform-specific code into linux.c, darwin.c, etc., and have a UV_ENOSYS stub for platforms where it's unimplemented. (#4258 (comment))

I put the conditional inclusion due to, macOS way won't need stdlib, just link to IOKit. Does that worth move split all this into separate files? (I'm genuinely asking, I don't know)

Refs: libuv#4172 Signed-off-by: Juan José Arboleda <[email protected]>

juanarbol · 2024-03-11T16:51:50Z

ping @bnoordhuis

calvin2021y · 2024-04-11T15:34:51Z

Is android/ios supported ?

juanarbol · 2024-04-12T18:50:52Z

Is android/ios supported ?

I think IOKit gives immediate support to iOS, not quite sure about android.

bnoordhuis reviewed Dec 22, 2023

View reviewed changes

juanarbol force-pushed the juan/battery-mac branch 3 times, most recently from 643554f to c3f9e3f Compare January 17, 2024 04:30

juanarbol force-pushed the juan/battery-mac branch 2 times, most recently from 2106a75 to 6b67cde Compare January 17, 2024 05:21

juanarbol force-pushed the juan/battery-mac branch 4 times, most recently from 5b5cef5 to 9f62cfb Compare January 21, 2024 01:29

juanarbol marked this pull request as ready for review January 21, 2024 01:36

juanarbol changed the title ~~darwin: add battery status (WIP)~~ linux,darwin: introduce battery info API Jan 21, 2024

cjihrig reviewed Jan 21, 2024

View reviewed changes

bnoordhuis reviewed Jan 21, 2024

View reviewed changes

juanarbol commented Jan 21, 2024

View reviewed changes

juanarbol force-pushed the juan/battery-mac branch 4 times, most recently from 10391f6 to 511125e Compare February 22, 2024 21:05

linux,darwin: introduce battery info API

cb01607

Refs: libuv#4172 Signed-off-by: Juan José Arboleda <[email protected]>

juanarbol force-pushed the juan/battery-mac branch from 511125e to cb01607 Compare February 27, 2024 17:48

ptlomholt mentioned this pull request Feb 27, 2024

win: implement battery (Draft) #4271

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linux,darwin: introduce battery info API #4258

linux,darwin: introduce battery info API #4258

juanarbol commented Dec 18, 2023

bnoordhuis left a comment

bnoordhuis Dec 22, 2023

juanarbol Dec 26, 2023

oluan Dec 31, 2023

bnoordhuis commented Jan 12, 2024

juanarbol commented Jan 15, 2024 •

edited

juanarbol commented Jan 17, 2024

juanarbol commented Jan 17, 2024 •

edited

juanarbol commented Jan 21, 2024

cjihrig Jan 21, 2024

bnoordhuis Jan 21, 2024

bnoordhuis Jan 21, 2024

bnoordhuis Jan 21, 2024

bnoordhuis commented Jan 21, 2024

juanarbol left a comment

juanarbol commented Feb 2, 2024

juanarbol commented Feb 22, 2024

juanarbol commented Mar 11, 2024

calvin2021y commented Apr 11, 2024

juanarbol commented Apr 12, 2024

linux,darwin: introduce battery info API #4258

Are you sure you want to change the base?

linux,darwin: introduce battery info API #4258

Conversation

juanarbol commented Dec 18, 2023

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 22, 2023

Choose a reason for hiding this comment

juanarbol Dec 26, 2023

Choose a reason for hiding this comment

oluan Dec 31, 2023

Choose a reason for hiding this comment

bnoordhuis commented Jan 12, 2024

juanarbol commented Jan 15, 2024 • edited

juanarbol commented Jan 17, 2024

juanarbol commented Jan 17, 2024 • edited

juanarbol commented Jan 21, 2024

cjihrig Jan 21, 2024

Choose a reason for hiding this comment

bnoordhuis Jan 21, 2024

Choose a reason for hiding this comment

bnoordhuis Jan 21, 2024

Choose a reason for hiding this comment

bnoordhuis Jan 21, 2024

Choose a reason for hiding this comment

bnoordhuis commented Jan 21, 2024

juanarbol left a comment

Choose a reason for hiding this comment

juanarbol commented Feb 2, 2024

juanarbol commented Feb 22, 2024

juanarbol commented Mar 11, 2024

calvin2021y commented Apr 11, 2024

juanarbol commented Apr 12, 2024

juanarbol commented Jan 15, 2024 •

edited

juanarbol commented Jan 17, 2024 •

edited