[PATCH] battery: Fix charge_now returned by broken batteries

[PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

Some broken batteries like my DELL NR2227 or a friend's DELL GK4798 return
the design_capacity (charge_full_design) as capacity_now (charge_now)
when completely charged.

I noticed this when looking at a battery plugin that reported "127% charged".
Some of these plugins have already "fixed" this in userspace by coding
something like min(percentage, 100)).

Reading /sys/class/power_supply/BAT0/* my battery reported:

	charge_full = 5980000
	charge_full_design = 7650000

I let my battery discharge a little bit and then I started charging it.
At the same time, I read charge_now every second:

...
5850000 (charging)
5850000 (charging)
5850000 (charging)
...
5900000 (charging)
5900000 (charging)
5900000 (charging)
...
5950000 (charging)
5950000 (charging)
5950000 (charging)
7650000 (charged)
7650000 (charged)
7650000 (charged)
7650000 (charged)
...

So I discovered that the battery wrongly returns charge_full_design when
completely charged instead of charge_full.

This patch fixes this by returning min(capacity_now, full_charge_capacity)
on both procfs and sysfs.

Now the userspace plugins report the correct 100% and their userspace check
may not be needed (if this error is the only one producing >100% results).

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 drivers/acpi/battery.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--- linux-2.6.31.1/drivers/acpi/battery.c.old	2009-10-04 15:50:37.743999621 +0200
+++ linux-2.6.31.1/drivers/acpi/battery.c	2009-10-04 16:33:47.843696066 +0200
@@ -210,7 +210,10 @@ static int acpi_battery_get_property(str
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		val->intval = battery->capacity_now * 1000;
+		/* broken batteries return the design_capacity
+		as capacity_now when completely charged */
+		val->intval = min(battery->capacity_now,
+				  battery->full_charge_capacity) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
@@ -617,8 +620,12 @@ static int acpi_battery_print_state(stru
 	if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN)
 		seq_printf(seq, "remaining capacity:      unknown\n");
 	else
+		/* broken batteries return the design_capacity
+		as capacity_now when completely charged */
 		seq_printf(seq, "remaining capacity:      %d %sh\n",
-			   battery->capacity_now, acpi_battery_units(battery));
+			   min(battery->capacity_now,
+			       battery->full_charge_capacity),
+			   acpi_battery_units(battery));
 	if (battery->voltage_now == ACPI_BATTERY_VALUE_UNKNOWN)
 		seq_printf(seq, "present voltage:         unknown\n");
 	else

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Henrique de Moraes Holschuh]

On Sun, 04 Oct 2009, Miguel Ojeda wrote:
> Some broken batteries like my DELL NR2227 or a friend's DELL GK4798 return
> the design_capacity (charge_full_design) as capacity_now (charge_now)
> when completely charged.
> 
> I noticed this when looking at a battery plugin that reported "127% charged".
> Some of these plugins have already "fixed" this in userspace by coding
> something like min(percentage, 100)).

A battery can be charged above 100%.  It just depends what you call 100%,
and the "I am full" level *varies* in a non-monotonic way during the battery
lifetime...

So, if you don't want to see > 100%, you have to clamp it to 100% and lose
information (when your "100%" level is actually increasing as the thing
keeps charging and you keep raising the baseline so that it doesn't go over
100%).

> So I discovered that the battery wrongly returns charge_full_design when
> completely charged instead of charge_full.

Ick.

> This patch fixes this by returning min(capacity_now, full_charge_capacity)
> on both procfs and sysfs.

What will it cause on non-broken batteries?  Or during gauge reset, when any
battery that updates full_charge_capacity only at the end of the cycle will
really have capacity_now > full_charge_capacity ?

> Now the userspace plugins report the correct 100% and their userspace check
> may not be needed (if this error is the only one producing >100% results).

Like I said, > 100% can happen, unless what you define to be 100% is very
elastic (and gets updated all the time).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Sun, Oct 4, 2009 at 6:45 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Sun, 04 Oct 2009, Miguel Ojeda wrote:
>> Some broken batteries like my DELL NR2227 or a friend's DELL GK4798 return
>> the design_capacity (charge_full_design) as capacity_now (charge_now)
>> when completely charged.
>>
>> I noticed this when looking at a battery plugin that reported "127% charged".
>> Some of these plugins have already "fixed" this in userspace by coding
>> something like min(percentage, 100)).
>
> A battery can be charged above 100%.  It just depends what you call 100%,
> and the "I am full" level *varies* in a non-monotonic way during the battery
> lifetime...
>
> So, if you don't want to see > 100%, you have to clamp it to 100% and lose
> information (when your "100%" level is actually increasing as the thing
> keeps charging and you keep raising the baseline so that it doesn't go over
> 100%).

If the 100% level increased, then full_charge_capacity (a.k.a. "_last_
full capacity" as seen in /proc) will increase as well, won't it? If
the battery went over that 100% that means there is a "new" 100%, why
are we losing information?.

I am asking, I am not an expert on battery stuff.

>
>> So I discovered that the battery wrongly returns charge_full_design when
>> completely charged instead of charge_full.
>
> Ick.
>
>> This patch fixes this by returning min(capacity_now, full_charge_capacity)
>> on both procfs and sysfs.
>
> What will it cause on non-broken batteries?  Or during gauge reset, when any
> battery that updates full_charge_capacity only at the end of the cycle will
> really have capacity_now > full_charge_capacity ?

Well, does it make sense to have capacity_now higher than
full_charge_capacity? Wouldn't that information be broken too?

Again, I am just wondering.

>
>> Now the userspace plugins report the correct 100% and their userspace check
>> may not be needed (if this error is the only one producing >100% results).
>
> Like I said, > 100% can happen, unless what you define to be 100% is very
> elastic (and gets updated all the time).

I still think it does not make sense to have a battery charged over
its 100% capacity whatever the definition of 100% is. Maybe I do not
understand your point.

>
> --
>  "One disk to rule them all, One disk to find them. One disk to bring
>  them all and in the darkness grind them. In the Land of Redmond
>  where the shadows lie." -- The Silicon Valley Tarot
>  Henrique Holschuh
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Alexey Starikovskiy]

