per device config

[BKSalman]

related to #100

this adds per device config

current status:
it kinda works, but I'm currently trying to make the sysfs config path more user-friendly, but I'm still figuring out how sysfs works

TODOs:

• pass constants as parameters instead of using globals
• make writes and reads more safe by locking the device parameters/config on writes
• use user-friendly sysfs config path
• parse LUT on write
• fix test suite to work with the new changes
• test all acceleration modes

[BKSalman]

I'm picking this up again after I left it for a while, so I'm kinda rebuilding the context of what I'm working on (basically I forgot what I was doing lol)

[BKSalman]

config paths now look something like /sys/class/yeetmouse/mouse_name/accel_config/acceleration

in my case /sys/class/yeetmouse/Kensington_ORBIT_WIRELESS_TB_Mouse/accel_config/acceleration

I have tested configuring multiple mice with different configurations and it's working :)

[BKSalman]

I'm not really familiar with all the acceleration modes, so I'm not sure how to test them, @AndyFilter if you have any suggestion it would be helpful, or maybe some collection of input values to test manually

[AndyFilter]

I have tested configuring multiple mice with different configurations and it's working :)

Awesome! That's great to hear, the progress is faster than I expected.

I'll test the code myself too soon. Quickly looking at the CI that failed - I think it's just an issue with some includes, so once that's fixed you'll be able to build and run the unit tests. But I doubt anything has changed, as you didn't change any acceleration function implementation. I'll take a look at the tests right now, maybe it's an easy fix. (Also, when running the tests, you don't need to copy and files, or use any python scripts anymore, I just forgot to remove that from the instructions.)

I'm not really familiar with all the acceleration modes, so I'm not sure how to test them, AndyFilter if you have any suggestion it would be helpful, or maybe some collection of input values to test manually

I'm not sure what exactly are you asking about, simple unit tests for all the acceleration modes/functions, if so, then the Tests suite is your friend: https://github.com/AndyFilter/YeetMouse/tree/master/tests. If you want to check if a given mode applied on your system ACTUALLY applies the correct acceleration, then you need a bit more sophisticated setup; and a external device that can emulate a mouse. You don't have to test for this, I'll take care of it, in some spare time, as I already have everything set up, just need to plug the microcomputer to my PC.

[AndyFilter]

I didn't have time to look through the driver code yet. But I left some general comments. About the headers - I will push the commit fixing this later today or tomorrow, as I had to go and didn't have time to finish it. Also, I moved the ModesConstants struct to accel.h, as it's not really a shared definition, because from what I can see only the driver uses it. This also solved most of the compilation issues for the GUI and the Testing Suite. I already managed to get the GUI to compile and work locally, next I'll try to work on the Testing Suite, but that should be a similar story. About the headers in general, it's a slippery topic, as one compiler will accept one thing while the other will not, but I'd try to stick to what was already in the code, as no one seemed to complain about it for some time - meaning it might be working just fine.

[AndyFilter]

This header is already included in Fixed64.h, just in the other order. I swapped the order and it seems to work.
Edit: There is quite a bit of comments like these regarding the includes, I have fixed these issues in my local code, so I won't leave comments on the rest.

[BKSalman]

it didn't work for me for some reason, I will wait for your fixes to try them out 🫡

PS: might be an LSP thing

[AndyFilter]

Could we instead of passing all the parameters that a given function uses pass the struct accel_params *params? This would greatly reduce the API's complexity for the driver as well as the Testing Suite.

[BKSalman]

I was thinking that would make it clearer, but I can change it no problem

[AndyFilter]

Hey, I pushed the commit I was talking about earlier that fixes the GUI building. But I didn't entirely fix the Test Suite as it would require fixing all the - now changed - acceleration modes functions (like accel_linear()) calls; and since we want to move passing the entire accel_params to them instead, it's be a work done for nothing. So I think you can update this, when changing the prototypes prototypes of these functions.
Also, this change is quite a significant impact on the Test Suite ("system" wise), so if you're unsure about something I'll handle it, as I sometimes like things done my way.

btw. let me know if the driver compiles now, after I removed some includes. I tested it and it did compile and work, but I couldn't find the /sys/class/yeetmouse/ directory, but I didn't add any special rules for any of the devices. I'm not sure how it's supposed to work right now, as also there are not new GUI elements yet.

[BKSalman]

Hey, I pushed the commit I was talking about earlier that fixes the GUI building. But I didn't entirely fix the Test Suite as it would require fixing all the - now changed - acceleration modes functions (like accel_linear()) calls; and since we want to move passing the entire accel_params to them instead, it's be a work done for nothing. So I think you can update this, when changing the prototypes prototypes of these functions. Also, this change is quite a significant impact on the Test Suite ("system" wise), so if you're unsure about something I'll handle it, as I sometimes like things done my way.

got it

btw. let me know if the driver compiles now, after I removed some includes.

ok I will try compiling it soon, this week and maybe the next I will be kinda busy (won't have access to the machine I'm developing with)

I tested it and it did compile and work, but I couldn't find the /sys/class/yeetmouse/ directory, but I didn't add any special rules for any of the devices. I'm not sure how it's supposed to work right now, as also there are not new GUI elements yet.

hm, that's weird, could you check dmesg for any crashes or something?

[BKSalman]

I haven't fixed the tests yet, but now the GUI works! (I used some AI for the GUI since I don't know C++ much, hope you're ok with that, but I took a general look at the generated code and made sure it worked)

I saw there was an unused RefreshDevice macro so I reused it in the GUI code

I have included the checkbox from your comment here but I can remove it (disabling acceleration is currently not implemented)

[AndyFilter]

Hey, I took look at the code, and it does look good now. I managed to quickly patch up the tests, and they all pass.

