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

bugfix(ch32-hs-dcd): fix ch32 DATAx managment and long packet transmit #2392

Merged
merged 5 commits into from May 21, 2024

Conversation

Krasutski
Copy link
Contributor

@Krasutski Krasutski commented Dec 28, 2023

Old implementations have some issues:

  • EP0 OUT transfer just copied data from the buffer without any checking
  • TOG1 bit set not correctly for OUT xfers
  • Long packets(more than ep size) xfer incorrectly

Fixed this issue #1891 also other examples now work stable

UPD:
Pushed one more commit, fixed iso transfers, probably this issue fixed: #2325

@Krasutski Krasutski force-pushed the bugfix/ch32-hs-dcd branch 5 times, most recently from 7b3fb59 to ddf329f Compare January 4, 2024 10:22
@zx96
Copy link
Contributor

zx96 commented Jan 21, 2024

This MR seems to resolve issues I was having getting writes to work reliably with the MSC device class. 👍

@WilheJo
Copy link

WilheJo commented Jan 22, 2024

My problem in #2377 is also fixed with this MR!

@Krasutski
Copy link
Contributor Author

@hathach Could you check and merge PR?

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.

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.

@Krasutski
Copy link
Contributor Author

Krasutski commented Apr 9, 2024

@hathach how can I help to complete this PR?

@HiFiPhile
Copy link
Collaborator

Thank you for your great work, although I don't have CH32 device to test with...
I just merged upstream SOF support to make test easier for other people, please take a look if it's good for you.

- merge USBHS_ISO_ACT_FLAG, USBHS_TRANSFER_FLAG handler since they are similar
- improve uart output
- add note for link speed in bus reset
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.

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.

@HiFiPhile
Copy link
Collaborator

Just it seems dcd_event_sof is lost on merge.

@hathach
Copy link
Owner

hathach commented May 21, 2024

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) {
Copy link
Owner

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 ?

Copy link
Collaborator

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.

Copy link
Owner

@hathach hathach May 21, 2024

Choose a reason for hiding this comment

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

HOST SOF bit in the interrupt imply that this is only for host most, not sure if it is also used for SOF detection in device mode
image

never mind, found it in the int en, SOF generate transfer complete

image

we will need to use the SOF token to detect it

Copy link
Owner

@hathach hathach May 21, 2024

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)

image

PS: adding frame number

Copy link
Collaborator

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.

Copy link
Owner

@hathach hathach May 21, 2024

Choose a reason for hiding this comment

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

thank you, I figure that out, but we should mask 11 bit only, since upper 3 bit is micro-frame index which actually increased faster than frame count. Phew I should be more careful with rebasing, thank you for reviewing the changes.

image

Copy link
Collaborator

@HiFiPhile HiFiPhile May 21, 2024

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 ?

Copy link
Owner

@hathach hathach May 21, 2024

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

image

@hathach hathach merged commit 1f259b3 into hathach:master May 21, 2024
76 checks passed
@Krasutski Krasutski deleted the bugfix/ch32-hs-dcd branch May 21, 2024 13:25
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.

None yet

5 participants