ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Richard Hughes]

When I insert the power lead or attach my T61 to the powered dock, the
battery power supply status goes like this:

0.00s Discharging
{dock}
0.10s Fully charged
1.00s Charging

This causes userspace (in my case gnome-power-manager) to pop up a
dialog telling me the battery is full, and the icon flickers to fully
green, then 10% green and charging.

This seems to have existed as long as the power supply class was being
used by the acpi battery, but we've always relied on a userspace fudge.

This is tested with 2.6.27.9-159.fc10.i686, although I seem to get the
same thing with git from kernel.org.

Is this a known issue? Does anybody have any insight into what causes
this? I'm fully willing to test patches or debug this further myself.

Thanks,

Richard.

Re: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Alexey Starikovskiy]

Hi Richard,

This is probably related to this piece of code (drivers/acpi/battery.c),
similar code exists in drivers/acpi/sbs.c, but it is not relevant to your case:
	case POWER_SUPPLY_PROP_STATUS:
		if (battery->state & 0x01)
			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
		else if (battery->state & 0x02)
			val->intval = POWER_SUPPLY_STATUS_CHARGING;
		else if (battery->state == 0)
			val->intval = POWER_SUPPLY_STATUS_FULL;
		else
			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
		break;

Actually, state==0 means POWER_SUPPLY_STATUS_NOT_CHARGING, so if that is preferred it could be changed.

Regards,
Alex.


Richard Hughes wrote:
> When I insert the power lead or attach my T61 to the powered dock, the
> battery power supply status goes like this:
> 
> 0.00s Discharging
> {dock}
> 0.10s Fully charged
> 1.00s Charging
> 
> This causes userspace (in my case gnome-power-manager) to pop up a
> dialog telling me the battery is full, and the icon flickers to fully
> green, then 10% green and charging.
> 
> This seems to have existed as long as the power supply class was being
> used by the acpi battery, but we've always relied on a userspace fudge.
> 
> This is tested with 2.6.27.9-159.fc10.i686, although I seem to get the
> same thing with git from kernel.org.
> 
> Is this a known issue? Does anybody have any insight into what causes
> this? I'm fully willing to test patches or debug this further myself.
> 
> Thanks,
> 
> Richard.
> 
> 

Re: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Henrique de Moraes Holschuh]

On Fri, 23 Jan 2009, Alexey Starikovskiy wrote:
> This is probably related to this piece of code (drivers/acpi/battery.c),
> similar code exists in drivers/acpi/sbs.c, but it is not relevant to your case:
> 	case POWER_SUPPLY_PROP_STATUS:
> 		if (battery->state & 0x01)
> 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> 		else if (battery->state & 0x02)
> 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> 		else if (battery->state == 0)
> 			val->intval = POWER_SUPPLY_STATUS_FULL;
> 		else
> 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> 		break;
>
> Actually, state==0 means POWER_SUPPLY_STATUS_NOT_CHARGING, so if that is preferred it could be changed.

It should be fixed, yes.  Batteries being idle without being full are really
common in laptops with battery-life-saving functions (charge/stop-charge
threshold control).

-- 
  "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: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Richard Hughes]

On Fri, 2009-01-23 at 20:02 -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 23 Jan 2009, Alexey Starikovskiy wrote:
> > This is probably related to this piece of code (drivers/acpi/battery.c),
> > similar code exists in drivers/acpi/sbs.c, but it is not relevant to your case:
> > 	case POWER_SUPPLY_PROP_STATUS:
> > 		if (battery->state & 0x01)
> > 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > 		else if (battery->state & 0x02)
> > 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > 		else if (battery->state == 0)
> > 			val->intval = POWER_SUPPLY_STATUS_FULL;
> > 		else
> > 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> > 		break;
> >
> > Actually, state==0 means POWER_SUPPLY_STATUS_NOT_CHARGING, so if that is preferred it could be changed.
> 
> It should be fixed, yes.  Batteries being idle without being full are really
> common in laptops with battery-life-saving functions (charge/stop-charge
> threshold control).

I suspected this might be the case. Is there a way we can get true
battery state out of acpi for the battery? I always thought there could
be two booleans: charging and discharging.

