Add hotplugging support, among other things

[dcdeepesh]

This is a late PR for changes I'd made a while ago but considered keeping separate. Now, however, I thought I should let you decide whether you'd like these changes merged upstream or not, before considering maintaining a fork. You may merge these if you think they align with the goals of this project, close this request if they don't, and are welcome to start a conversation for any case in between.

---

Currently, keyboard-backlightd (kbd) reads LED device IDs and input device IDs from the command line, adds a poll for input devices once at start, and waits for (e)poll events for the rest of its lifetime; unless a device/file is suddenly not found (e.g. when the device is unplugged), in which case it crashes. Upon a crash, systemd tries to restart it, but kbd crashes again because the device is still not present, and the cycle continues. All of this spams the system log. On my daily machine, kbd crash messages made up half of the log (in terms of log size and to a lesser extent log message count).

Clearly not an ideal situation for, say, laptop users who would want to plug their mouse (dongle) in and out as needed, or maybe plug any other devices as needed. The situation gets more complex when virtual devices (e.g. keyd) and/or multiple USB ports get involved. kbd crashes upon the first plug-out and remains crashed until a plug-in. If the daemon starts while the device was plugged out, plug-in isn't recognised until a crash or manual restart. There are far too many issues to even list here, so I'll limit it here.

Changes

The changes this request aims to pull provide the following enhancements:

• Hotplugging support - Devices can be plugged in and out while the daemon is running and they'll all be recognised and handled without any user intervention. This prevents all crashes, keeps the logs clean, and opens the door for:
• No explicit ID requirement - Users no longer have to provide device IDs on the command line, simplifying the start command. The -i argument is therefore made optional. Note that for a regular user, they shouldn't need to use -i at all.
• Heuristic LED recognition - Per kernel docs (this, among others), LED backlight device paths follow certain conventions. kbd will now automatically detect a backlight device if found, eliminating the need for the -l argument.
• Heuristic device recognition - If a user has a touchscreen-capable laptop, we probably don't want to light up the keyboard if the touchscreen is used. Same goes for, say, a drawing tablet. The events are checked for their source, and are only considered if a "sensible" device sent those events.
• Normalized device path handling - Device paths specified/used may actually be a symlink, for one reason or another. It was in my case, causing kbd to crash. All device paths are now resolved before use. An overlapping scenario is when the same device is provided multiple times, causing issues. This is also now handled properly.

Result

As a result of all this, the startup command (the example in README) changes from this:

keyboard-backlightd
  -i /dev/input/by-path/platform-i8042-serio-0-event-kbd \
  -i /dev/input/by-path/platform-thinkpad_acpi-event \
  -i /dev/input/by-path/pci-0000:00:1f.4-event-mouse \
  -i /dev/input/by-path/pci-0000:00:1f.4-serio-2-event-mouse \
  -i /dev/input/by-id/usb-Raydium_Corporation_Raydium_Touch_System-event-if00 \
  -l /sys/class/leds/tpacpi::kbd_backlight
  -t 15000

to this:

keyboard-backlightd -t 15000

not mentioning the lack of need to go through the figuring out process. Since these changes I've had very rare edge case crashes, nothing concerning thanks to the daemon restart policy. The logs are clean, all devices are handled, and I, as the user, never have to actively think about kbd before handling my devices; it all works fine automatically.

Developer notes

Notes that may help future development:

• Some identifiers were renamed and code was slightly restructured to make more sense.
• Default device selection can be found in MONITORED_PROPERTIES in get_devnode_if_default().
• Seemingly, event listeners were multiplexed for epoll. This was removed to make things simpler and more sensible.

[VorpalBlade]

It will be at least a few days before I have time to do a proper code review and look at this in depth. I will however address your comment right now. I'm positive towards the idea of improving the functionality in general.

On my daily machine, kbd crash messages made up half of the log (in terms of log size and to a lesser extent log message count).

Yeah, that is not great. I made this originally for laptop built-in keyboards, so that never got tested. My external keyboard doesn't have backlight, and when I'm typing on it I didn't want the laptop keyboard to light up. I can see different preferences though, or wanting to pair up a keyboard to the associated backlight (if both keyboards have backlight).

LED backlight device paths follow certain conventions

