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.

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

spresense: fix setup processed flag #680

Merged
merged 1 commit into from
Feb 24, 2021
Merged

Conversation

kamtom480
Copy link
Collaborator

Describe the PR
Fix usb setup flag which sometimes caused DTR bit detection problem.

@kamtom480 kamtom480 requested a review from hathach February 24, 2021 16:43
@kamtom480 kamtom480 merged commit 280297b into hathach:master Feb 24, 2021
@kamtom480
Copy link
Collaborator Author

@hathach I merged it. I hope that's not a problem.

@@ -329,6 +329,7 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t *buffer, uint16_t to
{
if (total_bytes == 0)
{
usbdcd_driver.setup_processed = true;
Copy link
Owner

@hathach hathach Feb 24, 2021

Choose a reason for hiding this comment

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

if you want to detect status packet with this logic, it can mis-inteprete other event. For example when receiving 64 (or 128) bytes of a descriptor, to end the data transfer, ZLP is sent in the DATA stage on the IN endpoint. Afterwards there is another ZLP on OUT endpoint for status. This cause the usbdcd_driver.setup_processed set to true before the ZLP is scheduled (and set to true twice).

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

There is a better way to catch status, it is an optional callback dcd_edpt0_status_complete() which is added later after your port. The usbd will call this function right after status phase is complete (full transferred). You could use this to pull setup packet without the need of usbdcd_driver.setup_processed)
https://github.com/hathach/tinyusb/blob/master/src/device/dcd.h#L124

@hathach
Copy link
Owner

hathach commented Feb 24, 2021

@hathach I merged it. I hope that's not a problem.

Not a problem at all, you understand the dcd much better than me. Though looking through the chance, I think there is probably a better approach with dcd_edpt0_status_complete() some dcd make use of this for changing address after status complete and prepare the next setup https://github.com/hathach/tinyusb/blob/master/src/portable/microchip/samd/dcd_samd.c#L187, some doesn't need it therefore it is optional.

PS: There is another breaking API as well with supporting multiple hid interfaces #678, that could break your circuitpython ci. It is a easy fix, I will comment on the circuitpython PR.

@kamtom480
Copy link
Collaborator Author

Thanks for your comments! This is very useful feedback. I'll try to use dcd_edpt0_status_complete() instead of usbdcd_driver.setup_processed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants