From: Anthony Ruhier <aruhier@mailbox.org>

The value for the POWER_NOW property is by default negative when the
battery is discharging, positive when charging.

However on x1e laptops it breaks several userland tools that give a
prediction of the battery run time (such as the acpi command, powertop
or the waybar battery module), as these tools do not expect a negative
value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They
estimate the battery run time by dividing the value of energy_full by
power_now. The battery percentage is calculated by dividing energy_full
by energy_now, therefore it is not impacted.

While having a negative number during discharge makes sense, it is not
standard with how other battery drivers expose it. Instead, it seems
standard to have a positive value for power_now, and rely on the status
file instead to know if the battery is charging or discharging. It is
what other x86 laptops do.

Without the patch:
    $ acpi
    Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge.

With the patch:
    $ acpi
    Battery 0: Discharging, 97%, 10:18:27 remaining

---
Signed-off-by: Anthony Ruhier <aruhier@mailbox.org>
---
 drivers/power/supply/qcom_battmgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
index 47d29271ddf400b76dd5b0a1b8d1ba86c017afc0..3e2e0c5af2814df0eb0bfc408d4b3d26399ab4e4 100644
--- a/drivers/power/supply/qcom_battmgr.c
+++ b/drivers/power/supply/qcom_battmgr.c
@@ -530,7 +530,7 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy,
 		val->intval = battmgr->status.current_now;
 		break;
 	case POWER_SUPPLY_PROP_POWER_NOW:
-		val->intval = battmgr->status.power_now;
+		val->intval = abs(battmgr->status.power_now);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		if (unit != QCOM_BATTMGR_UNIT_mAh)

---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250128-patch-qcomm-bat-uint-power-5793f3638c56

Best regards,
-- 
Anthony Ruhier <aruhier@mailbox.org>

On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote:
> From: Anthony Ruhier <aruhier@mailbox.org>
> 
> The value for the POWER_NOW property is by default negative when the
> battery is discharging, positive when charging.
> 
> However on x1e laptops it breaks several userland tools that give a
> prediction of the battery run time (such as the acpi command, powertop
> or the waybar battery module), as these tools do not expect a negative
> value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They
> estimate the battery run time by dividing the value of energy_full by
> power_now. The battery percentage is calculated by dividing energy_full
> by energy_now, therefore it is not impacted.
> 
> While having a negative number during discharge makes sense, it is not
> standard with how other battery drivers expose it. Instead, it seems
> standard to have a positive value for power_now, and rely on the status
> file instead to know if the battery is charging or discharging. It is
> what other x86 laptops do.

Documentation/ABI does not define ABI for the power_now. However for
current_now it clearly defines that it can be positive or negative.

> 
> Without the patch:
>     $ acpi
>     Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge.
> 
> With the patch:
>     $ acpi
>     Battery 0: Discharging, 97%, 10:18:27 remaining
> 
> ---
> Signed-off-by: Anthony Ruhier <aruhier@mailbox.org>
> ---
>  drivers/power/supply/qcom_battmgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

-- 
With best wishes
Dmitry

On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote:
> On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote:
> > From: Anthony Ruhier <aruhier@mailbox.org>
> >
> > The value for the POWER_NOW property is by default negative when the
> > battery is discharging, positive when charging.
> >
> > However on x1e laptops it breaks several userland tools that give a
> > prediction of the battery run time (such as the acpi command, powertop
> > or the waybar battery module), as these tools do not expect a negative
> > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They
> > estimate the battery run time by dividing the value of energy_full by
> > power_now. The battery percentage is calculated by dividing energy_full
> > by energy_now, therefore it is not impacted.
> >
> > While having a negative number during discharge makes sense, it is not
> > standard with how other battery drivers expose it. Instead, it seems
> > standard to have a positive value for power_now, and rely on the status
> > file instead to know if the battery is charging or discharging. It is
> > what other x86 laptops do.
>
> Documentation/ABI does not define ABI for the power_now. However for
> current_now it clearly defines that it can be positive or negative.
>
> >
> > Without the patch:
> >     $ acpi
> >     Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge.
> >
> > With the patch:
> >     $ acpi
> >     Battery 0: Discharging, 97%, 10:18:27 remaining
> >
> > ---
> > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org>
> > ---
> >  drivers/power/supply/qcom_battmgr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --
> With best wishes
> Dmitry

I see. But as it breaks existing tools when power_now is negative, should we
change the behavior of these tools or adapt the driver?

As it does not seem common that power_now and current_now are negative in
other drivers, tools using these values rely on the status anyway. I'm
wondering if it provides anything to keep this behavior.

--
Regards,
Anthony Ruhier

