Skip to content

Fix thinkpad_function_keys feature #755

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrhpearson
Copy link

The thinkpad_function_keys feature is incorrectly named, so rename it to sysfs_acpi_monitor to be more correct.

It is also a useful feature - if profiles are updated on Fedora 41 outside of tuned, then they are not reflected in tuned or gnome-settings (or likely KDE, though I didn't test that). Enable the feature by default.

Tested on multiple devices. As a note - I expect this feature to be useful to any vendor supporting platform profiles, not just Thinkpads (though it does mean our function keys work correctly again which is nice)

Thanks
Mark

The thinkpad_function_keys feature really doesn't have anything to
do with Thinkpad function keys. It is just monitoring for sysfs events
from firmware/acpi/platform_profile

Rename the feature to be more accurate. It is useful to other platforms
than Thinkpads :)

Signed-off-by: Mark Pearson <[email protected]>
@zacikpa
Copy link
Contributor

zacikpa commented Mar 3, 2025

Thanks, @mrhpearson.

While I agree that the original name was poorly chosen, some users are already using it (see #710), so we should at least make the old name an alias of the new one.

@mrhpearson
Copy link
Author

Hi @zacikpa

Good point - though I'd argue that as the 2nd commit enables the feature by default the user won't see any impact will they? The feature will continue working for them correctly.
The issue would only be if anybody was disabling the feature in ppd.conf, but I doubt anybody is doing that.

Happy to change the commits if needed, just not sure if it's actually necessary. Let me know if I'm missing something.

Thanks for the review!
Mark

@zacikpa
Copy link
Contributor

zacikpa commented Mar 4, 2025

The problem is that the configuration file is defined as "noreplace" in the spec file, and thus it won't be replaced if the user has already made changes to it. I believe it works similarly in other distros.

@mrhpearson
Copy link
Author

Right - is that a problem? You have a config file with an extra useless line in it is all.

I don't want to make more of this than is needed, so I'm happy to remove the naming change if that's preferred, and just enable the feature by default. Just figured it was a good opportunity to make it a bit more accurate.

I'll get that ready but just wanted to be sure it was really required.

Copy link
Contributor

@zacikpa zacikpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Please update as requested and this should be good to go.

@zacikpa
Copy link
Contributor

zacikpa commented Mar 10, 2025

Thanks! If you could just squash (fixup) the last change into the second commit, that would be perfect.

If the platform profile is changed outside of tuned then the update
does not get reflected in tuned-adm and gnome-settings.
This is particularly noticeable on Thinkpads where the F8 Mode key
changes the profile but the change is not reflected. It can also be
demonstrated by changing the profile from the command line.

Enable the feature by default. It works well and is useful.

Tested on multiple Thinkpads, but I expect it to be useful to other
vendor platforms too.

Signed-off-by: Mark Pearson <[email protected]>
@mrhpearson mrhpearson force-pushed the fix_thinkpad_fn_keys branch from b3aa029 to bd5f285 Compare March 10, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants