[PATCH] power: supply: cw2015: Add CHARGE_NOW support

[PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Martin Ashby]

CHARGE_NOW is expected by some user software (such as waybar)
instead of 'CAPACITY', in order to correctly calculate remaining battery
life.

Signed-off-by: Martin Ashby <martin@ashbysoft.com>
---
 drivers/power/supply/cw2015_battery.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
index 0146f1bfc..aa1f1771b 100644
--- a/drivers/power/supply/cw2015_battery.c
+++ b/drivers/power/supply/cw2015_battery.c
@@ -511,6 +511,11 @@ static int cw_battery_get_property(struct power_supply *psy,
 			val->intval = 0;
 		break;
 
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		val->intval = cw_bat->battery.charge_full_design_uah;
+		val->intval = val->intval * cw_bat->soc / 100;
+		break;
+
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (cw_battery_valid_time_to_empty(cw_bat) &&
 		    cw_bat->battery.charge_full_design_uah > 0) {
@@ -542,6 +547,7 @@ static enum power_supply_property cw_battery_properties[] = {
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 };
 
-- 
2.30.0

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Tobias Schramm]

Hi Martin,

thanks for the patch. Now everything is looking good.

Just one small thing for the future: Please version your patches. Even 
when the patch subject changes. It helps everyone to distinguish clearly 
between different versions of a patch.

> CHARGE_NOW is expected by some user software (such as waybar)
> instead of 'CAPACITY', in order to correctly calculate remaining battery
> life.
> 
> Signed-off-by: Martin Ashby <martin@ashbysoft.com>
> ---
>   drivers/power/supply/cw2015_battery.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
> index 0146f1bfc..aa1f1771b 100644
> --- a/drivers/power/supply/cw2015_battery.c
> +++ b/drivers/power/supply/cw2015_battery.c
> @@ -511,6 +511,11 @@ static int cw_battery_get_property(struct power_supply *psy,
>   			val->intval = 0;
>   		break;
>   
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		val->intval = cw_bat->battery.charge_full_design_uah;
> +		val->intval = val->intval * cw_bat->soc / 100;
> +		break;
> +
>   	case POWER_SUPPLY_PROP_CURRENT_NOW:
>   		if (cw_battery_valid_time_to_empty(cw_bat) &&
>   		    cw_bat->battery.charge_full_design_uah > 0) {
> @@ -542,6 +547,7 @@ static enum power_supply_property cw_battery_properties[] = {
>   	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>   	POWER_SUPPLY_PROP_CHARGE_FULL,
>   	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
>   	POWER_SUPPLY_PROP_CURRENT_NOW,
>   };
>   
> 

Reviewed-by: Tobias Schramm <t.schramm@manjaro.org>
Tested-by: Tobias Schramm <t.schramm@manjaro.org>

Cheers,
Tobias

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Paul Fertser]

Hello Martin,

On Thu, Feb 18, 2021 at 07:42:50AM -0500, Martin Ashby wrote:
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		val->intval = cw_bat->battery.charge_full_design_uah;
> +		val->intval = val->intval * cw_bat->soc / 100;

Are you sure the chip reports current state of charge relative to the
full design capacity rather than to the latest auto-calibrated full
capacity? That would mean that over time as the cells wear out
cw_bat->soc (PROP_CAPACITY) is never going to be reaching 100; does
this match your experience?

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Tobias Schramm]

Hi all,

Am 18.06.21 um 14:30 schrieb Paul Fertser:
...
> 
> Are you sure the chip reports current state of charge relative to the
> full design capacity rather than to the latest auto-calibrated full
> capacity? That would mean that over time as the cells wear out
> cw_bat->soc (PROP_CAPACITY) is never going to be reaching 100; does
> this match your experience?
> 
As far as I'm aware the gauge reports SoC relative to what it believes 
to be the the current, auto-calibrated full battery capacity.
However, since I don't know how to extract that value from the gauge we 
just always assume it to be the design capacity [1].
Since we inquire the gauge about current SoC directly [2] there is no 
way for any miscalculation on our end preventing it from reaching 100%.

Cheers,
Tobias

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/cw2015_battery.c#n507

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/cw2015_battery.c#n368

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Paul Fertser]

Hi Tobias,