On Fri, Feb 14, 2025 at 02:36:17AM +0100, aruhier@mailbox.org wrote:
> On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote:
> > On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote:
> > > From: Anthony Ruhier <aruhier@mailbox.org>
> > >
> > > The value for the POWER_NOW property is by default negative when the
> > > battery is discharging, positive when charging.
> > >
> > > However on x1e laptops it breaks several userland tools that give a
> > > prediction of the battery run time (such as the acpi command, powertop
> > > or the waybar battery module), as these tools do not expect a negative
> > > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They
> > > estimate the battery run time by dividing the value of energy_full by
> > > power_now. The battery percentage is calculated by dividing energy_full
> > > by energy_now, therefore it is not impacted.
> > >
> > > While having a negative number during discharge makes sense, it is not
> > > standard with how other battery drivers expose it. Instead, it seems
> > > standard to have a positive value for power_now, and rely on the status
> > > file instead to know if the battery is charging or discharging. It is
> > > what other x86 laptops do.
> >
> > Documentation/ABI does not define ABI for the power_now. However for
> > current_now it clearly defines that it can be positive or negative.
> >
> > >
> > > Without the patch:
> > >     $ acpi
> > >     Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge.
> > >
> > > With the patch:
> > >     $ acpi
> > >     Battery 0: Discharging, 97%, 10:18:27 remaining
> > >
> > > ---
> > > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org>
> > > ---
> > >  drivers/power/supply/qcom_battmgr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --
> > With best wishes
> > Dmitry
> 
> I see. But as it breaks existing tools when power_now is negative, should we
> change the behavior of these tools or adapt the driver?
> 
> As it does not seem common that power_now and current_now are negative in
> other drivers, tools using these values rely on the status anyway. I'm
> wondering if it provides anything to keep this behavior.

I think it is a problem of the 'acpi' tool. At least 'upower -d' uses
fabs internally since the initial commit in 2008.

-- 
With best wishes
Dmitry

Hi,

On Fri, Feb 14, 2025 at 05:01:08AM +0200, Dmitry Baryshkov wrote:
> On Fri, Feb 14, 2025 at 02:36:17AM +0100, aruhier@mailbox.org wrote:
> > On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote:
> > > On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote:
> > > > From: Anthony Ruhier <aruhier@mailbox.org>
> > > >
> > > > The value for the POWER_NOW property is by default negative when the
> > > > battery is discharging, positive when charging.
> > > >
> > > > However on x1e laptops it breaks several userland tools that give a
> > > > prediction of the battery run time (such as the acpi command, powertop
> > > > or the waybar battery module), as these tools do not expect a negative
> > > > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They
> > > > estimate the battery run time by dividing the value of energy_full by
> > > > power_now. The battery percentage is calculated by dividing energy_full
> > > > by energy_now, therefore it is not impacted.
> > > >
> > > > While having a negative number during discharge makes sense, it is not
> > > > standard with how other battery drivers expose it. Instead, it seems
> > > > standard to have a positive value for power_now, and rely on the status
> > > > file instead to know if the battery is charging or discharging. It is
> > > > what other x86 laptops do.
> > >
> > > Documentation/ABI does not define ABI for the power_now. However for
> > > current_now it clearly defines that it can be positive or negative.
> > >
> > > >
> > > > Without the patch:
> > > >     $ acpi
> > > >     Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge.
> > > >
> > > > With the patch:
> > > >     $ acpi
> > > >     Battery 0: Discharging, 97%, 10:18:27 remaining
> > > >
> > > > ---
> > > > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org>
> > > > ---
> > > >  drivers/power/supply/qcom_battmgr.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > 
> > I see. But as it breaks existing tools when power_now is negative, should we
> > change the behavior of these tools or adapt the driver?
> > 
> > As it does not seem common that power_now and current_now are negative in
> > other drivers, tools using these values rely on the status anyway. I'm
> > wondering if it provides anything to keep this behavior.

There are other drivers reporting negative values as documented.
Most of the embedded ones do this actually and there surely are
(embedded) userspace programs relying on this by now. But the
most used driver - generic ACPI battery - does not. That's why
quite a few userspace tools handle it wrong without anyone
noticing for quite some time. Fixing it to follow the ABI would
obviously end up in a bunch of regression reports, so things are
a bit messy :(

> I think it is a problem of the 'acpi' tool. At least 'upower -d' uses
> fabs internally since the initial commit in 2008.

It's definitely sensible to fix the userspace tools. We can't change
the documented ABI for current_now after that many years and while
documentation for power_now is missing, it would be quite unexpected
to have it behave differently than current_now. Also userspace
tooling needs to handle current_now and power_now anyways. And we
surely can't change the behaviour for all drivers reporting signed
data. So let's keep qcom_battmgr as is. It follows the documented
ABI and hopefully helps giving this more exposure (I'm typing this
on a X1E laptop right now and can see your problem with waybar).

But we should document the power_now property. It somehow fell
through the cracks :)

-- Sebastian