I guess fully charged isn't just !charging and !discharging.

Richard.

Re: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Alexey Starikovskiy]

Richard Hughes wrote:
> On Fri, 2009-01-23 at 20:02 -0200, Henrique de Moraes Holschuh wrote:
>> On Fri, 23 Jan 2009, Alexey Starikovskiy wrote:
>>> This is probably related to this piece of code (drivers/acpi/battery.c),
>>> similar code exists in drivers/acpi/sbs.c, but it is not relevant to your case:
>>> 	case POWER_SUPPLY_PROP_STATUS:
>>> 		if (battery->state & 0x01)
>>> 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>>> 		else if (battery->state & 0x02)
>>> 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>>> 		else if (battery->state == 0)
>>> 			val->intval = POWER_SUPPLY_STATUS_FULL;
>>> 		else
>>> 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
>>> 		break;
>>>
>>> Actually, state==0 means POWER_SUPPLY_STATUS_NOT_CHARGING, so if that is preferred it could be changed.
>> It should be fixed, yes.  Batteries being idle without being full are really
>> common in laptops with battery-life-saving functions (charge/stop-charge
>> threshold control).
> 
> I suspected this might be the case. Is there a way we can get true
> battery state out of acpi for the battery? I always thought there could
> be two booleans: charging and discharging.
> 
> I guess fully charged isn't just !charging and !discharging.
> 
> Richard.
How about such definition: Fully charged == current_capacity >= 90 % of last_capacity && !charging and !discharging? 

Alex.

Re: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Henrique de Moraes Holschuh]

On Sat, 24 Jan 2009, Alexey Starikovskiy wrote:
> Richard Hughes wrote:
>> On Fri, 2009-01-23 at 20:02 -0200, Henrique de Moraes Holschuh wrote:
>>> On Fri, 23 Jan 2009, Alexey Starikovskiy wrote:
>>>> This is probably related to this piece of code (drivers/acpi/battery.c),
>>>> similar code exists in drivers/acpi/sbs.c, but it is not relevant to your case:
>>>> 	case POWER_SUPPLY_PROP_STATUS:
>>>> 		if (battery->state & 0x01)
>>>> 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>>>> 		else if (battery->state & 0x02)
>>>> 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>>>> 		else if (battery->state == 0)
>>>> 			val->intval = POWER_SUPPLY_STATUS_FULL;
>>>> 		else
>>>> 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
>>>> 		break;
>>>>
>>>> Actually, state==0 means POWER_SUPPLY_STATUS_NOT_CHARGING, so if that is preferred it could be changed.
>>> It should be fixed, yes.  Batteries being idle without being full are really
>>> common in laptops with battery-life-saving functions (charge/stop-charge
>>> threshold control).
>>
>> I suspected this might be the case. Is there a way we can get true
>> battery state out of acpi for the battery? I always thought there could
>> be two booleans: charging and discharging.
>>
>> I guess fully charged isn't just !charging and !discharging.
>>
>> Richard.
> How about such definition: Fully charged == current_capacity >= 90 % of 
> last_capacity && !charging and !discharging? 

Not good.  It is usual to tell the EC to stop charging at 95% or 98%.

I don't understand, why this guesswork over fully charged?  If you cannot
detect fully charged, then *don't*.

But if you must sinthesize it, and you can get an up-to-date "last full
capacity" from the battery when comparing, I suggest:

full = (current capacity == last full capacity) && !charging &&
       !discharging

That would *still* be wrong in a few corner cases, but at least they're rare
corner cases that happens only when the pack recalibrates its fuel gauge.

If there isn't a reliable way to detect the "full" state, just drop the
fully charged detection altogether.

-- 
  "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: ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Alexey Starikovskiy]