On Sat, Jun 19, 2021 at 11:52:34AM +0200, Tobias Schramm wrote:
> > Are you sure the chip reports current state of charge relative to the
> > full design capacity rather than to the latest auto-calibrated full
> > capacity? That would mean that over time as the cells wear out
> > cw_bat->soc (PROP_CAPACITY) is never going to be reaching 100; does
> > this match your experience?
> > 
> As far as I'm aware the gauge reports SoC relative to what it believes to be
> the the current, auto-calibrated full battery capacity.
> However, since I don't know how to extract that value from the gauge we just
> always assume it to be the design capacity [1].
> Since we inquire the gauge about current SoC directly [2] there is no way
> for any miscalculation on our end preventing it from reaching 100%.

After reading the datasheet for the gauge I'm rather disappointed. Not
using a shunt resistor for real current measurements and instead
relying on some magic doesn't make any sense in my book.

So to sum up, what you can get from the chip itself:

0. Battery voltage; pretty accurate.

1. Its idea of current state of charge; this is percentage relative to
the charge_full; I'd expect that to be reasonably accurate for not too
worn-out batteries.

2. Its idea of the remaining run time; involves secret magic to
somehow guess the current; I'd expect this to be relatively
inaccurate, would be interesting to learn how this performs for your
real life loads.

That's it. We can't learn charge_full and current_now.


Your driver code always reports charge_full equal to
charge_full_design which is plain incorrect IMHO. It's just wrong and
misleading, as after e.g. 2 years of usage the battery might lose half
its capacity; and people are used to get real data from power_supply
subsystem to be able to judge about battery health and performance.

Now you're also using the same value to calculate the current
reported. But the CW2015 datasheet says that SOC is relative to the
full capacity, not full design capacity, so the current you report
might get wildly inaccurate too. Same about charge_now that Martin
added: you just can't rely on unknown values when doing calculations.


If I was using this driver I would certainly prefer it to _not_ report
any grossly inaccurate guessimations. So I propose to remove
POWER_SUPPLY_PROP_CHARGE_FULL, POWER_SUPPLY_PROP_CHARGE_NOW and
POWER_SUPPLY_PROP_CURRENT_NOW altogether since they can't be
meaningfully obtained from anywhere. Please do not fool userspace into
thinking they have some information when in fact it's pulled out of
thin air.

I'm not the authority here so it would be nice to hear the opinion of
the subsystem maintainers, adding Sebastian to Cc. Ready to be proven
wrong :)

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Tobias Schramm]

Hey Paul,

Am 19.06.21 um 12:21 schrieb Paul Fertser:
...
> After reading the datasheet for the gauge I'm rather disappointed. Not
> using a shunt resistor for real current measurements and instead
> relying on some magic doesn't make any sense in my book.

I felt similarly when reading it for the first time. However, by now some
more well known manufacturers have started offering similar products [1].
It seems the manufacturers are relying on internal resistance of the 
battery to measure current draw.
In general it seems to work pretty well in the Pine64 Pinebook Pro for 
about two years now.

> So to sum up, what you can get from the chip itself:
> 
> 0. Battery voltage; pretty accurate.
> 
> 1. Its idea of current state of charge; this is percentage relative to
> the charge_full; I'd expect that to be reasonably accurate for not too
> worn-out batteries.
> 
> 2. Its idea of the remaining run time; involves secret magic to
> somehow guess the current; I'd expect this to be relatively
> inaccurate, would be interesting to learn how this performs for your
> real life loads.
> 

That is one of the characteristics I find very surprising. Even after 
over a year of use the remaining runtime estimation still seems to be 
pretty accurate. Maybe I got lucky but it works remarkably well :)

> That's it. We can't learn charge_full and current_now.
> 
> 
> Your driver code always reports charge_full equal to
> charge_full_design which is plain incorrect IMHO. It's just wrong and
> misleading, as after e.g. 2 years of usage the battery might lose half
> its capacity; and people are used to get real data from power_supply
> subsystem to be able to judge about battery health and performance.

It most definitely is. I'm not really happy with reporting them either.
However it seems that a lot of battery widgets get very confused if 
those stats are not reported.
Thus I went for some level of guestimation there. Please feel free to 
remove them and test whether all the usual battery monitoring tools 
still work. I'll be more than happy to remove them if it is not a 
problem anymore.

