[PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Tony Lindgren]

Get a rough battery charge estimate until we've seen a high or low
battery level. After that we can use the coulomb counter to calculate
the battery capacity.

Note that I should probably update this to support ocv-capacity-table
before this makes sense to apply. With ocv-capacity-table we should be
able to estimate battery state as described in the documentation for
Documentation/devicetree/bindings/power/supply/battery.txt.

We do have some unknown battery data available over 1-wire, but the
format is unkonwn. If somebody ever figures out that format, we can
then switch to use the real battery data instead of ocv-capacity-table.

Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Not-yet-Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 70 +++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 12 deletions(-)

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
@@ -136,6 +136,29 @@ struct cpcap_battery_ddata {
 	u16 vendor;
 };
 
+struct cpcap_battery_capacity {
+	int capacity;
+	int voltage;
+	int percentage;
+};
+
+#define CPCAP_CAP(l, v, p)			\
+{						\
+	.capacity = (l),			\
+	.voltage = (v),				\
+	.percentage = (p),			\
+},
+
+/* 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,   0)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL, 3100000,   0)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_LOW,      3300000,   2)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_NORMAL,   3700000,  50)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_HIGH,     4000000,  75)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL,     4200000 - 18000, 100)
+};
+
 #define CPCAP_NO_BATTERY	-400
 
 static struct cpcap_battery_state_data *
@@ -411,6 +434,40 @@ static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata)
 	return 0;
 }
 
+static void cpcap_battery_get_rough(struct cpcap_battery_ddata *ddata,
+				    int *level, int *percentage)
+{
+	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;
+
+	if (level)
+		*level = cap->capacity;
+	if (percentage)
+		*percentage = cap->percentage;
+}
+
+static int cpcap_battery_get_rough_capacity(struct cpcap_battery_ddata *ddata)
+{
+	int capacity = 0;
+
+	cpcap_battery_get_rough(ddata, &capacity, NULL);
+
+	return capacity;
+}
+
 static enum power_supply_property cpcap_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -516,18 +573,7 @@ static int cpcap_battery_get_property(struct power_supply *psy,
 		val->intval = div64_s64(tmp, 100);
 		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_rough_capacity(ddata);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		val->intval = ddata->config.info.charge_full_design;
-- 
2.24.1

[PATCH 2/3] RFC: power: supply: cpcap-battery: Save high and low states

[Tony Lindgren]

If we save the battery high and low states, we can use those to estimate
the battery capacity in the following patch.

Note that this too should probably use ocv-capacity-table as AFAIK that
has already similar data structures needed.

Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Not-yet-Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 38 +++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

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_LOW,
+	CPCAP_BATTERY_STATE_HIGH,
 	CPCAP_BATTERY_STATE_NR,
 };
 
@@ -183,6 +185,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_lowest(struct cpcap_battery_ddata *ddata)
+{
+	return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_LOW);
+}
+
+static struct cpcap_battery_state_data *
+cpcap_battery_get_highest(struct cpcap_battery_ddata *ddata)
+{
+	return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_HIGH);
+}
+
 static int cpcap_charger_battery_temperature(struct cpcap_battery_ddata *ddata,
 					     int *value)
 {
@@ -400,9 +414,19 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata)
 	return false;
 }
 
+static bool cpcap_battery_low(struct cpcap_battery_ddata *ddata)
+{
+	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
+
+	if (state->current_ua && state->voltage <= 3300000)
+		return true;
+
+	return false;
+}
+
 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, *tmp;
 	ktime_t now;
 	int error;
 
@@ -431,6 +455,18 @@ 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)) {
+		tmp = cpcap_battery_get_highest(ddata);
+		/* Update highest charge seen? */
+		if (latest->counter_uah <= tmp->counter_uah)
+			memcpy(tmp, latest, sizeof(*tmp));
+	} else if (cpcap_battery_low(ddata)) {
+		tmp = cpcap_battery_get_lowest(ddata);
+		/* Update lowest charge seen? */
+		if (latest->counter_uah >= tmp->counter_uah)
+			memcpy(tmp, latest, sizeof(*tmp));
+	}
+
 	return 0;
 }
 
-- 
2.24.1

[PATCH 3/3] RFC: power: supply: cpcap-battery: Implement capacity percentage

[Tony Lindgren]

Based on the saved high and low states, we can estimate battery state
based on the coulomb counter:

# cat /sys/class/power_supply/battery/capacity
...

Note that this too should probably be updated to use ocv-capacity-table.

Also note that I have not verified the shown capacity is very accurate.

Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Not-yet-Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 35 ++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

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
@@ -504,6 +504,15 @@ static int cpcap_battery_get_rough_capacity(struct cpcap_battery_ddata *ddata)
 	return capacity;
 }
 
+static int cpcap_battery_get_rough_percentage(struct cpcap_battery_ddata *ddata)
+{
+	int percentage = 0;
+
+	cpcap_battery_get_rough(ddata, NULL, &percentage);
+
+	return percentage;
+}
+
 static enum power_supply_property cpcap_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -518,6 +527,7 @@ static enum power_supply_property cpcap_battery_props[] = {
 	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,
@@ -528,10 +538,10 @@ 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, *low, *high;
 	u32 sample;
 	s32 accumulator;
-	int cached;
+	int cached, delta, est;
 	s64 tmp;
 
 	cached = cpcap_battery_update_status(ddata);
@@ -608,6 +618,27 @@ 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:
+		est = cpcap_battery_get_rough_percentage(ddata);
+		high = cpcap_battery_get_highest(ddata);
+		if (high->voltage) {
+			delta = latest->counter_uah - high->counter_uah;
+			val->intval = (ddata->config.info.charge_full_design -
+				       delta) * 100;
+			val->intval /= ddata->config.info.charge_full_design;
+			delta = abs(val->intval - est);
+			break;
+		}
+		low = cpcap_battery_get_lowest(ddata);
+		if (low->voltage) {
+			delta = low->counter_uah - latest->counter_uah;
+			val->intval = (delta * 100) /
+				ddata->config.info.charge_full_design;
+			delta = abs(val->intval - est);
+			break;
+		}
+		val->intval = est;
+		break;
 	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
 		val->intval = cpcap_battery_get_rough_capacity(ddata);
 		break;
-- 
2.24.1

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Pavel Machek]

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

Hi!

> Get a rough battery charge estimate until we've seen a high or low
> battery level. After that we can use the coulomb counter to calculate
> the battery capacity.
> 
> Note that I should probably update this to support ocv-capacity-table
> before this makes sense to apply. With ocv-capacity-table we should be
> able to estimate battery state as described in the documentation for
> Documentation/devicetree/bindings/power/supply/battery.txt.

Maybe we should let userspace do that?

https://github.com/pavelmachek/libbattery

