search

skraus-dev / cherryrgb-rs

Cherry RGB Keyboard - Multi platform tool to set RGB LEDs for Cherry keyboards
MIT License
41 stars 3 forks source link

Special keys no longer work after running cli #22

Open mpldr opened 1 year ago

mpldr commented 1 year ago

It seems the keys are "removed" from the keyboards firmware as no key events are sent at all.

After factory reset:

# libinput debug-events
 event2   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group4  cap:k
-event4   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group4  cap:k
-event6   DEVICE_REMOVED          CHERRY CHERRY Keyboard Mouse      seat0 default group4  cap:p
-event6   DEVICE_ADDED            CHERRY CHERRY Keyboard Mouse      seat0 default group7  cap:p left scroll-nat scroll-button
-event4   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group7  cap:kp scroll-nat
-event2   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group7  cap:k
-event4   KEYBOARD_KEY            +12.178s  KEY_VOLUMEDOWN (114) pressed
 event4   KEYBOARD_KEY            +12.356s  KEY_VOLUMEDOWN (114) released
 event4   KEYBOARD_KEY            +13.150s  KEY_MUTE (113) pressed
 event4   KEYBOARD_KEY            +13.311s  KEY_MUTE (113) released
 event4   KEYBOARD_KEY            +13.756s  KEY_MUTE (113) pressed
 event4   KEYBOARD_KEY            +13.895s  KEY_MUTE (113) released
 event4   KEYBOARD_KEY            +14.446s  KEY_VOLUMEUP (115) pressed
 event4   KEYBOARD_KEY            +14.609s  KEY_VOLUMEUP (115) released
 event4   KEYBOARD_KEY            +15.167s  KEY_CALC (140) pressed
 event4   KEYBOARD_KEY            +15.329s  KEY_CALC (140) released
 event4   KEYBOARD_KEY            +19.792s  KEY_PLAYPAUSE (164) pressed
 event4   KEYBOARD_KEY            +19.936s  KEY_PLAYPAUSE (164) released
 event4   KEYBOARD_KEY            +20.253s  KEY_PLAYPAUSE (164) pressed
 event4   KEYBOARD_KEY            +20.418s  KEY_PLAYPAUSE (164) released

after running cherryrgb_cli --product-id 223 --brightness full animation wave slow 500475

# libinput debug-events
-event2   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group4  cap:k
-event4   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group7  cap:k
-event6   DEVICE_ADDED            CHERRY CHERRY Keyboard Mouse      seat0 default group7  cap:p left scroll-nat scroll-button
-event2   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group7  cap:k

Used Keyboard: Cherry MX 10.0n RGB

skraus-dev commented 1 year ago

Thanks for the report.

17 - this might be responsible for overwriting the special keys on keyboard init.

Needs further investigation and research on what these commands actually trigger/set.

mpldr commented 1 year ago

Happy to help with debugging, not much of a rustacean though, so I probably can't help with providing code :)

luv4bytes commented 1 year ago

Could be worth a try to comment out the state fetching and try to set the LEDs without this. This would tell us if it's really the state fetching that "removes" the keys.

mpldr commented 1 year ago

No change when removing the state fetching.

luv4bytes commented 1 year ago

You could try commenting out individual payloads in the function that sets the animation as a next step. I mean it's got to be somewhere. If that doesn't give us the result we want, I'm pretty much clueless.

felfert commented 1 year ago

Confirmed. I just noticed this issue and since I have the same keyboard (I thought see below), I tried it out immediately. I still use the old version 0.2.1 (with my PR applied) so it's not something of the newer changes that have been committed since. Here, the four special keys above the Numeric block (Vol-, Mute, Vol+, Calc) are not working. I just never noticed, because I never used these before. To be shure, I tested all other keys as well and they do work.

What is strange though: Where is this key (KEY_PLAYPAUSE) on your keyboard? My keyboard does not have that. So there might be subtle differences between keyboard models named "MX 10.0N" Also: What layout does your keyboard have? (Mine is german.)

Also: A simple unplug/plug of the keyboard brings back those 4 keys to life. No special factory reset necessary.

Cheers -Fritz

mpldr commented 1 year ago

Mine is also german. KEY_PLAYPAUSE is sent by Fn+F11 and indeed, replugging brings them back. Though that's hardly an optimal solution.

felfert commented 1 year ago

Found it and fixed it here :smiley:

The reason is: The kernel_driver gets detached before claiming the device, but never gets attached again, after the program is finished.

