Bug 473281 - MSI battery thresholds missing from power settings
Summary: MSI battery thresholds missing from power settings
Status: RESOLVED UPSTREAM
Alias: None
Product: Powerdevil
Classification: Unmaintained
Component: general (other bugs)
Version First Reported In: 5.27.6
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Plasma Bugs List
URL: https://github.com/BeardOverflow/msi-...
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-11 13:59 UTC by andretiagob
Modified: 2023-08-22 16:30 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description andretiagob 2023-08-11 13:59:32 UTC
Dear developers,

Recently a new kernel driver was added since kernel 6.4 that adds some functionalities for MSI laptops,
including battery thresholds.

Running the latest kernel in Fedora 38 KDE spin there is no option for battery thresholds in power settings,
I can see that it appears for a split second and then disappears when selecting advanced power settings.

Would it be possible to make plasma aware of the recent msi driver in order to have features like that?

Thank you for your time 👍


STEPS TO REPRODUCE
1. Open system settings
2. Next click on Power Manegement
3. Next click on Advanced power settings

OBSERVED RESULT
Battery thresholds appear for a split second and then disappears.

EXPECTED RESULT
Battery thresholds should appear normally.

SOFTWARE/OS VERSIONS
Fedora 38 KDE Spin
KDE Plasma Version 5.27.6
KDE Frameworks Version:  5.108.0
Qt Version:  5.15.10
Kernel Version: 6.4.9

