Add HID descriptors #6
Conversation
| class HIDDescriptor(ComplexDescriptorEmitter): | ||
| DESCRIPTOR_FORMAT = HIDDescriptorType | ||
|
|
||
| def add_report(self, report_enum, *report_data): |
There was a problem hiding this comment.
Where does report come from here? To me, when I talk about HID reports, they are the reports exchanged between the host and the device. This is actually a HID report descriptor item, I would rename it add_item. I would also rename add_input to add_input_item, etc. but that's just nitpicking.
There was a problem hiding this comment.
I do suppose this can be documented better. A HID report item consists of a 4 bit tag and a 2 bit type, however, tags are not quite consistent within types, so only the 6 bit combined tag/type value has any map-able meaning. Input/output/feature items are special, because while other report items pretty consistently store byte-oriented data within their additional data fields, input/output/feature items contain a 9-bit bitmap of flags (only 8 bits of which are supported within this PR, as setting the 9th bit will make things a bit of a mess). Input/output/feature items have convenience functions for this reason, as generating them is less trivial than other report items.
There was a problem hiding this comment.
Sorry for not explaining correctly. My point was not about the documentation, although that is always better, but about the naming of the method. I believe the name add_report is not correct, IMO it should be called add_item instead 😊
There was a problem hiding this comment.
Do the changes in 2e7f95f address this?
There was a problem hiding this comment.
I would also rename add_report_raw acordingly, but overall is fine.
Renamed to clarify that the HIDDescriptor convenience functions add report items, not full reports.
De-plural-ify HIDPrefixes to match common Enum naming conventions
Change report_enum argument to report_prefix to clarify that it should be a usb_protocol.types.descriptors.hid.HIDPrefix enum element.
There was a problem hiding this comment.
Thanks for the contribution!
I really do want to have HID emitters/parsers added; but I'm not sure I agree with the high-level approach, here:
- It'd be best to treat the different HID descriptors / elements separately in our emitters. I like the idea of these behaving similarly to the other emitters -- e.g. we could have a
HIDDescriptorand aHIDReportDescriptoras separate classes, and then haveHIDReportDescriptorbe capable of stringing together items to make a single report descriptor. I don't see any issues with adding these two descriptors separately to their parent descriptor. - I'm not sure I understand the logic behind
add_input_item-- unless I'm misunderstanding, it seems to create a single-item report descriptor, rather than allowing a collection of items in a single report descriptor.
|
|
||
| _hid_item_length = [ 0, 1, 2, 4 ] | ||
|
|
||
| class HIDDescriptor(ComplexDescriptorEmitter): |
There was a problem hiding this comment.
There seem to be a bunch of naming inconsistencies, here.
For example, this looks like it produces more than a HID descriptor (it also builds report descriptors), so perhaps this would be better called e.g. a HIDDescriptorCollection?
HID does require 2 separate descriptors, but for some reason, the protocol decided to put the report descriptor length in the HID descriptor. This means that the HID descriptor needs to be aware of the length of the HID report, and I've made the decision to tie the two descriptors together. Not saying it's the right decision, I actually think it's pretty ugly and I'd be happy to hear alternate ideas. Additionally, the HID report descriptor doesn't follow the traditional pattern of "second byte is the type." This means that if the user needs to add the HID report descriptor manually, they need to be aware that the descriptor type must be manually set.
The INPUT, OUTPUT, and FEATURE items are a bit special in that they (as far as I can see) are the only report items that utilize a bitmap in their data field. |
|
I did a little work on this myself, but I think I'm going to abandon the work I did. I can't get construct to be bidirectional to deal w/ the fact that input/output report item's data can be 1 or 2 bytes. but in most cases this isn't needed, so if someone wants to rip that code out, and adapt the tests from my change, feel free. https://github.com/jmgurney/python-usb-protocol/tree/hid |
|
I'm going to close this until we have a RFC in place specifying how we're going to add support for USB HID Descriptors. |
Add basic emitters for USB HID.