Message ID | 20181103065732.12134-1-jprvita@endlessm.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | ACPI / battery: Fix reporting "Not charging" when capacity is 100% | expand |
Hi, On 03-11-18 07:57, João Paulo Rechi Vita wrote: > Commit 19fffc8450d4378580a8f019b195c4617083176f fixed reporting > "Discharging" on some machines when AC was connected but the battery was > not charging. But now on these machines the battery status is reported > as "Not charging" even when the battery is fully charged. > > This commit takes the battery capacity into consideration when checking > if "Not charging" should be returned and "Full" is returned when the > capacity is 100%. > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> acpi_battery_handle_discharging() only gets called if the ACPI_BATTERY_STATE_DISCHARGING bit is set by the firmware in that case if we are not actually discharging returning POWER_SUPPLY_STATUS_NOT_CHARGING is the only correct thing to do, we should never return POWER_SUPPLY_STATUS_FULL when the ACPI_BATTERY_STATE_DISCHARGING bit is set. I was about to point you to the upower bug for upower not handling POWER_SUPPLY_STATUS_NOT_CHARGING as well as it could atm, but I see you've already found that and are working on fixing that. That is great, thank you. As for this kernel-side fix I do not believe that fixing thus in the kernel is the right thing to do. We try to stay away from heuristics using full_charge_capacity in the kernel since that is not really reliable / deterministic. Regards, Hans > --- > drivers/acpi/battery.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index cb97b6105f52..82e194290f01 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -217,8 +217,12 @@ static int acpi_battery_handle_discharging(struct acpi_battery *battery) > * was plugged in and the device thus did not start a new charge cycle. > */ > if ((battery_ac_is_broken || power_supply_is_system_supplied()) && > - battery->rate_now == 0) > + battery->rate_now == 0) { > + if (battery->capacity_now && battery->full_charge_capacity && > + battery->capacity_now / battery->full_charge_capacity == 1) > + return POWER_SUPPLY_STATUS_FULL; > return POWER_SUPPLY_STATUS_NOT_CHARGING; > + } > > return POWER_SUPPLY_STATUS_DISCHARGING; > } >
On Fri 2018-11-02 23:57:32, João Paulo Rechi Vita wrote: > Commit 19fffc8450d4378580a8f019b195c4617083176f fixed reporting > "Discharging" on some machines when AC was connected but the battery was > not charging. But now on these machines the battery status is reported > as "Not charging" even when the battery is fully charged. > > This commit takes the battery capacity into consideration when checking > if "Not charging" should be returned and "Full" is returned when the > capacity is 100%. > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> We have people trying to modify this and it caused regressions in MATE, IIRC. Plus, I don't think "100% charge" is right test for "battery full". At least on thinkpads, there's configuration option, and it is common _not_ to charge batterry above 95% or so (to increase its lifetime). Pavel > * was plugged in and the device thus did not start a new charge cycle. > */ > if ((battery_ac_is_broken || power_supply_is_system_supplied()) && > - battery->rate_now == 0) > + battery->rate_now == 0) { > + if (battery->capacity_now && battery->full_charge_capacity && > + battery->capacity_now / battery->full_charge_capacity == 1) > + return POWER_SUPPLY_STATUS_FULL; Division? Really? Pavel
Hello Hans, thanks for your quick response, On Sat, Nov 3, 2018 at 4:28 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 03-11-18 07:57, João Paulo Rechi Vita wrote: > > Commit 19fffc8450d4378580a8f019b195c4617083176f fixed reporting > > "Discharging" on some machines when AC was connected but the battery was > > not charging. But now on these machines the battery status is reported > > as "Not charging" even when the battery is fully charged. > > > > This commit takes the battery capacity into consideration when checking > > if "Not charging" should be returned and "Full" is returned when the > > capacity is 100%. > > > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > > acpi_battery_handle_discharging() only gets called if the ACPI_BATTERY_STATE_DISCHARGING > bit is set by the firmware in that case if we are not actually discharging > returning POWER_SUPPLY_STATUS_NOT_CHARGING is the only correct thing to do, > we should never return POWER_SUPPLY_STATUS_FULL when the > ACPI_BATTERY_STATE_DISCHARGING bit is set. > I understand the reason behind your original patch, but can you elaborate on why would it be wrong to return POWER_SUPPLY_STATUS_FULL if the battery is full and we know it is not actually discharging? IIUC that is the state returned on hw / firmware that behaves correctly when there is AC power and the battery has 100% of charge. > I was about to point you to the upower bug for upower not > handling POWER_SUPPLY_STATUS_NOT_CHARGING as well as it > could atm, but I see you've already found that and are working > on fixing that. That is great, thank you. > Yes, I'm trying to fix this problem, but it touches different points across the stack and there are different possible approaches for how to present the "Not charging" state to the user. This patch is not necessarily part of, although closely related to, the bigger problem of better handling "Not charging". It actually addresses the problem that the kernel returns "Not charging" when AC is present and the battery is fully charged, which seems wrong to me, despite of "Not charging" not being strictly defined anywhere (at least that I know of). > As for this kernel-side fix I do not believe that fixing thus in > the kernel is the right thing to do. We try to stay away from > heuristics using full_charge_capacity in the kernel since that > is not really reliable / deterministic. > At first I thought that returning POWER_SUPPLY_STATUS_FULL when the battery is full and AC is present was deterministic enough to have it in the kernel. But I don't have any prior experience working with this kind of hardware, so I may very well be wrong on this assumption. It would be great to get some extra clarification though. Thanks, -- João Paulo Rechi Vita http://about.me/jprvita
On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote: > > On Fri 2018-11-02 23:57:32, João Paulo Rechi Vita wrote: > > Commit 19fffc8450d4378580a8f019b195c4617083176f fixed reporting > > "Discharging" on some machines when AC was connected but the battery was > > not charging. But now on these machines the battery status is reported > > as "Not charging" even when the battery is fully charged. > > > > This commit takes the battery capacity into consideration when checking > > if "Not charging" should be returned and "Full" is returned when the > > capacity is 100%. > > > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > > We have people trying to modify this and it caused regressions in > MATE, IIRC. > Do you have any pointers for further information? > Plus, I don't think "100% charge" is right test for "battery full". At > least on thinkpads, there's configuration option, and it is common > _not_ to charge batterry above 95% or so (to increase its lifetime). > This will only affect machines where the firmware wrongly reports the battery state as discharging, which the commit mentioned on this commit message fixed so we now report it as not charging. From your comment it does not sound like thinkpads are in this category. In any case deciding to report battery full for any percentage other than 100% is a policy decision, which is normally left out of the kernel. > > > > * was plugged in and the device thus did not start a new charge cycle. > > */ > > if ((battery_ac_is_broken || power_supply_is_system_supplied()) && > > - battery->rate_now == 0) > > + battery->rate_now == 0) { > > + if (battery->capacity_now && battery->full_charge_capacity && > > + battery->capacity_now / battery->full_charge_capacity == 1) > > + return POWER_SUPPLY_STATUS_FULL; > > Division? Really? > If you look further down in acpi_battery_get_property, that is how the capacity property is calculated. Do you have a better suggestion? -- João Paulo Rechi Vita http://about.me/jprvita
On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote: > Plus, I don't think "100% charge" is right test for "battery full". At > least on thinkpads, there's configuration option, and it is common > _not_ to charge batterry above 95% or so (to increase its lifetime). Hans also touched on this area in his response: > As for this kernel-side fix I do not believe that fixing thus in > the kernel is the right thing to do. We try to stay away from > heuristics using full_charge_capacity in the kernel since that > is not really reliable / deterministic. I'm not fully convinced by this argument though. The ACPI spec is not very clear on what conditions you should apply to decide when the battery is full. Instead, ACPI seems to provide a pretty decent amount of data, and the decision about whether to interpret that as "battery full" is left for consumers. The best that the ACPI spec offers here is the _BIF/_BIX "Last Full Charge Capacity" field (known as full_charge_capacity in this driver), and it does appear to consider the "only charge to 95%" consideration, although it is only documented as a prediction. There is also a sample calculation (section 3.9.3) of how to calculate the Remaining Battery Percentage which uses "Last Full Charge Capacity" too. Without a crystal clear definition of battery full, it seems up to the consumers of the data to decide when the battery is full via some form of heuristic. And this is exactly what acpi-battery already does in acpi_battery_is_charged() - and the main aspect is looking at full_charge_capacity. The result of this is then sent to userspace. So it does not seem fair to say that "you can't use basic heuristics around full_charge_capacity" because the kernel already does (and this is what the ACPI spec encourages), although JP's patch could be improved to more clearly and more completely follow the logic in acpi_battery_is_charged(). The question of whether we would want to interpret the numbers in this way when DISCHARGING is set is a good one though. The ACPI spec doesn't shine much light on this, but the calculations related to battery percentage and battery life in section 3.9.3 at least do not seem to consider charging/discharging status. Thanks Daniel
Hi! > > > * was plugged in and the device thus did not start a new charge cycle. > > > */ > > > if ((battery_ac_is_broken || power_supply_is_system_supplied()) && > > > - battery->rate_now == 0) > > > + battery->rate_now == 0) { > > > + if (battery->capacity_now && battery->full_charge_capacity && > > > + battery->capacity_now / battery->full_charge_capacity == 1) > > > + return POWER_SUPPLY_STATUS_FULL; > > > > Division? Really? > > If you look further down in acpi_battery_get_property, that is how the > capacity property is calculated. Do you have a better suggestion? if (battery->capacity_now >= battery->full_charge_capacity) ? Pavel
Hi, On 11/7/18 5:53 AM, Daniel Drake wrote: > On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote: >> Plus, I don't think "100% charge" is right test for "battery full". At >> least on thinkpads, there's configuration option, and it is common >> _not_ to charge batterry above 95% or so (to increase its lifetime). > > Hans also touched on this area in his response: > >> As for this kernel-side fix I do not believe that fixing thus in >> the kernel is the right thing to do. We try to stay away from >> heuristics using full_charge_capacity in the kernel since that >> is not really reliable / deterministic. > > I'm not fully convinced by this argument though. > > The ACPI spec is not very clear on what conditions you should apply to > decide when the battery is full. Instead, ACPI seems to provide a > pretty decent amount of data, and the decision about whether to > interpret that as "battery full" is left for consumers. Right, but in this case the "discharging" status bit is explicitly set, to me it feels wrong to report "full", when the firmware is reporting "discharging" IMHO, at best we are "not charging" (on AC, below the threshold where a new charge cycle starts) and that is what we are currently reporting. Anu heurstics to decide that "not charging" is close enough to full to report it as full to the user belongs in userspace IMHO. Anyways this ultimately is Rafael's call. If Rafael is ok with this patch then I would like to see Pavel's comment addressed and otherwise it is fine with me. Note that we will still often get the case where a laptop is charged, reports full, is unplugged for 5 minutes and then replugged and then reports a capacity of 97% combined with "not charging", so we will still need to fix userspace to handle this. Rafael, what is your take on this? Regards, Hans
On Sun 2018-11-11 12:57:12, Hans de Goede wrote: > Hi, > > On 11/7/18 5:53 AM, Daniel Drake wrote: > >On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote: > >>Plus, I don't think "100% charge" is right test for "battery full". At > >>least on thinkpads, there's configuration option, and it is common > >>_not_ to charge batterry above 95% or so (to increase its lifetime). > > > >Hans also touched on this area in his response: > > > >>As for this kernel-side fix I do not believe that fixing thus in > >>the kernel is the right thing to do. We try to stay away from > >>heuristics using full_charge_capacity in the kernel since that > >>is not really reliable / deterministic. > > > >I'm not fully convinced by this argument though. > > > >The ACPI spec is not very clear on what conditions you should apply to > >decide when the battery is full. Instead, ACPI seems to provide a > >pretty decent amount of data, and the decision about whether to > >interpret that as "battery full" is left for consumers. > > Right, but in this case the "discharging" status bit is explicitly > set, to me it feels wrong to report "full", when the firmware > is reporting "discharging" IMHO, at best we are "not charging" > (on AC, below the threshold where a new charge cycle starts) and > that is what we are currently reporting. > > Anu heurstics to decide that "not charging" is close enough to full > to report it as full to the user belongs in userspace IMHO. > > Anyways this ultimately is Rafael's call. If Rafael is ok with this > patch then I would like to see Pavel's comment addressed and otherwise > it is fine with me. > > Note that we will still often get the case where a laptop is charged, > reports full, is unplugged for 5 minutes and then replugged and then > reports a capacity of 97% combined with "not charging", so we will > still need to fix userspace to handle this. For the record, I don't think I'm okay with this. There's nothing special about 100% charge. This changes userland ABI and I don't think it has good enough reasons to do that. Best regards, Pavel
On Sun, Nov 11, 2018 at 4:22 AM Pavel Machek <pavel@ucw.cz> wrote: > > On Sun 2018-11-11 12:57:12, Hans de Goede wrote: > > Hi, > > > > On 11/7/18 5:53 AM, Daniel Drake wrote: > > >On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <pavel@ucw.cz> wrote: > > >>Plus, I don't think "100% charge" is right test for "battery full". At > > >>least on thinkpads, there's configuration option, and it is common > > >>_not_ to charge batterry above 95% or so (to increase its lifetime). > > > > > >Hans also touched on this area in his response: > > > > > >>As for this kernel-side fix I do not believe that fixing thus in > > >>the kernel is the right thing to do. We try to stay away from > > >>heuristics using full_charge_capacity in the kernel since that > > >>is not really reliable / deterministic. > > > > > >I'm not fully convinced by this argument though. > > > > > >The ACPI spec is not very clear on what conditions you should apply to > > >decide when the battery is full. Instead, ACPI seems to provide a > > >pretty decent amount of data, and the decision about whether to > > >interpret that as "battery full" is left for consumers. > > > > Right, but in this case the "discharging" status bit is explicitly > > set, to me it feels wrong to report "full", when the firmware > > is reporting "discharging" IMHO, at best we are "not charging" > > (on AC, below the threshold where a new charge cycle starts) and > > that is what we are currently reporting. > > > > Anu heurstics to decide that "not charging" is close enough to full > > to report it as full to the user belongs in userspace IMHO. > > > > Anyways this ultimately is Rafael's call. If Rafael is ok with this > > patch then I would like to see Pavel's comment addressed and otherwise > > it is fine with me. > > If we can get to an agreement on this I'll send a v2 without division. > > Note that we will still often get the case where a laptop is charged, > > reports full, is unplugged for 5 minutes and then replugged and then > > reports a capacity of 97% combined with "not charging", so we will > > still need to fix userspace to handle this. > Yes, I agree that should be addressed in userspace, as it is pretty much a policy decision. > For the record, I don't think I'm okay with this. > > There's nothing special about 100% charge. > I don't agree there is nothing special about 100% charge. There is a separate state to represent battery full for a reason, which is the user wanting to know when their battery is 100% charged and not being discharged. > This changes userland ABI and I don't think it has good enough reasons to do that. > This only changes which state will be reported when the battery is 100% charged and not discharging, it does not introduce / remove any values. I don't think that is considered ABI change, and on other hardware like the Dell Latitude 5480, /sys/class/power_supply/<supply_name>/status already reports "Full" under these conditions. I still believe it is a bug that makes the ABI inconsistent across different hardware. -- João Paulo Rechi Vita http://about.me/jprvita
Hi! > > > Anyways this ultimately is Rafael's call. If Rafael is ok with this > > > patch then I would like to see Pavel's comment addressed and otherwise > > > it is fine with me. > > > > > If we can get to an agreement on this I'll send a v2 without division. > > > > Note that we will still often get the case where a laptop is charged, > > > reports full, is unplugged for 5 minutes and then replugged and then > > > reports a capacity of 97% combined with "not charging", so we will > > > still need to fix userspace to handle this. > > > > Yes, I agree that should be addressed in userspace, as it is pretty > much a policy decision. But it is battery hardware doing this. So you need to take it into account. > > For the record, I don't think I'm okay with this. > > > > There's nothing special about 100% charge. > > > > I don't agree there is nothing special about 100% charge. There is a > separate state to represent battery full for a reason, which is the > user wanting to know when their battery is 100% charged and not being > discharged. Yes, there is full for a reason, and you can be full at 97%, for example, as described above. Pavel
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index cb97b6105f52..82e194290f01 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -217,8 +217,12 @@ static int acpi_battery_handle_discharging(struct acpi_battery *battery) * was plugged in and the device thus did not start a new charge cycle. */ if ((battery_ac_is_broken || power_supply_is_system_supplied()) && - battery->rate_now == 0) + battery->rate_now == 0) { + if (battery->capacity_now && battery->full_charge_capacity && + battery->capacity_now / battery->full_charge_capacity == 1) + return POWER_SUPPLY_STATUS_FULL; return POWER_SUPPLY_STATUS_NOT_CHARGING; + } return POWER_SUPPLY_STATUS_DISCHARGING; }
Commit 19fffc8450d4378580a8f019b195c4617083176f fixed reporting "Discharging" on some machines when AC was connected but the battery was not charging. But now on these machines the battery status is reported as "Not charging" even when the battery is fully charged. This commit takes the battery capacity into consideration when checking if "Not charging" should be returned and "Full" is returned when the capacity is 100%. Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> --- drivers/acpi/battery.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)