I used some AI for the GUI since I don't know C++ much, hope you're ok with that

Completely fine with me to speed up work with AI as long as you know the idea / concept behind the code, simply because I'm gonna be asking in the comments for it, haha.

Let's focus on the concerns I have about this change. I have a couple of points , but most of them revolve around the fact that I want this to be optional and opt-in rather than opt-out. I described it in this comment: #100 (reply in thread).

Now to get to the details. I've managed to install it, and the general functionality seems to work, although when plugging a second mouse I was greeted by a null pointer dereference in the kernel space:

[  678.236317] yeetmouse: found a possible mouse (HID) SINOWEALTH GXT 165 Gaming Mouse
[  678.236361] BUG: kernel NULL pointer dereference, address: 0000000000000000
[  678.236366] #PF: supervisor read access in kernel mode
[  678.236368] #PF: error_code(0x0000) - not-present page
[  678.236371] PGD 12da91067 P4D 12da91067 PUD 0 
[  678.236377] Oops: Oops: 0000 [#1] SMP NOPTI
[  678.236382] CPU: 11 UID: 0 PID: 214 Comm: kworker/11:1 Tainted: P           OE       7.0.0-30-generic #30~24.04.1-Ubuntu PREEMPT(lazy) 
[  678.236388] Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[  678.236390] Hardware name: Micro-Star International Co., Ltd. MS-7C56/MPG B550 GAMING PLUS (MS-7C56), BIOS 1.I0 07/13/2024
[  678.236393] Workqueue: usb_hub_wq hub_event
[  678.236401] RIP: 0010:__mutex_add_waiter+0x3c/0xb0
[  678.236407] Code: 54 49 89 fc 53 48 89 f3 48 85 ff 74 77 48 8b 88 60 0e 00 00 48 85 c9 75 7d 41 f6 c4 03 75 07 4c 89 a0 60 0e 00 00 48 8b 72 08 <48> 3b 16 75 4b 48 39 f3 74 46 48 39 d3 74 41 48 89 5a 08 48 89 13
[  678.236410] RSP: 0018:ffffcb88c0853030 EFLAGS: 00010046
[  678.236414] RAX: ffff896801408000 RBX: ffffcb88c0853070 RCX: 0000000000000000
[  678.236416] RDX: ffff896814eb7c28 RSI: 0000000000000000 RDI: ffff896814eb7c18
[  678.236419] RBP: ffffcb88c0853040 R08: 0000000000000000 R09: 0000000000000000
[  678.236421] R10: 0000000000000000 R11: 0000000000000000 R12: ffff896814eb7c18
[  678.236424] R13: ffff896814eb7c18 R14: ffffcb88c0853070 R15: 0000000000000246
[  678.236427] FS:  0000000000000000(0000) GS:ffff897364763000(0000) knlGS:0000000000000000
[  678.236431] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  678.236435] CR2: 0000000000000000 CR3: 000000023aeb5000 CR4: 0000000000f50ef0
[  678.236438] PKRU: 55555554
[  678.236440] Call Trace:
[  678.236443]  <TASK>
[  678.236447]  __mutex_lock.constprop.0+0x23a/0x9d0
[  678.236459]  __mutex_lock_killable_slowpath+0x13/0x20
[  678.236463]  mutex_lock_killable+0x3b/0x50
[  678.236468]  hid_hw_open+0x20/0xa0 [hid]
[  678.236476]  hidinput_open+0x15/0x20 [hid]
[  678.236482]  input_open_device+0xdb/0x140
[  678.236489]  driver_connect+0x545/0x610 [yeetmouse]
[  678.236496]  input_attach_handler.isra.0+0x7e/0xc0
[  678.236500]  input_register_device+0x1d6/0x370
[  678.236504]  hidinput_connect+0x53b/0xb20 [hid]
[  678.236510]  ? dma_alloc_attrs+0x141/0x1d0
[  678.236518]  hid_connect+0x2d1/0x400 [hid]
[  678.236524]  ? usb_alloc_urb+0x49/0x60
[  678.236530]  hid_hw_start+0x4c/0x70 [hid]
[  678.236537]  hid_generic_probe+0x2d/0x40 [hid_generic]
[  678.236540]  __hid_device_probe+0x112/0x1f0 [hid]
[  678.236547]  hid_device_probe+0xa7/0xb0 [hid]
...

This also bricked the whole usb_hid driver I think, as I was unable to plug-in new mice or unload the yeetmouse driver after this incident. But I have confirmed that the mouse acceleration does work on the mouse I had plugged in before starting the PC. I'll look into that later, but there are things to take a look at first.
Like for example the systemd config. Currently it does chown root:yeetmouse /sys/module/yeetmouse/parameters/*, but the parameters are now stored in /sys/class/yeetmouse/*/accel_config/*, so I tried to change it to 'chown root:yeetmouse /sys/class/yeetmouse/*/accel_config/*', and it does seem to work, but the problem is that it only works for mice that are already plugged in when the driver (service) starts. I guess one way to to solve this would be to use udev. This chown part needs to be done, because the GUI expects to have permission to the files in the /sys/class/yeetmouse/{MOUSE_NAME}/accel_config/ directory, otherwise it will show errors and won't work.
Next - are the per-device configs stored persistently (across reboots)? I believe that if all the configs are stored in /sys/class/..., the data will get wiped on power off.
About the GUI, I'll have to think about it, as (like I said before) I want to keep this functionality optional. So I'll probably want the device selection somewhat hidden - for example - on the top bar.

But like I said earlier, most of these just go back to the comment I referenced earlier. I want this to be an add-on, rather than a replacement for the current config handling, so that people who are currently using the program won't have to deal with a new set of possible bugs.

I'll push the test changes, because it's a simple one file change, but I'll hold with the systemd config for now, as it depends on the final approach