diff mbox series

[06/15] power: supply: cpcap-battery: Initialize with user provided data

Message ID 20200315151206.30909-6-spinal.by@gmail.com (mailing list archive)
State New, archived
Headers show
Series [01/15] power: supply: cpcap-battery: Fix battery full status reporting | expand

Commit Message

Arthur D. March 15, 2020, 3:11 p.m. UTC
If the user provides us with charge_full value (which it could save in a
permanent storage between reboots), initialize low and high counter_uah
with calculated values.

Signed-off-by: Arthur Demchenkov <spinal.by@gmail.com>
---
 drivers/power/supply/cpcap-battery.c | 39 +++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

Comments

Tony Lindgren March 21, 2020, 2:54 p.m. UTC | #1
* Arthur Demchenkov <spinal.by@gmail.com> [200315 15:15]:
> If the user provides us with charge_full value (which it could save in a
> permanent storage between reboots), initialize low and high counter_uah
> with calculated values.

Hmm I'm getting -EINVAL when I do echo 1600000 > charge_now but yet the
value does get updated?

And I'm still not seeing capacity show up after that though even with full
battery.. I think we should be able to calculate it if either a high or
low value has been seen?

Also, we should have the driver default to using the charge_full_design
value if nothing is written from userspace and we see a high or low value.

Maybe some pessimistic estimate could be used instead of just using
charge_full_design if no value is initialized from the userspace?
Something like (charge_full_design / 4) * 3 maybe?

Yes it will be off, but the driver should work also without user
interaction :)

Regards,

Tony
Arthur D. March 21, 2020, 10:08 p.m. UTC | #2
Hi.

> Hmm I'm getting -EINVAL when I do echo 1600000 > charge_now but yet the
> value does get updated?

Hm. I'm getting "Permission denied" when trying to do this.
Anyway, charge_now value is not supposed to be initialized by the user.
Only charge_full should be writable.

> And I'm still not seeing capacity show up after that though even with  
> full battery.. I think we should be able to calculate it if either a  
> high or
> low value has been seen?

There are two steps needed to calibrate the battery: to hit the "fully  
charged"
state and to hit "battery empty" state. When the both states were hit the  
driver
will initialize charge_full value. And it will start reporting correct  
charge_now
and capacity (battery charge percentage) values.

Once the charge_full value is calculated it's recommended to be saved by  
the user
to a permanent storage between reboots.

Saving/restoring the value can be done in init scripts.

For saving the calibration value just use the command like:
cat /sys/class/power_supply/battery/charge_full > /battery_cf

To restore (after device reboot or power on):
cat /battery_cf > /sys/class/power_supply/battery/charge_full

This will allow the kernel to do fast calibration. I.e. you will
only need to fully charge _OR_ fully discharge the battery to start
seeing correct charge_now and capacity values.

> Also, we should have the driver default to using the charge_full_design
> value if nothing is written from userspace and we see a high or low  
> value.

I'd prefer to have charge_full value undefined until the battery is  
calibrated.
This way the userspace can estimate current battery capacity using voltage  
as a
fallback for uncalibrated battery. The voltage estimation will be  way more
accurate than having charge_full = charge_full_design on pretty old Droid 4
batteries. For example, my battery after calibration has about 1000 mAh,
while charge_full_design is 1740000.

> Maybe some pessimistic estimate could be used instead of just using
> charge_full_design if no value is initialized from the userspace?
> Something like (charge_full_design / 4) * 3 maybe?

It's better to rely on voltage estimated percentage.
In Maemo Leste we patched upower to use the following formula, which gives  
pretty
good results:

percentage = (voltage - voltage_empty) / (voltage_full - voltage_empty) *  
100

Actually, the formula we use is a bit more complicated.

If you're curious, here's how it's actually done:
https://github.com/maemo-leste/upower/blob/master/src/linux/up-device-supply.c#L756

--
Best regards, Spinal
Tony Lindgren March 21, 2020, 10:21 p.m. UTC | #3
* Arthur D. <spinal.by@gmail.com> [200321 22:09]:
> Hi.
> 
> > Hmm I'm getting -EINVAL when I do echo 1600000 > charge_now but yet the
> > value does get updated?
> 
> Hm. I'm getting "Permission denied" when trying to do this.
> Anyway, charge_now value is not supposed to be initialized by the user.
> Only charge_full should be writable.

Sorry I meant writing to charge_full :) Anyways, looks like echo -n works
with no errors:

# echo 1305000 > charge_full
bash: echo: write error: Invalid argument
# echo -n 1305000 > charge_full

