Added led pass through to kmonad for linux uinput

[tyalie]

As mentioned in https://github.com/david-janssen/kmonad/issues/198, grabbing the uinput device blocks the access of the leds exposed by the device through /sys/class/leds/ and similar (writing EV_LED event).

This pull request is a very rough patch for this problem.

It creates a thread that polls from the sink for events and writes those into the source if the event was regarding leds. For this to work, the read write permissions needed to be changed, and the order of initialization of the source and sink device needed to be flipped. Otherwise the new uinput device, couldn't mirror the leds of the existing source uinput device.

This is mostly a very rough, but working proof of concept for now.

[slotThe]

Thank you for the pr! I won't pretend to know enough about how evdev works to properly review this; however, when trying to test this I was greeted with the rather unhelpful

❯ kmonad -ldebug ~/.config/kmonad/x220-slot-us-colemak-dh-z.kbd &
[1] 1341

Opening KeySource
Initiating ioctl grab
Opening KeySink
Registering Uinput device

[1]  + segmentation fault  kmonad -ldebug ~/.config/kmonad/x220-slot-us-colemak-dh-z.kbd

I'm not sure where to go from here. I can try slowly wading through the code if you don't have any immediate ideas.

[tyalie]

Interesting. I will add a few debug messages in the code and you can run it again. It'd be interesting to see where the segfault happens.

[tyalie]

Hmh. I assume the crash happens due to my c code. I think haskel as a script language shouldn't segfault that easily.

It could very well be, that something in acquire_uinput_keysink crashes. One option I can think of, is that the file descriptor referenced by source_fd was already closed or is not usable. I don't think that it should happen, but maybe there's some form of protection between threads happening here or multiple clients on a file descriptor?

It could also be, that your kernel doesn't support the ioctls UI_SET_LEDBIT and similar? But I think in this case, the system should have crashed during compile time.

Another option could be that I do some stupid stuff during thread creation (see second last line of the acquire_uinput_keysink method). So I would look into start_led_thread and _led_sync_thread and/or first comment the line out to see if it runs without the thread.

Is that good like that?

EDIT: Maybe it also happens somewhere later on? I'm right now deving on the master branch, so some features might be missing.

[slotThe]

Commenting out start_led_thread still segfaults. Through a bit of trial and errors I managed to narrow it down to the following line in acquire_uinput_keysink:

    ioctl(source_fd, EVIOCGBIT(EV_LED, LED_MAX), supported_leds);

If I comment this out kmonad runs fine; seems like this fits with your first guess as to what is causing this?

[tyalie]

Interesting. Can you see whether it segfaults, when you set the condition in line 38 always to false?

if (source_fd >= 0)  // replace here with false
    ...;
else
   support_leds[0] = 0b11110000;  // so that this always runs

It could be that the system segfaults whilst it tries to get the information about the leds, or it could be that it segfaults later on in the creation of the leds (line 47 ioctl(fd, UI_SET_LEDBIT, i)).

Can you also give me your kernel version?

[slotThe]

If I set that condition to false then everything works as expected (well, besides printing timeout a lot).

As for the kernel:

$ uname -rvsmo
Linux 5.11.9_1 #1 SMP 1616636388 x86_64 GNU/Linux

[tyalie]

Okay interesting. So the system crashes while trying to retrieve the existing leds. This is kinda interesting.

The timeout statement is expected. The method responsible for waiting for new led events is a blocking wait statement that needs a timeout specified (which is currently about 5s). I'm currently only doing a printf("timeout\n") there. See the code for the thread for more details.

[david-janssen]

I was wondering if this code has turned into something that doesn't segfault. Are you still working on this? Let me know if you need any support or help.

[tyalie]

Hey there. Honestly I need more input here from others. It doesn't and has never segfaulted for me :/ And the position, where it segfaults is very weird.

[david-janssen]

I tried to have a look at it, but it's still based on an old version of KMonad, back from when I was much more out of it than I am now. Would you mind rebasing or merging or whatever git-magic is required to turn this into a pull-request for the current state of master? Then I'd be very happy to try and run the code on my system to see if I get a segfault, and then if so, to see if we can track down the issue.

Sorry I wasn't around earlier when you first submitted.

[tyalie]

The newest update is now online with latest master merged into it. Thanks for taking a look.

[david-janssen]

So, I've pulled and compiled the code and it seems to work just fine. I think having LED-support is really cool. There are 2 things I would like to do before we merge this:

1. The printing of 'timeout' is a problem, since: A, it's not informative, B, it clutters up the log, and C, it doesn't respect logging rules. I would suggest just getting rid of it altogether?

2. I didn't get any segfaults, but I would like @slotThe to confirm it also runs well on his system, just to get a final confirmations.

