-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 provision for multiple configurations #318
Conversation
I like it. Last year, I made patch #167 to implement deconfiguration of interfaces (required by USB spec), but that branch is long out of date. I'm not sure if it's worth updating that PR, but I can if it's the direction that is chosen. Otherwise, I should close that PR. |
Mostly curious more than anything, but do OSes in practice support multiple configurations? Idk if We concluded that multiple configurations isn't supported well, so our plan is to use the control pipe to tell the device when it's time to switch between functionality. The device detaches itself from the bus and re-enumerates as a completely separate device with a single configuration. This gets around the multiple configuration problem when OSes don't support it meaningfully. |
I can speak to at least one scenario. I have an impending commit that has two configurations: RNDIS and CDC-ECM. Windows and Linux both pick the RNDIS configuration; macOS picks the CDC-ECM. I'm not aware of any OS actively changing configuration, but they certainly do pick their preferred choice of multiple configuration options. |
Thanks everyone for helpful discussion
Just take a quick look at your PR, the changes is not big, however the stack has changed a lot, and our understanding of the stack also changes as well. It is best to make a new PR, using that as inspiration, so don't close it yet for now. Implement multiple configuration isn't difficult, I think I did it when start writing the stack, but dropped it due to lack of way to test it (and I don't like to have code that isn't run at all in the repo). Configuration can be switched on-the-fly by Host's will, so we may need to add an close() API for class driver and DCD endpoint close. But that is easy enough. However @cr1901 is right, multiple configuration isn't a real thing in pratical world, the more common implementation is soft disconnect and change the description accordingly.
Yeah, RNDIS is Microsoft protocol, Apple is obviously has a reason to stay away. In fact I think they don't bundle RNDIS driver to their recent macOS. Meanwhile CDC-ECM isn't support natively by Windows, vendor mostly have to provide additional driver for it, much like In conclusion, I don't see any reasons why we couldn't support multiple configurations as long as we could test it. Since it is rarely used, we could have an additional option eg. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change config num as the 1st parameter of the descriptor helper.
// Interface count, string index, total length, attribute, power in mA | ||
TUD_CONFIG_DESCRIPTOR(ITF_NUM_TOTAL, 0, CONFIG_TOTAL_LEN, TUSB_DESC_CONFIG_ATT_REMOTE_WAKEUP, 100), | ||
// Interface count, string index, total length, attribute, power in mA, config number | ||
TUD_CONFIG_DESCRIPTOR(ITF_NUM_TOTAL, 0, CONFIG_TOTAL_LEN, TUSB_DESC_CONFIG_ATT_REMOTE_WAKEUP, 100, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the confi_number should be the 1st parameter though since it makes more sense that way. Even though it could break current usage. Most larger project use their own descriptor, smaller project should be easily change to this. Luckily I has just release 0.6.0 after like 1000 commit after 0.5.0 :). We can document it in changelog in migration section for next 0.7.0 release.
done |
Such a option macro should be unnecessary. All this PR does is change a macro to expose an additional argument; it doesn't add any code. The output binary should be identical to before. Any example code that uses multiple configurations only differs in its usb_descriptors.c |
Technically configuration can be switched on-the-fly by host back and forth. I don't know why host would do that, but it is possible. Those are additional code I am thinking about. However, yeah, we can do an trick to invoke an callback and let's application to do soft disconnect and reconnect when host switch configuration from non-zero to another different non-zero. !!! Actually I think we should start with that, throw callback into application, if user start to complain or bug arise, we will handle that later :D. Let's not write code that it didn't run :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR, this will probably break other repo builds. Although it is a quick fix :D
Presently, the bConfigurationValue is hard-coded to 1 in the macro TUD_CONFIG_DESCRIPTOR. That doesn't allow for multiple configurations.
This PR adds config_num as an additional argument to the macro, and revises the ./src/usb_descriptors.c of all the device examples to provide value 1 to this new argument. That way, future enhancements to the example code can provide multiple configurations.
Other groundwork for multiple configurations is already in place in (.bnumConfigurations and the argument to tud_descriptor_configuration_cb()).