Hi Miguel,

I am going to reject your patch on the basis, that the battery driver should report only
information it gained from battery hardware, not interpret it in any way.
As your patch fall into "interpret" category, it does not belong in the kernel and battery
driver in particular. You may suggest it to any/all user space battery monitoring applications,
this is the place for "interpretations".

Not-acknowledged-by: Alexey Starikovskiy


Regards,
Alex.


Miguel Ojeda пишет:
> On Sun, Oct 4, 2009 at 6:45 PM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
>> On Sun, 04 Oct 2009, Miguel Ojeda wrote:
>>> Some broken batteries like my DELL NR2227 or a friend's DELL GK4798 return
>>> the design_capacity (charge_full_design) as capacity_now (charge_now)
>>> when completely charged.
>>>
>>> I noticed this when looking at a battery plugin that reported "127% charged".
>>> Some of these plugins have already "fixed" this in userspace by coding
>>> something like min(percentage, 100)).
>> A battery can be charged above 100%.  It just depends what you call 100%,
>> and the "I am full" level *varies* in a non-monotonic way during the battery
>> lifetime...
>>
>> So, if you don't want to see > 100%, you have to clamp it to 100% and lose
>> information (when your "100%" level is actually increasing as the thing
>> keeps charging and you keep raising the baseline so that it doesn't go over
>> 100%).
> 
> If the 100% level increased, then full_charge_capacity (a.k.a. "_last_
> full capacity" as seen in /proc) will increase as well, won't it? If
> the battery went over that 100% that means there is a "new" 100%, why
> are we losing information?.
> 
> I am asking, I am not an expert on battery stuff.
> 
>>> So I discovered that the battery wrongly returns charge_full_design when
>>> completely charged instead of charge_full.
>> Ick.
>>
>>> This patch fixes this by returning min(capacity_now, full_charge_capacity)
>>> on both procfs and sysfs.
>> What will it cause on non-broken batteries?  Or during gauge reset, when any
>> battery that updates full_charge_capacity only at the end of the cycle will
>> really have capacity_now > full_charge_capacity ?
> 
> Well, does it make sense to have capacity_now higher than
> full_charge_capacity? Wouldn't that information be broken too?
> 
> Again, I am just wondering.
> 
>>> Now the userspace plugins report the correct 100% and their userspace check
>>> may not be needed (if this error is the only one producing >100% results).
>> Like I said, > 100% can happen, unless what you define to be 100% is very
>> elastic (and gets updated all the time).
> 
> I still think it does not make sense to have a battery charged over
> its 100% capacity whatever the definition of 100% is. Maybe I do not
> understand your point.
> 
>> --
>>  "One disk to rule them all, One disk to find them. One disk to bring
>>  them all and in the darkness grind them. In the Land of Redmond
>>  where the shadows lie." -- The Silicon Valley Tarot
>>  Henrique Holschuh
>>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Rafael J. Wysocki]

On Sunday 04 October 2009, Alexey Starikovskiy wrote:
> Hi Miguel,

Hi Alex,

> I am going to reject your patch on the basis, that the battery driver should report only
> information it gained from battery hardware, not interpret it in any way.
> As your patch fall into "interpret" category, it does not belong in the kernel and battery
> driver in particular. You may suggest it to any/all user space battery monitoring applications,
> this is the place for "interpretations".

Well, we do quirks for PCI devices, suspend quirks etc. in the kernel, so I'm
not really sure we should use the "no interpretation" as a general rule.  IMO,
if there's a known broken system needing a quirk, it may just be more
reasonable to put the quirk into the kernel than to put it into every single
user application out there.

In this particular case we have an evidently quirky hardware (or BIOS) and it's
not a fundamentally wrong idea to try to address that problem in the kernel.

Thanks,
Rafael

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Alexey Starikovskiy]

Hi Rafael,

This is not my rule, it was/is the rule of power device class. If you do not agree to it, please change
appropriate documentation.

Regards,
Alex.

Rafael J. Wysocki пишет:
> On Sunday 04 October 2009, Alexey Starikovskiy wrote:
>> Hi Miguel,
> 
> Hi Alex,
> 
>> I am going to reject your patch on the basis, that the battery driver should report only
>> information it gained from battery hardware, not interpret it in any way.
>> As your patch fall into "interpret" category, it does not belong in the kernel and battery
>> driver in particular. You may suggest it to any/all user space battery monitoring applications,
>> this is the place for "interpretations".
> 
> Well, we do quirks for PCI devices, suspend quirks etc. in the kernel, so I'm
> not really sure we should use the "no interpretation" as a general rule.  IMO,
> if there's a known broken system needing a quirk, it may just be more
> reasonable to put the quirk into the kernel than to put it into every single
> user application out there.
> 
> In this particular case we have an evidently quirky hardware (or BIOS) and it's
> not a fundamentally wrong idea to try to address that problem in the kernel.
> 
> Thanks,
> Rafael

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Sun, Oct 4, 2009 at 8:57 PM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> Hi Miguel,
>
> I am going to reject your patch on the basis, that the battery driver should
> report only
> information it gained from battery hardware, not interpret it in any way.
> As your patch fall into "interpret" category, it does not belong in the
> kernel and battery
> driver in particular. You may suggest it to any/all user space battery
> monitoring applications,
> this is the place for "interpretations".