Henrique de Moraes Holschuh wrote:
> On Sat, 24 Jan 2009, Alexey Starikovskiy wrote:
>> How about such definition: Fully charged == current_capacity >= 90 % of 
>> last_capacity && !charging and !discharging? 
> 
> Not good.  It is usual to tell the EC to stop charging at 95% or 98%.
No. You don't understand the idea. EC does not stop charging at 95%, it will treat 
battery with 95% of charge as fully charged, it will not *start* charging until charge of the 
battery drops below this threshold. This is needed to save limited number of battery recharges until it dies.
So, in other words, if EC does not charge battery, it thinks battery is *full* within some threshold.
Now we talk about sub-second interval, then EC makes a decision to charge or not the battery, 
and what to report in this sub-second interval. I am trying to apply same logic as EC does --
if it is above some threshold (90, 95, 98 %%) of last_capacity, report it full. 
> 
> I don't understand, why this guesswork over fully charged?  If you cannot
> detect fully charged, then *don't*.
> 
> But if you must sinthesize it, and you can get an up-to-date "last full
> capacity" from the battery when comparing, I suggest:
> 
> full = (current capacity == last full capacity) && !charging &&
>        !discharging
certainly, 90% is wrong, but 100% makes a day...
> 
> That would *still* be wrong in a few corner cases, but at least they're rare
> corner cases that happens only when the pack recalibrates its fuel gauge.
full in this case is not exact term. As any other term beside current_now and voltage_now.
Capacity of the battery is estimated, so any number that was depending on it, is estimated too.
> If there isn't a reliable way to detect the "full" state, just drop the
> fully charged detection altogether.
People are used to see "full" state of the battery. I think we could tolerate not-full-enough 
for sub-second interval instead of dropping full state altogether.

Regards,
Alex.

[patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Richard Hughes]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On Sat, 2009-01-24 at 19:37 +0300, Alexey Starikovskiy wrote:
> > I don't understand, why this guesswork over fully charged?  If you cannot
> > detect fully charged, then *don't*.
> > 
> > But if you must sinthesize it, and you can get an up-to-date "last full
> > capacity" from the battery when comparing, I suggest:
> > 
> > full = (current capacity == last full capacity) && !charging &&
> >        !discharging
> certainly, 90% is wrong, but 100% makes a day...

I think we need some sort of logic like this.
 
> > That would *still* be wrong in a few corner cases, but at least they're rare
> > corner cases that happens only when the pack recalibrates its fuel gauge.
> full in this case is not exact term. As any other term beside current_now and voltage_now.
> Capacity of the battery is estimated, so any number that was depending on it, is estimated too.

Right, never underestimate the brokenness of some people's batteries out
there. Coupled with broken BIOS's, some of the fix-up code in HAL is
'interesting'. I think all the fix-up code in HAL belongs in in the
kernel.

> > If there isn't a reliable way to detect the "full" state, just drop the
> > fully charged detection altogether.
> People are used to see "full" state of the battery. I think we could tolerate not-full-enough 
> for sub-second interval instead of dropping full state altogether.

We shouldn't drop the full state, we should just add some metric like
above. What about something like the attached (untested) patch? Would
something this be accepted?

Richard.


[-- Attachment #2: acpi-battery-charged.patch --]
[-- Type: text/x-patch, Size: 1776 bytes --]

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 65132f9..d400a11 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -138,6 +138,38 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 
 static int acpi_battery_get_state(struct acpi_battery *battery);
 
+static int acpi_battery_is_charged(struct acpi_battery *battery)
+{
+	int percentage;
+
+	/* either charging or discharging */
+	if (battery->state != 0)
+		return 0;
+
+	/* battery not reporting charge */
+	if (battery->full_charge_capacity == ACPI_BATTERY_VALUE_UNKNOWN ||
+	    battery->capacity_now == 0)
+		return 0;
+
+	/* good batteries update full_charge as the batteries degrade */
+	if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN &&
+	    battery->full_charge_capacity != 0) {
+		percentage = (100 * battery->capacity_now) / battery->full_charge_capacity;
+		if (percentage > 90)
+			return 1;
+	}
+
+	/* fallback to using design values for broken batteries */
+	if (battery->design_capacity != ACPI_BATTERY_VALUE_UNKNOWN &&
+	    battery->design_capacity != 0) {
+		percentage = (100 * battery->capacity_now) / battery->design_capacity;
+		if (percentage > 90)
+			return 1;
+	}
+
+	return 0;
+}
+
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
@@ -155,7 +187,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (battery->state & 0x02)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else if (battery->state == 0)
+		else if (acpi_battery_is_charged(battery))
 			val->intval = POWER_SUPPLY_STATUS_FULL;
 		else
 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Alexey Starikovskiy]

