twitchyliquid64 / usbd-hid

MIT License
88 stars 37 forks source link

Adding SET_REPORT support #26

Closed haata closed 2 years ago

haata commented 3 years ago
haata commented 3 years ago

On one level, I think I agree that possibly having two different versions of the class might make things a bit easier. Basically a "reduced feature set" HIDClass and then another one that implements all of the extra features that most HID devices don't need (e.g. set/get report). But on the other hand, documentation will need to be pretty clear indicating what is missing from the simple version of HIDClass as it's not quite as simple as "you're missing a couple functions".

Also, I have another PR (#25, that I need to rebase) that implements get/set protocol. This is actually a requirement of the HID spec for both keyboards and mice to implement (basically anything that uses the Boot mode protocol; i.e. a 6kro keyboard that works in the BIOS or a keyboard that works on macOS during the encryption login screen). Basically, all keyboards that don't implement this will be out of spec.

EDIT: As long as the keyboard descriptor is setup correctly, it's possible to support a boot mode keyboard. But NKRO keyboards pretty much require get/set protocol to function correctly.

twitchyliquid64 commented 3 years ago

Understood, lets get #25 in and then we can focus on this one.

The main implication I'm not 100% about with this PR is that HIDClass needs to store a report, which is making assumptions about storage / how space should be managed. What do you think about having the 'report' that the HIDClass keeps track of, being type that HIDClass is generic over? That way implementations can define their own 'backend' for how they provide support for SET_REPORT, if at all.

haata commented 3 years ago

Sounds good. I'm a bit under the weather atm, but will try to look at it in the couple of days.

twitchyliquid64 commented 3 years ago

Take it easy and feel better!

haata commented 3 years ago

https://www.usb.org/sites/default/files/hid1_11.pdf Appendix G has a good overview of what's required depending on the type of keyboard or mouse is being implemented.

In practice, I've only needed to implement SET_REPORT, GET_PROTOCOL and SET_PROTOCOL to get decent Windows, Linux and macOS support.

GET/SET_IDLE is mainly for legacy OSs (macOS will use it, but it's rather buggy and Apple doesn't use it for their own devices).

GET_REPORT seems uncommon, but according to the HID spec is required for all devices. I haven't seen it used for keyboards.

twitchyliquid64 commented 2 years ago

Sorry for the massive delay on this! Is this (and the other PR) good to merge?

haata commented 2 years ago

Yep! Haven't made any additional changes on my own forks.