> Now you're also using the same value to calculate the current
> reported. But the CW2015 datasheet says that SOC is relative to the
> full capacity, not full design capacity, so the current you report
> might get wildly inaccurate too. Same about charge_now that Martin
> added: you just can't rely on unknown values when doing calculations.
> 
> 
> If I was using this driver I would certainly prefer it to _not_ report
> any grossly inaccurate guessimations. So I propose to remove
> POWER_SUPPLY_PROP_CHARGE_FULL, POWER_SUPPLY_PROP_CHARGE_NOW and
> POWER_SUPPLY_PROP_CURRENT_NOW altogether since they can't be
> meaningfully obtained from anywhere. Please do not fool userspace into
> thinking they have some information when in fact it's pulled out of
> thin air.
> 
> I'm not the authority here so it would be nice to hear the opinion of
> the subsystem maintainers, adding Sebastian to Cc. Ready to be proven
> wrong :)

Neither am I. The supported parameters were mostly chosen based on the 
Android
reference code provided to me by CellWise and user feedback (battery 
applet compatibility). I'd hate breaking the latter.

Cheers,
Tobias

[1] https://www.ti.com/lit/ds/symlink/bq27621-g1.pdf

Re: [PATCH] power: supply: cw2015: Add CHARGE_NOW support

[Paul Fertser]

Hi Tobias,

On Sat, Jun 19, 2021 at 03:48:12PM +0200, Tobias Schramm wrote:
> Am 19.06.21 um 12:21 schrieb Paul Fertser:
> ...
> > After reading the datasheet for the gauge I'm rather disappointed. Not
> > using a shunt resistor for real current measurements and instead
> > relying on some magic doesn't make any sense in my book.
> 
> I felt similarly when reading it for the first time. However, by now some
> more well known manufacturers have started offering similar products [1].

Thank you for the link, that's rather interesting, especially combined
with the TRM for the product.

What I find of specific relevance to our current discussion is that
this chip provides not one but two "charge_full" values: one assuming
less than C/20 load, and another assuming the current load. While
cx2015 doesn't give even one. 

> It seems the manufacturers are relying on internal resistance of the battery
> to measure current draw.

I'm still puzzled about how they might be able to estimate the current
looking just at the voltage and temperature. Apparently they're
successful enough but how do they manage?

> In general it seems to work pretty well in the Pine64 Pinebook Pro for about
> two years now.

In other words, if you leave the laptop idling and check RRT every now
and then, you see reasonable values and it reaches zero at about the
time it turns off; and same when you're doing an e.g. stress-ng --cpu
run?

> > Your driver code always reports charge_full equal to
> > charge_full_design which is plain incorrect IMHO. It's just wrong and
> > misleading, as after e.g. 2 years of usage the battery might lose half
> > its capacity; and people are used to get real data from power_supply
> > subsystem to be able to judge about battery health and performance.
> 
> It most definitely is. I'm not really happy with reporting them either.
> However it seems that a lot of battery widgets get very confused if those
> stats are not reported.

Probably rightfully so? Having no reliable data to show _is_ indeed
confusing.

> Thus I went for some level of guestimation there. Please feel free to remove
> them and test whether all the usual battery monitoring tools still work.
> I'll be more than happy to remove them if it is not a problem anymore.

If I had a Pinebook Pro I would be using Xmobar on it and I would
propose a patch to fall back to show "capacity" and "time_to_empty" in
cases like that; in other words, knowing the limitations, I'd make my
own choices about what to show where and whether I need any
guessimations or not.

> The supported parameters were mostly chosen based on the Android
> reference code provided to me by CellWise

If anything, that should serve as a "requires careful
re-consideration" red flag ;)

> and user feedback (battery applet compatibility). I'd hate breaking
> the latter.

If some battery applet finds certain set of parameters to be
insufficient then it's just that, the applet is not compatible with
this hardware. I know the "we do not break userspace" mantra but
breaking something (that used to work before) is one thing and luring
the userspace into doing things by presenting it with fake data is
another.

My userspace is Xmobar and I'm ready to fix it for Pinebook Pro users
if you remove reporting of the fake data. 

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com