-
Notifications
You must be signed in to change notification settings - Fork 791
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
Add the ability to reuse USB endpoints for multiple alternative functions #3372
base: main
Are you sure you want to change the base?
Conversation
…ot being compilable
…hing works as before on non-rp platforms for now
df0df03
to
4ae7296
Compare
4ae7296
to
f13d310
Compare
I think the proposed API can't work for all hardware. For example in nRF, endpoints 1-7 are always Bulk or Interrupt, and endpoint 8 is always Isochronous. With the API in this PR, If the user makes an interface where one altsetting requires a Bulk EP and another requires an Iso EP, they'll do This is the reason why the Ideally the endpoint sharing should take this into account, to overlap things as much as possible if the hardware allows it but falling back to not overlapping if not, so for example:
I'm not sure what's the best API design for this, it seems ... interesting |
@Dirbaio Thank you for your thoughts. I'm definitely open to changing the API. Maybe integrate the underlying logic more with the actual drivers and make the allocation an actual request instead of an "order", returning a Success/Fail Enum if the reused endpoint cannot fulfill the requested requirements? I guess this wont impact the underlying logic that much as most of the heavy lifting then relies on a validated descriptor, after configuration. |
not sure, if you make it a "request" it means all class implementations will have to do "request reusing an endpoint as X, if it fails then allocate a new one". This logic is exactly the same for all classes. I think it'd make more sense to make the builder API do this behind the scenes if it can. So, for example, you'd do this on the builder:
but the returned This is for the "class -> embassy-usb" API. I'm not sure what about the "embassy-usb -> driver" API. This could be done by tracking endpoint allocation status:
in "start interface" you move endpoints from the "current interface" to "other interfaces". this'd ensure the allocator overlaps endpoints as much as possible, as long as supported by the hardware. it's just an idea though, i'm not sure how well it'd work in practice. |
I'd invite everyone seeing this PR to contribute ideas for a better API. @Dirbaio already presented good ideas and I feel like the Logical vs Physical distinction is a good starting point. Maybe a setting in the builder for selecting allocation schemes should also be added. |
Solution for #3212 and #2994 .
This PR addresses the problem that currently endpoint addresses are automatically increased when building a USB descriptor. It adds a copy of each endpoint-creation function, adding the
_allocated
suffix. These functions accept a &mut reference to the already created endpoint and merely write a new endpoint descriptor. They also grow the buffer of the endpoint if the new descriptor has a highermax_packet_size
than the already allocated endpoint. The methods handling interface reconfiguration also automatically adjust all endpoints to reflect the settings in the desciptor.The relevant functions are only implemented for embassy-rp for now, the other packages have the methods implemented with a warning, so it works like before the PR.
Suggestions regarding commit rebasing, or different function names or how it works in general, are welcome :)