I understand your point. However, there are other parts in the same
file that need to do "interpretation", for example:

http://lxr.linux.no/linux+*/drivers/acpi/battery.c#L157

 157        /* fallback to using design values for broken batteries */
 158        if (battery->design_capacity == battery->capacity_now)
 159                return 1;

I agree with Rafael, the kernel must interpret the hardware as best as
possible (it is its job, isn't it?). In countless places the kernel
has to "fix" (workaround) hardware bugs in order to present userspace
a unified interface, correct values, etc.

In this case my battery is reporting a obvious wrong value that causes
(apparently) correct userspace applications to misbehave.

Maybe the patch I proposed is not the correct solution; however, the
bug remains.

>
> Not-acknowledged-by: Alexey Starikovskiy
>
>
> Regards,
> Alex.
>
>
> Miguel Ojeda пишет:
>>
>> On Sun, Oct 4, 2009 at 6:45 PM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>>>
>>> On Sun, 04 Oct 2009, Miguel Ojeda wrote:
>>>>
>>>> Some broken batteries like my DELL NR2227 or a friend's DELL GK4798
>>>> return
>>>> the design_capacity (charge_full_design) as capacity_now (charge_now)
>>>> when completely charged.
>>>>
>>>> I noticed this when looking at a battery plugin that reported "127%
>>>> charged".
>>>> Some of these plugins have already "fixed" this in userspace by coding
>>>> something like min(percentage, 100)).
>>>
>>> A battery can be charged above 100%.  It just depends what you call 100%,
>>> and the "I am full" level *varies* in a non-monotonic way during the
>>> battery
>>> lifetime...
>>>
>>> So, if you don't want to see > 100%, you have to clamp it to 100% and
>>> lose
>>> information (when your "100%" level is actually increasing as the thing
>>> keeps charging and you keep raising the baseline so that it doesn't go
>>> over
>>> 100%).
>>
>> If the 100% level increased, then full_charge_capacity (a.k.a. "_last_
>> full capacity" as seen in /proc) will increase as well, won't it? If
>> the battery went over that 100% that means there is a "new" 100%, why
>> are we losing information?.
>>
>> I am asking, I am not an expert on battery stuff.
>>
>>>> So I discovered that the battery wrongly returns charge_full_design when
>>>> completely charged instead of charge_full.
>>>
>>> Ick.
>>>
>>>> This patch fixes this by returning min(capacity_now,
>>>> full_charge_capacity)
>>>> on both procfs and sysfs.
>>>
>>> What will it cause on non-broken batteries?  Or during gauge reset, when
>>> any
>>> battery that updates full_charge_capacity only at the end of the cycle
>>> will
>>> really have capacity_now > full_charge_capacity ?
>>
>> Well, does it make sense to have capacity_now higher than
>> full_charge_capacity? Wouldn't that information be broken too?
>>
>> Again, I am just wondering.
>>
>>>> Now the userspace plugins report the correct 100% and their userspace
>>>> check
>>>> may not be needed (if this error is the only one producing >100%
>>>> results).
>>>
>>> Like I said, > 100% can happen, unless what you define to be 100% is very
>>> elastic (and gets updated all the time).
>>
>> I still think it does not make sense to have a battery charged over
>> its 100% capacity whatever the definition of 100% is. Maybe I do not
>> understand your point.
>>
>>> --
>>>  "One disk to rule them all, One disk to find them. One disk to bring
>>>  them all and in the darkness grind them. In the Land of Redmond
>>>  where the shadows lie." -- The Silicon Valley Tarot
>>>  Henrique Holschuh
>>>
>
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> Hi Rafael,
>
> This is not my rule, it was/is the rule of power device class. If you do not
> agree to it, please change
> appropriate documentation.

Oh, I did not know that. Thank you for pointing it out. I think you
are refering to:

 158Q: Suppose, my battery monitoring chip/firmware does not provides capacity
 159   in percents, but provides charge_{now,full,empty}. Should I calculate
 160   percentage capacity manually, inside the driver, and register CAPACITY
 161   attribute? The same question about time_to_empty/time_to_full.
 162A: Most likely, no. This class is designed to export properties which are
 163   directly measurable by the specific hardware available.
 164
 165   Inferring not available properties using some heuristics or mathematical
 166   model is not subject of work for a battery driver. Such functionality
 167   should be factored out, and in fact, apm_power, the driver to serve
 168   legacy APM API on top of power supply class, uses a simple heuristic of
 169   approximating remaining battery capacity based on its charge, current,
 170   voltage and so on. But full-fledged battery model is likely not subject
 171   for kernel at all, as it would require floating point calculation to deal
 172   with things like differential equations and Kalman filters. This is
 173   better be handled by batteryd/libbattery, yet to be written.

We are not calculating anything new just by the pleasure of doing it,
we are correcting a wrong value provided by the hardware.

Please correct me if I misunderstood you.

>
> Regards,
> Alex.
>
> Rafael J. Wysocki пишет:
>>
>> On Sunday 04 October 2009, Alexey Starikovskiy wrote:
>>>
>>> Hi Miguel,
>>
>> Hi Alex,
>>
>>> I am going to reject your patch on the basis, that the battery driver
>>> should report only
>>> information it gained from battery hardware, not interpret it in any way.
>>> As your patch fall into "interpret" category, it does not belong in the
>>> kernel and battery
>>> driver in particular. You may suggest it to any/all user space battery
>>> monitoring applications,
>>> this is the place for "interpretations".
>>
>> Well, we do quirks for PCI devices, suspend quirks etc. in the kernel, so
>> I'm
>> not really sure we should use the "no interpretation" as a general rule.
>>  IMO,
>> if there's a known broken system needing a quirk, it may just be more
>> reasonable to put the quirk into the kernel than to put it into every
>> single
>> user application out there.
>>
>> In this particular case we have an evidently quirky hardware (or BIOS) and
>> it's
>> not a fundamentally wrong idea to try to address that problem in the
>> kernel.
>>
>> Thanks,
>> Rafael
>
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Alexey Starikovskiy]

