Skip to contentSkip to conversation

battery: Fix charge_now returned by broken batteries

PATCH In review Miguel Ojeda 5 participants 21 messages Oct 4, 2009 – Oct 10, 2009 ACPI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 battery: Fix charge_now returned by broken batteries

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).

---
 drivers/acpi/battery.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +9 -2
@@ -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
#1 lore raw
Quoted 7 lines · 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)).

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%).

Quoted 2 lines

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

Ick.

Quoted 2 lines

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 ?

Quoted 2 lines

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).

Signature · 4 lines
  "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
#2 lore raw
Quoted 9 lines · Henrique de Moraes Holschuh
Quoted 7 lines · 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)).

Quoted 8 lines

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.

Quoted 4 lines
Quoted 3 lines

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

Quoted 8 lines

Ick.

Quoted 2 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 7 lines

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

#3 lore raw

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 пишет:

Quoted 56 lines
Quoted 8 lines · Henrique de Moraes Holschuh
Quoted 7 lines · 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)).

Quoted 8 lines

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.

Quoted 9 lines
Quoted 2 lines

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

Ick.

Quoted 2 lines

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.

Quoted 4 lines
Quoted 2 lines

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.

Quoted 6 lines

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

#4 lore raw
Quoted 1 line · Alexey Starikovskiy

Hi Miguel,

Hi Alex,

Quoted 5 lines

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

#5 lore raw

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 пишет:

Quoted 17 lines · Alexey Starikovskiy
Quoted 1 line

Hi Miguel,

Hi Alex,

Quoted 5 lines

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.

Quoted 5 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
Quoted 5 lines · 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.

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.

Quoted 39 lines

Regards,
Alex.

Rafael J. Wysocki пишет:

Quoted 32 lines · Alexey Starikovskiy
Quoted 2 lines

Hi Miguel,

Hi Alex,

Quoted 8 lines

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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw

Miguel Ojeda пишет:

Quoted 28 lines
Quoted 5 lines · 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.

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.
Quoted 2 lines

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw
Quoted 45 lines · Alexey Starikovskiy

Miguel Ojeda пишет:

Quoted 41 lines
Quoted 7 lines · 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.

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.

Quoted 2 lines

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

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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?).

Quoted 2 lines

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.

Quoted 4 lines

Regards,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw

Miguel Ojeda пишет:

Quoted 54 lines
Quoted 41 lines · Alexey Starikovskiy

Miguel Ojeda пишет:

Quoted 38 lines
Quoted 6 lines · 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.

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.

Quoted 2 lines

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

Quoted 10 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 3 lines
Quoted 3 lines

Regards,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw
Quoted 77 lines · Alexey Starikovskiy

Miguel Ojeda пишет:

Quoted 67 lines
Quoted 50 lines · Alexey Starikovskiy

Miguel Ojeda пишет:

Quoted 45 lines
Quoted 8 lines · 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.

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.

Quoted 2 lines

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

Quoted 13 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 1 line

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?

Quoted 11 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines

Regards,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw

Hi!

Quoted 15 lines
Quoted 5 lines

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

Quoted 1 line

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.

Quoted 7 lines

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

Quoted 1 line

100% charge is possible.

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#12 lore raw
Quoted 21 lines · Pavel Machek

Hi!

Quoted 19 lines
Quoted 5 lines

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

Quoted 1 line

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%.

Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

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.

Quoted 6 lines

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

#13 lore raw

Hi!

Quoted 10 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

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).

Quoted 2 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#14 lore raw
Quoted 9 lines · Pavel Machek

Hi!

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

100% charge is possible.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 5 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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

#15 lore raw
Quoted 14 lines · Miguel Ojeda
Quoted 13 lines · Pavel Machek

Hi!

Quoted 10 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

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?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 9 lines
Quoted 1 line

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#16 lore raw
In reply to Pavel Machek
Quoted 15 lines · Pavel Machek
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

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).

Quoted 7 lines
Quoted 2 lines

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?

Signature · 4 lines
  "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
#17 lore raw
In reply to Alexey Starikovskiy
Quoted 1 line · Alexey Starikovskiy

Hi Rafael,

Alex,

Quoted 2 lines

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

#18 lore raw

Rafael J. Wysocki пишет:

Quoted 14 lines · Alexey Starikovskiy
Quoted 1 line

Hi Rafael,

Alex,

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#19 lore raw
Quoted 35 lines · Alexey Starikovskiy

Rafael J. Wysocki пишет:

Quoted 18 lines · Alexey Starikovskiy
Quoted 2 lines

Hi Rafael,

Alex,

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 1 line
Quoted 7 lines
Quoted 5 lines

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. ;)

Quoted 4 lines

Regards,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#20 lore raw
In reply to Alexey Starikovskiy
Quoted 10 lines · 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".

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.

Quoted 84 lines

Not-acknowledged-by: Alexey Starikovskiy

Regards,
Alex.

Miguel Ojeda пишет:

Quoted 73 lines
Quoted 18 lines · Henrique de Moraes Holschuh
Quoted 10 lines · 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)).

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

Quoted 6 lines

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.

Quoted 9 lines
Quoted 2 lines

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

Ick.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 6 lines

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