In fact, instead of implicit calling detach_kernel_driver(), libusb1 provides a function that handles everything automatically for us. the rusb signature is:

.set_auto_detach_kernel_driver(boolean)

Expect a PR soon ...

Cheers -Fritz

skraus-dev commented 1 year ago

Failed to check earlier, before merging the PR, but here the report comes:

Notice the KEY_UNKNOWN (240) inbetween each keypress/release.

-event11  KEYBOARD_KEY            +53.944s  *** (-1) released
-event12  KEYBOARD_KEY            +55.367s  KEY_UNKNOWN (240) released
 event12  KEYBOARD_KEY            +55.367s  KEY_UNKNOWN (240) pressed
-event11  KEYBOARD_KEY            +55.367s  *** (-1) pressed
-event12  KEYBOARD_KEY            +55.457s  KEY_UNKNOWN (240) released
 event12  KEYBOARD_KEY            +55.457s  KEY_UNKNOWN (240) pressed
-event11  KEYBOARD_KEY            +55.457s  *** (-1) released
-event12  KEYBOARD_KEY            +60.509s  KEY_UNKNOWN (240) released
 event12  KEYBOARD_KEY            +60.509s  KEY_UNKNOWN (240) pressed
 event12  KEYBOARD_KEY            +60.884s  KEY_UNKNOWN (240) released
 event12  KEYBOARD_KEY            +60.884s  KEY_UNKNOWN (240) pressed
 event12  KEYBOARD_KEY            +60.885s  KEY_UNKNOWN (240) released
felfert commented 1 year ago

I see the additional KEY_UNKNOWN (240) here but I cannot reproduce any slowness.

Note: The timestamps in column 3 are relative to the first displayed event. So if you see the following:

-event4   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group7  cap:k
-event6   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group7  cap:kp
-event8   DEVICE_REMOVED          CHERRY CHERRY Keyboard Mouse      seat0 default group7  cap:p
-event6   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group9  cap:kp scroll-nat
-event4   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group9  cap:k
-event8   DEVICE_ADDED            CHERRY CHERRY Keyboard Mouse      seat0 default group9  cap:p left scroll-nat scroll-button
-event4   KEYBOARD_KEY            +15.458s  *** (-1) pressed
 event4   KEYBOARD_KEY            +15.580s  *** (-1) released
 event4   KEYBOARD_KEY            +16.096s  *** (-1) pressed
 event4   KEYBOARD_KEY            +16.211s  *** (-1) released
 event4   KEYBOARD_KEY            +16.502s  *** (-1) pressed
 event4   KEYBOARD_KEY            +16.630s  *** (-1) released
-event6   KEYBOARD_KEY            +17.668s  KEY_MUTE (113) pressed
 event6   KEYBOARD_KEY            +17.778s  KEY_MUTE (113) released
 event6   KEYBOARD_KEY            +18.383s  KEY_MUTE (113) pressed
 event6   KEYBOARD_KEY            +18.480s  KEY_MUTE (113) released
-event4   KEYBOARD_KEY            +25.143s  *** (-1) pressed
=== here cherryrgb_cli detached the kernel driver
-event6   DEVICE_REMOVED          CHERRY CHERRY Keyboard            seat0 default group9  cap:kp
-event8   DEVICE_REMOVED          CHERRY CHERRY Keyboard Mouse      seat0 default group9  cap:p
-event4   KEYBOARD_KEY            +25.237s  *** (-1) released
=== here cherryrgb_cli re-attached the kernel driver
-event6   DEVICE_ADDED            CHERRY CHERRY Keyboard            seat0 default group9  cap:kp scroll-nat
-event8   DEVICE_ADDED            CHERRY CHERRY Keyboard Mouse      seat0 default group9  cap:p left scroll-nat scroll-button
-event6   KEYBOARD_KEY            +32.309s  KEY_UNKNOWN (240) pressed 
-event4   KEYBOARD_KEY            +32.310s  *** (-1) pressed
-event6   KEYBOARD_KEY            +32.437s  KEY_UNKNOWN (240) released
=== KEY_UNKNOWN was released after 32.437s - 32.309s sec (= 128ms)
 event6   KEYBOARD_KEY            +32.437s  KEY_UNKNOWN (240) pressed
-event4   KEYBOARD_KEY            +32.438s  *** (-1) released
-event6   KEYBOARD_KEY            +32.720s  KEY_UNKNOWN (240) released
 event6   KEYBOARD_KEY            +32.720s  KEY_UNKNOWN (240) pressed
