Implement acceleration using an input_handler modifying usbhid input events directly

[kitten]

Supersedes #2 (A prior draft of this PR)

Problem

Currently, the kernel module is a full driver, based on the boot-mode USB mouse driver in Linux. It then uses a slim USB HID descriptor parser (util.c) and creates a new driver to bind input devices to.

However, Linux contains a lot of modifications and custom drivers for input devices as well, and there's quite a long list of problematic devices that misreport events or HID descriptors, which are patched upstream in the kernel.

These patches are only available to the built-in usbhid driver though.

Likely, a combination of the USB HID descriptor parser being very slim and sacrificing accuracy for brevity, and missing device patches and workarounds, means that a lot of mice that'd usually work just don't 😞

While it's possible to improve the USB HID descriptor parser, it's also possible to find a new approach, with the goal of intercepting events and modifying them while still letting the usbhid driver handle the device. (This also has the benefit that it leaves outputs intact, i.e. on-device configuration would still work)

Halfway Solution: Input handlers and virtual devices

The maccel project, which essentially started as yet another fork of leetmouse (but has diverged quite a lot), has a solution to this that I've seen replicated in a few other places.
This approach uses regular input_handlers in the Linux Input subsystem implementation to filter out EV_REL events and to replace them by sending them to a virtual input device.

This is close to what we want but then comes with some drawbacks:

• every input (and optimally output) on the mouse device must be replicated to the input device
• the device is still duplicated and the system sees multiple input devices
  • input events are replicated to either a single input device, or we'd have to create virtual devices for each mouse, which basically just exacerbates this problem of having too many inputs
• this incurs a (very tiny) latency penalty (which, as far as I understand) goes against the goals of the yeetmouse project
  • fundamentally, the issue here is that we're queueing up new reports while events are incoming from the mouse

Full(?) Solution: Intercepting and modifying events

input_handlers are very powerful and their filter callback concept is essentially just a template that uses the underlying events API to omit events dynamically.
However, this can also be used to modify events.

The new driver in this PR intercepts events, collects them, passes them to the accelerate() function, then modifies the original events. If we make sure that our input_handler is registered at the front of the list of all handlers (which the kernel's input_register_handle() function usually only does for filters, i.e. handlers with ->filter set rather than ->events), then we're able to intercept and mutate the events as we please.

Further, we can also reach into the device and update the original events in a similar way, which makes sure that there's no residual data of the unaccelerated input (fwiw, I don't know if that's important, but who knows)

In theory this now means:

• the device isn't duplicated and systems observing the mouse (or the Input subsystem device in general) only see the accelerated input events
• we don't queue up more reported events to a virtual device and hence shouldn't incur any latency penalties (in theory)
• we reuse usbhid's handling of devices, sub-drivers, and quirks which should blanket-support every device that works out of the box with Linux

Note

There's one gotcha I found, which is that I don't yet know how events are queued up.
This means I'm not waiting for an EV_SYN event and just consider a batch of events as "one input" that we should accelerate. Optimally, we'd handle this better, but I'm not sure if that means we have to shift around some state. TBD

Set of changes

• Replace module_usb_driver module implementation with a module_init/module_exit module
• Implement an input_handler receiving events and updating them
• Update the raw device events
• Delete unused util.c USB HID descriptor parser code
• temp: Remove udev installation/code/etc
  • Note: This is marked as temp because this PR still requires testing, and I'm assuming we might want to work on a hybrid driver, that can function using both input_handlers and as a driver to bind a device to.

[kitten]

Another follow-up, for my mice this doesn't seem to matter (as far as I can observe at least), but I've got some additional changes here in case we want to be more strict about SYN_REPORT handling: https://github.com/kitten/YeetMouse/compare/feat/input-handler-overrides...kitten:YeetMouse:experiment/syn-handling?expand=0

This splits the batch of events into at most two parts. While it still applies the accelerated REL_X and REL_Y events to all values it receives, the calculation is based on all prior x/y values (before SYN_REPORT) and all the ones after are stored for the next events run.

So, this is technically more correct, but might not be necessary in a lot of cases.

Edit: I've tested this out and since it's not causing issues and is more correct, I'm picking these changes over to this branch

[AndyFilter]

Hey, I tried to run this PR, and I had some issues with the udev rules (or the lack of them). I know they should not be necessary, but I added all the udev stuff back in and it worked. The most important change I think is that there is actually no Leetmouse driver, at least not in the /sys/bus/usb/drivers/ directory. This kind of makes binding devices through the GUI unusable, as there is nothing to bind to.

Also, I had one small issue with the code. This is the declaration of usb_mouse_events:
https://github.com/kitten/YeetMouse/blob/d3f8f081e0b65e61b38f569758a0821fbb0cecd0/driver/usbmouse.c#L58.diff

static unsigned int usb_mouse_events(struct input_handle *handle,
     struct input_value *vals, unsigned int count)

while for me, the correct signature of the input_handler's event function is:

static void usb_mouse_events(struct input_handle *handle,
    const struct input_value *vals, unsigned int count)

the return type is void, and input_value's qualifier is constant. This is most likely a caused by different kernel version or something like this.
(looking at the older kernel versions, it seems the same as mine though)

After this change the code compiled smoothly.

Other than that, it looks great, I'll look a bit more into the code, and try to remove the udev dependencies (this might be causes, by the fact that I'm building everything using the install.sh and makefile scripts, instead of the NixOS files).