On Sat, Feb 15, 2025 at 04:08:25AM +0100, Sebastian Reichel wrote:
> Hi,
>
> There are other drivers reporting negative values as documented.
> Most of the embedded ones do this actually and there surely are
> (embedded) userspace programs relying on this by now. But the
> most used driver - generic ACPI battery - does not. That's why
> quite a few userspace tools handle it wrong without anyone
> noticing for quite some time. Fixing it to follow the ABI would
> obviously end up in a bunch of regression reports, so things are
> a bit messy :(
>
> > I think it is a problem of the 'acpi' tool. At least 'upower -d' uses
> > fabs internally since the initial commit in 2008.
>
> It's definitely sensible to fix the userspace tools. We can't change
> the documented ABI for current_now after that many years and while
> documentation for power_now is missing, it would be quite unexpected
> to have it behave differently than current_now. Also userspace
> tooling needs to handle current_now and power_now anyways. And we
> surely can't change the behaviour for all drivers reporting signed
> data. So let's keep qcom_battmgr as is. It follows the documented
> ABI and hopefully helps giving this more exposure (I'm typing this
> on a X1E laptop right now and can see your problem with waybar).
>
> But we should document the power_now property. It somehow fell
> through the cracks :)
>
> -- Sebastian

Hi,
As an update around this topic, I sent some patches in the different tools I'm
using to correctly handle negative values in current_now and power_now:

  * Waybar (included in release 0.12.0): https://github.com/Alexays/Waybar/pull/3942
  * Powertop (merged): https://github.com/fenrus75/powertop/pull/173
  * acpi-client (included in release 1.8): https://sourceforge.net/p/acpiclient/code/merge-requests/1/

It was quicker to get this merged than what I expected, which is good news!

There's probably other tools to fix, I just fixed the tools I'm using. I
encounter the issue on other tools, I'll send a patch.

--
Anthony Ruhier

Hi,

On Fri, Feb 28, 2025 at 04:25:47PM +0100, Anthony Ruhier wrote:
> On Sat, Feb 15, 2025 at 04:08:25AM +0100, Sebastian Reichel wrote:
> > There are other drivers reporting negative values as documented.
> > Most of the embedded ones do this actually and there surely are
> > (embedded) userspace programs relying on this by now. But the
> > most used driver - generic ACPI battery - does not. That's why
> > quite a few userspace tools handle it wrong without anyone
> > noticing for quite some time. Fixing it to follow the ABI would
> > obviously end up in a bunch of regression reports, so things are
> > a bit messy :(
> >
> > > I think it is a problem of the 'acpi' tool. At least 'upower -d' uses
> > > fabs internally since the initial commit in 2008.
> >
> > It's definitely sensible to fix the userspace tools. We can't change
> > the documented ABI for current_now after that many years and while
> > documentation for power_now is missing, it would be quite unexpected
> > to have it behave differently than current_now. Also userspace
> > tooling needs to handle current_now and power_now anyways. And we
> > surely can't change the behaviour for all drivers reporting signed
> > data. So let's keep qcom_battmgr as is. It follows the documented
> > ABI and hopefully helps giving this more exposure (I'm typing this
> > on a X1E laptop right now and can see your problem with waybar).
> >
> > But we should document the power_now property. It somehow fell
> > through the cracks :)
> >
> > -- Sebastian
> 
> Hi,
> As an update around this topic, I sent some patches in the different tools I'm
> using to correctly handle negative values in current_now and power_now:
> 
>   * Waybar (included in release 0.12.0): https://github.com/Alexays/Waybar/pull/3942
>   * Powertop (merged): https://github.com/fenrus75/powertop/pull/173
>   * acpi-client (included in release 1.8): https://sourceforge.net/p/acpiclient/code/merge-requests/1/
> 
> It was quicker to get this merged than what I expected, which is good news!
> 
> There's probably other tools to fix, I just fixed the tools I'm using. I
> encounter the issue on other tools, I'll send a patch.

Thanks, appreciated.

Greetings,

-- Sebastian

On Sat, Feb 15, 2025 at 04:08:25AM +0100, Sebastian Reichel wrote:
>
> There are other drivers reporting negative values as documented.
> Most of the embedded ones do this actually and there surely are
> (embedded) userspace programs relying on this by now. But the
> most used driver - generic ACPI battery - does not. That's why
> quite a few userspace tools handle it wrong without anyone
> noticing for quite some time. Fixing it to follow the ABI would
> obviously end up in a bunch of regression reports, so things are
> a bit messy :(
>
> > I think it is a problem of the 'acpi' tool. At least 'upower -d' uses
> > fabs internally since the initial commit in 2008.
>
> It's definitely sensible to fix the userspace tools. We can't change
> the documented ABI for current_now after that many years and while
> documentation for power_now is missing, it would be quite unexpected
> to have it behave differently than current_now. Also userspace
> tooling needs to handle current_now and power_now anyways. And we
> surely can't change the behaviour for all drivers reporting signed
> data. So let's keep qcom_battmgr as is. It follows the documented
> ABI and hopefully helps giving this more exposure (I'm typing this
> on a X1E laptop right now and can see your problem with waybar).
>
> But we should document the power_now property. It somehow fell
> through the cracks :)
>
> -- Sebastian

Hi Sebastian,
Thanks a lot for the detailed answer, that makes sense for me.
I was sending this patch more to know which direction to follow (changing the
driver or the userspace tools), and you answered it perfectly.

I started fixing the different desktop tools I use, starting with Waybar:
https://github.com/Alexays/Waybar/pull/3942

For powertop, the fix seems straightforward. For acpiclient, due to no activity
in almost 10 years, we'll see if it goes through.

--
Thanks,
Anthony Ruhier