tud_hid_set_report_cb() is invoked with inconsistent arguments

[mjrgh]

Operating System

Others

Board

All (portable)

Firmware

class/hid/hid_device.c

tud_hid_set_report_cb() is invoked in two contexts, with inconsistent argument usage:

• From hidd_control_xfer_cb(). Before invoking the callback, the caller removes the first byte of the buffer IF it matches the HID report ID in the control request.
• From hidd_xfer_cb(). In this case, the caller leaves the whole buffer intact unconditionally and passes HID report ID == 0.

It's up to the callee to distinguish these two cases, to determine whether or not the buffer contains the HID report ID prefix byte. In versions prior to 0.17.0, it was possible to distinguish the cases based on the report type, since the second case always passed HID_REPORT_TYPE_INVALID. Starting in 0.17.0, both cases pass HID_REPORT_TYPE_OUTPUT, making the cases harder for the callback to distinguish.

The fix would be to either STOP stripping the report ID byte prefix in hidd_control_xfer_cb(), or START parsing it in hidd_xfer_cb(). Either way is going to be a breaking change, but the latter will probably have lower impact. And code that uses report IDs and implements tud_hid_set_report_cb() is ALREADY probably broken - not functioning as intended - unless the application developer noticed the inconsistency and added special-case code to work around it like I did. None of the examples that use tud_hid_set_report_cb() use report IDs, which is why they all get away without special-casing it. Presumably that's the most common case for actual code in the wild as well.

Note that there appears to be a second, related bug in hidd_control_xfer_cb(): it strips the first byte of the data buffer any time the report ID matches, even if the report ID is zero, which means "no report ID". In that case, it incorrectly strips a data byte when the first data byte just coincidentally happens to be 0x00.

What happened ?

Incorrect behavior in endpoint output report processing, requiring special-case handling in user code. The user code shouldn't need a special case to work around what should be a consistent callback interface.

How to reproduce ?

Create a program that uses HID report IDs in the report descriptors and implements a tud_hid_set_report_cb() callback. Observe the different behavior in the buffer between callbacks from control requests vs endpoint transfers.

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

N/A

Screenshots

No response

I have checked existing issues, dicussion and documentation

• I confirm I have checked existing issues, dicussion and documentation.

[mjrgh]

A way to fix this without breaking any existing application code might be to layer in a new "version 2" callback:

void tud_hid_set_report_cb2(uint8_t instance, int report_id, hid_report_type_t report_type, uint8_t const* buffer, uint16_t bufsize);

hidd_control_xfer_cb() would be changed to call the new callback, passing in the report ID from the control request, and NOT stripping the report ID prefix byte from the buffer.

hidd_xfer_cb() would also be changed to call the new callback, passing in -1 as the report ID. -1 is a special value meaning "caller doesn't know the report ID". (That's why the type of the report_id argument to the new callback is int, not uint8_t, to allow for this new explicitly distinguished "unknown" value.)

The library (class/hid/hid_device.c) would provide a weak implementation of tud_hid_set_report_cb2() as follows:

TU_ATTR_WEAK void tud_hid_set_report_cb2(uint8_t instance, int report_id, hid_report_type_t report_type, uint8_t const* buffer, uint16_t bufsize)
{
    if (report_id > 0 && bufsize > 1 && buffer[0] == report_id) {
        buffer++;
        bufsize--;
    }
    tud_hid_set_report_cb(instance, report_id >= 0 ? (uint8_t) report_id : 0, report_type, buffer, bufsize);
}

This would yield identical behavior to the current implementation for applications that don't override the new "v2" callback, and would provide a consistent interface to applications that DO care about the report ID, allowing them to parse the report ID as needed without resorting to fragile heuristics.

[mjrgh]

One more note: src/class/hid/hid_device.c incorrectly compares HID report ID bytes to HID_REPORT_TYPE_INVALID at lines 302 and 327 (as of 090964c). This is a type clash that the compiler doesn't catch, thanks to automatic coercion to int: HID_REPORT_TYPE_INVALID is a member of enum hid_report_type_t, which is a Control Report element; report IDs are a separate namespace conceptually. This accidentally works because the value of HID_REPORT_TYPE_INVALID happens to be zero, but it's confusing and misleading to use that name here. The comparisons at lines 302 and 327 should just be to the literal constant 0, or to a a new symbolic constant that's specifically defined for the "null" HID report ID.