Miguel Ojeda пишет:
> On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
> <astarikovskiy@suse.de> wrote:
>> Hi Rafael,
>>
>> This is not my rule, it was/is the rule of power device class. If you do not
>> agree to it, please change
>> appropriate documentation.
> 
> Oh, I did not know that. Thank you for pointing it out. I think you
> are refering to:
> 
>  158Q: Suppose, my battery monitoring chip/firmware does not provides capacity
>  159   in percents, but provides charge_{now,full,empty}. Should I calculate
>  160   percentage capacity manually, inside the driver, and register CAPACITY
>  161   attribute? The same question about time_to_empty/time_to_full.
>  162A: Most likely, no. This class is designed to export properties which are
>  163   directly measurable by the specific hardware available.
>  164
>  165   Inferring not available properties using some heuristics or mathematical
>  166   model is not subject of work for a battery driver. Such functionality
>  167   should be factored out, and in fact, apm_power, the driver to serve
>  168   legacy APM API on top of power supply class, uses a simple heuristic of
>  169   approximating remaining battery capacity based on its charge, current,
>  170   voltage and so on. But full-fledged battery model is likely not subject
>  171   for kernel at all, as it would require floating point calculation to deal
>  172   with things like differential equations and Kalman filters. This is
>  173   better be handled by batteryd/libbattery, yet to be written.
> 
> We are not calculating anything new just by the pleasure of doing it,
> we are correcting a wrong value provided by the hardware.

You are guessing that normal battery can not jump charge value, and on this assumption you
cap charge_now with last full_charge. Immediate problem is that full_charge is not fixed value,
this is why it is separated from design_full_charge.
During battery life full_charge may go down and up, depending on outside temperature, battery discharge (full or partial).
I've seen batteries on some new machines reporting full charge of more than design charge.
Obviously, your patch will fail in some of the above situations.
Currently, reading from the driver is "last resort" to get un-interpreted value, if you have any doubts on it. With
your patch, this is gone, and user space will have to interpret results of kernel interpreter.

Return to the "bug still exists". We are referring to the value, which can not be measured directly with any known device.
Thus, it may be completely sane that battery manufacturer provides you with some charge curve, but knows only two values on it
which he may trust -- point where he can not get any power out of it (complete discharge) and point where he can not put any additional charge into the battery (due to various stop conditions (battery temperature being one)). In such a case manufacturer may choose to adjust charge curve to end up always at design_full_charge point, no matter how much of adjustment this requires.

Do you have the same jump from >100% to 99% on discharge?

Regards,
Alex.

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Rafael J. Wysocki]

On Sunday 04 October 2009, Alexey Starikovskiy wrote:
> Hi Rafael,

Alex,

> This is not my rule, it was/is the rule of power device class. If you do not agree to it, please change
> appropriate documentation.

I think we're talking about two different things.  One thing is that we
shouldn't put any _arbitrary_ interpretation rules into the kernel, which I
agree with.  The other one is that if there's a _known_ _broken_ hardware
and one possible way of handling it is to add a quirk into the kernel, we
should at least consider doing that.

In my opinion adding a quirk for a broken hardware is not equivalent to
"inferring not available properties using some heuristics or mathematical
model", if that's what you're referring to.

That said, the patch should not change the _default_ code in order to handle
the quirky hardware correctly.  IMO, the quirky hardware should be recognized
during initialisation, if possible, and later handled in a special way.  If
it's not possible to detect the broken hardware reliably, I agree that there's
nothing we can do about that in the kernel.

Thanks,
Rafael

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Alexey Starikovskiy]

Rafael J. Wysocki пишет:
> On Sunday 04 October 2009, Alexey Starikovskiy wrote:
>> Hi Rafael,
> 
> Alex,
> 
>> This is not my rule, it was/is the rule of power device class. If you do not agree to it, please change
>> appropriate documentation.
> 
> I think we're talking about two different things.  One thing is that we
> shouldn't put any _arbitrary_ interpretation rules into the kernel, which I
> agree with.  The other one is that if there's a _known_ _broken_ hardware
> and one possible way of handling it is to add a quirk into the kernel, we
> should at least consider doing that.
> 
> In my opinion adding a quirk for a broken hardware is not equivalent to
> "inferring not available properties using some heuristics or mathematical
> model", if that's what you're referring to.
No, this is not a clear "bug" and not a clear "fix". Please read my reply to Miguel.