Looks good to me.

Richard Hughes wrote:
> On Sat, 2009-01-24 at 19:37 +0300, Alexey Starikovskiy wrote:
>>> I don't understand, why this guesswork over fully charged?  If you cannot
>>> detect fully charged, then *don't*.
>>>
>>> But if you must sinthesize it, and you can get an up-to-date "last full
>>> capacity" from the battery when comparing, I suggest:
>>>
>>> full = (current capacity == last full capacity) && !charging &&
>>>        !discharging
>> certainly, 90% is wrong, but 100% makes a day...
> 
> I think we need some sort of logic like this.
>  
>>> That would *still* be wrong in a few corner cases, but at least they're rare
>>> corner cases that happens only when the pack recalibrates its fuel gauge.
>> full in this case is not exact term. As any other term beside current_now and voltage_now.
>> Capacity of the battery is estimated, so any number that was depending on it, is estimated too.
> 
> Right, never underestimate the brokenness of some people's batteries out
> there. Coupled with broken BIOS's, some of the fix-up code in HAL is
> 'interesting'. I think all the fix-up code in HAL belongs in in the
> kernel.
> 
>>> If there isn't a reliable way to detect the "full" state, just drop the
>>> fully charged detection altogether.
>> People are used to see "full" state of the battery. I think we could tolerate not-full-enough 
>> for sub-second interval instead of dropping full state altogether.
> 
> We shouldn't drop the full state, we should just add some metric like
> above. What about something like the attached (untested) patch? Would
> something this be accepted?
> 
> Richard.
> 
> 

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Henrique de Moraes Holschuh]

On Sun, 25 Jan 2009, Richard Hughes wrote:
> +	/* good batteries update full_charge as the batteries degrade */
> +	if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN &&
> +	    battery->full_charge_capacity != 0) {
> +		percentage = (100 * battery->capacity_now) / battery->full_charge_capacity;
> +		if (percentage > 90)
> +			return 1;

You guys have to remember that the hardware can know when the cells are
physically full.  That has _nothing_ to do with any level estimation,
battery cells behave differently when they are full, and that's what the
overcharge protection circuitry detects.

So, the above test will still break on any proper battery subsystem with the
high watermark set below 100%, as those systems update full_charge_capacity
*only* when the cells really are full (and not because the EC decided to
stop the charging before they were full).

Thus, it will break if you set the threshold to stop charging (high
watermark) above 90% but below 100% on a T-, X-, R- or W-series ThinkPad,
for example.

The test would still have to be:
	if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN &&
	    battery->full_charge_capacity != 0 &&
	    battery->capacity_now == battery->full_charge_capacity)
		return 1;

