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

Add the ability to reuse USB endpoints for multiple alternative functions #3372

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

M3gaFr3ak
Copy link

@M3gaFr3ak M3gaFr3ak commented Sep 25, 2024

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 higher max_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 :)

@Dirbaio
Copy link
Member

Dirbaio commented Sep 25, 2024

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 let ep = builder.endpoint_bulk_out(); builder.endpoint_isochronous_out_allocated(ep); which will force the two EPs to have exactly the same number. This won't work, because you can't dynamically switch an EP between bulk and iso in nRF.

This is the reason why the Driver::alloc_endpoint_(in|out) API requests the endpoint type: so the driver can allocate the right endpoint number based on which numbers can do which kinds.

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:

  • if you request "1 bulk OR 1 iso"
    • on nRF it'll give you "bulk=1, iso=8".
    • on most chips it'll give you "bulk=1, iso=1"
  • if you request "1 bulk OR 1 interrupt"
    • on all chips it'll give you "bulk=1, interrupt=1" (because nRF CAN dynamically switch between them)

I'm not sure what's the best API design for this, it seems ... interesting

@M3gaFr3ak
Copy link
Author

@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.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 25, 2024

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:

  • Start interface
  • Start interface altsetting
  • Alloc bulk endpoint -> returns an instance of Endpoint
  • Start interface altsetting
  • Alloc iso endpoint -> returns an instance of Endpoint

but the returned Endpoints are not "physical endpoints", they're some kind of "logical endpoints" where two logical endpoints can have the same endpoint number, just at most one of them can be active at a time. This'd be some kind of "indirection" between the classes and the driver, done by embassy-usb. So if you do the above sequence, the two endpoints can receive the same number or different numbers depending on the hardware, and the class doesn't have to care about it.

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 other interfaces
  • in other altsettings of the current interface
  • in the current altsetting of the current interface
  • unused

in "start interface" you move endpoints from the "current interface" to "other interfaces".
In "start interface altsetting" you move endpoints from "current altsetting of the current interface" to "other altsettings of the current interface"
When allocating endpoints, pick any that's either in "other altsettings of the current interface" or "unused" states, and that is capable of the requested endpoint type.

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.

@M3gaFr3ak
Copy link
Author

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.

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.

2 participants