Driver for mice with Holtek 8-bit MCUs #1561
Conversation
|
Hi! Thank you for your PR. I didn't look at the changes yet, but I noticed you didn't update the device file test. Could you update it too? libratbag/test/data-parse-test.py Line 321 in 6db5cc3 |
|
I rewrote the test a bit for sinowealth and holtek so they can have their own check_section functions |
Delay must be greater than 0
|
|
||
| switch (drv_data->api_version) { | ||
| case HOTLEK8_API_A: | ||
| assert(sizeof(drv_data->api_a.password) == 6 && sizeof(device_data->password) == 6); |
There was a problem hiding this comment.
Should be able to use _Static_assert here, but for me clangd complains with 'Expected expression' for some reason, even though it compiles fine with GCC.
As clang-tidy complains.
Even though switch cases above are exhaustive, GCC complains with 'control reaches end of non-void function' anyway. Well, *in theory* if we wrote user input to the switched on variable, this could be possible, but we don't do so. Anyway...
Regression from 4d4a179
There was a problem hiding this comment.
Looked through about 60% of changes, looks good and clean so far, with just a few minor suggestions! Good job!
Also pushed a few commits:
- some const correctness; otherwise mutableness spreads like plague sometimes, making it hard to write new const-correct code
- Don't use 'default' for exhaustive switch cases; when adding new fields you'd be able to get compile warning for unhandled values, and right now those should be unreachable anyway, although enums in C are a landmine
- fix some implicit conversion; another landmine, I will get another look at it with -Wconversions later
All 44 of them... :D
And remove useless comment while we're here.
Technically something which could be compatible with ver.A is also found in HT68FB571, but they are limited in hardware to 4 endpoints so they chose to break the HID spec and have an interface without IN interrupt endpoint so it can't even probe without linux kernel patches and as we don't even have a good way to support devices without input capability (like saving profiles to a file), let's not bother with those few devices for now.
|
Thank you for the review so far! I admit I'm a bit terrible at catching things like implicit conversions so I may need to experiment with various warnings enabled too. |
|
Sorry for taking long, I'll try taking a look at it again soon. I want to review it thoroughly and make a release of libratbag after merging it. |
|
Sure! If you plan a release soon, you can take a look at my other PR too as that may break distro builds with meson test enabled and Python 3.12 (happened to me on Fedora already). |
Enabled resolutions need to go first because there is no option to disable them in place so we need to reorder them on api A. But then we can store disabled resolution afterwards.
These two bytes can be interpreted in different ways, if a second byte is a special command identifier insted of a hid key, or if the command is WAIT then the next event is just two byte delay in big endian.
These functions clear all reports which may have happened after ratbagd started, for example device's responses to another configuration program. Useful especially for drivers which cannot differentiate between valid and invalid reports due to their raw structure.
This fixes a bug where writing a macro to one button could override other macros.
|
Thanks for the review! I addressed all suggestions and also fixed one bug with macro writes which required some changes to macro page handling. |
Hi! This pull request introduces a new driver for mice with Holtek 8-bit MCUs (such as HT68FB550 and HT68FB560) with OEM firmware. Unfortunately the ABI between devices is not stable but with supporting just 2 API types we are able to support a good fraction of devices. Devices which use these MCUs but with their own proprietary firmware such as Redragon are obviously not supported. I only have one device of each type so the rest is obviously untested and their device file are not yet added but I could verify with an usb emulator and official softwares of many devices that a lot of the functionality is common between them.
This includes reading and writing of profiles, resolutions, rates, buttons and macros and all of this is implemented in this driver. This doesn't include LEDs (illumination modes are different but with a bit of work it may be possible to represent in device file), debounce times (ranges are different but probably possible to have in device file) and liftoff distance (not planned, ABI are completly unstable and few devices support it).
Device file loading is based on sinowealth's because especially devices of api ver.A share a common vid:pid pair and devices are recognised based on their firmware version, which is conveniently stored in bcdDevice field of usb device descriptor.
Configuartion interface doesn't use numbered reports at all, instead exchanging data via constant length chunks, which are interpreted based on previously sent SET feature report with a command and parameters. This can lead to unsynchronized reads/writes which are known to soft brick devices even using official software, so we actually do this part better, checking if the device really is expecting data.
I decided to have both apis as seperate drivers so I could handle their behavior more clearly which led to some code duplication but I tried to handle as much shared behavior in a shared file as possible.