Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HID class driver fixes #2253

Merged
merged 5 commits into from
Apr 26, 2024
Merged

HID class driver fixes #2253

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
11fba84
Error handling on transfer
Rocky04 Sep 13, 2023
81e63ed
Fixing warning
Rocky04 Sep 13, 2023
4af67dd
Simplify transfer failure cb.
HiFiPhile Apr 26, 2024
268cc19
Merge remote-tracking branch 'remotes/tinyusb/master' into pr/2253
HiFiPhile Apr 26, 2024
24339db
Code format.
HiFiPhile Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/class/hid/hid_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,6 @@ bool hidd_control_xfer_cb (uint8_t rhport, uint8_t stage, tusb_control_request_t

bool hidd_xfer_cb(uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint32_t xferred_bytes)
{
(void) result;

uint8_t instance = 0;
hidd_interface_t * p_hid = _hidd_itf;

Expand All @@ -394,6 +392,25 @@ bool hidd_xfer_cb(uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint32_
}
TU_ASSERT(instance < CFG_TUD_HID);

// Check if there was a problem
if (XFER_RESULT_SUCCESS != result)
{
// Inform application about the issue
if (tud_hid_report_issue_cb)
{
tud_hid_report_issue_cb(instance, ep_addr, result, (uint16_t) xferred_bytes);
}

// Allow a new transfer to be received if issue happened on an OUT endpoint
if (ep_addr == p_hid->ep_out)
{
// Prepare the OUT endpoint to be able to receive a new transfer
TU_ASSERT(usbd_edpt_xfer(rhport, p_hid->ep_out, p_hid->epout_buf, sizeof(p_hid->epout_buf)));
}

return true;
}

// Sent report successfully
if (ep_addr == p_hid->ep_in)
{
Expand All @@ -402,10 +419,10 @@ bool hidd_xfer_cb(uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint32_
tud_hid_report_complete_cb(instance, p_hid->epin_buf, (uint16_t) xferred_bytes);
}
}
// Received report
// Received report successfully
else if (ep_addr == p_hid->ep_out)
{
tud_hid_set_report_cb(instance, 0, HID_REPORT_TYPE_INVALID, p_hid->epout_buf, (uint16_t) xferred_bytes);
tud_hid_set_report_cb(instance, 0, HID_REPORT_TYPE_OUTPUT, p_hid->epout_buf, (uint16_t) xferred_bytes);
TU_ASSERT(usbd_edpt_xfer(rhport, p_hid->ep_out, p_hid->epout_buf, sizeof(p_hid->epout_buf)));
}

Expand Down
4 changes: 4 additions & 0 deletions src/class/hid/hid_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, is this callback actually needed for your application ?

Copy link
Contributor Author

@Rocky04 Rocky04 Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@HiFiPhile HiFiPhile Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@Rocky04 Rocky04 Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Rocky04 Rocky04 Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new callback should also allow tracking issues for IN endpoints. Without that this not possible.

Copy link
Collaborator

@HiFiPhile HiFiPhile Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


//--------------------------------------------------------------------+
// Inline Functions
Expand Down Expand Up @@ -411,6 +413,8 @@ uint16_t hidd_open (uint8_t rhport, tusb_desc_interface_t const * itf
bool hidd_control_xfer_cb (uint8_t rhport, uint8_t stage, tusb_control_request_t const * request);
bool hidd_xfer_cb (uint8_t rhport, uint8_t ep_addr, xfer_result_t event, uint32_t xferred_bytes);

void test(void);

#ifdef __cplusplus
}
#endif
Expand Down