> 
> That said, the patch should not change the _default_ code in order to handle
> the quirky hardware correctly.  IMO, the quirky hardware should be recognized
It will change behaviour of at least Samsung notebooks, for which I personally saw the 
charge_now/full_charge being greater then design_charge.
> during initialisation, if possible, and later handled in a special way.  If
> it's not possible to detect the broken hardware reliably, I agree that there's
> nothing we can do about that in the kernel.
I am still not sure if we have a broken hardware here.

Regards,
Alex.

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Mon, Oct 5, 2009 at 12:38 AM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> Miguel Ojeda пишет:
>>
>> On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
>> <astarikovskiy@suse.de> wrote:
>>>
>>> Hi Rafael,
>>>
>>> This is not my rule, it was/is the rule of power device class. If you do
>>> not
>>> agree to it, please change
>>> appropriate documentation.
>>
>> Oh, I did not know that. Thank you for pointing it out. I think you
>> are refering to:
>>
>>  158Q: Suppose, my battery monitoring chip/firmware does not provides
>> capacity
>>  159   in percents, but provides charge_{now,full,empty}. Should I
>> calculate
>>  160   percentage capacity manually, inside the driver, and register
>> CAPACITY
>>  161   attribute? The same question about time_to_empty/time_to_full.
>>  162A: Most likely, no. This class is designed to export properties which
>> are
>>  163   directly measurable by the specific hardware available.
>>  164
>>  165   Inferring not available properties using some heuristics or
>> mathematical
>>  166   model is not subject of work for a battery driver. Such
>> functionality
>>  167   should be factored out, and in fact, apm_power, the driver to serve
>>  168   legacy APM API on top of power supply class, uses a simple
>> heuristic of
>>  169   approximating remaining battery capacity based on its charge,
>> current,
>>  170   voltage and so on. But full-fledged battery model is likely not
>> subject
>>  171   for kernel at all, as it would require floating point calculation
>> to deal
>>  172   with things like differential equations and Kalman filters. This is
>>  173   better be handled by batteryd/libbattery, yet to be written.
>>
>> We are not calculating anything new just by the pleasure of doing it,
>> we are correcting a wrong value provided by the hardware.
>
> You are guessing that normal battery can not jump charge value, and on this
> assumption you
> cap charge_now with last full_charge. Immediate problem is that full_charge
> is not fixed value,
> this is why it is separated from design_full_charge.
> During battery life full_charge may go down and up, depending on outside
> temperature, battery discharge (full or partial).
> I've seen batteries on some new machines reporting full charge of more than
> design charge.
> Obviously, your patch will fail in some of the above situations.

I don't see why. The patch compares against full_charge every time
(which is updated as you say), not against design_full_charge.

1. full_charge > design_full_charge => OK, design_full_charge is not
involved in the min() operation.
2. full_charge goes down => If charge_now > full_charge then hardware
is wrong and we should read full_charge. OK.
3. full_charge goes up => Same.

 What do you mean by failing in such situations? My point is that,
AFAIK charge_now shouldn't be higher than full_charge *. The patch
does not care about design_full_charge, neither the original code.

* I do not know about some overcharging issues that Henrique mentioned.

> Currently, reading from the driver is "last resort" to get un-interpreted
> value, if you have any doubts on it. With
> your patch, this is gone, and user space will have to interpret results of
> kernel interpreter.
>
> Return to the "bug still exists". We are referring to the value, which can
> not be measured directly with any known device.
> Thus, it may be completely sane that battery manufacturer provides you with
> some charge curve, but knows only two values on it
> which he may trust -- point where he can not get any power out of it
> (complete discharge) and point where he can not put any additional charge
> into the battery (due to various stop conditions (battery temperature being
> one)). In such a case manufacturer may choose to adjust charge curve to end
> up always at design_full_charge point, no matter how much of adjustment this
> requires.

I understand that and you may be right; however, AFAIK, a userspace
application has no way to know that it should compare against
full_charge every time _except_ when in "charged" state, has it? Is
there any kind of protocol/documentation that establish what an
userspace application should do?

The battery reports correctly full_charge in order to compare with
charge_now (and as I checked, some userspace plugins always check
against that full_charge, not design_full_charge, which seems to be
the logical thing to do, who cares what the design full charge level
was when reporting the actual charge level?).

>
> Do you have the same jump from >100% to 99% on discharge?

Yes: When in "charged" state, the battery reports a fixed value
(desing_full_charge, it is always the same). In "charging" or
"discharging" it correctly reports the current charge. It also
correctly reports full_charge, not matter what state.

So, maybe the battery works as you suggested; still, the kernel should
provide a common meaning to its interfaces. If some batteries report
full_charge when in "charged" state and others report
design_full_charge, then the kernel should convert all of them into
one unique convention, or there is no sane way to write userspace
applications.

