-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
bugfix(ch32-hs-dcd): fix ch32 DATAx managment and long packet transmit #2392
Conversation
7b3fb59
to
ddf329f
Compare
This MR seems to resolve issues I was having getting writes to work reliably with the MSC device class. 👍 |
My problem in #2377 is also fixed with this MR! |
@hathach Could you check and merge PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still haven't got time to test this out. I am off for TET holidays for a couple of weeks. Will try to review afterwards, thank you for your patient.
@hathach how can I help to complete this PR? |
Thank you for your great work, although I don't have CH32 device to test with... |
1a5eac8
to
3e604d1
Compare
…p0_buffer reformat indent to 2 spaces
- merge USBHS_ISO_ACT_FLAG, USBHS_TRANSFER_FLAG handler since they are similar - improve uart output - add note for link speed in bus reset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superb ! thank you very much for the fix as well as ISO support. I have rebased, merge interrupt handler for ISO and Transfer (since they are similar), also reformat for the code, and add some minor clean up. All is good, tested with v307. Will merge when ci passed.
Just it seems dcd_event_sof is lost on merge. |
oops, sorry, let me double check that |
} else { | ||
USBHSD->INT_EN &= ~(USBHS_SOF_ACT_EN); | ||
} | ||
void dcd_sof_enable(uint8_t rhport, bool en) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HiFiPhile does this look good ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dcd_sof_enable() looks good, only dcd_event_sof() is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HiFiPhile does this look similar to your previous changes (sorry to loose it while rebasing)
PS: adding frame number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sof was added in #2181, dcd_event_sof(rhport, USBHSD->FRAME_NO, true);
is better as it has frame number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documentation is a bit lacking for frame count on HS. For example uac2 have msof is easier to do calculation. Dwc2 doesn't mention msof at all but interrupt is correctly at 8khz.
I think better to standardize msof is used or not.
Another idea is to to add a virtual sof counter in _usbd_dev
to overcome dcd differences, what do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we probably need to normalize the micro-frame/frame counter, maybe using the top 3 msb bit in uint32_t for application, or just change the tud_sof_cb() to have an typedef struct. I will do that with follow-up PR later on.
USB specs say there are 8 indentical sof packet for highspeed device, which is kind of annoying
Old implementations have some issues:
Fixed this issue #1891 also other examples now work stable
UPD:
Pushed one more commit, fixed iso transfers, probably this issue fixed: #2325