Use systemd instance template for device dependencies

[samiconductor]

I think I've got something working for #8. I was having to unplug / plug in the keyboard to get it working after reboot or power cycle. But with this it picks it up on startup! I was excited anyway.

It changes up things a bit though and the service bypasses sort-and-run.sh which could be cleaned up if you're good with the approach in this PR. Also it runs two separate instances of kfreestyle2d, one for each device. You can get the status with systemctl status [email protected] for example. AFAIK two instances are fine?

Changes:

• Udev uses the systemd tag and SYSTEMD_WANTS to kick off the service
  • See systemd.device - The udev Database
• The service is now an instance template that takes the kinesis device name as a parameter
  • See the note here for a good example of how parameters work
• The service depends on the kinesis device
  • As explained in systemd.device above

Here's a good example of putting it all together. It's also where I got the idea to depend on user session service. (edit: works fine without waiting for systemd-user-sessions.service)

[whereswaldon]

Thank you for putting this together! I will try to look at it this weekend!

[whereswaldon]

Firstly, thank you for putting this together! I think it's an excellent approach to solving the problem. I didn't know about template units, but they definitely are an elegant solution to running this service properly.

Unfortunately, there is a problem with this approach. It's not your fault, but rather an inadequacy of the existing codebase that has implications for your approach. I've documented it inline in the code.

I will attempt to find a way forward in the coming days, as I think that this problem should be solvable. You're welcome to help with the investigation if you have time. I've attempted to explain the problem and necessary fix inline, but definitely ask any followup questions if you have them.

Thanks again!

[whereswaldon]

When you plug the kinesis keyboards in, they actually advertise themselves as two separate USB peripherals (something to do with the number of key inputs available for a single input). This results in the creation of two different /dev/hidrawX files for each keyboard. One of these inputs is normal keypresses, and one of them is the multimedia keys targeted by this driver.

Unfortunately, I have yet to find a programmatic way to differentiate the two other than the fact that the multimedia keys device is always discovered second and therefore has a higher number in its name. The purpose of sort-and-run.sh is to ensure that we choose this second device and then run the driver against it.

The approach here runs the driver twice for each kinesis keyboard that you plug in. One of the instances listens to the normal keyboard input, and one listens to the multimedia key input. This results in an apparently-working configuration (it will respond to multimedia keys), but it also introduces the potential for normal keyboard input to be erroneously interpreted as multimedia key input.

Essentially, the driver reads each sequence of three bytes from the raw HID data and searches the second byte for a key identifier. I think that the driver listening on the normal keyboard input could easily misread some arbitrary three-byte sequence of data as a multimedia key. The effect would be that you might suddenly play/pause your music or change your volume as you are typing normal text.

I think the best fix for this problem is to finally find a programmatic strategy for differentiating the two HID devices so that we can filter to only run against the multimedia input. This isn't a problem of your creation, but it does block taking this approach until we can do it. I suspect that there is a difference in the data available in udev, but that I just didn't find it back when I wrote this (I was learning about it for the first time).

If you're interested in helping me hunt for a difference between the two devices, I documented the debugging techniques that I used for this on my blog.

[samiconductor]

Ok. Ya it certainly runs two instances. I assumed either side of the keyboard was a device. I'm not familiar with udev so would be a learning xp for me as well to find the difference between the two devices.

From what I saw of udev / systemd, there's no way to kick off one service for both devices then run sort-and-run.sh. You can depend on multiple devices in a systemd unit but you can't assume the number e.g. BindsTo=/dev/kinesis1 /dev/kinesis2, it could be 0 and 1.

Maybe it's as easy as reverting the systemd service and using SYSTEMD_WANTS. I think the current udev RUN+="/bin/systemctl --no-block start kfreestyle2d.service" just runs too early on startup.

I'll try some things.

[samiconductor]

Alright, I couldn't see any way to distinguish the two devices with udev. I tried reverting everything and just used the SYSTEMD_WANTS approach to see if it would wait for both devices before starting it. But you could get the first or second device when sort-and-run.sh on start up. See #11 for a workaround.

By the way, I had both devices running as instance services for a week now and never ran into any errant media inputs. Also, the two services might be useful if the other keys could be mapped too (copy paste, web back/forward, etc). Not sure if those keys are also always the first device.