>
> Regards,
> Alex.
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Mon, Oct 5, 2009 at 12:56 AM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> Rafael J. Wysocki пишет:
>>
>> On Sunday 04 October 2009, Alexey Starikovskiy wrote:
>>>
>>> Hi Rafael,
>>
>> Alex,
>>
>>> This is not my rule, it was/is the rule of power device class. If you do
>>> not agree to it, please change
>>> appropriate documentation.
>>
>> I think we're talking about two different things.  One thing is that we
>> shouldn't put any _arbitrary_ interpretation rules into the kernel, which
>> I
>> agree with.  The other one is that if there's a _known_ _broken_ hardware
>> and one possible way of handling it is to add a quirk into the kernel, we
>> should at least consider doing that.
>>
>> In my opinion adding a quirk for a broken hardware is not equivalent to
>> "inferring not available properties using some heuristics or mathematical
>> model", if that's what you're referring to.
>
> No, this is not a clear "bug" and not a clear "fix". Please read my reply to
> Miguel.
>
>>
>> That said, the patch should not change the _default_ code in order to
>> handle
>> the quirky hardware correctly.  IMO, the quirky hardware should be
>> recognized
>
> It will change behaviour of at least Samsung notebooks, for which I
> personally saw the charge_now/full_charge being greater then design_charge.
>>
>> during initialisation, if possible, and later handled in a special way.
>>  If
>> it's not possible to detect the broken hardware reliably, I agree that
>> there's
>> nothing we can do about that in the kernel.
>
> I am still not sure if we have a broken hardware here.

I have no idea about batteries, but jumping from 5950 mAh to 7650 mAh
within one second does not seem non-broken to me. ;)

>
> Regards,
> Alex.
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Alexey Starikovskiy]

Miguel Ojeda пишет:
> On Mon, Oct 5, 2009 at 12:38 AM, Alexey Starikovskiy
> <astarikovskiy@suse.de> wrote:
>> Miguel Ojeda пишет:
>>> On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
>>> <astarikovskiy@suse.de> wrote:
>>>> Hi Rafael,
>>>>
>>>> This is not my rule, it was/is the rule of power device class. If you do
>>>> not
>>>> agree to it, please change
>>>> appropriate documentation.
>>> Oh, I did not know that. Thank you for pointing it out. I think you
>>> are refering to:
>>>
>>>  158Q: Suppose, my battery monitoring chip/firmware does not provides
>>> capacity
>>>  159   in percents, but provides charge_{now,full,empty}. Should I
>>> calculate
>>>  160   percentage capacity manually, inside the driver, and register
>>> CAPACITY
>>>  161   attribute? The same question about time_to_empty/time_to_full.
>>>  162A: Most likely, no. This class is designed to export properties which
>>> are
>>>  163   directly measurable by the specific hardware available.
>>>  164
>>>  165   Inferring not available properties using some heuristics or
>>> mathematical
>>>  166   model is not subject of work for a battery driver. Such
>>> functionality
>>>  167   should be factored out, and in fact, apm_power, the driver to serve
>>>  168   legacy APM API on top of power supply class, uses a simple
>>> heuristic of
>>>  169   approximating remaining battery capacity based on its charge,
>>> current,
>>>  170   voltage and so on. But full-fledged battery model is likely not
>>> subject
>>>  171   for kernel at all, as it would require floating point calculation
>>> to deal
>>>  172   with things like differential equations and Kalman filters. This is
>>>  173   better be handled by batteryd/libbattery, yet to be written.
>>>
>>> We are not calculating anything new just by the pleasure of doing it,
>>> we are correcting a wrong value provided by the hardware.
>> You are guessing that normal battery can not jump charge value, and on this
>> assumption you
>> cap charge_now with last full_charge. Immediate problem is that full_charge
>> is not fixed value,
>> this is why it is separated from design_full_charge.
>> During battery life full_charge may go down and up, depending on outside
>> temperature, battery discharge (full or partial).
>> I've seen batteries on some new machines reporting full charge of more than
>> design charge.
>> Obviously, your patch will fail in some of the above situations.
> 
> I don't see why. The patch compares against full_charge every time
> (which is updated as you say), not against design_full_charge.
> 
> 1. full_charge > design_full_charge => OK, design_full_charge is not
> involved in the min() operation.
> 2. full_charge goes down => If charge_now > full_charge then hardware
> is wrong and we should read full_charge. OK.
> 3. full_charge goes up => Same.
full_charge_capacity is the value of last full charge. It will be updated to
current full charge, when the charging is complete. It may end up lower or greater than
previous value.
Comparing current charge with the last full charge may correctly give you >100%.
Now we have a decision to make -- do we update full charge to be greater than charge now,
or do we update charge now to be lower than full charge. 
> So, maybe the battery works as you suggested; still, the kernel should
> provide a common meaning to its interfaces. If some batteries report
> full_charge when in "charged" state and others report
> design_full_charge, then the kernel should convert all of them into
> one unique convention, or there is no sane way to write userspace
> applications.