Laptop:
MSI Summit E16 Flip, A12UCT model
Comment 1 andretiagob 2023-08-11 14:00:20 UTC
The new msi driver in question: https://github.com/torvalds/linux/blob/master/drivers/platform/x86/msi-ec.c
Comment 2 Nate Graham 2023-08-16 21:22:27 UTC
Hopefully we can support it. Lets see if we can find out how it was implemented. Can you run `find  /sys/devices/ | grep charge_ | xargs tail -n +1` in a terminal window and paste the output here?
Comment 3 andretiagob 2023-08-17 09:33:36 UTC
(In reply to Nate Graham from comment #2)
> Hopefully we can support it. Lets see if we can find out how it was
> implemented. Can you run `find  /sys/devices/ | grep charge_ | xargs tail -n
> +1` in a terminal window and paste the output here?

Hi Nate, thanks for responding!

Sure!

[andre@MSI ~]$ find  /sys/devices/ | grep charge_ | xargs tail -n +1
==> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:3f/PNP0C09:00/PNP0C0A:00/power_supply/BAT1/charge_full_design <==
5280000

==> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:3f/PNP0C09:00/PNP0C0A:00/power_supply/BAT1/charge_now <==
2562000

==> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:3f/PNP0C09:00/PNP0C0A:00/power_supply/BAT1/charge_control_start_threshold <==
-138

==> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:3f/PNP0C09:00/PNP0C0A:00/power_supply/BAT1/charge_full <==
5097000

==> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:3f/PNP0C09:00/PNP0C0A:00/power_supply/BAT1/charge_control_end_threshold <==
-128
[andre@MSI ~]$
Comment 4 Nate Graham 2023-08-21 17:40:06 UTC
Thanks. That looks okay. I wonder if our code is getting tripped up by the negative numbers in charge_control_start_threshold and charge_control_end_threshold.
Comment 5 Nate Graham 2023-08-21 17:41:51 UTC
Indeed, we do. In powerdevil/daemon/chargethresholdhelper.cpp:

if (threshold < 0 || threshold > 100) {
    qWarning() << file.fileName() << "contains invalid threshold" << threshold;
    continue;
}

The question now is whether these negative threshold numbers are valid and we should adjust our code to be more lenient, or whether they're invalid and this needs to be reported to the kernel folks.
Comment 6 andretiagob 2023-08-21 17:59:30 UTC
(In reply to Nate Graham from comment #5)
> Indeed, we do. In powerdevil/daemon/chargethresholdhelper.cpp:
> 
> if (threshold < 0 || threshold > 100) {
>     qWarning() << file.fileName() << "contains invalid threshold" <<
> threshold;
>     continue;
> }
> 
> The question now is whether these negative threshold numbers are valid and
> we should adjust our code to be more lenient, or whether they're invalid and
> this needs to be reported to the kernel folks.

Yeah that's what i ended up discovering too :)

After doing some experimenting I found out that after using the following commands:

echo 60 | sudo tee /sys/class/power_supply/BAT1/charge_control_end_threshold
echo 50 | sudo tee /sys/class/power_supply/BAT1/charge_control_end_threshold

The battery thresholds appear in advanced power settings!

I then did a EC reset on my laptop and tried again, then I used the command: /sys/devices/ | grep charge_ | xargs tail -n
> +1 and it showed those -138 and -128 values again, also the battery thresholds disappeared again in advanced power settings.
Comment 7 andretiagob 2023-08-21 18:06:24 UTC
Changing the charge percentage in system settings doesn't work anyway but I don't think this is a plasma problem because if I do change the thresholds in the terminal it doesn’t work either. I'm currently trying to get help here: https://github.com/BeardOverflow/msi-ec/discussions/53

Changing the battery thresholds with the correct values first in the terminal does make them appear in the advanced power settings though. Don't know what can be done about that in case a user gets a new MSI laptop and tries to find those thresholds in system settings, he must use the terminal first.
Comment 8 andretiagob 2023-08-21 18:15:48 UTC
In the above in meant: 
echo 60 | sudo tee /sys/class/power_supply/BAT1/charge_control_end_threshold
echo 50 | sudo tee /sys/class/power_supply/BAT1/charge_control_start_threshold
Comment 9 Nate Graham 2023-08-21 18:37:00 UTC
When you manually set those values, do the thresholds actually work as expected? That is to say, does charging stop at 60%?
Comment 10 andretiagob 2023-08-21 18:46:45 UTC
(In reply to Nate Graham from comment #9)
> When you manually set those values, do the thresholds actually work as
> expected? That is to say, does charging stop at 60%?

No, they don't work. Doesn’t matter if I change them in the terminal or in system settings.
Comment 11 Nate Graham 2023-08-21 18:50:40 UTC
Ok. And what about if you use that same method to set the threshold to somewhere between -138 and -128? Does that work to limit the charge level?
Comment 12 andretiagob 2023-08-21 18:58:58 UTC
(In reply to Nate Graham from comment #11)
> Ok. And what about if you use that same method to set the threshold to
> somewhere between -138 and -128? Does that work to limit the charge level?

I can't, it says invalid argument in the terminal.
Comment 13 Nate Graham 2023-08-21 20:30:48 UTC
Can you paste the full text from the terminal window, including the exact command you're running?
Comment 14 andretiagob 2023-08-21 21:24:00 UTC
(In reply to Nate Graham from comment #13)
> Can you paste the full text from the terminal window, including the exact
> command you're running?

Sure, it's no in English though:

[andre@MSI ~]$ echo -138 | sudo tee /sys/class/power_supply/BAT1/charge_control_end_threshold
[sudo] senha para andre: 
-138
tee: /sys/class/power_supply/BAT1/charge_control_end_threshold: Argumento inválido
[andre@MSI ~]$ ^C

It says "Invalid Argument"
Comment 15 Nate Graham 2023-08-21 21:27:58 UTC
My Spanish is not so bad that I can't understand "Argumento inválido"! :)

What about if you put -138 in quotes, like "-138"?
Comment 16 andretiagob 2023-08-21 21:33:54 UTC
(In reply to Nate Graham from comment #15)
> My Spanish is not so bad that I can't understand "Argumento inválido"! :)
> 
> What about if you put -138 in quotes, like "-138"?

It's actually Portuguese :D 

Here you go:

[andre@MSI ~]$ echo "-138" | sudo tee /sys/class/power_supply/BAT1/charge_control_end_threshold
[sudo] senha para andre: 
-138
tee: /sys/class/power_supply/BAT1/charge_control_end_threshold: Argumento inválido
[andre@MSI ~]$ 

I will try to test with a different kernel version to see if it fixes the problem. 

But again maybe these battery thresholds are not related to Plasma, maybe is an issue with this new MSI driver or something else going on with my laptop. 
Having to first input the correct values in the terminal in order for the option to appear in system settings is weird though..
Comment 17 andretiagob 2023-08-22 13:16:14 UTC
(In reply to andretiagob from comment #16)
> (In reply to Nate Graham from comment #15)
> > My Spanish is not so bad that I can't understand "Argumento inválido"! :)
> > 
> > What about if you put -138 in quotes, like "-138"?
> 
> It's actually Portuguese :D 
> 
> Here you go:
> 
> [andre@MSI ~]$ echo "-138" | sudo tee
> /sys/class/power_supply/BAT1/charge_control_end_threshold
> [sudo] senha para andre: 
> -138
> tee: /sys/class/power_supply/BAT1/charge_control_end_threshold: Argumento
> inválido
> [andre@MSI ~]$ 
> 
> I will try to test with a different kernel version to see if it fixes the
> problem. 
> 
> But again maybe these battery thresholds are not related to Plasma, maybe is
> an issue with this new MSI driver or something else going on with my laptop. 
> Having to first input the correct values in the terminal in order for the
> option to appear in system settings is weird though..

The charging problem was fixed!

There's this pull request that maybe will fix the negative values and thus fixes the battery thresholds not appearing in system settings:

https://github.com/BeardOverflow/msi-ec/pull/67

Thank you Nate!
Comment 18 Nate Graham 2023-08-22 16:27:50 UTC
Aha, I guess it really us purely an upstream problem, then. Thanks for following up with them!
Comment 19 andretiagob 2023-08-22 16:30:17 UTC
(In reply to Nate Graham from comment #18)
> Aha, I guess it really us purely an upstream problem, then. Thanks for
> following up with them!

Thank you!