johnfanv2 / LenovoLegionLinux

Driver and tools for controlling Lenovo Legion laptops in Linux including fan control and power mode.
https://github.com/johnfanv2/LenovoLegionLinux
GNU General Public License v2.0
1.52k stars 55 forks source link

Architecture: Implement background Service legiond in Python #216

Open johnfanv2 opened 4 months ago

johnfanv2 commented 4 months ago

Background Service Legiond

Recently, a background service legiond was added by @st0nie with the help of @MrDuartePT . Thank you for your great work and the huge effort that was needed to implement it all in C.

However, I propose implementing legiond in Python. There are the following reasons for it.

Benefits of Python

Reusing existing Code and Single Source of Truth

Legiond currently implements the same code as in Python. So the logic is now in two places. Reading the power mode is e.g. implemented in powerstate.c and also in the Python class PlatformProfileFeature( https://github.com/johnfanv2/LenovoLegionLinux/blob/9e8482d76c54a45fe6ec56076b7fbf4fa08b3de2/python/legion_linux/legion_linux/legion.py#L498). Note that these two features sometimes ready different values (PState vs. ACPI Platform Profile). So it would be better to organize it in one place (Single Source of Truth).

Better Integration with Existing Python Code

We can do a lot more using the features that are already implemented in the existing Python code, e.g. we can add more config values to the config file. We could add everything that can be configured in the GUI/CLI easily to the config file.

Easier in Python

Downsides of Python

Performance and Memory Consumption

The C program might be faster but still relies on the legion_cli. So it is not faster if you consider the whole task. Though, I this does not matter since the program does not do work continuously. It just performs a task, e.g. every minute. However, the C legiond program definitely uses less memory then a Python implementation. This is relevant for a background service.

Conclusion

All in all, I suggest converting the legiond to Python using the existing code as a basis before we add any more features to it.

What are your thoughts about these ideas?

st0nie commented 4 months ago

Yeah, legiond was originally intended to replace the fancurve-set script (which was really, really slow and has no timer support).

It works well, at least for this purpose, converting the most time-consuming process of using legion_cli to non-blocking, as well as simpler configuration (at least for the user, that is)

The current legiond still works fine and remains low on resources when you press fn+q hysterically. This is actually the main issue I want to deal with.

st0nie commented 4 months ago

I actually think that this daemon may not have any more functionality that can still be added, except for fn+r.

Maybe the effort put into converting to python code wasn't quite worth it? But I'm in favor of reducing code that has duplicate functionality, if it's done without sacrificing efficiency (powerstate.c is still fast when not using ppd)

MrDuartePT commented 4 months ago

I actually think that this daemon may not have any more functionality that can still be added, except for fn+r.

The fn+r will be a bit challenged I will have to give a look at the ideapad_driver on the Linux kernel and create a patch to add the missing Lenovo button (the missing button are fn+r, fn+printscr and fn+start if I not missing any)

Maybe the effort put into converting to python code wasn't quite worth it? But I'm in favor of reducing code that has duplicate functionality, if it's done without sacrificing efficiency (powerstate.c is still fast when not using ppd)

Maybe what can be done is creating a legion_cli (if doesn’t already exist) called legion_cli profile_set that set the correct power profile for the state of charger and power profile.

@st0nie I also think would be better to read the /sys/firmware/acpi/platform_profile instead of calling powerprofilesctl in powerstate.c since any user could decide to not have power-profile-daemon install

MrDuartePT commented 4 months ago

The current legiond still works fine and remains low on resources when you press fn+q hysterically. This is actually the main issue I want to deal with.

I would agree at least for daemon to still in C. I don't know if the daemon will react weirdly in that situation if written in python.

The last time I checked daemon written in python tend to be more were kinda slow when use with a timer

st0nie commented 4 months ago

@st0nie I also think would be better to read the /sys/firmware/acpi/platform_profile instead of calling powerprofilesctl in powerstate.c since any user could decide to not have power-profile-daemon install

Current behavior is to use ppd only when /sys/firmware/acpi/platform_profile is unavailable. Using ppd is much less efficient and does not support balanced-performance mode.

I don't think it's a major problem.

johnfanv2 commented 4 months ago

It works well, at least for this purpose, converting the most time-consuming process of using legion_cli to non-blocking, as well as simpler configuration (at least for the user, that is)

Very interesting insight @st0nie . Great work!

  1. Why is legion_cli called so often? How otten is it called and how slow is legion_cli?
  2. Very good idea making the call to legion_cli non blocking. Is this needed because of systemd? How did you do it? Is this done by seperating it in legiond-cli and legiond?

The current legiond still works fine and remains low on resources when you press fn+q hysterically. This is actually the main issue I want to deal with.

Interesting. So the problem is that if we press fn+q with high frequency, we get throttled by systemd? Was the solution to convert it in two program legiond-cli and legiond?

johnfanv2 commented 4 months ago

Maybe what can be done is creating a legion_cli (if doesn’t already exist) called legion_cli profile_set that set the correct power profile for the state of charger and power profile.

Good idea, @MrDuartePT . What do you think what properties should be set? All from legiond?

johnfanv2 commented 4 months ago

The last time I checked daemon written in python tend to be more were kinda slow when use with a timer

Interesting! Was this with a systemd timer or python timer?

MrDuartePT commented 4 months ago

Maybe what can be done is creating a legion_cli (if doesn’t already exist) called legion_cli profile_set that set the correct power profile for the state of charger and power profile.

Good idea, @MrDuartePT . What do you think what properties should be set? All from legiond?

My ideia is that legiond should be maintain how it is, what it can be done to simplify is to create a command for legion_cli to automatically set the fan profile (depending of the ac state and power mode):

1 - Make the daemon simple 2 - The profile names are hardcoded

This way would reduce complexity, (well the parser will still exist to set the cpu control and gpu control, but that feature are separate from legion_cli).

Also I made that addition because Power Option tab on legion_gui only seem to work on custom mode. At least cpu_control and gpu_control (at least for amd, nvidia disable tdp control on newer driver) work outside of custom mode, and can accept any cli application to control the parameters.

The last time I checked daemon written in python tend to be more were kinda slow when use with a timer

Interesting! Was this with a systemd timer or python timer?

For my knowledge with any init system any python daemon with timer, not work really well and are more heavy in resource that a C daemon.

What you think as well @st0nie

MrDuartePT commented 4 months ago

Also one more thing @st0nie @johnfanv2 I also thinking changing legiond acpi files to udev instead, since udev is a hard required for many system instead of acpid.

For exemple TLP uses udev to check if the charger was unplugged or not, that way rerunning the tlp auto script: https://github.com/linrunner/TLP/blob/main/tlp.rules.in

st0nie commented 4 months ago

Also one more thing @st0nie @johnfanv2 I also thinking changing legiond acpi files to udev instead, since udev is a hard required for many system instead of acpid.

But how can we handle fn+q ? A dirty solution might be let the kmod call legionctl directly

MrDuartePT commented 4 months ago

Also one more thing @st0nie @johnfanv2 I also thinking changing legiond acpi files to udev instead, since udev is a hard required for many system instead of acpid.

But how can we handle fn+q ? A dirty solution might be let the kmod call legionctl directly

Yes you a right I forget udev can't check for keypress

Let scratch the udev ideia

MrDuartePT commented 1 month ago

what you think @st0nie (4cdcc02) Also @johnfanv2 I feel like we can close #216 besides the refresh rate control not being implement yet the daemon now only uses interfaces expose by the driver and ibinih (for reading the config file). So implementing in python at this point would be double effort.