Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Battery protection and fn-lock support #8

Closed
aymanbagabas opened this issue Apr 17, 2019 · 51 comments
Closed

Battery protection and fn-lock support #8

aymanbagabas opened this issue Apr 17, 2019 · 51 comments
Labels
enhancement New feature or request

Comments

@aymanbagabas
Copy link
Owner

aymanbagabas commented Apr 17, 2019

Using Windows Huawei PC Manager, you can enable features like battery protection and reversing the behavior of Fn. These features are 'obviously' missing on Linux. I'm working on integrating these to the driver using WMI calls.

Using WMI is easier than controlling these features using ACPI/EC since most of these devices has the same WMI device driver.

So far, I've seen repetitions when it comes to using WMI calls to control Fn-lock, battery protection/thresholds, and mic led in multiple models including:

  1. MACH-WX9 - Matebook X Pro (2018)
  2. WRT-WX9 - Matebook 13 (2019)
  3. KPL-W0X - Matebook D 14 AMD

I haven't seen that on the older model Matebook X (2017). DSDT/SSDT tables for other models are welcome!

@aymanbagabas aymanbagabas added the enhancement New feature or request label Apr 17, 2019
@aymanbagabas
Copy link
Owner Author

@nekr0z can you please try the newly updated module? I've added battery protection, fn-lock. I still need to find a way of distinguishing if a laptop supports these features or not.

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

I'd love to, but I'm on Debian (kernel 4.19), and this is very much my production laptop, so simply wiping the system and getting fresh Arch is not something I'd consider. You may have just tempted me to give compiling 5.0 a try, but I have never done it all from scratch (I usually rely on packages to patch and recompile kernel), so I need some time to figure it all out.

@aymanbagabas
Copy link
Owner Author

@nekr0z you could try booting from usb. As long as you're using kernel >= 5.0 you should be good.

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

Ok, 5.0.2 is already in Debian experimental, why not give it a try while I'm at it... Sweet Discordia, touchpad patching again!..

Alright, ready. Aaaaaand... here we go!

$ sudo make install
make -C /lib/modules/5.0.0-trunk-amd64/build/ M=/home/evgeny/bin/huawei-wmi-2.0 modules_install
make[1]: вход в каталог «/usr/src/linux-headers-5.0.0-trunk-amd64»
  INSTALL /home/evgeny/bin/huawei-wmi-2.0/huawei-wmi.ko
  DEPMOD  5.0.0-trunk-amd64
Warning: modules_install: missing 'System.map' file. Skipping depmod.

Am I missing something? Because after sudo modprobe huawei-wmi I don't get huawei-wmi in /sys/devices/platform...

@aymanbagabas
Copy link
Owner Author

Am I missing something? Because after sudo modprobe huawei-wmi I don't get huawei-wmi in /sys/devices/platform...

Instead try

$ sudo rmmod huawei_wmi
$ make
$ sudo insmod huawei-wmi.c

Don't install!

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

$ sudo insmod huawei-wmi.c

You meant huawei-wmi.ko, right?

$ sudo rmmod huawei_wmi
$ sudo insmod huawei-wmi.ko
$ ls /sys/devices/platform/ | grep huawei

Nothing.

@aymanbagabas
Copy link
Owner Author

$ sudo insmod huawei-wmi.c

You meant huawei-wmi.ko, right?

Yeah my bad. What about dmesg, do you get any output?

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

input: Huawei WMI hotkeys as /devices/platform/PNP0C14:03/wmi_bus/wmi_bus-PNP0C14:03/ABBC0F5C-8EA1-11D1-A000-C90629100000/input/input23

@aymanbagabas
Copy link
Owner Author

Could you try this and see if you get any messages in dmesg.

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

Yep.

input: Huawei WMI hotkeys as /devices/platform/PNP0C14:03/wmi_bus/wmi_bus-PNP0C14:03/ABBC0F5C-8EA1-11D1-A000-C90629100000/input/input24                                                   
done wmi dri
leds setup
finish init

