Bug 100359

Created attachment 130402 [details] [review]
linux: Add support for "capacity_level" attribute

Some devices, such as a number of wireless Logitech unifying devices
don't have a precise battery level reporting, and use the Linux
POWER_SUPPLY_CAPACITY_LEVEL_* values.

This minimal fix matches the levels against approximate percentage
values. This is good enough to make the Logitech T650 report battery
again when using the kernel HID++ battery support.

Ideally, we'd proxy the "capacity level" itself through D-Bus, but it's really quite a bit easier to convert this to an approximate percentage.

Not great for devices with only 2 levels though:
https://bugzilla.gnome.org/show_bug.cgi?id=780360

Ideas?

Created attachment 130414 [details] [review]
linux: Add support for "capacity_level" attribute

Some devices, such as a number of wireless Logitech unifying devices
don't have a precise battery level reporting, and use the Linux
POWER_SUPPLY_CAPACITY_LEVEL_* values.

This minimal fix matches the levels against approximate percentage
values. This is good enough to make the Logitech T650 report battery
again when using the kernel HID++ battery support.

(In reply to Bastien Nocera from comment #3)
> Created attachment 130414 [details] [review] [review]
> linux: Add support for "capacity_level" attribute

2 changes:
- remove the "capacity_level" support for laptop batteries (non-device batteries), it's not actually needed
- Update the code and the tests, the "capacity_level" strings have linefeeds in them...

*** Bug 96857 has been marked as a duplicate of this bug. ***

After talking to Richard on IRC, I think I'll add a patch on top of this one to export the actual "capacity_level" value to API consumers. This will allow older apps to still work with the newer kernels, and not regress in supporting devices which use it.

The API would allow front-ends to not show a full battery gauge when the reporting is too coarse to show any changes over time.

Created attachment 130694 [details] [review]
daemon: Make warning levels for devices inclusive

A 5% battery level should already be in "critical", this matches the
configurable thresholds where the bounds of critical warnings are
inclusive. This also makes it easier to create fake percentages for the
Logitech devices with coarse battery level reporting.

Created attachment 130695 [details] [review]
linux: Add support for "capacity_level" attribute

Some devices, such as a number of wireless Logitech unifying devices
don't have a precise battery level reporting, and use the Linux
POWER_SUPPLY_CAPACITY_LEVEL_* values.

This minimal fix matches the levels against approximate percentage
values. This is good enough to make the Logitech T650 report battery
again when using the kernel HID++ battery support.

Created attachment 130696 [details] [review]
lib: Add more members to UpDeviceLevel struct

Those levels will not be used to cover warning levels, but approximate
battery levels that devices can use, exported by the kernel as
POWER_SUPPLY_CAPACITY_LEVEL_* values.

See linux/power_supply.h

Created attachment 130697 [details] [review]
all: Add BatteryLevel property

Export approximate battery levels that devices can use, exported by
the kernel as POWER_SUPPLY_CAPACITY_LEVEL_* values.

This avoids bizarrely accurate values showing up in UIs when we only
have ok/warning levels of accuracy in some cases.

Created attachment 130698 [details] [review]
daemon: Move a number of constants to a shared file

Comment on attachment 130694 [details] [review]
daemon: Make warning levels for devices inclusive

Review of attachment 130694 [details] [review]:
-----------------------------------------------------------------

::: src/up-daemon.c
@@ +645,4 @@
>  	 * into critical (or off) before any warnings */
>  	if (kind == UP_DEVICE_KIND_MOUSE ||
>  	    kind == UP_DEVICE_KIND_KEYBOARD) {
> +		if (percentage <= 5.0f)

This is from pre-existing code and but I am curious: percentage is a double, any reason the 'f' float suffix is used?

Created attachment 130731 [details] [review]
all: Add BatteryLevel property

Export approximate battery levels that devices can use, exported by
the kernel as POWER_SUPPLY_CAPACITY_LEVEL_* values.

This avoids bizarrely accurate values showing up in UIs when we only
have ok/warning levels of accuracy in some cases.

Comment on attachment 130696 [details] [review]
lib: Add more members to UpDeviceLevel struct

Review of attachment 130696 [details] [review]:
-----------------------------------------------------------------

::: libupower-glib/up-types.c
@@ +296,4 @@
>  		return UP_DEVICE_LEVEL_CRITICAL;
>  	if (g_strcmp0 (level, "action") == 0)
>  		return UP_DEVICE_LEVEL_ACTION;
> +	if (g_strcmp0 (level, "normal") == 0)

Just for completeness: the first 'if'-case takes care of the case that level is NULL, so a non-null-safe comparison would suffice (like e.g. g_str_equal, strcmp).

Looks all good to me.

Attachment 130694 [details] pushed as 27a3eea - daemon: Make warning levels for devices inclusive
Attachment 130695 [details] pushed as 6b14798 - linux: Add support for "capacity_level" attribute
Attachment 130696 [details] pushed as 4f92309 - lib: Add more members to UpDeviceLevel struct
Attachment 130698 [details] pushed as 660c8f3 - daemon: Move a number of constants to a shared file
Attachment 130731 [details] pushed as 499d05b - all: Add BatteryLevel property