TeXitoi / keyberon

A rust crate to create a pure rust keyboard firmware.
MIT License
1.08k stars 79 forks source link

Allow alternate key code types #99

Closed ithinuel closed 1 year ago

ithinuel commented 2 years ago

These changes allow to use alternate types like those available in usbd-human-device-interface, or a union of them.

jedrzejboczar commented 2 years ago

Hi, I wonder what is the use case for these changes alone? I suppose you still need to implement your own HID class with different HID descriptor, am I right?

I was thinking about replacing the current implementation of keyberon::hid_class with usbd-hid. The primary issue I see in the current implemenation is that media keys (like next/previous track) are not properly supported (not working on Mac/Windows). usbd-hid already defines things like MediaKey and MediaKeyboardReport, which would allow extending keyberon to send media keys using HID Usage Page: Consumer, which should also work on Mac/Windows.

jedrzejboczar commented 2 years ago

When thinking more about it, it's probably better for users to write their own HID class implementations if more functionality than the default is needed. I initially wanted to extend the class in keyberon, but it is hard to create something generic enough for all the possible use cases (boot keyboard, n-key rollover, mouse, consumer keys, etc.) while also keeping in mind that some microcontrollers may have limited number of endpoints and/or limited endpoints memory.

And if one decides to write their own HID interfaces (e.g. with the help of usbd-hid which provides the necessary framework) then Action::Custom can be used for anything that is not from the Keyboard Usage Page. I am not sure if this is a good idea to make KeyCode generic, as it adds a lot of complexity (passing that generic parameter all around) and I feel like Action::Custom is a good alternative.

ithinuel commented 2 years ago

Hi @jedrzejboczar , as indicated in the first post, writing ones own HID class is exactly what usbd-human-device-interface does.

I switched to it after hitting some incompleteness in the class provided in Keyberon (I cannot remember which request wasn't handled). You can check an example integration here. Note renaming is purely to shorten the key's name and keep the line length reasonable.

Using directly the right keycode types saves me from having to convert them again before pushing to the HID class.

Also, note the introduce generic defaults to the value it replaces (KeyCode) so for any user of keyberon::action::Action and keyberon::layout::Layout the change is transparent.

jedrzejboczar commented 2 years ago

@ithinuel Thanks for explanation. I'm currently trying to improve HID on my keyboard (boot-compatible NKRO, System & Media keys) and this was really helpful. I was using usbd-hid, but I might consider switching to usbd-human-device-interface, though it looks more complex and I wonder how this would affect code size.

Anyway, I tried compiling my firmware with keyberon version from this PR and it compiled without any issues, so that changes is indeed transparent.

I suppose this could be used to replace KeyCode with some enum { KeyCode(K), Consumer(K), ... } and a custom KeyboardReport for a custom HidClass would check list of keycodes from this enum to generate the report. On the other hand, this could be implemented without this PR by using CustomAction - when generating KeyboardReport: first scan keycodes, then apply any CustomActions. The main difference is that layout.keycodes() provides an iterator, while layout.tick() will return a single CustomEvent per tick, so it will require additional work to maintain "pressed" CustomActions in the report.

dlkj commented 2 years ago

@jedrzejboczar If you do take a look at usbd-human-device-interface, I'd love to get any feedback or thoughts you have. It's my first serious attempt at a rust library, and I already have a short list of simplifications and improvements for a v1. I'm friends with @ithinuel IRL, I'm not just stalking every mention of my projects ;-).

jedrzejboczar commented 2 years ago

@dlkj Sure, I'll probably try to get something working with usbd-hid for now as I'm already using it, but if I move to usbd-human-device-interface then I'll try to give some feedback :)

ithinuel commented 1 year ago

@TeXitoi Is there anything blocking the PR from being merged in?

TeXitoi commented 1 year ago

I didn't had the time to review. Sorry for the delay.

TeXitoi commented 1 year ago

Thanks

ithinuel commented 1 year ago

Thank you!