[whereswaldon]

Well you inspired me to look at the two devices in Udev again as well, and I did find something. Notably, within their device tree they have these parent devices that (for me) look like: /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/ and /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.1/. Note that the final digit varies. I believe this is an indication that they are two "logical" devices connected on the same physical USB connection.

When you run udevadm info -a -n /dev/hidraw9 against one of them, one of their ancestor devices has an ATTR called bInterfaceNumber that captures this value.

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device '/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/0003:058F:9410.002F/hidraw/hidraw9':
    KERNEL=="hidraw9"
    SUBSYSTEM=="hidraw"
    DRIVER==""

  looking at parent device '/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/0003:058F:9410.002F':
    KERNELS=="0003:058F:9410.002F"
    SUBSYSTEMS=="hid"
    DRIVERS=="maltron"
    ATTRS{country}=="00"

  looking at parent device '/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0':
    KERNELS=="1-6:1.0"
    SUBSYSTEMS=="usb"
    DRIVERS=="usbhid"
    ATTRS{bInterfaceProtocol}=="01"
    ATTRS{bAlternateSetting}==" 0"
    ATTRS{bInterfaceNumber}=="00"
    ATTRS{authorized}=="1"
    ATTRS{bInterfaceSubClass}=="01"
    ATTRS{supports_autosuspend}=="1"
    ATTRS{bInterfaceClass}=="03"
    ATTRS{bNumEndpoints}=="01"

  looking at parent device '/devices/pci0000:00/0000:00:14.0/usb1/1-6':
    KERNELS=="1-6"
    SUBSYSTEMS=="usb"
    DRIVERS=="usb"
    ATTRS{bcdDevice}=="0122"
    ATTRS{bmAttributes}=="a0"
    ATTRS{urbnum}=="15"
    ATTRS{bNumInterfaces}==" 2"
    ATTRS{configuration}==""
    ATTRS{busnum}=="1"
    ATTRS{bDeviceSubClass}=="00"
    ATTRS{tx_lanes}=="1"
    ATTRS{version}==" 1.10"
    ATTRS{bMaxPacketSize0}=="8"
    ATTRS{bMaxPower}=="50mA"
    ATTRS{maxchild}=="0"
    ATTRS{bNumConfigurations}=="1"
    ATTRS{ltm_capable}=="no"
    ATTRS{product}=="KB800 Kinesis Freestyle"
    ATTRS{authorized}=="1"
    ATTRS{manufacturer}=="KINESIS FREESTYLE KB800"
    ATTRS{avoid_reset_quirk}=="0"
    ATTRS{idVendor}=="058f"
    ATTRS{bDeviceProtocol}=="00"
    ATTRS{bConfigurationValue}=="1"
    ATTRS{speed}=="1.5"
    ATTRS{rx_lanes}=="1"
    ATTRS{removable}=="removable"
    ATTRS{bDeviceClass}=="00"
    ATTRS{quirks}=="0x0"
    ATTRS{devpath}=="6"
    ATTRS{idProduct}=="9410"
    ATTRS{devnum}=="14"

  looking at parent device '/devices/pci0000:00/0000:00:14.0/usb1':
    KERNELS=="usb1"
    SUBSYSTEMS=="usb"
    DRIVERS=="usb"
    ATTRS{idProduct}=="0002"
    ATTRS{ltm_capable}=="no"
    ATTRS{tx_lanes}=="1"
    ATTRS{avoid_reset_quirk}=="0"
    ATTRS{bDeviceProtocol}=="01"
    ATTRS{manufacturer}=="Linux 5.4.0-7634-generic xhci-hcd"
    ATTRS{authorized}=="1"
    ATTRS{busnum}=="1"
    ATTRS{removable}=="unknown"
    ATTRS{bNumConfigurations}=="1"
    ATTRS{product}=="xHCI Host Controller"
    ATTRS{bDeviceSubClass}=="00"
    ATTRS{bConfigurationValue}=="1"
    ATTRS{version}==" 2.00"
    ATTRS{bmAttributes}=="e0"
    ATTRS{urbnum}=="3017"
    ATTRS{serial}=="0000:00:14.0"
    ATTRS{configuration}==""
    ATTRS{speed}=="480"
    ATTRS{interface_authorized_default}=="1"
    ATTRS{devnum}=="1"
    ATTRS{bMaxPower}=="0mA"
    ATTRS{maxchild}=="16"
    ATTRS{idVendor}=="1d6b"
    ATTRS{bDeviceClass}=="09"
    ATTRS{bMaxPacketSize0}=="64"
    ATTRS{bcdDevice}=="0504"
    ATTRS{authorized_default}=="1"
    ATTRS{quirks}=="0x0"
    ATTRS{rx_lanes}=="1"
    ATTRS{devpath}=="0"
    ATTRS{bNumInterfaces}==" 1"

  looking at parent device '/devices/pci0000:00/0000:00:14.0':
    KERNELS=="0000:00:14.0"
    SUBSYSTEMS=="pci"
    DRIVERS=="xhci_hcd"
    ATTRS{consistent_dma_mask_bits}=="64"
    ATTRS{msi_bus}=="1"
    ATTRS{revision}=="0x31"
    ATTRS{device}=="0xa12f"
    ATTRS{driver_override}=="(null)"
    ATTRS{vendor}=="0x8086"
    ATTRS{subsystem_device}=="0x5007"
    ATTRS{numa_node}=="-1"
    ATTRS{subsystem_vendor}=="0x1458"
    ATTRS{dbc}=="disabled"
    ATTRS{local_cpus}=="ff"
    ATTRS{irq}=="131"
    ATTRS{class}=="0x0c0330"
    ATTRS{dma_mask_bits}=="64"
    ATTRS{d3cold_allowed}=="1"
    ATTRS{enable}=="1"
    ATTRS{ari_enabled}=="0"
    ATTRS{local_cpulist}=="0-7"
    ATTRS{broken_parity_status}=="0"

  looking at parent device '/devices/pci0000:00':
    KERNELS=="pci0000:00"
    SUBSYSTEMS==""
    DRIVERS==""

Armed with that difference, I thought that I would be able to write the one Udev rule to rule them all. Seemed like it should be as simple as adding ATTRS{bInterfaceNumber}=="01" to the existing rule. Alas, that doesn't work because udev only allows you to match against attributes from a single parent device. I didn't realize that before.

So anyway, this udev rule correctly recognizes the device, but isn't matching against the HIDRAW device, so isn't useful to us:

SUBSYSTEM=="usb", ATTRS{idVendor}=="058f", ATTRS{idProduct}=="9410", ATTR{bInterfaceNumber}=="01", ENV{KINESIS_SECOND_DEVICE}="1"

I was trying the ENV variable to see whether it would be inherited by child devices (so that I could then match against it), but that isn't working for me right now. I'm probably misunderstanding it.

Anyway, this does bring us closer. There is a difference between the devices in udev, it's just a question of how to write the rule.

By the way, I had both devices running as instance services for a week now and never ran into any errant media inputs. Also, the two services might be useful if the other keys could be mapped too (copy paste, web back/forward, etc). Not sure if those keys are also always the first device.

I understand. It seems unlikely that other keyboard input would do that, but I can't convince myself that it won't happen. I think that there is a way forward though, with some more digging.

As far as handing the other keys specially, it really depends on how they're implemented. Linux does seem to understand them when they are pressed, so I think the proper way to handle those would be to remap them at a higher level of abstraction (libinput, say). This driver mostly exists to translate unintelligible binary garbage into input that Linux understands. Once Linux can process the input, there are probably better tools for managing it.

I've looked at your second PR, and that does seem like it would (at least usually) address the race condition that we are concerned with. However, I'm pretty convinced that we can solve this purely by narrowing the udev rule to only apply to the special keys device file if we work at it for a little while longer. I'd prefer to go that route unless we find that it's impossible.

Thank you so much for your investigation and effort on this! I know that it's probably discouraging to have this PR not be accepted, but I really do think we're close to a really elegant solution that will enable supporting multiple Kinesis keyboards at the same time (an outstanding feature request). The systemd template units are definitely a great idea, and I'm excited to apply them once we iron udev out!

[samiconductor]

Not discouraging at all. I'd rather get something working correctly than a workaround.

Also, booted up today and #11 already didn't work once so it's still not what we want.