> > And I'm still not seeing capacity show up after that though even with
> > full battery.. I think we should be able to calculate it if either a
> > high or
> > low value has been seen?
> 
> There are two steps needed to calibrate the battery: to hit the "fully
> charged"
> state and to hit "battery empty" state. When the both states were hit the
> driver
> will initialize charge_full value. And it will start reporting correct
> charge_now
> and capacity (battery charge percentage) values.

Hmm OK. So far no luck triggering that though.

> Once the charge_full value is calculated it's recommended to be saved by the
> user
> to a permanent storage between reboots.
> 
> Saving/restoring the value can be done in init scripts.
> 
> For saving the calibration value just use the command like:
> cat /sys/class/power_supply/battery/charge_full > /battery_cf
> 
> To restore (after device reboot or power on):
> cat /battery_cf > /sys/class/power_supply/battery/charge_full

OK

> This will allow the kernel to do fast calibration. I.e. you will
> only need to fully charge _OR_ fully discharge the battery to start
> seeing correct charge_now and capacity values.

OK

> > Also, we should have the driver default to using the charge_full_design
> > value if nothing is written from userspace and we see a high or low
> > value.
> 
> I'd prefer to have charge_full value undefined until the battery is
> calibrated.
> This way the userspace can estimate current battery capacity using voltage
> as a
> fallback for uncalibrated battery. The voltage estimation will be  way more
> accurate than having charge_full = charge_full_design on pretty old Droid 4
> batteries. For example, my battery after calibration has about 1000 mAh,
> while charge_full_design is 1740000.

OK

> > Maybe some pessimistic estimate could be used instead of just using
> > charge_full_design if no value is initialized from the userspace?
> > Something like (charge_full_design / 4) * 3 maybe?
> 
> It's better to rely on voltage estimated percentage.
> In Maemo Leste we patched upower to use the following formula, which gives
> pretty
> good results:
> 
> percentage = (voltage - voltage_empty) / (voltage_full - voltage_empty) *
> 100
> 
> Actually, the formula we use is a bit more complicated.

OK

> If you're curious, here's how it's actually done:
> https://github.com/maemo-leste/upower/blob/master/src/linux/up-device-supply.c#L756

Thanks, I'll give the calibration another try. Maybe I did not wait low
enough, I think I went down to 3.3V.

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index db1b519e87c6..669ed1513201 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -434,7 +434,7 @@  static bool cpcap_battery_low(struct cpcap_battery_ddata *ddata)
 
 static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata)
 {
-	struct cpcap_battery_state_data state, *latest, *previous, *tmp;
+	struct cpcap_battery_state_data state, *latest, *previous, *low, *high;
 	ktime_t now;
 	int error;
 
@@ -464,15 +464,40 @@  static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata)
 	memcpy(latest, &state, sizeof(*latest));
 
 	if (cpcap_battery_full(ddata)) {
-		tmp = cpcap_battery_get_highest(ddata);
+		high = cpcap_battery_get_highest(ddata);
 		/* Update highest charge seen? */
-		if (latest->counter_uah <= tmp->counter_uah)
-			memcpy(tmp, latest, sizeof(*tmp));
+		if (latest->counter_uah <= high->counter_uah ||
+		    !high->voltage) {
+			memcpy(high, latest, sizeof(*high));
+
+			low = cpcap_battery_get_lowest(ddata);
+			if (low->voltage && low->voltage != -1)
+				ddata->charge_full =
+					low->counter_uah - high->counter_uah;
+			else if (ddata->charge_full) {
+				/* Initialize with user provided data */
+				low->counter_uah =
+					high->counter_uah + ddata->charge_full;
+				/* Mark it as initialized */
+				low->voltage = -1;
+			}
+		}
 	} else if (cpcap_battery_low(ddata)) {
-		tmp = cpcap_battery_get_lowest(ddata);
+		low = cpcap_battery_get_lowest(ddata);
 		/* Update lowest charge seen? */
-		if (latest->counter_uah >= tmp->counter_uah)
-			memcpy(tmp, latest, sizeof(*tmp));
+		if (latest->counter_uah >= low->counter_uah ||
+		    !low->voltage) {
+			memcpy(low, latest, sizeof(*low));
+
+			high = cpcap_battery_get_highest(ddata);
+			if (high->voltage)
+				ddata->charge_full =
+					low->counter_uah - high->counter_uah;
+			else if (ddata->charge_full)
+				/* Initialize with user provided data */
+				high->counter_uah =
+					low->counter_uah - ddata->charge_full;
+		}
 	}
 
 	return 0;