I still want to receive raw data from driver, and have 1 level of interpreters.
As I understand, there is HAL, which may do interpretations for you and keep it in single location.
>> Regards,
>> Alex.
>>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Mon, Oct 5, 2009 at 2:18 AM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> Miguel Ojeda пишет:
>>
>> On Mon, Oct 5, 2009 at 12:38 AM, Alexey Starikovskiy
>> <astarikovskiy@suse.de> wrote:
>>>
>>> Miguel Ojeda пишет:
>>>>
>>>> On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
>>>> <astarikovskiy@suse.de> wrote:
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> This is not my rule, it was/is the rule of power device class. If you
>>>>> do
>>>>> not
>>>>> agree to it, please change
>>>>> appropriate documentation.
>>>>
>>>> Oh, I did not know that. Thank you for pointing it out. I think you
>>>> are refering to:
>>>>
>>>>  158Q: Suppose, my battery monitoring chip/firmware does not provides
>>>> capacity
>>>>  159   in percents, but provides charge_{now,full,empty}. Should I
>>>> calculate
>>>>  160   percentage capacity manually, inside the driver, and register
>>>> CAPACITY
>>>>  161   attribute? The same question about time_to_empty/time_to_full.
>>>>  162A: Most likely, no. This class is designed to export properties
>>>> which
>>>> are
>>>>  163   directly measurable by the specific hardware available.
>>>>  164
>>>>  165   Inferring not available properties using some heuristics or
>>>> mathematical
>>>>  166   model is not subject of work for a battery driver. Such
>>>> functionality
>>>>  167   should be factored out, and in fact, apm_power, the driver to
>>>> serve
>>>>  168   legacy APM API on top of power supply class, uses a simple
>>>> heuristic of
>>>>  169   approximating remaining battery capacity based on its charge,
>>>> current,
>>>>  170   voltage and so on. But full-fledged battery model is likely not
>>>> subject
>>>>  171   for kernel at all, as it would require floating point calculation
>>>> to deal
>>>>  172   with things like differential equations and Kalman filters. This
>>>> is
>>>>  173   better be handled by batteryd/libbattery, yet to be written.
>>>>
>>>> We are not calculating anything new just by the pleasure of doing it,
>>>> we are correcting a wrong value provided by the hardware.
>>>
>>> You are guessing that normal battery can not jump charge value, and on
>>> this
>>> assumption you
>>> cap charge_now with last full_charge. Immediate problem is that
>>> full_charge
>>> is not fixed value,
>>> this is why it is separated from design_full_charge.
>>> During battery life full_charge may go down and up, depending on outside
>>> temperature, battery discharge (full or partial).
>>> I've seen batteries on some new machines reporting full charge of more
>>> than
>>> design charge.
>>> Obviously, your patch will fail in some of the above situations.
>>
>> I don't see why. The patch compares against full_charge every time
>> (which is updated as you say), not against design_full_charge.
>>
>> 1. full_charge > design_full_charge => OK, design_full_charge is not
>> involved in the min() operation.
>> 2. full_charge goes down => If charge_now > full_charge then hardware
>> is wrong and we should read full_charge. OK.
>> 3. full_charge goes up => Same.
>
> full_charge_capacity is the value of last full charge. It will be updated to
> current full charge, when the charging is complete. It may end up lower or
> greater than
> previous value.
> Comparing current charge with the last full charge may correctly give you
>>100%.

Then maybe we can write something like...

val->intval = acpi_battery_is_charged(battery)
	? min(battery->capacity_now, battery->full_charge_capacity) * 1000
	: battery->capacity_now * 1000;

So we only use the min() operation when it is fully charged (returning
to 100%) without losing information when charging.

The problem is that percentage may jump from >100% to 100% in
batteries whose full capacity increase, but I think that is OK, since
when completely charged, the >100% is the new 100%.

In "broken" batteries (is it broken finally? or is it expected
behaviour?) like mine the old problem will be corrected, as it was
only present in the charged state.

Still other special cases may appear. What do you think?

> Now we have a decision to make -- do we update full charge to be greater
> than charge now,
> or do we update charge now to be lower than full charge.
>>
>> So, maybe the battery works as you suggested; still, the kernel should
>> provide a common meaning to its interfaces. If some batteries report
>> full_charge when in "charged" state and others report
>> design_full_charge, then the kernel should convert all of them into
>> one unique convention, or there is no sane way to write userspace
>> applications.
>
> I still want to receive raw data from driver, and have 1 level of
> interpreters.
> As I understand, there is HAL, which may do interpretations for you and keep
> it in single location.

AFAIK, the battery plugins I checked read /proc or /sys directly. I
will check other battery plugins too.

>>>
>>> Regards,
>>> Alex.
>>>
>
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Pavel Machek]

Hi!

> > current full charge, when the charging is complete. It may end up lower or
> > greater than
> > previous value.
> > Comparing current charge with the last full charge may correctly give you
> >>100%.
> 
> Then maybe we can write something like...
> 
> val->intval = acpi_battery_is_charged(battery)
> 	? min(battery->capacity_now, battery->full_charge_capacity) * 1000
> 	: battery->capacity_now * 1000;
> 
> So we only use the min() operation when it is fully charged (returning
> to 100%) without losing information when charging.
> 
> The problem is that percentage may jump from >100% to 100% in
> batteries whose full capacity increase, but I think that is OK, since
> when completely charged, the >100% is the new 100%.
> 
> In "broken" batteries (is it broken finally? or is it expected
> behaviour?) like mine the old problem will be corrected, as it was
> only present in the charged state.

I believe you better work around this in userspace... or agree that
>100% charge is possible.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Sat, Oct 10, 2009 at 2:04 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > current full charge, when the charging is complete. It may end up lower or
>> > greater than
>> > previous value.
>> > Comparing current charge with the last full charge may correctly give you
>> >>100%.
>>
>> Then maybe we can write something like...
>>
>> val->intval = acpi_battery_is_charged(battery)
>>       ? min(battery->capacity_now, battery->full_charge_capacity) * 1000
>>       : battery->capacity_now * 1000;
>>
>> So we only use the min() operation when it is fully charged (returning
>> to 100%) without losing information when charging.
>>
>> The problem is that percentage may jump from >100% to 100% in
>> batteries whose full capacity increase, but I think that is OK, since
>> when completely charged, the >100% is the new 100%.
>>
>> In "broken" batteries (is it broken finally? or is it expected
>> behaviour?) like mine the old problem will be corrected, as it was
>> only present in the charged state.
>
> I believe you better work around this in userspace... or agree that
>>100% charge is possible.

