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

usbhs: implement USBHS USB device driver #63

Merged
merged 8 commits into from
Nov 4, 2024

Conversation

Dummyc0m
Copy link
Member

@Dummyc0m Dummyc0m commented Nov 3, 2024

Implement the Embassy USB driver for the USBHS peripheral. The
implementation largely mirrors the OTG_FS driver and has similar
limitation such as non-configurable endpoint buffer sizes, in/out on the
same endpoint index, lack of iso transfer, and untested bulk transfer.

This change requires the latest fixes in ch32-metapac due to incorrect
striding of the endpoint control registers.

Tested on CH32V305 with the following applications:

  • USB HID device, for testing non-control endpoints interrupt transfer.
  • USB DFU, for testing the control endpoint.

Co-authored-by: Harry Brooke harry.brooke1@hotmail.co.uk
Co-authored-by: Codetector codetector@codetector.org
Co-authored-by: Dummyc0m xieyuanchu@gmail.com

Codetector1374 and others added 6 commits November 2, 2024 23:13
Since all uses of naked_functions have been removed, remove the feature
declaration to build on the stable toolchain.
ExplodingWaffle pointed out[1] the host is allowed to send a shorter
packet than what we were expecting. Remove the assertions and links to
embassy.

[1] ch32-rs#59 (comment)

Reported-by: Harry Brooke <harry.brooke1@hotmail.co.uk>
Refactor to improve code reuse in USBHS.
Calling .unwrap can result in extra strings and bloat. Use the defmt
unwrap macro.
Setup USBHS related RCC register values.

Co-authored-by: Harry Brooke <harry.brooke1@hotmail.co.uk>
Co-authored-by: Codetector <codetector@codetector.org>
Co-authored-by: Dummyc0m <xieyuanchu@gmail.com>
@Dummyc0m
Copy link
Member Author

Dummyc0m commented Nov 3, 2024

@ExplodingWaffle I didn't include your usb serial example because I was lazy and didn't want to polish it up. Feel free to add it as a follow-up.

@Dummyc0m Dummyc0m requested a review from andelf November 3, 2024 09:20
Copy link
Member

@Codetector1374 Codetector1374 left a comment

Choose a reason for hiding this comment

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

LGTM

@ExplodingWaffle please also take a look to make sure nothing is crazy

Copy link
Contributor

@ExplodingWaffle ExplodingWaffle left a comment

Choose a reason for hiding this comment

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

looks good to me! only saw two things that stuck out. thanks you guys 🙂

src/usbhs/mod.rs Outdated
Direction::Out => {
T::dregs().ep_config().modify(|v| v.set_r_en(index - 1, enabled));
T::dregs().ep_rx_ctrl(index).write(|v| {
v.set_mask_uep_r_tog(EpTog::DATA1);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn’t this be DATA0? same for below

Copy link
Member Author

@Dummyc0m Dummyc0m Nov 3, 2024

Choose a reason for hiding this comment

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

Ah, I'll switch the data toggling to where NAK is set in the endpoint read/write. This is DATA1 because the data is toggled when the endpoint is set to ACK, so the first one can be DATA0. I'll test when I wake up lol.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we should test with checking the tog_ok, otherwise this is actually not doing much

src/usbhs/mod.rs Outdated

let ep0_buf = self.allocator.alloc_endpoint(control_max_packet_size).unwrap();
d.ep0_dma().write_value(ep0_buf.addr() as u32);
d.ep_max_len(0).write(|w| w.set_len(64));
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be control_max_packet_size. not that it’ll be anything other than 64 unless ls/fs support is added later

Dummyc0m and others added 2 commits November 3, 2024 12:24
Implement the Embassy USB driver for the USBHS peripheral. The
implementation largely mirrors the OTG_FS driver and has similar
limitation such as non-configurable endpoint buffer sizes, in/out on the
same endpoint index, lack of iso transfer, and untested bulk transfer.

This change requires the latest fixes in ch32-metapac due to incorrect
striding of the endpoint control registers.

Tested on CH32V305 with the following applications:
- USB HID device, for testing non-control endpoints interrupt transfer.
- USB DFU, for testing the control endpoint.

Co-authored-by: Harry Brooke <harry.brooke1@hotmail.co.uk>
Co-authored-by: Codetector <codetector@codetector.org>
Co-authored-by: Dummyc0m <xieyuanchu@gmail.com>
Small cleanup in EndpointBufferAllocator to ensure we don't request max_packet_size > 64
Copy link
Contributor

@andelf andelf left a comment

Choose a reason for hiding this comment

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

LGTM. Let's ship this 🚀

@andelf andelf merged commit 5596607 into ch32-rs:main Nov 4, 2024
11 checks passed
@Codetector1374 Codetector1374 deleted the ch32-hal-usbhs-feat branch November 26, 2024 23:31
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.

4 participants