| Message ID | 1232879297.3518.26.camel@hughsie-work.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;