[kitten]

This kind of makes binding devices through the GUI unusable, as there is nothing to bind to.

Yup, that's by design for now and I've basically remove the driver entirely, as you've noticed here ✌️ If this works well though and we're happy to go ahead with this, what I'd suggest for implementation next is:

• remove the auto-binding udev rules,
• add the current driver in parallel to the input handler
• make sure that when a device is bound to the driver, the input handler ignores it

This should allow us to seamlessly bind devices to the driver with the GUI, while still having acceleration applied to all other devices (or if no devices are bound of course)

while for me, the correct signature of the input_handler's event function is:

That's pretty odd! I've checked this against the latest kernel code, and this is a public API that shouldn't have changed. Odd!
Basically, the return value is the modified length of events, so it definitly shouldn't be missing. I'll look into that.

Other than that, it looks great, I'll look a bit more into the code, and try to remove the udev dependencies (this might be causes, by the fact that I'm building everything using the install.sh and makefile scripts

Nice! I suppose if we want to have both a driver and this input handler approach in parallel, then if I build off of this, we'd be fine?

[kitten]

Ah, alright, torvalds/linux@14498e9
I'll have to see what the alternative was before. The easiest way to fix this is to probably replace removed events with noops in older kernel versions

[AndyFilter]

I think something like this will work too:

#if LINUX_VERSION_CODE <= KERNEL_VERSION(6,12,3)
    static void usb_mouse_events(struct input_handle *handle, const struct input_value *vals, unsigned int count)
#else
    static unsigned int usb_mouse_events(struct input_handle *handle, struct input_value *vals, unsigned int count)
#endif

For me it uses the top (void with const version, so it's correct). I'm using version 6.8

[AndyFilter]

Also, I think that udev_trigger should be removed from this line:

YeetMouse/install_files/dkms/dkms.conf

Line 4 in ba430f6

MAKE="KERNELDIR=/lib/modules/${kernelver}/build make driver udev_trigger"

It caused errors for me, because there is no "udev_trigger" in the makefile. I tried suggesting this change for this PR, but it seems as though I can only edit the files that were already altered in some way.

[kitten]

@AndyFilter: Sorry 😅 Was having some issues getting the last version to compile on both kernel versions, but I just made some adjustments and it's working fine for me now. I'm choosing not to handle the const warnings. They should be fine and not affect anything though (?). As long as it's non-const on the newer kernel, the event dropping should still work.

[AndyFilter]

It think the const's are fine either way. Current version builds for me just fine (using the install.sh script), but it doesn't install the driver. I have to manually do make driver, cd driver/, sudo insmod leetmouse.ko. Which was not the case in the original version. This obviously can be automated, but it just makes me wonder why it didn't load the driver on it's own.

[kitten]

sorry for dropping the ball here! I've been a little busy lately. I'll have to set up some virtual machines to test this a little more once I've got more time, e.g. on a clean Arch install, to make sure we're in a good state here. That should ultimately also help me resolve the build issues for you ❤️
I'll push an update here though once I'll get back to this