I haven't touched this in a long time (it mostly worked for me, though I did notice some bugs around suspend/resume), but I seem to remember at least on laptops it was not sensible always. Controls would be found under ACPI platform devices and the like, not connected to the keyboard node under /sys. But perhaps this has improved?

touchscreen-capable laptop, we probably don't want to light up the keyboard if the touchscreen is used

Actually, I did configure mine to light up. But if you have one of those that can be turned over into a tablet form I can see not wanting that. I'm a bit wary of making assumptions of how people want to use things.

Side note: Gnome makes a lot of assumptions about font rendering, which I disagree strongly with (they cause literal headaches for me). KDE let's you configure those settings. And that is one of many reasons I use KDE Plasma rather than Gnome. Similarly any "smarts" should be possible to turn off I believe.

As a result of all this, the startup command (the example in README) changes from this [...]

dev/input/by-id/usb-Raydium_Corporation_Raydium_Touch_System-event-if00 is the touchscreen in that example, so clearly it still needs configurability

Seemingly, event listeners were multiplexed for epoll. This was removed to make things simpler and more sensible.

Not sure what you mean (it has also been a while), but I did use epoll manually here. In this project I was kind of doing an experiment to see how small and fast I can make the build, and with how few dependencies. I also hadn't learnt async rust yet. I think today I would use an async runtime (probably smol rather than tokio).

One thing of note here is that on some devices (such as thinkpads), you can use inotify to listen to the Fn+space key combo being used to change the backlight level. On other laptops (I think it was some Asus or Acer) there is no way to be notified (but you can read it). Is that what you are referring to?

[VorpalBlade]

Seems from the CI output that you need to update https://github.com/VorpalBlade/keyboard-backlightd/blob/main/Cross.toml to install the dev package for udev to fix cross compiling.

[dcdeepesh]

Hopefully you've had the time to look at it in some depth by now.

About the smarts and their requisite conventions, I agree that it's a good idea to have an off(/on) switch. This can be implemented as a CLI switch to turn them all off, have individual off switches, or to keep them off by default and instead have on switches, etc. I'll leave the decision of which permutation(s) to use, up to you.

By event listeners being multiplexed, I meant that all listeners (LED, device events, etc.) were being "molded" to fit into a common shape and then handled similarly. This meant using stuff like ListenType, using hacks for epoll IDs, etc. In the new model, there is no concept of a generic "listener", instead there's an "evdev listener", a "software brightness change listener", and a "hardware brightness change listener". Concerns are better separated, and unrelated data isn't being forced to fit into a common container.

(Side note: I found this multiplexing peculiar since I first looked at the code, as it seemed like an additional step to make things complex for no apparent reason. Regardless of the "why", I considered leaving it as-is; until it became a blocker for hotplugging-related changes, at which point I rewrote it.)

About async/await, I see your runtime choice aligning with the goal of keeping kbd a lite app. Personally, considering the event loop of kbd, and how much "load" it is under in most situations, I don't think it warrants the use of async/await. Note that the app is still asynchronous, in the actual meaning of the term, I just don't consider Rust's async/await a worthy addition to the code, given the cost and such.

Any light on how this PR should go forward?

[VorpalBlade]

Hopefully you've had the time to look at it in some depth by now.

Very sorry. Life has been happening, haven't coded more than a handful lines outside of my day job in recent months.

About the smarts and their requisite conventions, I agree that it's a good idea to have an off(/on) switch. This can be implemented as a CLI switch to turn them all off, have individual off switches, or to keep them off by default and instead have on switches, etc. I'll leave the decision of which permutation(s) to use, up to you.

As a user I prefer granular flags. As a developer I prefer simpler code that is easier to reason about. These two can come in conflict, I would have to look at the code to really say for certain in this case.

By event listeners being multiplexed, I meant that all listeners (LED, device events, etc.) were being "molded" to fit into a common shape and then handled similarly. This meant using stuff like ListenType, using hacks for epoll IDs, etc. In the new model, there is no concept of a generic "listener", instead there's an "evdev listener", a "software brightness change listener", and a "hardware brightness change listener". Concerns are better separated, and unrelated data isn't being forced to fit into a common container.

Yeah that sounds better.

I will try to get some time to review. The upcoming Christmas vacation should give me some time. I can't commit to anything earlier than that.

[dcdeepesh]

All good, no rush here. I'll mark the PR as a draft and then this can be a long term/low priority goal.