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

[Miguel Ojeda]

On Mon, Oct 5, 2009 at 12:38 AM, Alexey Starikovskiy
<astarikovskiy@xxxxxxx> wrote:
> Miguel Ojeda ÐÐÑÐÑ:
>>
>> On Sun, Oct 4, 2009 at 11:36 PM, Alexey Starikovskiy
>> <astarikovskiy@xxxxxxx> 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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/