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 <[email protected]>
---
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;
}
--
2.19.1
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 <[email protected]>
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 <[email protected]>
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello Hans, thanks for your quick response,
On Sat, Nov 3, 2018 at 4:28 AM Hans de Goede <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi,
On 11/7/18 5:53 AM, Daniel Drake wrote:
> On Mon, Nov 5, 2018 at 1:19 AM Pavel Machek <[email protected]> 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 <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, Nov 11, 2018 at 4:22 AM Pavel Machek <[email protected]> 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 <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html