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

Allow use of MSC/CDC USB endpoints for bulk on sam3u2 #1022

Closed
wants to merge 3 commits into from

Conversation

9names
Copy link

@9names 9names commented Apr 1, 2023

The sam3u2 USB peripheral does not have enough endpoints after CDC and MSC are enabled to enable the BULK endpoints. Bulk endpoints are required for CMSIS-DAP v2 support - the existing projects for this HIC use HID+MSC+CDC

This PR makes it possible for the sam3u2 to enable BULK if either CDC or MSC are disabled.
In order to demonstrate this setup, the sam3u2c_bulk_test_if project has been added which uses HID+CDC+BULK,

I have tested this configuration with a nanoDAP-hs, probe-rs-cli and few computers I had at my disposal:
raspberry pi 4 - hid: 90KB/s, bulk: 152KB/s
AMD 7700x, ubuntu 22.10 - hid: 17KB/s, bulk: 280KB/s
intel u5200, windows 10 - hid: 100KB/s, bulk: 160KB/s

Not sure why my AMD box is so slow with HID, but the speed-up is very significant in this case.

@9names
Copy link
Author

9names commented Apr 2, 2023

Would it make more sense to move usb-msc out of bl and if, and have it added by default to each HIC where it currently exists (except for sam3u2)?
The HICs already add usb-bulk and usb-hid themselves, so it would seem reasonable to include usb-msc there as well.

@mbrossard
Copy link
Contributor

The additive nature of DAPLink's build configuration is problematic for interface that have limited amount of endpoints. I did also consider your idea of splitting further the configuration of bl / if projects, but I got worried it would grow the verbosity of the projects.yaml. Instead I am leaning towards adding the option to disable functionality from the default interface configuration. I ported some of the changes I had started to make onto the feature/flexible-usb-configuration branch and modified sam3u2c_if to disable MSC endpoint and replace HID with BULK. I am not 100% sure about modifying sam3u2c_if rather than adding a new project, but this is what I am leaning towards.

I would have usually preferred to go with your change to usb_config.c (which is nicely compact). But in this case where things are a little bit complicated (I don't understand how things work with USBD_HID_EP_INTOUT being set to 0), I would rather be very explicit on the endpoint assignments.

@9names
Copy link
Author

9names commented Apr 3, 2023

Your version looks better to me :)

I opted out of modifying existing projects or removing HID because I was worried existing users might expect everything to stay the same, but I assume you know your users better than I do.
Turning off HID when BULK is enabled is something I would have done if it were as simple as it is for your branch.

If this were a pure cmake project, I would have made these things selectable at configure time and overridable at build time, but I don't know pygen well enough to know what is possible and what is considered idiomatic usage

@mathias-arm
Copy link
Collaborator

Replaced by #1041

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.

3 participants