-event4   KEYBOARD_KEY            +32.720s  *** (-1) pressed
-event6   KEYBOARD_KEY            +32.834s  KEY_UNKNOWN (240) released
 event6   KEYBOARD_KEY            +32.834s  KEY_UNKNOWN (240) pressed
-event4   KEYBOARD_KEY            +32.835s  *** (-1) released
-event6   KEYBOARD_KEY            +32.834s  KEY_UNKNOWN (240) released
felfert commented 1 year ago

As you can see, the timestamp differences before running cherryrgb_cli and after are roughly the same (around 120ms).

felfert commented 1 year ago

The additional key might be a side effect of detaching/attaching the kernel driver. What makes me curious: When plugging in, 3 event-sources (usb-enpoints?) are added, but when libusb detaches the driver, only 2 of them are removed and later added. What about the one that was left untouched? Maybe we need to detach/attach this one too? Perhaps I will look at the libusb source when I have some more time (probably next weekend).

skraus-dev commented 1 year ago

As you can see, the timestamp differences before running cherryrgb_cli and after are roughly the same (around 120ms).

I might have phrased it incorrectly - for me it appears like a buffering issue on keyevents. When pressing just a single key, its registered quite fast. when however pressing keys multiple times in a short timespan it takes a while to get registered.

Will see how this behaviour can be logged/ evaluated in a proper fashion.

felfert commented 1 year ago

I did play around some more:

So: short of writing a dedicated kernel driver, I'm afraid we hit a dead end here :-(

luv4bytes commented 1 year ago

Could it be that we need to explicitly release the interfaces? Haven't seen a line that releases them. Or should this be handled by re-attaching the kernel driver?

When I wrote the program for the MX Board 3.0s I had similar issues regarding latency of the keys. I solved it with a factory reset. However one significant difference between the programs I see is the missing release of the interfaces.

But that is just a wild guess.

felfert commented 1 year ago

Could it be that we need to explicitly release the interfaces? Haven't seen a line that releases them. Or should this be handled by re-attaching the kernel driver?

Nope. That is done by libusb. In fact, the re-attaching of the kernel driver is done in its realease function

felfert commented 1 year ago

On more observation: The same KEY_UNKNOWN mess happens, if I fire up my Win10-VM, attach the Keyboard to it, let the CHERRY windows utility configure some colors and then revert the attachment. So this seems to be some generic bug somewhere in the kernel's HID drivers.

luv4bytes commented 1 year ago

That means by specifying the auto attach, detach stuff, we shouldn't need to worry about this, right?

felfert commented 1 year ago

That means by specifying the auto attach, detach stuff, we shouldn't need to worry about this, right?

Exactly

felfert commented 1 year ago

Explanation for the slowness/sluggishness:

