HID class driver fixes #2253
HID class driver fixes #2253
Conversation
src/class/hid/hid_device.h
Outdated
| @@ -118,6 +118,8 @@ TU_ATTR_WEAK bool tud_hid_set_idle_cb(uint8_t instance, uint8_t idle_rate); | |||
| // Note: For composite reports, report[0] is report ID | |||
| TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint16_t len); | |||
|
|
|||
| // Invoked when a transfer wasn't successful | |||
| TU_ATTR_WEAK void tud_hid_report_issue_cb(uint8_t instance, uint8_t ep_addr, xfer_result_t result, uint16_t len); | |||
There was a problem hiding this comment.
Sorry for the delay, is this callback actually needed for your application ?
There was a problem hiding this comment.
Right now the app would not be informed if there was an issue due to the fasle positives. Any data received for a dedicated OUT endpoint would wrongly flagged as INVALID even there wasn't a communication issue.
Due to this the behavior between a dedicated OUT endpoint and if there is none, so that the data is transferred over the control endpoint, is different which isn't good in my opinion.
The callback is basically only there to pass the result state and so to may provide additional info to the app when an issue occurred. If there is no interest in that tud_hid_set_report_cb with HID_REPORT_TYPE_INVALID as a parameter would be fine.
I don't think there is actually a differentiation done between failed, stalled, a timeout or invalid. But depending on the app this is maybe interesting to know.
There was a problem hiding this comment.
I agree seeing INVALID is confusing, better rename into UNUSED.
With a dedicated EP we don't know the Report type so better to use UNUSED instead of OUTPUT.
I prefer to keep it simple for the moment, we can add the callback when there is really need. (anyway transfer failed is not implement by dcd so it won't be called)
There was a problem hiding this comment.
The data a dedicated OUT endpoint receives is from the type HID_REPORT_TYPE_OUTPUT per definition. It's not invalid nor unused!
Having a dedicated OUT endpoint or let it handled via the control endpoint only depends how you want to configure the device. I don't think it's good that TinyUSB handles received data for an interface differently only because the way it's done is different. After all the purpose is the same, a specific interface receives data and so the implementation should not care which endpoint was used.
If there is no interest in the new callback, the call for that should be replaced by:
tud_hid_set_report_cb(instance, 0, HID_REPORT_TYPE_INVALID, p_hid->epout_buf, (uint16_t) xferred_bytes);
In that case the app will at least be informed about an issue even it has no chance figuring out why it failed.
There was a problem hiding this comment.
The new callback should also allow tracking issues for IN endpoints. Without that this not possible.
There was a problem hiding this comment.
The data a dedicated OUT endpoint receives is from the type HID_REPORT_TYPE_OUTPUT per definition. It's not invalid nor unused!
True.
I've simplified xfer_result_t result as failure reasons are only used by hcd.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- rename tud_hid_report_fail_cb() to tud_hid_report_failed_cb() and change its signature - use default implementation for hid callbacks to be compatible with keil compiler - code format
Addressing some drawbacks of the HID class driver implementation.
tud_hid_set_report_cbcallback would always inform about an invalid result state if a dedicated OUT endpoint is used. In the case the optional OUT endpoint is used outgoing transfers are then handled byhidd_xfer_cband not byhidd_control_xfer_cbanymore. Then the HID examples wouldn't work anymore because the result state wouldn't be right.tud_hid_report_complete_cbandtud_hid_set_report_cbif the result state of the corresponding transfer was successful. While the comment for thetud_hid_report_complete_cbindicate that this is only invoked when the report was sent successfully it wasn't even checked that this is the case. This can cause problems in the case there is a communication issue.