Mic led started working immediately. Fn-lock state can be read and is accurate (althought can't be written, do you support some way of setting these variables too?)
Batpro settings are a bit off. Your driver shows start threshold of 0, stop threshold of 40, while in fact they are 40-70.

@aymanbagabas
Copy link
Owner Author

Mic led started working immediately. Fn-lock state can be read and is accurate (althought can't be written, do you support some way of setting these variables too?)

Great! You need root to write into these files. echo 1 | sudo tee /sys/devices/platform/huawei-wmi/FILE

Batpro settings are a bit off. Your driver shows start threshold of 0, stop threshold of 40, while in fact they are 40-70.

Aha! I see why. In your model, these data are off (location wise) by one byte. Idk how this would be handled correctly. But setting these values should work. setting it to 0-100 disables protection.

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

But setting these values should work. setting it to 0-100 disables protection.

Setting them to 0-100 indeed disables protection. Setting them to any other value pair does nothing at all.

@aymanbagabas
Copy link
Owner Author

But setting these values should work. setting it to 0-100 disables protection.

Setting them to 0-100 indeed disables protection. Setting them to any other value pair does nothing at all.

Oh! I see why. It's the same problem because the driver gets the current thresholds values before it updates them. This is there because the driver uses two sysfs attributes (files) to control the thresholds. But the way WMI controls these values in Huawei laptops is using one method passing both low and high values. I will change it from two attributes to one containing both low and high thresholds and work on a workaround of retrieving the correct values from WMI.
Thank you for your contribution!

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

Keep me posted, I'll be glad to test. And thank you so much for the mic led! (and I guess thanks for moving me to kernel 5, too ;-)

Do you envision any way to eventually make it possible to set those values as non-privileged user? You might have already checked up my crude attempt on UI, and that currently requires a lot of hoop-jumping that is not too much user-friendly...

@aymanbagabas
Copy link
Owner Author

Keep me posted, I'll be glad to test. And thank you so much for the mic led! (and I guess thanks for moving me to kernel 5, too ;-)

Do you envision any way to eventually make it possible to set those values as non-privileged user? You might have already checked up my crude attempt on UI, and that currently requires a lot of hoop-jumping that is not too much user-friendly...

I like that! You could also create a very simple UI, along with sys applet, that access those values under /sys. You could use a udev rule to change the permission of these values so that a particular group ex. wheel can access them maybe it asks for root privileges if otherwise. That would be really the best way to do it. You could change the owner of these sysfs files but as you can tell it's not a good idea for security reasons.

@nekr0z
Copy link

nekr0z commented Apr 23, 2019

You could also create a very simple UI, along with sys applet, that access those values under /sys.

That will definitely be my next step as soon as you get battery protection settings sorted out and "API" stabilized.

You could use a udev rule to change the permission of these values so that a particular group ex. wheel can access them maybe it asks for root privileges if otherwise.

I like it. A separate group, like huawei-wmi, and those udev rules packaged together with DKMS - yep, that should be the way to go. Would you be up to adding those when you're ready to package?

You could change the owner of these sysfs files but as you can tell it's not a good idea for security reasons.

Totally. No, the udev+group approach is much more sane. And IMHO the group needs to be something separate, not general things like wheel.

@wasakakero
Copy link

@aymanbagabas I just wanted to check this "by the book", your solution to change values using:

echo 1 | sudo tee /sys/devices/platform/huawei-wmi/FILE

Always throws me an invalid argument with "tee" as shown here:

[wasakakero@localhost ~]$ echo 50 | sudo tee /sys/devices/platform/huawei-wmi/charge_stop_threshold
50
tee: /sys/devices/platform/huawei-wmi/charge_stop_threshold: Invalid argument

@aymanbagabas
Copy link
Owner Author

I've updated the driver hopefully it resolves this issue. Please take your time testing it while monitoring dmesg -w for any error messages and if you could, try playing with these options

  1. set/get charging thresholds
  2. set/get fn lock

Bear in mind that I've combined the two attributes responsible for charging into one.
Thank you!

@wasakakero
Copy link

@aymanbagabas At least for my device, it seems to be working.

Micmute led still doesn't turn on/off (in case the fix in #7 is included in this update)

FN Lock State I can confirm works perfectly fine with both 0 and 1 values.

I was in 100% and set the threshold to 0-80, I'm plugged in but discharging, I will keep playing with the values and see if anything doesn't work as intended.

if it helps you at all, here is the dmesg of when I loaded the driver:


[12560.678826] huawei_wmi: loading out-of-tree module taints kernel.
[12560.678884] huawei_wmi: module verification failed: signature and/or required key missing - tainting kernel
[12560.679316] start init
[12560.679354] done pf driver
[12560.679410] done pf dev
[12560.679470] input: Huawei WMI hotkeys as /devices/platform/PNP0C14:00/wmi_bus/wmi_bus-PNP0C14:00/ABBC0F5C-8EA1-11D1-A000-C90629100000/input/input14
[12560.680047] done wmi dri
[12560.680077] finish init

@nekr0z
Copy link

nekr0z commented Apr 24, 2019

@aymanbagabas First of all, on MateBook 13 it works. I mean, everything works as expected. Tested fnlock, tested various thresholds.

I hope these lengthy dmesg outputs I see appearing upon every interaction (both gets and sets) are just for debugging and will be switched off in release, right?

[45420.151322] wmi exec 0x3 0x11 0x0 0x0
[45420.151325] wmi exec 0x00001103
[45420.151769]  Buffer:
[45420.151771]  0x0
[45420.151772]  0x0
[45420.151773]  0x28
[45420.151773]  0x46
[45420.151774]  0x0
[45420.151775]  0x0
[45420.151776]  0x0
[45420.151777]  0x0
[45420.151778]  0x0
[45420.151779]  0x0

But no, no errors or warnings in dmesg. Great work, you're awesome!

@wasakakero
Copy link

So I played with some values.

0-80= still keeps discharging below 80
75-80 = "plugged in, not charging" (which I believe is intended)
70-90 = same as above.

If doing ranges like 70-90, 40-70 and so on (which I believe is how they were presented in PC Manager) is the intended use case, then yes, it is working as intended no problem.

hope I could be of help!

Thank you for your hard work!

@aymanbagabas
Copy link
Owner Author

I hope these lengthy dmesg outputs I see appearing upon every interaction (both gets and sets) are just for debugging and will be switched off in release, right?

Of course! There just there for debugging. Mainly to see returned buffer from bios since each model has a different implementation.

@aymanbagabas
Copy link
Owner Author

So I played with some values.

0-80= still keeps discharging below 80

The thing is, setting the thresholds to 0-100 disables it so I would assume that it's not working because you have your low value set to 0. Try 1-80 and see if anything changes. Generally, avoid setting the low value to 0 and high to 100.

@wasakakero
Copy link

@aymanbagabas just tried 1-80, it indeed works.

@nekr0z
Copy link

nekr0z commented Apr 25, 2019

I updated my UI applet to work with the new /sys/devices/platform/ interface, so it should also work on other MateBooks that are supported by Huawei-WMI. Testing would be much appreciated.

@nekr0z
Copy link

nekr0z commented Apr 25, 2019

By the way, there's one thing I've been noticing; I'm not sure where this bug is rooted. Every time I set fnlock state, no matter on or off (either by setting register directly, or by Huawei-WMI interface), my keyboard layout switching stops working, and I need to reapply settings in KDE again (I believe KDE relies on xkb in that regard)...

@wasakakero
Copy link

@nekr0z I'm more than happy to test the applet, but I notice the script it is based on (according to the readme) is not the solution discussed here, I take it is just outdated, would it be possible for you to explain me how to actually get it working?

Would you also mind telling me what do you mean by keyboard layout switching and how you do it? I'm also on KDE and I can check if my installation also suffers from it.

@nekr0z
Copy link

nekr0z commented Apr 25, 2019

but I notice the script it is based on (according to the readme) is not the solution discussed here

I'm writing a readme update right now, will have new readme and new packaged release packaged in 10-15 minutes ;)

Would you also mind telling me what do you mean by keyboard layout switching and how you do it?

It's the thing you do when you need to type in languages that don't use latin letters (such as Hebrew, Chinese, Russian etc). In this case you usually have several (well, at least two, for your language and English) keyboard modes (called 'layouts') and some hotkey to switch between them (most common are Alt+Shift (Windows way), Meta+Space (MacOS way), I personally prefer CapsLock). All these settings are available in KDE System Settings -> Input Devices -> Keyboard.

I found that toggling Fn-Lock messes with layouts system, making it impossible to switch layouts until I go to settings (where everything looks just fine), change something, change it back again and "Apply".

@aymanbagabas
Copy link
Owner Author

By the way, there's one thing I've been noticing; I'm not sure where this bug is rooted. Every time I set fnlock state, no matter on or off (either by setting register directly, or by Huawei-WMI interface), my keyboard layout switching stops working, and I need to reapply settings in KDE again (I believe KDE relies on xkb in that regard)...

I have my layout switch set to Super+Space and it works fine. Now I have changed it to CapsLock, and it worked fine except that when I change layout it turns the capslock on so I get either always caps letters or always lower ones. I'm running Gnome, it might be KDE specific, idk.

@nekr0z
Copy link

nekr0z commented Apr 25, 2019

I'm running Gnome, it might be KDE specific, idk.

KDE is a little different in that it has two separate systems for layout changing. One is KDE proper, and that one sets CapsLock as you described, another one is I believe somewhat rooted in X.org ways of handling this, and that one automatically remaps CapsLock behaviour to Shift+CapsLock if CapsLock is used for layout switching (the same as in the good old days ten years ago when we set all this behaviour somewhere in xorg.conf). Anyway, both these systems occasionally lose layout switching altogether, and that is definitely connected with Fn-Lock switching in some way, but I have yet to nail it down to reproduce reliably.

@aymanbagabas
Copy link
Owner Author

@nekr0z So there is an edge case I just stumbled on after I used your applet. I just noticed that on my model setting the battery to 0-100 doesn't particularly set the thresholds. Remember the 0xc0 that corresponds to the mode being on/off. That's what it's doing in my model. Matebook D is not affected, it actually sets whatever values given even if they were off limit which is kinda interesting.

Here is the code from DSDT

    Method (SBTT, 1, NotSerialized)
    {
        Name (BUFF, Buffer (0x0100){})
        Local0 = Arg0
        CreateByteField (Arg0, 0x02, STCP)
        CreateByteField (Arg0, 0x03, SOCP)
        CreateByteField (BUFF, Zero, STAT)
        If (((STCP == Zero) && (SOCP == 0x64)))
        {
            \_SB.PCI0.LPCB.EC0.ECXT (0xC7, Zero, Zero, Zero, Zero, Zero)
        }
        Else
        {
            \_SB.PCI0.LPCB.EC0.ECXT (0xC7, One, STCP, SOCP, Zero, Zero)
        }

        STAT = Zero
        Return (BUFF) /* \SBTT.BUFF */
    }

When it gets 0-100, it doesn't set the thresholds values, instead it turns it off. Notice STCP & SOCP are low/high thresholds.

I guess a workaround for this is to check the values if they're this case 0-100, it sets 0 and 100 in two separate calls. This would eliminate the problem of getting low/high values even though it's disabled.

@nekr0z
Copy link

nekr0z commented Apr 26, 2019

What the applet is currently doing is interpret 0-x (where x<=100) as protection off, and setting 0-100 when asked to switch the protection off. Is this sane behaviour for this edge case, or is there something I need to change?

@aymanbagabas
Copy link
Owner Author

What the applet is currently doing is interpret 0-x (where x<=100) as protection off, and setting 0-100 when asked to switch the protection off. Is this sane behaviour for this edge case, or is there something I need to change?

Exactly, 0-100 is sane behavior. But with my machine, 0-100 doesn't change the thresholds, it switches the protection off. My workaround was to set the thresholds to 0-0 before setting it to 0-100. But for some reason, this doesn't work unless I have a delay between the two functions. The first call to the function gets ignored it might be something with scheduling, I have to dig in more.

@nekr0z
Copy link

nekr0z commented Apr 26, 2019

My workaround was to set the thresholds to 0-0 before setting it to 0-100. But for some reason, this doesn't work unless I have a delay between the two functions.

Yeah, from what you say it looks like it should better be handled by the driver. Alternatively, you could add some marker for your model in /sys so I could catch the fact we're running on this model and introduce double switching and additional checks in the applet, but that looks dirty. Would be better if you found the way to get this on the driver level.

Keep me posted one way or the other, and keep up awesome work!

@aymanbagabas
Copy link
Owner Author

aymanbagabas commented Apr 27, 2019

Yeah, from what you say it looks like it should better be handled by the driver. Alternatively, you could add some marker for your model in /sys so I could catch the fact we're running on this model and introduce double switching and additional checks in the applet, but that looks dirty. Would be better if you found the way to get this on the driver level.

Keep me posted one way or the other, and keep up awesome work!

That's probably what's happening, according to this.

Timing issues - e.g. interacting with the Embedded Controller, mis-timed read/writes. AML may execute correctly in Windows but not in Linux because of the different speed AML operations are being executed in different host operating systems.

Used schedule_delayed_work and mutex to workaround this. Agh, it took a while!
Please test it before I push it to master. Thank you!
huawei-wmi.zip

@nekr0z
Copy link

nekr0z commented Apr 27, 2019

huawei-wmi.c:530:14: error: ‘cmd_mutex’ undeclared (first use in this function); did you mean ‘wmi_mutex’?
  mutex_init(&cmd_mutex);

@aymanbagabas
Copy link
Owner Author

aymanbagabas commented Apr 27, 2019 via email

@nekr0z
Copy link

nekr0z commented Apr 27, 2019

With that line removed it compiles and works. The only caveat I see is that setting battery protection to OFF takes some time (but happens eventually), so that the applet updates BP status (top of the menu) before it is set, so it's displaying it wrong. Do you have a guesstimate of how long a delay is generally expected, so I could put a sleep call in place?

@aymanbagabas
Copy link
Owner Author

With that line removed it compiles and works. The only caveat I see is that setting battery protection to OFF takes some time (but happens eventually), so that the applet updates BP status (top of the menu) before it is set, so it's displaying it wrong. Do you have a guesstimate of how long a delay is generally expected, so I could put a sleep call in place?

I don’t have an estimated time but I would say 500ms to 1s is more than enough since reading the file many times generates a lot of interrupts. That’s actually the problem the driver has with this case. You could read the status only once when the applet is accessed.

@nekr0z
Copy link

nekr0z commented Apr 27, 2019

You could read the status only once when the applet is accessed.

I'd rather not generate reads every time applet is accessed (and it is not trivial with the library I use anyway), so I'll go the sleep way. From what you're saying I gather that up to five reading attempts with 500 ms interval should be not too much taxing.

@nekr0z
Copy link

nekr0z commented Apr 27, 2019

with 500 ms interval

Did some testing, never got thresholds 0-100 set in under 850 ms, so 900 ms interval it is. Commit in master now.

@aymanbagabas
Copy link
Owner Author

with 500 ms interval

Did some testing, never got thresholds 0-100 set in under 850 ms, so 900 ms interval it is. Commit in master now.

That's actually a problem. The driver should be able to handle multiple calls simultaneously without skipping any call. I'll work on implementing some sort of a wait queue for any call with a short delay after each call. The applet shouldn't worry about any delays because that's the driver's responsibility.

@nekr0z
Copy link

nekr0z commented Apr 27, 2019

Makes sense. I'll revert the commit as soon as you sort it out, leave it in for now just in case you don't. You've done impressive work anyway, I think I'm speaking for all the community when I say I admire it.

@aymanbagabas
Copy link
Owner Author

Thank you! I wouldn't get this far if it wasn't for you guys.

@aymanbagabas
Copy link
Owner Author

@nekr0z please try this one. The driver now has a workqueue that executes commands in a FIFO order.
huawei-wmi.zip

Turning BP off always has a delay. Now when you think about it, the user would not stay and watch the values get updated in the file. In this case, a short delay wouldn't matter and is not noticeable.

So the way I see it with the applet is to get the status once the user accesses the applet. Once an item is clicked, the menu should disappear and a command is executed. So the applet would get the status only once at the beginning.
appletClick -> getStatus -> showMenu
itemClick -> hideMenu -> executeCmd

@nekr0z
Copy link

nekr0z commented Apr 28, 2019

So the way I see it with the applet is to get the status once the user accesses the applet. Once an item is clicked, the menu should disappear and a command is executed. So the applet would get the status only once at the beginning.
appletClick -> getStatus -> showMenu
itemClick -> hideMenu -> executeCmd

Yes, I got the idea several comments ago. The problem is the library I'm using for system tray is a very basic one and doesn't have appletClick events, only itemClick ones.

The driver now has a workqueue that executes commands in a FIFO order.

That's for sets, not gets, right? Because what I'm seeing is

$ cd /sys/devices/platfurm/huawei-wmi/
$ cat charge_thresholds
0 100

$ echo "40 70" | sudo tee charge_thresholds && cat charge_thresholds && sleep 1 && cat charge_thresholds
40 70
0 100
40 70

That 0 100 immediately after setting is what I'm having issues with.

@nekr0z
Copy link

nekr0z commented Apr 29, 2019

That 0 100 immediately after setting is what I'm having issues with.

@aymanbagabas Just wanted to make things clear: while obviously disturbing, this issue is far from being a show-stopper, and I have already implemented a workaround in the applet, so if that's the only thing stopping you from making a release, just table it ;)

@aymanbagabas
Copy link
Owner Author

That 0 100 immediately after setting is what I'm having issues with.

@aymanbagabas Just wanted to make things clear: while obviously disturbing, this issue is far from being a show-stopper, and I have already implemented a workaround in the applet, so if that's the only thing stopping you from making a release, just table it ;)

I'm trying to get around this using a different/simpler approach. The kernel API has an EmbeddedController API, I'm trying to use that instead of the dirty workqueue approach. I still have to look more into that and I'm not sure if it's doable this way. The problem with the current approach is, imagine two calls came at the same time, which one is going first? Using EC API we could eliminate potential errors coming from this issue.

@nekr0z
Copy link

nekr0z commented Apr 29, 2019

You obviously know these things much deeper than I do, so go ahead and do your magic whichever way you deem proper! ;)

@aymanbagabas
Copy link
Owner Author

So I've come to conclude that the driver should be as simple as possible. This means no workqueues, waitqueues, or anything else. This problem only happens with my model (MACH-WX9), right? Still setting it to 0-100 disables BP but it doesn't set these values. This could be fixed using driver quirks which is not implemented yet. Or a possible route is to patch DSDT table but that's not really a solution.

Right now, I'll open a new issue for that one and close this since these features are implemented in v3. @nekr0z you shouldn't worry about this issue in matebook-applet just keep it simple and if reading from the driver gives false values then it's the driver's fault.

Thank you for your awesome work @nekr0z @wasakakero

@nekr0z
Copy link

nekr0z commented May 2, 2019

@aymanbagabas Awesome work, thank you again! I think I'll make waiting for changes to be reported back optional in the applet, so that it can be switched on with a command line flag, since the code is in there anyway. I'll keep following you work in case changes to the applet need to be made in future, and of course feel free to ping me if you need any testing or anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants