-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
tuxedo-keyboard: fix compilation for kernel 6.10 and 6.11 #336633
base: master
Are you sure you want to change the base?
Conversation
Please change the commit message format to fit the guidelines in CONTRIBUTING.md |
Also, it may be worth renaming the package to fit its presumably now extended scope? |
66c5da9
to
19cd864
Compare
I also renamed the related config options to the new scope of the tuxedo driver. I hope this is ok. |
There are already commit in the new upstream repo to fix compile errors with kernel 6.11. Thus, I will most likely provide a follow up pull request the coming days to update this driver to version 4.6.3. |
pkgs/top-level/linux-kernels.nix
Outdated
@@ -488,7 +488,7 @@ in { | |||
|
|||
rust-out-of-tree-module = if lib.versionAtLeast kernel.version "6.7" then callPackage ../os-specific/linux/rust-out-of-tree-module { } else null; | |||
|
|||
tuxedo-keyboard = if lib.versionAtLeast kernel.version "4.14" then callPackage ../os-specific/linux/tuxedo-keyboard { } else null; | |||
tuxedo-driver = if lib.versionAtLeast kernel.version "4.14" then callPackage ../os-specific/linux/tuxedo-driver { } else null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you renamed the package here, could you add an alias to block at line 608, in-case someone didn't use the module.
Something like this:
tuxedo-keyboard = self.tuxedo-drivers;
f5a12de
to
a7b81bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still works on my Pulse Gen 1.
Now it also runs with kernel 6.11 on my Aura 15 Gen1. |
c3395c6
to
22dba23
Compare
@blanky0230 What do you think? As you are the maintainer could you review my changes please so it's clear to committers in what state this is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hate to nitpick catching up with reviews this late but: Since the manufacturer's repo is named "tuxedo-drivers" (plural), I think the corresponding linuxPackage and nixosModule in nixpkgs should share the same name in order to minimize the possibility for mix-ups and user's confusion in the future.
Other than that this PR is looking good to me 👍
doesn't build for me:
I just updated my flake (on unstable) to the latest version and copied tuxedo-drivers/default.nix into boot.extraModulePackages = [
(config.boot.kernelPackages.callPackage ./tuxedo-new.nix { })
]; |
@WhiteBlackGoose. I tested you method myself and it compiles. Your error message still mentions In my {
inputs = {
nixpkgs-master = {
type = "github";
owner = "NixOS";
repo = "nixpkgs";
ref = "master";
rev = "c5989b0173fa669297d1f92cee201cf77aa6c3eb";
};
};
outputs = {
nixpkgs-master,
}@inputs: {
nixosConfigurations = {
"myMachineName" = nixpkgs-master.lib.nixosSystem {
};
};
};
} |
I added the config as @Keksgesicht mentioned in his comment. Build works fine, however somehow I still don't see any backlight on my keyboard. I also have this
|
Why haven't you tried using Also, did you reboot after a |
I would love to help testing this. Is there a way to enable this without setting all of my main config to this branch? |
@chmanie disable the old Then try to something like that to your config: { config, ... }:
let
cfgLinuxKernel = config.boot.kernelPackages;
tuxedo-pkg-file = pkgs/os-specific/linux/tuxedo-drivers/default.nix;
tuxedo-drivers = cfgLinuxKernel.callPackage tuxedo-pkg-file {};
in
{
boot.kernelModules = [ "tuxedo_keyboard" ];
boot.extraModulePackages = [ tuxedo-drivers ];
} An alternative would be to clone your current nixpkgs commit and rebase the changes of this PR onto it. Then use this version of nixpkgs for your machine config. Just add the local path to your input. Another thought that I had is to only use the package from the commit of this PR, but this does not work as the kernel minor version already changed over the time: { inputs, system, ... }:
let
pkgs-master = import inputs.nixpkgs-master { # see a few messages above
inherit system;
};
cfgLinuxKernel = pkgs-master.linuxKernel.packages.linux_6_11;
tuxedo-drivers = cfgLinuxKernel.tuxedo-keyboard;
in
{
boot.kernelModules = [ "tuxedo_keyboard" ];
boot.extraModulePackages = [ tuxedo-drivers ];
} |
I also did try
Yes both use the switch parameter and then reboot |
@papanito does this show any file: realpath /run/*-system/kernel-modules/lib/modules/*/updates/src/tuxedo* or (this is only the case before the latest commit where the build commands changed) realpath /run/*-system/kernel-modules/lib/modules/*/tuxedo* Also look whether your kernel loaded the modules: lsmod | grep tuxedo Report what is not the case. Did it work with an LTS kernel and the older |
It never worked on my InfinityBook, but I assumed also maybe cause the package seemed very outdated.
|
@papanito do you have something like this? /sys/devices/platform/tuxedo_keyboard/leds/rgb:kbd_backlight Try changing echo "51" | sudo tee /sys/devices/platform/tuxedo_keyboard/leds/rgb:kbd_backlight/brightness
echo "0 255 0" | sudo tee /sys/devices/platform/tuxedo_keyboard/leds/rgb:kbd_backlight/multi_intensity |
My tuxedo has
hence I can see
But when I do this
The keyboard still shows no light :-( |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/installing-tuxedo-keyboard-drivers/40161/7 |
If does not create any error. Not even in the log (see |
Thanks @Keksgesicht for your time and effort. I probably have to talk to them yes.... |
@Keksgesicht thank you, I tested it the way you described. Here are some logs, not sure if this is expected or not (Tuxedo InfinityBook 14 Gen8):
And testing tuxedo-rs
|
Lol I feel a bit stupid, but actually it works once I enable the backlight using |
@chmanie looks fine so far. You could test if the For the failed temperature reading I am not sure whether this driver or a generic one for the Intel or AMD platform provides this sensor. Try running |
Co-Authored-By: April John <aprl@acab.dev> Co-Authored-By: Dominik Xaver Hörl <hoe.dom@gmx.de> thanks to the work of: - NixOS#293017 - NixOS#343483
So, I rebased and fixed a merge conflict because someone added some system76 things above of my changes in Is there something else I have to do? How do I get the attention of a committer to merge this? |
The |
Description of changes
It seems like the tuxedo-keyboard has moved to GitLab a little time ago. Thus, I changed the source git the new GitLab URL and updated the version number. This also fixes a bug when compiling with Kernel 6.10.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.