> +struct cpcap_battery_capacity {
> +	int capacity;
> +	int voltage;
> +	int percentage;
> +};
> +
> +#define CPCAP_CAP(l, v, p)			\
> +{						\
> +	.capacity = (l),			\
> +	.voltage = (v),				\
> +	.percentage = (p),			\
> +},
> +
> +/* 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,   0)
> +	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL, 3100000,   0)
> +	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_LOW,      3300000,   2)
> +	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_NORMAL,   3700000,  50)
> +	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_HIGH,     4000000,  75)
> +	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL,     4200000 - 18000, 100)
> +};

No, not like this; it is simultaneously too complex and not complex
enough.

Too complex: we don't really want a table, when there's well know
formula implementing it:
http://git.goldelico.com/?p=gta04-kernel.git;a=patch;h=22ab047ae296e998379c1aa29fe1210043cfa040 

Not complex enough: we know the current (and there are going to be big
differences between CPU idle and CPU loaded with GSM active), and
there's dependency on temperature, too...

Best regards,

									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Merlijn Wajer]

[-- Attachment #1.1: Type: text/plain, Size: 842 bytes --]

On 21/01/2020 10:57, Pavel Machek wrote:
> Hi!
> 
>> Get a rough battery charge estimate until we've seen a high or low
>> battery level. After that we can use the coulomb counter to calculate
>> the battery capacity.
>>
>> Note that I should probably update this to support ocv-capacity-table
>> before this makes sense to apply. With ocv-capacity-table we should be
>> able to estimate battery state as described in the documentation for
>> Documentation/devicetree/bindings/power/supply/battery.txt.
> 
> Maybe we should let userspace do that?
> 
> https://github.com/pavelmachek/libbattery

Unless this gets integrated in UPower, My point of view is that
userspace probably wants some uniform approach. Kernel interface
definitely is one uniform interface that would also get picked up by UPower.

Cheers,
Merlijn


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Pavel Machek]

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

Hi!

> >> Get a rough battery charge estimate until we've seen a high or low
> >> battery level. After that we can use the coulomb counter to calculate
> >> the battery capacity.
> >>
> >> Note that I should probably update this to support ocv-capacity-table
> >> before this makes sense to apply. With ocv-capacity-table we should be
> >> able to estimate battery state as described in the documentation for
> >> Documentation/devicetree/bindings/power/supply/battery.txt.
> > 
> > Maybe we should let userspace do that?
> > 
> > https://github.com/pavelmachek/libbattery
> 
> Unless this gets integrated in UPower, My point of view is that
> userspace probably wants some uniform approach. Kernel interface
> definitely is one uniform interface that would also get picked up by UPower.

I see that it is convenient to have it in kernel. I just assumed
Sebastian would not take that, because it does not _need_ to be in
kernel...

And maybe it is easier in userspace, since userspace has access to
persistent storage, so it can store detailed battery calibration
parameters? (And Gnome/Mate even does something like that, see
mate-power-statistics).

So... not my decision. Don't care too much either way. I have been
doing this in userspace, but...

What I'd like to see is some kind of indication that "this attribute
is not from hardware, kernel computed it for you" so clever userspace
can ignore it/compute it in more advanced way.

Best regards,

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [200121 22:26]:
> Hi!
> 
> > >> Get a rough battery charge estimate until we've seen a high or low
> > >> battery level. After that we can use the coulomb counter to calculate
> > >> the battery capacity.
> > >>
> > >> Note that I should probably update this to support ocv-capacity-table
> > >> before this makes sense to apply. With ocv-capacity-table we should be
> > >> able to estimate battery state as described in the documentation for
> > >> Documentation/devicetree/bindings/power/supply/battery.txt.
> > > 
> > > Maybe we should let userspace do that?
> > > 
> > > https://github.com/pavelmachek/libbattery
> > 
> > Unless this gets integrated in UPower, My point of view is that
> > userspace probably wants some uniform approach. Kernel interface
> > definitely is one uniform interface that would also get picked up by UPower.
> 
> I see that it is convenient to have it in kernel. I just assumed
> Sebastian would not take that, because it does not _need_ to be in
> kernel...

I think we should find a way where we can keep the existing kernel
interface as all the apps rely on that. If we allow userspace to
optionally set the estimated battery full and battery empty coulomb
counter values we should be all set.

For temperature compensation it seems that the ocv-capacity-table
is the way to go to describe the battery if the battery data is
unsuable or does not exist.

> And maybe it is easier in userspace, since userspace has access to
> persistent storage, so it can store detailed battery calibration
> parameters? (And Gnome/Mate even does something like that, see
> mate-power-statistics).

Agreed, at minimum the userspace should save estimated coulomb
counter full and empty values. After that calculations are trivial
as seen with this series.

> So... not my decision. Don't care too much either way. I have been
> doing this in userspace, but...
> 
> What I'd like to see is some kind of indication that "this attribute
> is not from hardware, kernel computed it for you" so clever userspace
> can ignore it/compute it in more advanced way.

Well we do have that via IIO already. Not sure if that is needed
for power supply.

Regards,

Tony

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Arthur D.]

Hi, Tony.

I used the kernel with the patch serieas applied for a while.

What I expected from the userspace perspective was having
/sys/class/power_supply/battery/capacity undefined until
kernel calculates more or less accurate values for it.

Until then, the userspace should estimate percentage on its own
using voltage and current values provided by the kernel.
Like it's already done with bq27200 on Nokia N900.

Right now the values which the kernel provides with
/sys/class/power_supply/battery/capacity after a system boot
are confusing.

The user can see the battery plugin doesn't change its gauge
gradually like it was done with previous kernel versions.
Sometimes it gets suddenly empty, sometimes it changes from empty
to half full. And it always reports "battery full" in advance -
when the battery is being charged with relatively high current.

The following part of mail dedicated to what I think should be fixed
in the commits.

1) RFC: power: supply: cpcap-battery: Add helper for rough capacity

CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)

This line seems wrong, because Droid4 full battery is 4.35V, not 4.2V.

2.1) RFC: power: supply: cpcap-battery: Implement capacity percentage

val->intval = (ddata->config.info.charge_full_design - delta) * 100;
val->intval /= ddata->config.info.charge_full_design;

IMHO, charge_full_design should never be used in battery charge
percentage calculations. Li-ion batteries loose their capacity
with age. Therefore such calculations are likely to always be wrong.

I'd prefer to have the full charge value to be dynamically calculated by
the kernel with a formula like this:
charge_full = high->counter_uah - low->counter_uah;

This which will give us accurate estimation for the battery charge, esti-
mated time left on the battery and so on. It would be good, if we allow
the userspace to store the full charge value between reboots and to feed
it to the kernel after, so it could start providing accurated data faster:
the battery should only go full or empty once and we are ready.

I'd like to mention explicitly: I think the kernel should return -ENODATA
for capacity values until the battery is "calibrated". By "calibration"
I mean having high and low counter_uah values initialized with the data
collected when the battery went a full charge or discharge cycle (from
empty to full or vice versa).

--
Best regards, Spinal

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Tony Lindgren]

* Arthur D. <spinal.by@gmail.com> [200310 04:55]:
> Hi, Tony.
> 
> I used the kernel with the patch serieas applied for a while.
> 
> What I expected from the userspace perspective was having
> /sys/class/power_supply/battery/capacity undefined until
> kernel calculates more or less accurate values for it.
> 
> Until then, the userspace should estimate percentage on its own
> using voltage and current values provided by the kernel.
> Like it's already done with bq27200 on Nokia N900.
> 
> Right now the values which the kernel provides with
> /sys/class/power_supply/battery/capacity after a system boot
> are confusing.

OK yeah I like the idea of not showing anything until userspace
as configured estimated high and low values.

> The user can see the battery plugin doesn't change its gauge
> gradually like it was done with previous kernel versions.
> Sometimes it gets suddenly empty, sometimes it changes from empty
> to half full. And it always reports "battery full" in advance -
> when the battery is being charged with relatively high current.
> 
> The following part of mail dedicated to what I think should be fixed
> in the commits.
> 
> 1) RFC: power: supply: cpcap-battery: Add helper for rough capacity
> 
> CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)
>
> This line seems wrong, because Droid4 full battery is 4.35V, not 4.2V.

Hmm yeah this is now configurable and defaults to 4.2V so we
should use the confgured value. The reason we default to 4.2V
now is because we suspect that if left connected to the charger
at higher than 4.2V the battery ages faster.

> 2.1) RFC: power: supply: cpcap-battery: Implement capacity percentage
> 
> val->intval = (ddata->config.info.charge_full_design - delta) * 100;
> val->intval /= ddata->config.info.charge_full_design;
> 
> IMHO, charge_full_design should never be used in battery charge
> percentage calculations. Li-ion batteries loose their capacity
> with age. Therefore such calculations are likely to always be wrong.
> 
> I'd prefer to have the full charge value to be dynamically calculated by
> the kernel with a formula like this:
> charge_full = high->counter_uah - low->counter_uah;

OK makes sense. I guess we can assuem if both are set to the
the same number like 0 on boot, there's no estimate available.

> This which will give us accurate estimation for the battery charge, esti-
> mated time left on the battery and so on. It would be good, if we allow
> the userspace to store the full charge value between reboots and to feed
> it to the kernel after, so it could start providing accurated data faster:
> the battery should only go full or empty once and we are ready.

Yes userspace is the only place that can eventually generate accurate
estimate based on battery wear, temperature etc.

> I'd like to mention explicitly: I think the kernel should return -ENODATA
> for capacity values until the battery is "calibrated". By "calibration"
> I mean having high and low counter_uah values initialized with the data
> collected when the battery went a full charge or discharge cycle (from
> empty to full or vice versa).

Yes I like this idea. That leaves out the need for poor estimates
in the kernel.

I guess we should have the following new properties:

POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL
POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY

And these would be 0 on boot and then userspace can
update these based on battery data.

Regards,

Tony

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Arthur D.]

Hi.

>> 1) RFC: power: supply: cpcap-battery: Add helper for rough capacity
>>
>> CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)
>>
>> This line seems wrong, because Droid4 full battery is 4.35V, not 4.2V.
>
> Hmm yeah this is now configurable and defaults to 4.2V so we
> should use the confgured value. The reason we default to 4.2V
> now is because we suspect that if left connected to the charger
> at higher than 4.2V the battery ages faster.

1. How to configure this value? I tried:
echo 4350000 > /sys/class/power_supply/battery/constant_charge_voltage

...but it didn't change the fully charged battery voltage.

2. If the value is configurable, it probably shouldn't be hardcoded like
this in the code:
PCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)

>> I think the kernel should return  -ENODATA
>> for capacity values until the battery is "calibrated". By "calibration"
>> I mean having high and low counter_uah values initialized with the data
>> collected when the battery went a full charge or discharge cycle (from
>> empty to full or vice versa).
>
> Yes I like this idea. That leaves out the need for poor estimates
> in the kernel.
>
> I guess we should have the following new properties:
>
> POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL
> POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY
>
> And these would be 0 on boot and then userspace can
> update these based on battery data.

My idea is to only have one extra property which will store the delta of
counter values for full and empty battery states. In this case, it's going
to be more accurate than allowing the userspace to store/restore both full
and empty counters.

Some users may have a dual boot setup. In this case restoring full & empty
counters after reboot can make the kernel to report wrong battery
percentage
values.

Let's imagine the following situation:

1. The user charges the battery to the full state and discharges it to be
almost depleted, so the kernel has "calibrated" values for counter_full and
counter_empty.
2. The user decides to reboot to another OS. Let's assume he has a fully
charged battery before doing the reboot.
For example the kernel provides the following values:
POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL = -250000
POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY = 950000
/sys/class/power_supply/battery/charge_counter = -250000
A daemon saves these values to a permanent storage.
3. The user boots another OS, works there for a while, the battery charge
becomes 50%.
4. The user reboots back to our system, we restore the values:
POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL <- -250000
POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY <- 950000
5. /sys/class/power_supply/battery/charge_counter is initialized to 0 or
-250000, it doesn't matter, because in both cases we will get the wrong
battery percentage. We should report 50%, remember? In any case it can't
be calculated to this value.

And here's how I suggest it to be implemented:
1. Same as above
2. Same as above, except the kernel provides a single value that the user
should store between reboots (it's the delta I mentioned above):
POWER_SUPPLY_PROP_CHARGE_FULL = 1200000
3. Same as above
4. The user reboots back to our system, we restore the value:
POWER_SUPPLY_PROP_CHARGE_FULL <- 1200000
5. The kernel reports -ENODATA as battery capacity value until the battery
goes to a reference point (fully charged or almost discharged state).
When the battery is fully charged or discharged, the kernel can immediately
calculate the opposite counter value. And no matter what was the initial
value of /sys/class/power_supply/battery/charge_counter when the system
booted.

Of course, the value of the delta will not be constant, it should be
recalculated over time. The good news is that this value will be changed
slowly over time. That is, it can vary from 2% to 10% per year.

--
Best regards, Spinal

Re: [PATCH 1/3] RFC: power: supply: cpcap-battery: Add helper for rough capacity

[Tony Lindgren]

* Arthur D. <spinal.by@gmail.com> [200310 18:36]:
> Hi.
> 
> > > 1) RFC: power: supply: cpcap-battery: Add helper for rough capacity
> > > 
> > > CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)
> > > 
> > > This line seems wrong, because Droid4 full battery is 4.35V, not 4.2V.
> > 
> > Hmm yeah this is now configurable and defaults to 4.2V so we
> > should use the confgured value. The reason we default to 4.2V
> > now is because we suspect that if left connected to the charger
> > at higher than 4.2V the battery ages faster.
> 
> 1. How to configure this value? I tried:
> echo 4350000 > /sys/class/power_supply/battery/constant_charge_voltage
> 
> ...but it didn't change the fully charged battery voltage.

If you really want to use it, first enable it for the battery in
/sys/class/power/supply/usb to allow it, then also enable it for the
charger in /sys/class/power/supply/usb to actually use it.

But as discussed earlier, it really seems the battery ages faster at
the higher charge rates. Especially if left connected to the charger
for longer times as also seen with Android earlier.

> 2. If the value is configurable, it probably shouldn't be hardcoded like
> this in the code:
> PCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL, 4200000 - 18000, 100)

Right, we should now use the configured voltage :)

> > > I think the kernel should return  -ENODATA
> > > for capacity values until the battery is "calibrated". By "calibration"
> > > I mean having high and low counter_uah values initialized with the data
> > > collected when the battery went a full charge or discharge cycle (from
> > > empty to full or vice versa).
> > 
> > Yes I like this idea. That leaves out the need for poor estimates
> > in the kernel.
> > 
> > I guess we should have the following new properties:
> > 
> > POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL
> > POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY
> > 
> > And these would be 0 on boot and then userspace can
> > update these based on battery data.
> 
> My idea is to only have one extra property which will store the delta of
> counter values for full and empty battery states. In this case, it's going
> to be more accurate than allowing the userspace to store/restore both full
> and empty counters.

Hmm OK. Sounds like you might have some patches coming then?

> Some users may have a dual boot setup. In this case restoring full & empty
> counters after reboot can make the kernel to report wrong battery
> percentage
> values.

Yeah no idea how that is handled. Also changing the battery can cause
issues with this. I was expecting the 1-wire EEPROM to contain some
battery specific data like a serial number, but it seems this data is
the same for all the same type batteries.

> Let's imagine the following situation:
> 
> 1. The user charges the battery to the full state and discharges it to be
> almost depleted, so the kernel has "calibrated" values for counter_full and
> counter_empty.
> 2. The user decides to reboot to another OS. Let's assume he has a fully
> charged battery before doing the reboot.
> For example the kernel provides the following values:
> POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL = -250000
> POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY = 950000
> /sys/class/power_supply/battery/charge_counter = -250000
> A daemon saves these values to a permanent storage.
> 3. The user boots another OS, works there for a while, the battery charge
> becomes 50%.
> 4. The user reboots back to our system, we restore the values:
> POWER_SUPPLY_PROP_CHARGE_COUNTER_FULL <- -250000
> POWER_SUPPLY_PROP_CHARGE_COUNTER_EMPTY <- 950000
> 5. /sys/class/power_supply/battery/charge_counter is initialized to 0 or
> -250000, it doesn't matter, because in both cases we will get the wrong
> battery percentage. We should report 50%, remember? In any case it can't
> be calculated to this value.

Maybe some charge counter offset limit can be used to validate
the saved data in the userspace before updating values for kernel?

> And here's how I suggest it to be implemented:
> 1. Same as above
> 2. Same as above, except the kernel provides a single value that the user
> should store between reboots (it's the delta I mentioned above):
> POWER_SUPPLY_PROP_CHARGE_FULL = 1200000
> 3. Same as above
> 4. The user reboots back to our system, we restore the value:
> POWER_SUPPLY_PROP_CHARGE_FULL <- 1200000
> 5. The kernel reports -ENODATA as battery capacity value until the battery
> goes to a reference point (fully charged or almost discharged state).
> When the battery is fully charged or discharged, the kernel can immediately
> calculate the opposite counter value. And no matter what was the initial
> value of /sys/class/power_supply/battery/charge_counter when the system
> booted.
> 
> Of course, the value of the delta will not be constant, it should be
> recalculated over time. The good news is that this value will be changed
> slowly over time. That is, it can vary from 2% to 10% per year.

Sounds good to me :)

Tony