or, if there is crap so bad out there that requires it:
	if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN &&
	    battery->full_charge_capacity != 0 &&
	    battery->capacity_now == battery->full_charge_capacity) {
		percentage = (100 * battery->capacity_now) / battery->full_charge_capacity;
		if (percentage > 90)
			return 1;

to work well on proper battery firmware set to stop before the battery is
full.

If that can't work well enough with the crap out there, I am out of ideas,
and I'd say we'd have to quirk either the good or the bad systems to
properly implement the FULL state.

As for the sub-second "blinks" the EC does in battery states, if one wants
to avoid that, better report states only after they have been stable for one
or two seconds.  Not that a report of CHARGING->IDLE->FULL is incorrect or a
problem IMHO, but I don't know if that's what you guys are observing.

-- 
  "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] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Richard Hughes]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Sun, 2009-01-25 at 11:42 -0200, Henrique de Moraes Holschuh wrote:
>  So, the above test will still break on any proper battery subsystem with the
> high watermark set below 100%, as those systems update full_charge_capacity
> *only* when the cells really are full (and not because the EC decided to
> stop the charging before they were full).

This is what I observed when testing my patch.

The attached patch checks the last full and design charge, as this seems
to work in all cases I have here. In the case of broken batteries or
broken hardware, we just return UNKNOWN in this "settling" state, which
is much better for userspace than falling back to full.

With the attached patch userspace gets the right states. In the future,
maybe we can do some sort of metric over time (watching to see if the
present charge changes) but for the most part the patch fixes things up
for userspace.

Please review,

Richard.


[-- Attachment #2: 0001-battery-don-t-assume-we-are-fully-charged-when-not.patch --]
[-- Type: text/x-patch, Size: 2258 bytes --]

>From 3b0fb1239e5bc064766ffa3d7a45265e722fb9eb Mon Sep 17 00:00:00 2001
From: Richard Hughes <hughsient@gmail.com>
Date: Sun, 25 Jan 2009 15:05:50 +0000
Subject: [PATCH] battery: don't assume we are fully charged when not charging or discharging

On hardware like the T61 it can take a couple of seconds for the battery
to start charging after the power is connected, and we incorrectly tell
userspace that we are fully charged, and then go back to charging.

Only mark a battery as fully charged when the preset charge matches either
the last full charge, or the design charge.

Signed-off-by: Richard Hughes <hughsient@gmail.com>
---
 drivers/acpi/battery.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 65132f9..69cbc57 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -138,6 +138,29 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 
 static int acpi_battery_get_state(struct acpi_battery *battery);
 
+static int acpi_battery_is_charged(struct acpi_battery *battery)
+{
+	/* either charging or discharging */
+	if (battery->state != 0)
+		return 0;
+
+	/* battery not reporting charge */
+	if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN ||
+	    battery->capacity_now == 0)
+		return 0;
+
+	/* good batteries update full_charge as the batteries degrade */
+	if (battery->full_charge_capacity == battery->capacity_now)
+		return 1;
+
+	/* fallback to using design values for broken batteries */
+	if (battery->design_capacity == battery->capacity_now)
+		return 1;
+
+	/* we don't do any sort of metric based on percentages */
+	return 0;
+}
+
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
@@ -155,7 +178,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (battery->state & 0x02)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else if (battery->state == 0)
+		else if (acpi_battery_is_charged(battery))
 			val->intval = POWER_SUPPLY_STATUS_FULL;
 		else
 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
-- 
1.6.0.6

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Alexey Starikovskiy]

Richard Hughes wrote:
> On Sun, 2009-01-25 at 11:42 -0200, Henrique de Moraes Holschuh wrote:
>>  So, the above test will still break on any proper battery subsystem with the
>> high watermark set below 100%, as those systems update full_charge_capacity
>> *only* when the cells really are full (and not because the EC decided to
>> stop the charging before they were full).
> 
> This is what I observed when testing my patch.
> 
> The attached patch checks the last full and design charge, as this seems
> to work in all cases I have here. In the case of broken batteries or
> broken hardware, we just return UNKNOWN in this "settling" state, which
> is much better for userspace than falling back to full.
> 
> With the attached patch userspace gets the right states. In the future,
> maybe we can do some sort of metric over time (watching to see if the
> present charge changes) but for the most part the patch fixes things up
> for userspace.
> 		else
> 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
This last else should return STATUS_NOT_CHARGING, IMHO. We know this little bit about it.
> 
> Please review,
> 
> Richard.
> 
> 

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in

[Richard Hughes]

On Sun, 2009-01-25 at 22:50 +0300, Alexey Starikovskiy wrote:
>  >                       val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> This last else should return STATUS_NOT_CHARGING, IMHO. We know this
> little bit about it.

I don't think so; I mean it just as well means STATUS_NOT_DISCHARGING.
We don't actually know what the battery is doing internally, hence the
UNKNOWN.

Richard.

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in (resend)

[Richard Hughes]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Sun, 2009-01-25 at 11:42 -0200, Henrique de Moraes Holschuh wrote:
>  So, the above test will still break on any proper battery subsystem with the
> high watermark set below 100%, as those systems update full_charge_capacity
> *only* when the cells really are full (and not because the EC decided to
> stop the charging before they were full).

This is what I observed when testing my patch.

The attached patch checks the last full and design charge, as this seems
to work in all cases I have here. In the case of broken batteries or
broken hardware, we just return UNKNOWN in this "settling" state, which
is much better for userspace than falling back to full.

