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

On Mon, Oct 5, 2009 at 2:18 AM, Alexey Starikovskiy
<astarikovskiy@xxxxxxx> wrote:
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.

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.



--
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/

Relevant Pages

  • Re: [PATCH] battery: Fix charge_now returned by broken batteries
    ... my battery monitoring chip/firmware does not provides ...  159   in percents, but provides charge_. ... You are guessing that normal battery can not jump charge value, ... the battery reports a fixed value ...
    (Linux-Kernel)
  • Re: Lead acid Batteries
    ... You may be able to restore most of the capacity depending on the condition and age of the battery, but you will need a charge rate of 30 amps or so to boil off the plates. ... This charge and discharge cycling seems to improve the capacity ...
    (sci.electronics.repair)
  • Re: Unused Li-ion battery pack
    ... Store at about 50% charge in the coldest place you can find. ... a typical Li-ion laptop battery that is full ... 2/3rds of original capacity. ... 50% charge refreshed every two weeks at room temperature. ...
    (sci.electronics.repair)
  • Re: NiMH new battery conditioning
    ... Though it is possible to charge an eneloop battery in a "Quick ... We recommend charging eneloop ... Like the alleged increase in NiMH capacity produced by initially ...
    (sci.electronics.repair)
  • Re: Laptop battery
    ... without degrading to half capacity, ... and you must take a look on the charge from time to ... At least newer ones don't work without the battery ... "One disk to rule them all, ...
    (Debian-User)