| Message ID | 20190321230956.13562-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/3] HID: logitech-hidpp: simplify printing of HID++ version | expand |
On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: > According to hidpp20_batterylevel_get_battery_info my Logitech K270 > keyboard reports only 2 battery levels. This matches with what I've > seen > after testing with batteries at varying level of fullness, it always > reports either 5% or 30%. > > Windows reports "battery good" for the 30% level. I've captured an > USB > trace of Windows reading the battery and it is getting the same info > as the Linux hidpp code gets. > > Now that Linux handles these devices as hidpp devices, it reports the > battery as being low as it treats anything under 31% as low, this > leads > to the user constantly getting a "Keyboard battery is low" warning > from > GNOME3, which is very annoying. > > This commit fixes this by changing the low threshold to anything > under > 30%, which I assume is what Windows does. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/hid-logitech-hidpp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > logitech-hidpp.c > index 421a190583eb..d811c3bab495 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -1054,7 +1054,7 @@ static int hidpp_map_battery_level(int > capacity) > { > if (capacity < 11) > return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; > - else if (capacity < 31) > + else if (capacity < 30) > return POWER_SUPPLY_CAPACITY_LEVEL_LOW; > else if (capacity < 81) > return POWER_SUPPLY_CAPACITY_LEVEL_NORMAL; Shouldn't the other values be changed as well to keep somewhat of a standard? People reading this code without prior knowledge of the file history will just scratch their head when seeing this. Filipe Laíns 3DCE 51D6 0930 EBA4 7858 BA41 46F6 33CB B0EB 4BF2
Hi, On 22-03-19 00:27, Filipe Laíns wrote: > On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: >> According to hidpp20_batterylevel_get_battery_info my Logitech K270 >> keyboard reports only 2 battery levels. This matches with what I've >> seen >> after testing with batteries at varying level of fullness, it always >> reports either 5% or 30%. >> >> Windows reports "battery good" for the 30% level. I've captured an >> USB >> trace of Windows reading the battery and it is getting the same info >> as the Linux hidpp code gets. >> >> Now that Linux handles these devices as hidpp devices, it reports the >> battery as being low as it treats anything under 31% as low, this >> leads >> to the user constantly getting a "Keyboard battery is low" warning >> from >> GNOME3, which is very annoying. >> >> This commit fixes this by changing the low threshold to anything >> under >> 30%, which I assume is what Windows does. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/hid/hid-logitech-hidpp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- >> logitech-hidpp.c >> index 421a190583eb..d811c3bab495 100644 >> --- a/drivers/hid/hid-logitech-hidpp.c >> +++ b/drivers/hid/hid-logitech-hidpp.c >> @@ -1054,7 +1054,7 @@ static int hidpp_map_battery_level(int >> capacity) >> { >> if (capacity < 11) >> return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; >> - else if (capacity < 31) >> + else if (capacity < 30) >> return POWER_SUPPLY_CAPACITY_LEVEL_LOW; >> else if (capacity < 81) >> return POWER_SUPPLY_CAPACITY_LEVEL_NORMAL; > > Shouldn't the other values be changed as well to keep somewhat of a > standard? People reading this code without prior knowledge of the file > history will just scratch their head when seeing this. Well, the values come from the HIDPP 2.0 specification, but in this case some devices seem to not stick to the spec (and Windows accepts the "30" value from these devices as indicating "Good" batteries) But you are right that this deviation from the spec warrants some explanation for future readers of the code, so I've added the following comment for v2: /* * The spec says this should be < 31 but some devices report 30 * with brand new batteries and Windows reports 30 as "Good". */ else if (capacity < 30) return POWER_SUPPLY_CAPACITY_LEVEL_LOW; Regards, Hans
On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: > According to hidpp20_batterylevel_get_battery_info my Logitech K270 > keyboard reports only 2 battery levels. This matches with what I've > seen > after testing with batteries at varying level of fullness, it always > reports either 5% or 30%. > > Windows reports "battery good" for the 30% level. I've captured an > USB > trace of Windows reading the battery and it is getting the same info > as the Linux hidpp code gets. > > Now that Linux handles these devices as hidpp devices, it reports the > battery as being low as it treats anything under 31% as low, this > leads > to the user constantly getting a "Keyboard battery is low" warning > from > GNOME3, which is very annoying. > > This commit fixes this by changing the low threshold to anything > under > 30%, which I assume is what Windows does. upower doesn't use the battery percentage if there's a "Capacity" set. If something else in the stack does, then there's a bug in that component and the problem needs to be fixed there. See: https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L514 https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L621
On Fri, Mar 22, 2019 at 11:14 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: > > According to hidpp20_batterylevel_get_battery_info my Logitech K270 > > keyboard reports only 2 battery levels. This matches with what I've > > seen > > after testing with batteries at varying level of fullness, it always > > reports either 5% or 30%. > > > > Windows reports "battery good" for the 30% level. I've captured an > > USB > > trace of Windows reading the battery and it is getting the same info > > as the Linux hidpp code gets. > > > > Now that Linux handles these devices as hidpp devices, it reports the > > battery as being low as it treats anything under 31% as low, this > > leads > > to the user constantly getting a "Keyboard battery is low" warning > > from > > GNOME3, which is very annoying. > > > > This commit fixes this by changing the low threshold to anything > > under > > 30%, which I assume is what Windows does. > > upower doesn't use the battery percentage if there's a "Capacity" set. > If something else in the stack does, then there's a bug in that > component and the problem needs to be fixed there. I think that is precisely what Hans is fixing here: on his device, the discrete battery percentage reported by the mouse for the normal capacity level is 30. With the current code, this is interpreted as Low and upower interprets it as low as well. I think the patch is safe to apply as it is. I'll let you the chance to ack/nack it though. Cheers, Benjamin > > See: > https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L514 > https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L621 >
On Fri, 2019-03-22 at 11:24 +0100, Benjamin Tissoires wrote: > On Fri, Mar 22, 2019 at 11:14 AM Bastien Nocera <hadess@hadess.net> > wrote: > > On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: > > > According to hidpp20_batterylevel_get_battery_info my Logitech > > > K270 > > > keyboard reports only 2 battery levels. This matches with what > > > I've > > > seen > > > after testing with batteries at varying level of fullness, it > > > always > > > reports either 5% or 30%. > > > > > > Windows reports "battery good" for the 30% level. I've captured > > > an > > > USB > > > trace of Windows reading the battery and it is getting the same > > > info > > > as the Linux hidpp code gets. > > > > > > Now that Linux handles these devices as hidpp devices, it reports > > > the > > > battery as being low as it treats anything under 31% as low, this > > > leads > > > to the user constantly getting a "Keyboard battery is low" > > > warning > > > from > > > GNOME3, which is very annoying. > > > > > > This commit fixes this by changing the low threshold to anything > > > under > > > 30%, which I assume is what Windows does. > > > > upower doesn't use the battery percentage if there's a "Capacity" > > set. > > If something else in the stack does, then there's a bug in that > > component and the problem needs to be fixed there. > > I think that is precisely what Hans is fixing here: on his device, > the > discrete battery percentage reported by the mouse for the normal > capacity level is 30. > With the current code, this is interpreted as Low and upower > interprets it as low as well. Please remove the mentions of GNOME from the commit message. The commit message makes it sound like the percentage is being used, and this patch is a work-around for a GNOME problem. Cheers > I think the patch is safe to apply as it is. I'll let you the chance > to ack/nack it though. > > Cheers, > Benjamin > > > See: > > https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L514 > > https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L621 > >
Hi, On 3/22/19 11:47 AM, Bastien Nocera wrote: > On Fri, 2019-03-22 at 11:24 +0100, Benjamin Tissoires wrote: >> On Fri, Mar 22, 2019 at 11:14 AM Bastien Nocera <hadess@hadess.net> >> wrote: >>> On Fri, 2019-03-22 at 00:09 +0100, Hans de Goede wrote: >>>> According to hidpp20_batterylevel_get_battery_info my Logitech >>>> K270 >>>> keyboard reports only 2 battery levels. This matches with what >>>> I've >>>> seen >>>> after testing with batteries at varying level of fullness, it >>>> always >>>> reports either 5% or 30%. >>>> >>>> Windows reports "battery good" for the 30% level. I've captured >>>> an >>>> USB >>>> trace of Windows reading the battery and it is getting the same >>>> info >>>> as the Linux hidpp code gets. >>>> >>>> Now that Linux handles these devices as hidpp devices, it reports >>>> the >>>> battery as being low as it treats anything under 31% as low, this >>>> leads >>>> to the user constantly getting a "Keyboard battery is low" >>>> warning >>>> from >>>> GNOME3, which is very annoying. >>>> >>>> This commit fixes this by changing the low threshold to anything >>>> under >>>> 30%, which I assume is what Windows does. >>> >>> upower doesn't use the battery percentage if there's a "Capacity" >>> set. >>> If something else in the stack does, then there's a bug in that >>> component and the problem needs to be fixed there. >> >> I think that is precisely what Hans is fixing here: on his device, >> the >> discrete battery percentage reported by the mouse for the normal >> capacity level is 30. >> With the current code, this is interpreted as Low and upower >> interprets it as low as well. > > Please remove the mentions of GNOME from the commit message. The commit > message makes it sound like the percentage is being used, and this > patch is a work-around for a GNOME problem. The commit message does not suggest that this is a GNOME problem at all: "Now that Linux handles these devices as hidpp devices, it reports the battery as being low as it treats anything under 31% as low," Where it clearly points to "Linux" from the part of the sentence before the comma. After that it continues: "this leads to the user constantly getting a "Keyboard battery is low" warning from GNOME3" Where "this" clearly refers to *Linux* reporting the battery as being low. I don't really feel like respinning this patch set again just to remove an innocent reference to GNOME3, that just feels like busy work to me and I'm busy enough as is. Regards, Hans > > Cheers > >> I think the patch is safe to apply as it is. I'll let you the chance >> to ack/nack it though. >> >> Cheers, >> Benjamin >> >>> See: >>> https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L514 >>> https://gitlab.freedesktop.org/upower/upower/blob/master/dbus/org.freedesktop.UPower.Device.xml#L621 >>> >
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 421a190583eb..d811c3bab495 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -1054,7 +1054,7 @@ static int hidpp_map_battery_level(int capacity) { if (capacity < 11) return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; - else if (capacity < 31) + else if (capacity < 30) return POWER_SUPPLY_CAPACITY_LEVEL_LOW; else if (capacity < 81) return POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
According to hidpp20_batterylevel_get_battery_info my Logitech K270 keyboard reports only 2 battery levels. This matches with what I've seen after testing with batteries at varying level of fullness, it always reports either 5% or 30%. Windows reports "battery good" for the 30% level. I've captured an USB trace of Windows reading the battery and it is getting the same info as the Linux hidpp code gets. Now that Linux handles these devices as hidpp devices, it reports the battery as being low as it treats anything under 31% as low, this leads to the user constantly getting a "Keyboard battery is low" warning from GNOME3, which is very annoying. This commit fixes this by changing the low threshold to anything under 30%, which I assume is what Windows does. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/hid/hid-logitech-hidpp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)