I agree that >100% charge is possible while charging (because that
would mean the battery is over the last charged level); however, what
does it mean when charged?

In any case, my laptop's battery is not charging over 100% its
original capacity anyway, just reporting a wrong value.

>
>                                                                Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Pavel Machek]

Hi!

> >> In "broken" batteries (is it broken finally? or is it expected
> >> behaviour?) like mine the old problem will be corrected, as it was
> >> only present in the charged state.
> >
> > I believe you better work around this in userspace... or agree that
> >>100% charge is possible.
> 
> I agree that >100% charge is possible while charging (because that
> would mean the battery is over the last charged level); however, what
> does it mean when charged?

Well, maybe the battery only updates full_charge_capacity during
powerdown or when the moon is full or something? (IOW you may be
breaking already working machines).

> In any case, my laptop's battery is not charging over 100% its
> original capacity anyway, just reporting a wrong value.

True. But I do not think  you are fixing it properly. Maybe ask for
fixed BIOS?

Or perhaps add quirk based on DMI or something?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Miguel Ojeda]

On Sat, Oct 10, 2009 at 11:25 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> In "broken" batteries (is it broken finally? or is it expected
>> >> behaviour?) like mine the old problem will be corrected, as it was
>> >> only present in the charged state.
>> >
>> > I believe you better work around this in userspace... or agree that
>> >>100% charge is possible.
>>
>> I agree that >100% charge is possible while charging (because that
>> would mean the battery is over the last charged level); however, what
>> does it mean when charged?
>
> Well, maybe the battery only updates full_charge_capacity during
> powerdown or when the moon is full or something? (IOW you may be
> breaking already working machines).
>

I do not know about batteries as I said, I was waiting for someone to
point out how batteries (normally/should) work.

>> In any case, my laptop's battery is not charging over 100% its
>> original capacity anyway, just reporting a wrong value.
>
> True. But I do not think  you are fixing it properly. Maybe ask for
> fixed BIOS?

Well, many BIOS/systems are broken and Linux has to workaround them. I
am sure there are a lot of broken batteries out there.

>
> Or perhaps add quirk based on DMI or something?

My battery is easily identified by its model name, so maybe we can
apply the workaround only to known hardware. Still, that is a "heavy"
solution, as I suppose there are many many battery models in the
world. That is why I was asking whether some kind of standard/unified
batteries' values/behavior exists (or implement it) that we can try to
achieve easily with some heuristics like the one I proposed, instead
of some kind of huge table-based workaround system.

>                                                                        Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Henrique de Moraes Holschuh]

On Sat, 10 Oct 2009, Pavel Machek wrote:
> > >> In "broken" batteries (is it broken finally? or is it expected
> > >> behaviour?) like mine the old problem will be corrected, as it was
> > >> only present in the charged state.
> > >
> > > I believe you better work around this in userspace... or agree that
> > >>100% charge is possible.
> > 
> > I agree that >100% charge is possible while charging (because that
> > would mean the battery is over the last charged level); however, what
> > does it mean when charged?
> 
> Well, maybe the battery only updates full_charge_capacity during
> powerdown or when the moon is full or something? (IOW you may be
> breaking already working machines).
> 
> > In any case, my laptop's battery is not charging over 100% its
> > original capacity anyway, just reporting a wrong value.
> 
> True. But I do not think  you are fixing it properly. Maybe ask for
> fixed BIOS?
> 
> Or perhaps add quirk based on DMI or something?

FIW, I do think we should attempt to fix situations that are always wrong,
we have a long history of doing that, and it doesn't make sense to send to
userspace stuff that we _know_ to be crap.

The problem is that to, e.g., fix last_full_capacity, you need to be able to
trust your reports of battery state (needs to be idle or discharging), and
current capacity.

So it ends up needing to be quirk-based, as different broken crap will fail
in conflicting ways, so you can't fix them all in one sweep, you'll just
make it worse.  And the _one_ thing we must never do is to make it worse for
the hardware/firmware that gets it right...

I agree with Pavel, can you make it quirk-based?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] battery: Fix charge_now returned by broken batteries

[Pavel Machek]

On Sat 2009-10-10 23:44:42, Miguel Ojeda wrote:
> On Sat, Oct 10, 2009 at 11:25 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> >> In "broken" batteries (is it broken finally? or is it expected
> >> >> behaviour?) like mine the old problem will be corrected, as it was
> >> >> only present in the charged state.
> >> >
> >> > I believe you better work around this in userspace... or agree that
> >> >>100% charge is possible.
> >>
> >> I agree that >100% charge is possible while charging (because that
> >> would mean the battery is over the last charged level); however, what
> >> does it mean when charged?
> >
> > Well, maybe the battery only updates full_charge_capacity during
> > powerdown or when the moon is full or something? (IOW you may be
> > breaking already working machines).
> >
> 
> I do not know about batteries as I said, I was waiting for someone to
> point out how batteries (normally/should) work.

Different batteries work very differently, I'm afraid.

> > Or perhaps add quirk based on DMI or something?
> 
> My battery is easily identified by its model name, so maybe we can
> apply the workaround only to known hardware. Still, that is a "heavy"
> solution, as I suppose there are many many battery models in the
> world. That is why I was asking whether some kind of standard/unified
> batteries' values/behavior exists (or implement it) that we can try to
> achieve easily with some heuristics like the one I proposed, instead
> of some kind of huge table-based workaround system.

The heuristics you proposed _will_ break other batteries, so it is not
acceptable without checking for model first.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html