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

usb: Make driver generic over max interfaces #1102

Closed

Conversation

yodaldevoid
Copy link
Contributor

It would be nice if a default value could be used so old code would mostly just work, but defaults are not used while determining types so adding a default does nothing here.

It would also be nice if we could calculate the number of interfaces needed at compile time, but I'll leave that as an exercise for later.

rust-lang/rust#95486
rust-lang/rust#99727
rust-lang/rust#27336

@yodaldevoid yodaldevoid force-pushed the usb_interfaces_generic branch 3 times, most recently from 48375bd to fe7d0c4 Compare December 9, 2022 05:34
embassy-usb-logger/src/lib.rs Outdated Show resolved Hide resolved
@alexmoon
Copy link
Contributor

alexmoon commented Dec 9, 2022

I’m not a big fan on the generics/turbofish spam this generates. Could you use an associated const in the driver to define the max interfaces a driver supports and use that in the builder? The builder should be short lived and on the stack so a little extra memory usage shouldn’t be a big problem there. The build method could then be generic on the number of interfaces for the device with a default value.

@yodaldevoid
Copy link
Contributor Author

I’m not a big fan on the generics/turbofish spam this generates. Could you use an associated const in the driver to define the max interfaces a driver supports and use that in the builder? The builder should be short lived and on the stack so a little extra memory usage shouldn’t be a big problem there. The build method could then be generic on the number of interfaces for the device with a default value.

I am also not happy with the noise this creates, though it should only require one annotation per device. It is amplified here due to the number of examples in this project that use USB. The only way I can think to decrease this noise in the typical case where a user doesn't need many interfaces and doesn't care about minimizing the number of interfaces is to set a default, but as stated in the top post this doesn't work at the moment as we would like.

As for your suggestion, the maximum number of interfaces isn't dependent on the device but rather how much memory you want to give to store descriptors and interface handlers.

@alexmoon
Copy link
Contributor

As for your suggestion, the maximum number of interfaces isn't dependent on the device but rather how much memory you want to give to store descriptors and interface handlers.

Ah, right. I was thinking of endpoints not interfaces. Oh well. In that case I can't think of a good alternative to avoid the generic spam and still allow arbitrary numbers of interfaces without excess memory consumption. So this looks ok to me.

It would be nice if a default value could be used so old code would
mostly just work, but defaults are not used while determining types so
adding a default does nothing here.

It would also be nice if we could calculate the number of interfaces
needed at compile time, but I'll leave that as an exercise for later.

rust-lang/rust#95486
rust-lang/rust#99727
rust-lang/rust#27336
@alexmoon
Copy link
Contributor

alexmoon commented Feb 7, 2023

@Dirbaio suggested on matrix that we use features to allow for compile time configuration of the max interface value rather than const generics to avoid the generics spam. For example: smoltcp-rs/smoltcp#742

@Dirbaio
Copy link
Member

Dirbaio commented Feb 7, 2023

I'm going to close this PR in favor of #1202, which does that. Thanks anyway for the contribution! :)

@Dirbaio Dirbaio closed this Feb 7, 2023
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