| Message ID | 20210218124250.128008-1-martin@ashbysoft.com |
|---|---|
| State | Accepted |
| Commit | bf3841073bf34c9568ee5d6a6020b3902b3eef81 |
| Headers | show |
| Series | power: supply: cw2015: Add CHARGE_NOW support | expand |
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
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
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
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
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
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
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, };
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(+)