Once I have the confirmation from @slotThe and you've done something about the timeout printing, I will merge this commit and commend you on making an excellent contribution.

[slotThe]

No dice, I'm afraid :/

$ kmonad -ldebug ~/.config/kmonad/x220-slot-us-colemak-dh-z.kbd
Opening KeySource
Initiating ioctl grab
Opening KeySink
Registering Uinput device
zsh: segmentation fault  kmonad -ldebug ~/.config/kmonad/x220-slot-us-colemak-dh-z.kbd

It has the same behaviour as before, in that if I do

diff --git a/c_src/keyio.c b/c_src/keyio.c
index c50fbf5..cd3171d 100644
--- a/c_src/keyio.c
+++ b/c_src/keyio.c
@@ -35,7 +35,7 @@ int acquire_uinput_keysink(int fd, char *name, int vendor, int product, int vers
   // setup leds
   // 1. retrieve available leds from source
   u_int8_t supported_leds[LED_MAX / 8 + 1] = {0};
-  if (source_fd >= 0)
+  if (0)
     ioctl(source_fd, EVIOCGBIT(EV_LED, LED_MAX), supported_leds);
   else

then things work. The offending line really seems to be

     ioctl(source_fd, EVIOCGBIT(EV_LED, LED_MAX), supported_leds);

[tyalie]

Yeah. The timeout print statement can be removed. You can find it in the keyio.c on line 138. But I think the whole printf's in there should be changed according to the logging rules.

The timeout is created because I poll the keyboard device for new input and the polling statements stops it's polling after 5s. I don't know if there's a better way to handle this.

[david-janssen]

No dice, I'm afraid :/

Very good. Thanks for the feedback. I'll try and dig into it.

The timeout is created because I poll the keyboard device for new input and the polling statements stops it's polling after 5s.

Hmmm, alright. I'll give it some thought. I was hoping to merge this quickly, but I guess I'll have to do a bit more digging and adjustments. Such is the way of things. :-)

[tyalie]

For reference I'm using this for polling: https://man7.org/linux/man-pages/man2/poll.2.html
I'm unsure whether this is a better alternative in the realms of C.

But the "timeout" branch has one benefit: One can check there whether the loop should be shut down or not. Even though this can be handled by destroying the file descriptor. I think right now the polling will crash in this case and take the whole thread with it, which would lead to some memory not being freed correctly. This can be fixed by changing the events parameter from line 119. But I'm not quite sure into what.

I'm sorry for the code quality, but this was a very basic proof-of-concept. I'm also happy that you seem to feel a bit better. Thank you for your outstanding work

[david-janssen]

Just popping in to say: I haven't forgotten this, but we've been really busy with a major refactor that has been a bit of a bottle-neck for other contributions. Still not done yet, and I will keep directing my efforts in that direction for now, but when the dust settles I'm coming back to this.

[david-janssen]

Sorry for getting back to this so late. I ended up getting sick and well and then sick again. Such is life.

We have just finished merging in this major refactor, and I noticed this pull request was still open. Want to have a look at exactly what you did and how I could merge these changes now? Is this still an issue?

Going to leave this open for a few days, but if I hear nothing I'm just going to close this pull-request with my apologies for inactivity.

[tyalie]

Heyo. I hope your feeling better now. ^^

I find this part still kinda relevant to be honest, as would give the emulated keyboard the full functionality of an original one. But I sadly don't posses the time in the next few months to fix it. :/

But my changes in the part of Haskell files were very small. I mostly changed order of initialization and read / write permissions to the libinput device pipes there. The order of initialization can be ignored if the virtual keyboard should just init with all possible usable LEDs and than pass through what the hardware keyboard supports. This would also be more future proof if one want's to allow kmonad to support multiple keyboards.

Lilly

[david-janssen]

Heyo. I hope your feeling better now. ^^

Very kind, but sadly no. But I am embracing the practice of making an effort regardless of the circumstances ;)

But my changes in the part of Haskell files were very small. I mostly changed order of initialization and read / write permissions to the libinput device pipes there. The order of initialization can be ignored if the virtual keyboard should just init with all possible usable LEDs and than pass through what the hardware keyboard supports. This would also be more future proof if one want's to allow kmonad to support multiple keyboards.

Thanks, I'll take that into consideration and see if I can just merge the changes manually.

[tyalie]

Great. ^^
I've been running with this version of the code for almost 7 months now without trouble.

EDIT: If it takes you a long time to respond, you don't need to apologize for that. Regardless of illness or not ^^

[slotThe]

Thanks, I'll take that into consideration and see if I can just merge the changes manually.

I will note that this still segfaults for me in its current state, so please do ping me before merging so I can try again

[david-janssen]

Yo, putting this on a backburner for now. We'll consider this the cherry on top after we merge keycode-refactor.