| Message ID | 20200315151206.30909-1-spinal.by@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [01/15] power: supply: cpcap-battery: Fix battery full status reporting | expand |
Hi! > Don't report that the battery is fully charged if the charging current > exceeds 100 mA. Could you merge patches together for possibility of reasonable review? > @@ -408,7 +408,8 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata) > struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata); > > if (state->voltage >= > - (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) > + (ddata->config.bat.constant_charge_voltage_max_uv - 18000) && > + state->current_ua > -100000) It seems that this 100mA threshold is changed about 3 times in the series :-(. Plus, it might be better to place booleans into struct, rather than using "static bool" inside a function. Could we get some kind of explanations for the whole series? 100mA is rather high current for end of charge. If you stop updating full capacity when "your" end of charge is met, you'll have battery charged to more than 100%. I... don't think that makes sense. Best regards, Pavel
* Pavel Machek <pavel@ucw.cz> [200315 18:59]: > Hi! > > > Don't report that the battery is fully charged if the charging current > > exceeds 100 mA. > > Could you merge patches together for possibility of reasonable review? > > > @@ -408,7 +408,8 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata) > > struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata); > > > > if (state->voltage >= > > - (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) > > + (ddata->config.bat.constant_charge_voltage_max_uv - 18000) && > > + state->current_ua > -100000) > > It seems that this 100mA threshold is changed about 3 times in the > series :-(. > > Plus, it might be better to place booleans into struct, rather than > using "static bool" inside a function. > > Could we get some kind of explanations for the whole series? 100mA is > rather high current for end of charge. If you stop updating > full capacity when "your" end of charge is met, you'll have battery > charged to more than 100%. I... don't think that makes sense. Yeh please compress out the unncessary parts of my RFC patches :) For reference only, below is a diff of all the changes for cpcap-battery.c against v5.6-rc5. I applied this series on top of a droid4-pending-v5.6 based branch that has the earlier RFC changes applied. Regards, Tony 8< ----------------- diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c --- a/drivers/power/supply/cpcap-battery.c +++ b/drivers/power/supply/cpcap-battery.c @@ -110,6 +110,8 @@ struct cpcap_coulomb_counter_data { enum cpcap_battery_state { CPCAP_BATTERY_STATE_PREVIOUS, CPCAP_BATTERY_STATE_LATEST, + CPCAP_BATTERY_STATE_EMPTY, + CPCAP_BATTERY_STATE_FULL, CPCAP_BATTERY_STATE_NR, }; @@ -132,10 +134,32 @@ struct cpcap_battery_ddata { struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR]; u32 cc_lsb; /* μAms per LSB */ atomic_t active; + int charge_full; int status; u16 vendor; }; +struct cpcap_battery_capacity { + int capacity; + int voltage; +}; + +#define CPCAP_CAP(l, v) \ +{ \ + .capacity = (l), \ + .voltage = (v), \ +}, + +/* Pessimistic battery capacity mapping before high or low value is seen */ +static const struct cpcap_battery_capacity cpcap_battery_cap[] = { + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN, 0) + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL, 3100000) + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_LOW, 3300000) + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_NORMAL, 3700000) + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_HIGH, 4000000) + CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000) +}; + #define CPCAP_NO_BATTERY -400 static struct cpcap_battery_state_data * @@ -160,6 +184,18 @@ cpcap_battery_previous(struct cpcap_battery_ddata *ddata) return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_PREVIOUS); } +static struct cpcap_battery_state_data * +cpcap_battery_get_empty(struct cpcap_battery_ddata *ddata) +{ + return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_EMPTY); +} + +static struct cpcap_battery_state_data * +cpcap_battery_get_full(struct cpcap_battery_ddata *ddata) +{ + return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_FULL); +} + static int cpcap_charger_battery_temperature(struct cpcap_battery_ddata *ddata, int *value) { @@ -369,17 +405,39 @@ static int cpcap_battery_cc_get_avg_current(struct cpcap_battery_ddata *ddata) static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata) { struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata); + static bool is_full; + + if (state->voltage < + (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) { + is_full = false; + } else if (is_full) { + if (state->current_ua < -170000) + is_full = false; + } else if (state->current_ua >= -170000 && + cpcap_battery_cc_get_avg_current(ddata) >= -112000) { + is_full = true; + } + + return is_full; +} - if (state->voltage >= - (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) - return true; +static bool cpcap_battery_low(struct cpcap_battery_ddata *ddata) +{ + struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata); + static bool is_low; - return false; + if (state->current_ua > 0 && (state->voltage <= 3350000 || is_low)) + is_low = true; + else + is_low = false; + + return is_low; } static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata) { - struct cpcap_battery_state_data state, *latest, *previous; + struct cpcap_battery_state_data state, *latest, *previous, + *empty, *full; ktime_t now; int error; @@ -408,9 +466,56 @@ static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata) memcpy(previous, latest, sizeof(*previous)); memcpy(latest, &state, sizeof(*latest)); + if (cpcap_battery_full(ddata)) { + full = cpcap_battery_get_full(ddata); + memcpy(full, latest, sizeof(*full)); + + empty = cpcap_battery_get_empty(ddata); + if (empty->voltage && empty->voltage != -1) { + empty->voltage = -1; + ddata->charge_full = + empty->counter_uah - full->counter_uah; + } else if (ddata->charge_full) { + empty->voltage = -1; + empty->counter_uah = + full->counter_uah + ddata->charge_full; + } + } else if (cpcap_battery_low(ddata)) { + empty = cpcap_battery_get_empty(ddata); + memcpy(empty, latest, sizeof(*empty)); + + full = cpcap_battery_get_full(ddata); + if (full->voltage) { + full->voltage = 0; + ddata->charge_full = + empty->counter_uah - full->counter_uah; + } + } + return 0; } +static int cpcap_battery_get_capacity_level(struct cpcap_battery_ddata *ddata) +{ + struct cpcap_battery_state_data *latest; + const struct cpcap_battery_capacity *cap = NULL; + int voltage, i; + + latest = cpcap_battery_latest(ddata); + voltage = latest->voltage; + + for (i = ARRAY_SIZE(cpcap_battery_cap) - 1; i >=0; i--) { + cap = &cpcap_battery_cap[i]; + if (voltage >= cap->voltage) + break; + } + + if (!cap) + return 0; + + return cap->capacity; +} + static enum power_supply_property cpcap_battery_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_PRESENT, @@ -421,10 +526,13 @@ static enum power_supply_property cpcap_battery_props[] = { POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, POWER_SUPPLY_PROP_CURRENT_AVG, POWER_SUPPLY_PROP_CURRENT_NOW, + POWER_SUPPLY_PROP_CHARGE_FULL, + POWER_SUPPLY_PROP_CHARGE_NOW, POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, POWER_SUPPLY_PROP_CHARGE_COUNTER, POWER_SUPPLY_PROP_POWER_NOW, POWER_SUPPLY_PROP_POWER_AVG, + POWER_SUPPLY_PROP_CAPACITY, POWER_SUPPLY_PROP_CAPACITY_LEVEL, POWER_SUPPLY_PROP_SCOPE, POWER_SUPPLY_PROP_TEMP, @@ -435,7 +543,7 @@ static int cpcap_battery_get_property(struct power_supply *psy, union power_supply_propval *val) { struct cpcap_battery_ddata *ddata = power_supply_get_drvdata(psy); - struct cpcap_battery_state_data *latest, *previous; + struct cpcap_battery_state_data *latest, *previous, *empty; u32 sample; s32 accumulator; int cached; @@ -515,19 +623,33 @@ static int cpcap_battery_get_property(struct power_supply *psy, tmp *= ((latest->voltage + previous->voltage) / 20000); val->intval = div64_s64(tmp, 100); break; + case POWER_SUPPLY_PROP_CAPACITY: + empty = cpcap_battery_get_empty(ddata); + if (!empty->voltage || !ddata->charge_full) + return -ENODATA; + /* (ddata->charge_full / 200) is needed for rounding */ + val->intval = empty->counter_uah - latest->counter_uah + + ddata->charge_full / 200; + val->intval = clamp(val->intval, 0, ddata->charge_full); + val->intval = val->intval * 100 / ddata->charge_full; + break; case POWER_SUPPLY_PROP_CAPACITY_LEVEL: - if (cpcap_battery_full(ddata)) - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_FULL; - else if (latest->voltage >= 3750000) - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_HIGH; - else if (latest->voltage >= 3300000) - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL; - else if (latest->voltage > 3100000) - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_LOW; - else if (latest->voltage <= 3100000) - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; - else - val->intval = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN; + val->intval = cpcap_battery_get_capacity_level(ddata); + break; + case POWER_SUPPLY_PROP_CHARGE_NOW: + empty = cpcap_battery_get_empty(ddata); + if (!empty->voltage) + return -ENODATA; + val->intval = empty->counter_uah - latest->counter_uah; + if (val->intval < 0) + val->intval = 0; + else if (ddata->charge_full && ddata->charge_full < val->intval) + val->intval = ddata->charge_full; + break; + case POWER_SUPPLY_PROP_CHARGE_FULL: + if (!ddata->charge_full) + return -ENODATA; + val->intval = ddata->charge_full; break; case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: val->intval = ddata->config.info.charge_full_design; @@ -590,6 +712,15 @@ static int cpcap_battery_set_property(struct power_supply *psy, ddata->config.bat.constant_charge_voltage_max_uv = val->intval; return cpcap_battery_update_charger(ddata, val->intval); + case POWER_SUPPLY_PROP_CHARGE_FULL: + if (val->intval < 0) + return -EINVAL; + if (val->intval > ddata->config.info.charge_full_design) + return -EINVAL; + + ddata->charge_full = val->intval; + + return 0; default: return -EINVAL; } @@ -602,6 +733,7 @@ static int cpcap_battery_property_is_writeable(struct power_supply *psy, { switch (psp) { case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: + case POWER_SUPPLY_PROP_CHARGE_FULL: return 1; default: return 0;
> Could we get some kind of explanations for the whole series? 100mA is > rather high current for end of charge. If you stop updating > full capacity when "your" end of charge is met, you'll have battery > charged to more than 100%. I... don't think that makes sense. The aim was to allow userspace to see accurate values for charge_now, charge_full and capacity (percentage) of the battery. It will allow the user to estimate how long the device can work on the battery with current power consumption. For this the user will need to do a battery calibration. I.e. he will need to fully charge and then discharge the battery. Or vice versa: discharge -> charge. Once the user completes the calibration cycle, he will be able to see pretty accurate values for charge_now, charge_full and capacity. This is similar to how bq27200 IC from Nokia N900 works. Also this patchset allows the userspace to restore the calibration value after reboot. By the calibration value I mean charge_full. We can't rely on restoring charge_now value, because the user may have multiple operating systems installed and if he works in another OS for a while the charge_now value will become invalid. So, after a reboot the user may want to restore the charge_full value, so the kernel will be able to estimate the percentage and capacity values without forcing the user to do a full calib- ration cycle again. The only thing the user will have to do is to fully charge OR fully discharge the battery at least once. And he will get all values set. Now about the chosen limits. For some reason the charging is interrupted (and restarted after a while) when the following conditions are met: 1) the charging current is < 112 mA 2) the display backlight is off This behaviour was observed in Maemo Leste with hildon-desktop running. I tested these patches for several days, so I picked up the parameters for optimal (from my point of view) work in practice taking into account the current "features" of Droid 4 drivers. If we could somehow fix this behaviour (charging interruption), I'd reconsider the end of charge current value to be 50 mA. Making it lower than 50 mA doesn't seem to make much sence because of the extended charging time visibility without giving significant improvement in charge_full accuracy. > If you stop updating full capacity when "your" end of charge is met, > you'll have battery charged to more than 100%. No worries. With the implemented algorithm, the user will not notice that the battery is more than 100% charged (which is just a convention here). And this situation gives an advantage in that it has a slightly pessimistic charge_full value, which in practice turns out to be good: the user will be warned about low battery a little ahead of time. -- Best regards, Spinal
* Arthur D. <spinal.by@gmail.com> [200315 20:51]: > Now about the chosen limits. For some reason the charging is > interrupted (and restarted after a while) when the following > conditions are met: > 1) the charging current is < 112 mA > 2) the display backlight is off > > This behaviour was observed in Maemo Leste with hildon-desktop > running. I tested these patches for several days, so I picked up > the parameters for optimal (from my point of view) work in practice > taking into account the current "features" of Droid 4 drivers. > > If we could somehow fix this behaviour (charging interruption), > I'd reconsider the end of charge current value to be 50 mA. Hmm well we do have two chargers, the usb charger and the unknown inductive charger for the pins on the back. It would be best to keep cpcap-battery.c independent of the chargers to avoid depndencies as the chargers do usually start charging briefly always when connected. Maybe just adding something like below would be enough of a check: static int cpcap_battery_get_counter_rate(struct cpcap_battery_ddata *ddata, int poll_time_ms); And then based on the value being negative or positive you would know if it's charging or not. I guess we could then use this also for POWER_SUPPLY_PROP_CHARGE_NOW with poll_time_ms value of 0. I think the charge counter is configure to poll at 250 ms right now. Regards, Tony
Hi, Tony. It seems like a misunderstanding here. There's no problem in detecting if the charging is in progress. The green led is switched off and the battery current sign is changed from "-" to "+" (which means that the battery is being discharged). So there's no need in additional checks. For cpcap-battery this situation seems like a battery stopped charging. And it doesn't matter if that was a user who disconnected the charger or it was done somewhere in a driver/firmware/hardware. The problem is that the charging current cant get to the point <100 mA, not talking about <50 mA. And that's why I set the value of 112 mA for the end of charge current: to help the kernel to detect this plateau and to stop the calibration cycle, so the userspace can get all the battery parameters I mentioned in the previous mail. Please note, that the behaviour I mentioned was observed only when the conditions written in my last mail were met. The important one was: > 2) the display backlight is off Because when I unlocked the display the charging current was able to go below 112 mA. Of course I couldn't rely on something like this: the user should stay with backlight on to have the battery calibrated. Think about it: waiting for the charging current to drop from 100 mA to 50 mA can take dozens of minutes (it depends on the age of battery - the older the battery the longer it will take), and the user should force somehow the device to not switch off the display hightlight until the battery is calibrated. Of course it's unacceptable, so I decided to set the end of charge current limit to 112 mA. Which allows the user to just put the device on a table and to wait until it's fully charged without a need to interfere the charging process with some action from the user. -- Best regards, Spinal >> Now about the chosen limits. For some reason the charging is >> interrupted (and restarted after a while) when the following >> conditions are met: >> 1) the charging current is < 112 mA >> 2) the display backlight is off >> >> This behaviour was observed in Maemo Leste with hildon-desktop >> running. I tested these patches for several days, so I picked up >> the parameters for optimal (from my point of view) work in practice >> taking into account the current "features" of Droid 4 drivers. >> >> If we could somehow fix this behaviour (charging interruption), >> I'd reconsider the end of charge current value to be 50 mA. > > Hmm well we do have two chargers, the usb charger and the > unknown inductive charger for the pins on the back. > > It would be best to keep cpcap-battery.c independent of the > chargers to avoid depndencies as the chargers do usually start > charging briefly always when connected. > > Maybe just adding something like below would be enough of a check: > > static int > cpcap_battery_get_counter_rate(struct cpcap_battery_ddata *ddata, > int poll_time_ms); > > And then based on the value being negative or positive you > would know if it's charging or not. I guess we could then > use this also for POWER_SUPPLY_PROP_CHARGE_NOW with poll_time_ms > value of 0. I think the charge counter is configure to poll > at 250 ms right now.
* Arthur D. <spinal.by@gmail.com> [200316 01:31]: > Hi, Tony. > > It seems like a misunderstanding here. There's no problem in detecting > if the charging is in progress. The green led is switched off and > the battery current sign is changed from "-" to "+" (which means > that the battery is being discharged). So there's no need in additional > checks. For cpcap-battery this situation seems like a battery stopped > charging. And it doesn't matter if that was a user who disconnected > the charger or it was done somewhere in a driver/firmware/hardware. > > The problem is that the charging current cant get to the point <100 mA, > not talking about <50 mA. And that's why I set the value of 112 mA for > the end of charge current: to help the kernel to detect this plateau and > to stop the calibration cycle, so the userspace can get all the battery > parameters I mentioned in the previous mail. OK I guess that's easy to change if we figure out something better :) Maybe add some define for it like CPCAP_BATTERY_FULL_CHARGE_CURRENT or similar? > Please note, that the behaviour I mentioned was observed only when the > conditions written in my last mail were met. The important one was: > > 2) the display backlight is off > > Because when I unlocked the display the charging current was able > to go below 112 mA. Of course I couldn't rely on something like this: > the user should stay with backlight on to have the battery calibrated. > Think about it: waiting for the charging current to drop from 100 mA > to 50 mA can take dozens of minutes (it depends on the age of battery - > the older the battery the longer it will take), and the user should > force somehow the device to not switch off the display hightlight > until the battery is calibrated. > > Of course it's unacceptable, so I decided to set the end of charge > current limit to 112 mA. Which allows the user to just put the device > on a table and to wait until it's fully charged without a need > to interfere the charging process with some action from the user. Yeah OK thanks. Tony
diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c index d7b3234ec264..34a9dbcd1a23 100644 --- a/drivers/power/supply/cpcap-battery.c +++ b/drivers/power/supply/cpcap-battery.c @@ -408,7 +408,8 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata) struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata); if (state->voltage >= - (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) + (ddata->config.bat.constant_charge_voltage_max_uv - 18000) && + state->current_ua > -100000) return true; return false;
Don't report that the battery is fully charged if the charging current exceeds 100 mA. Signed-off-by: Arthur Demchenkov <spinal.by@gmail.com> --- drivers/power/supply/cpcap-battery.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)