With the attached patch userspace gets the right states. In the future,
maybe we can do some sort of metric over time (watching to see if the
present charge changes) but for the most part the patch fixes things up
for userspace.

Please review,

Richard.


[-- Attachment #2: 0001-battery-don-t-assume-we-are-fully-charged-when-not.patch --]
[-- Type: text/x-patch, Size: 2258 bytes --]

>From 3b0fb1239e5bc064766ffa3d7a45265e722fb9eb Mon Sep 17 00:00:00 2001
From: Richard Hughes <hughsient@gmail.com>
Date: Sun, 25 Jan 2009 15:05:50 +0000
Subject: [PATCH] battery: don't assume we are fully charged when not charging or discharging

On hardware like the T61 it can take a couple of seconds for the battery
to start charging after the power is connected, and we incorrectly tell
userspace that we are fully charged, and then go back to charging.

Only mark a battery as fully charged when the preset charge matches either
the last full charge, or the design charge.

Signed-off-by: Richard Hughes <hughsient@gmail.com>
---
 drivers/acpi/battery.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 65132f9..69cbc57 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -138,6 +138,29 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 
 static int acpi_battery_get_state(struct acpi_battery *battery);
 
+static int acpi_battery_is_charged(struct acpi_battery *battery)
+{
+	/* either charging or discharging */
+	if (battery->state != 0)
+		return 0;
+
+	/* battery not reporting charge */
+	if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN ||
+	    battery->capacity_now == 0)
+		return 0;
+
+	/* good batteries update full_charge as the batteries degrade */
+	if (battery->full_charge_capacity == battery->capacity_now)
+		return 1;
+
+	/* fallback to using design values for broken batteries */
+	if (battery->design_capacity == battery->capacity_now)
+		return 1;
+
+	/* we don't do any sort of metric based on percentages */
+	return 0;
+}
+
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
@@ -155,7 +178,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (battery->state & 0x02)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else if (battery->state == 0)
+		else if (acpi_battery_is_charged(battery))
 			val->intval = POWER_SUPPLY_STATUS_FULL;
 		else
 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
-- 
1.6.0.6

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in (resend)

[Henrique de Moraes Holschuh]

On Wed, 28 Jan 2009, Richard Hughes wrote:
> The attached patch checks the last full and design charge, as this seems
> to work in all cases I have here. In the case of broken batteries or
> broken hardware, we just return UNKNOWN in this "settling" state, which
> is much better for userspace than falling back to full.
> 
> With the attached patch userspace gets the right states. In the future,
> maybe we can do some sort of metric over time (watching to see if the
> present charge changes) but for the most part the patch fixes things up
> for userspace.

I am ok with this patch, but it does have *one* corner case.

Brand new good batteries will be reported as full while still charging, when
they're near 100% (they will be above the design capacity).

That isn't enough for me to complain about the patch, and the patch makes it
MUCH better than the current broken behaviour.

So you can have my Acked-by if you want it, for whatever little it is worth.

-- 
  "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] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in (resend)

[Len Brown]

On Fri, 30 Jan 2009, Henrique de Moraes Holschuh wrote:

> On Wed, 28 Jan 2009, Richard Hughes wrote:
> > The attached patch checks the last full and design charge, as this seems
> > to work in all cases I have here. In the case of broken batteries or
> > broken hardware, we just return UNKNOWN in this "settling" state, which
> > is much better for userspace than falling back to full.
> > 
> > With the attached patch userspace gets the right states. In the future,
> > maybe we can do some sort of metric over time (watching to see if the
> > present charge changes) but for the most part the patch fixes things up
> > for userspace.
> 
> I am ok with this patch, but it does have *one* corner case.
> 
> Brand new good batteries will be reported as full while still charging, when
> they're near 100% (they will be above the design capacity).
> 
> That isn't enough for me to complain about the patch, and the patch makes it
> MUCH better than the current broken behaviour.
> 
> So you can have my Acked-by if you want it, for whatever little it is worth.

Applied to acpi-test.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [patch] ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in (resend)

[Richard Hughes]

On Sat, 2009-02-07 at 22:59 -0500, Len Brown wrote:
> Applied to acpi-test.

Thanks dude.

Richard.