libinbut does not display these (artificial key repeat events), but if you use evtest on the corresponding /dev/input/eventX you see the kernel HID driver permanently delivering repeated events of that KEY_UNKNOWN (240). This happens, because (visible in libinput) the last event is always a KEY_UNKNOWN (240) pressed. Obviously this might slow down in some cases (perhaps depending on how fast the upper layers can handle these events E.g. X11 vs. Wayland, KDE Plasma vs. some other DTE etc - you get the idea.

Having arrived at this point, I think there are only 2 ways to solve this:

  1. Find some control frame that fixes this behavior (unlikely unless we get some help from Cherry).
  2. Write a modified HID driver (or patching the existing one) in that simply ignores KEY_UNKNOWN (240) INTERUPT_URBs when dealing with cherry keyboards.

What do you guys think?

felfert commented 1 year ago

Searching for linux HID driver filter keys, I stumbled over this:

https://docs.kernel.org/hid/hid-bpf.html

Apparently, this is very new (kernel 6.3.0-rc7) but it looks promising.

So just out of curiosity:

What kernel versions are you guys running right now?

mpldr commented 1 year ago

I'm on Kernel 6.2.11

skraus-dev commented 1 year ago

Kernel 6.2.9 over here

felfert commented 1 year ago

Oh and reading more of the HID kernel docs, there seems to be a way out of this without the need for a new kernel driver or waiting for 6.3.x kernel:

UHID (Userlevel HID transport drivers) - there is even a rust wrapper for those: https://crates.io/crates/uhid-virt This also comes in handy at a later point when we want to map keys to text strings. Will start playing with this ...

skraus-dev commented 1 year ago

This sounds very close to our scenario: https://lkml.org/lkml/2022/12/24/93

For now only supports one model and only filters out bogus reports sent when the keyboard has been configured through hidraw. Without this, as events are not released, soft repeat floods userspace with unknown key events.

Currently testing this patch, which has not landed in mainline yet.

felfert commented 1 year ago

Currently testing this patch, which has not landed in mainline yet.

Please note, that there is already a drivers/hid/cherry-hid.c which does similar things and is already in the kernel (even with older kernels back to at least 3.x). Maybe this just needs the relevant ProductIDs added and perhaps some small tweak in

Regardless, I would prefer a userland (UHID) driver written in rust. Reasons:

Of course, if we want this, then the project needs some redesign:

Proposal: I will try to implement a standalone POC of the UHID driver (without the rgb stuff, just to figure out how intercepton works) and you can pursue the kernel path.

After that, you can decide which way we go in the future...

What do you think?

CU -Fritz

skraus-dev commented 1 year ago

thanks for the thorough explaination.

I can make the answer short: Your proposal sounds good, let's go this route!

felfert commented 1 year ago

Well, I forgot something: Currently, there are releases for macos and windows. Obviously, UHID is a feature which is available on Linux only. This has some implications:

  1. Any future key remapping feature will only work on a key -> single key basis. Mapping of key -> text won't be available.
  2. This bug (Special keys no longer work after running cli) and my first solution leading to the KEY_UNKNOWN mess might be apply to MacOS as well. So somebody with a real Mac should probaly test v0.2.3 and current master. If this happens on MacOS as well, then we should decide, which of the two behaviors is the lesser evil, as without any driver implementation there cannot be any fix.
  3. I expect windows not to be affected, because there the driver is never detached and the cherry driver should already perform the proper filtering anyway. To be on the safe side however, somebody with a real windows (not a VM) should test this too.
skraus-dev commented 1 year ago
  1. Any future key remapping feature will only work on a key -> single key basis. Mapping of key -> text won't be available.

Imho, key -> key mapping should be the feature-scope here anyway. Pretty much every operating system provides key -> text mappings out-of-the-box. I do not see it as a valuable feature tbh.

  1. This bug (Special keys no longer work after running cli) and my first solution leading to the KEY_UNKNOWN mess might be apply to MacOS as well. So somebody with a real Mac should probaly test v0.2.3 and current master. If this happens on MacOS as well, then we should decide, which of the two behaviors is the lesser evil, as without any driver implementation there cannot be any fix.

Yeah thats interesting to look into for sure. I am still trying to find out the cause.. and especially: why is it causing a major slowdown in key processing for me but not anybody else.

If it's a specific linux-issue, maybe we can get around it via userspace evdev / udev filtering. Got no hard evidence on that matter just yet tho..

mpldr commented 1 year ago

On Fri Apr 21, 2023 at 7:59 PM CEST, Sebastian Kraus wrote:

  1. Any future key remapping feature will only work on a key -> single key basis. Mapping of key -> text won't be available. Imho, key -> key mapping should be the feature-scope here anyway. Pretty much every operating system provides key -> text mappings out-of-the-box. I do not see it as a valuable feature tbh.

I agree. Since F13-F24 are a thing (and even more special keys), there is certainly no shortage of key that can be used to trigger a macro.

  1. This bug (Special keys no longer work after running cli) and my first solution leading to the KEY_UNKNOWN mess might be apply to MacOS as well. So somebody with a real Mac should probaly test v0.2.3 and current master. If this happens on MacOS as well, then we should decide, which of the two behaviors is the lesser evil, as without any driver implementation there cannot be any fix.

Yeah thats interesting to look into for sure. I am still trying to find out the cause.. and especially: why is it causing a major slowdown in key processing for me but not anybody else.

I have also seen a need to replug my keyboard after booting, as logins are not possible in TTY as (what I assume to be) the unknown key is kept being pressed, showing up as ^@ and being actually considered for username and password.

-- Moritz Poldrack https://moritz.sh

skraus-dev commented 1 year ago

Ok lol, I should have done proper research earlier:

Check this out: https://bbs.archlinux.org/viewtopic.php?id=267365

Apparently it's an acknowledged bug in the keyboard firmware!

Temporary workaround

Add this to /etc/udev/rules.d/99-cherryrgb.rules

ACTION=="add", SUBSYSTEM=="input", ATTRS{idVendor}=="046a", ATTRS{idProduct}=="*", ATTR{capabilities/ev}=="10001f", ATTR{inhibited}="1"

This just works around the sluggish input, it does not enable the special keys to work.

Proper fix

Email Cherry support and request the test firmware for your model: https://www.cherry.de/service/kontakt

felfert commented 1 year ago

Sorry but that is all wrong. I have seen this article too, but did not mention it, because there because it does NOT solve the problem. Also it is almost 2 years old and no firmware has been available on cherrys website since then.

  1. In the linked discussion, the udev rule disables the special keys completely. In other words: It creates the situation that caused this bug report by @mpldr. in the first place. The explaining text reads: "Luckily, the keypresses come from one of the USB devices that don't seem to have any valid reason to exist" and that statement is completely wrong in our case (MX 10.0 N RGB) it might be true only for the keyboard model mentioned there: MX Board 3.0S
  2. Same goes for the Firmware. True, probably only for the MX 3.0S. Very likely the various models have different firmwares and therefore the firmware for a MX 3.0S is NOT suitable for a MX 10.0 N RGB
  3. The issue there was also slightly different (worse) in that the UNKNOWN_KEY mess happened even, if the keyboard was disconnected from the Windows machin and then connected to the Linux machine, which - obviously - involved unplugging/plugging. In our case, unplugging (losing power) of the keyboard cures the problem.
felfert commented 1 year ago

BTW my UHID driver POC turned out to work very well. I was really amazed how easy it was to create an UHID driver in rust using the uhid-virt crate. I am now creating a real service daemon which does all I/O including the RGB stuff.

As total rust-noob I currently struggling with parameterizing the thread handlers with the keyboard object of type CherryKeyboard. (The service daemon needs 2 threads (one worker for the UHID driver and another one for a UnixSocket server, which handles IPC from the cli to the service daemon and performs the RGB stuff when bein told so by the cli). Of course access to the keyboard object from the threads will be guarded by a Mutex.

In C++ this would be a std::shared_ptr and I experimented with std::rc:Rc but did not have any luck until now. Well, i learn more every minute ;-)

felfert commented 1 year ago

Ok, I just pushed the branch uhid-service in my fork: https://github.com/felfert/cherryrgb-rs/tree/uhid-service

This is WIP, but it works here nicely, so @mpldr perhaps you can give it a try.

In order to keep backwards compatibility, I have NOT changed cherryrgb_cli. Instead, i have added 2 sub-projects named service and ncli. So in order to build everything, you now must use the --all flag when building. E.g.: cargo build --all This creates 2 new binaries cherryrgb_service and cherryrgb_ncli.

The service should run as root and provides the UHID driver as well as socket server, listening on /run/cherryrgb.sock by default. The cherryrgb_ncli works almost identical to the cherryrgb_cli, except it communicates with the service (using the unix socket). Therefore, it does not have the --product-id option anymore (which has been moved to the service). Instead, it now has an option --socket, which can be used to specify a non-standard socket-path. The rest of the options and commands are identical to cherryrgb_cli.

The service has some related options. Run it with --help to get an overwiew. For systems that provide systemd, i also have provided a .service file and a configuration file in the hierarchy below service/etc/. If you want to use these, install everything like this:

sudo cp target/debug/cherryrgb_service /usr/libexec/
sudo cp service/etc/systemd/system/cherryrgb.service /etc/systemd/system/
sudo cp service/etc/default/cherryrgb /etc/default/

Also, perhaps you want to edit /etc/default/cherryrgb. For example, my /etc/default/cherryrgb looks like this:

# Configuration for cherryrgb.service
#
# Possible cmdline options. Shown values are defaults
#
# --socket /run/cherryrgb.sock
# --socketmode 0664
# --socketgroup root
# --product-id <None>
#
# run cherryrgb_service -h for more info
#
CHERRYRGB_OPTIONS="--socketgroup wheel"

With this, I can use the cherryrgb_ncli as admin user on my Fedora system.

After adjusting the config to your needs, run:

sudo systemctl start cherryrgb.service

and check for errors with:

sudo systemctl status cherryrgb.service

If the service is running, you should be able to run cherryrgb_ncli with the same commands you used with cherryrgb_cli.

TODO (before considering a PR):

Also: As a rust NOOB, I need advice on the following from @skraus-dev (Solved with commit bc4f175f554a2083ba66424890b87e4e4165b496)