-
Notifications
You must be signed in to change notification settings - Fork 75
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
Implement get and set alternate settings #61
Conversation
This looks promising. I will have to write an example implementation to your functionality to test if it even works as it should. |
Okay, i tried implementing a POC into my DFU bootloader project using your proposed branch. Something seems off because DFU (i think) should check couple of alt settings when listing a device but it only displays the first one all the time. Impl of POC: https://github.com/redpfire/dfu-boot/blob/c56c1fcc6f33c3d02767a5a0acd00264b8ca73cb/src/dfu.rs#L227 Output of dfu-util:
As you can see it only lists pub(crate) const ALT_SETTINGS: usize = 2;
pub(crate) const ALT_STRS: &'static [&'static str] = &[concat!("DFU Bootloader ", env!("CARGO_PKG_VERSION")), "TEST"]; |
@redpfire You only seem to write one alt setting descriptor: https://github.com/redpfire/dfu-boot/blob/c56c1fcc6f33c3d02767a5a0acd00264b8ca73cb/src/dfu.rs#L203 The descriptors written must contain all the alternate settings that exist (call |
Hey, cool project! I don't know my way around DFU, so I don't really know what you need the alternate settings for. As far as I know, you need to describe all alternate settings in the descriptor, i.e. loop over you alt settings in I also want to stress, that you need to check, whether fn get_alt_setting(&mut self, interface: InterfaceNumber) -> Option<u8> {
if interface == self.comm_if {
Some(self.curr_alt)
} else {
None
}
} Otherwise, your class implementation might interfere with another class implementation on the same device in the future. |
Oh okay, that makes much more sense. Will correct this and hopefully it will work now :)
|
Okay, calling Output from dfu-util is as expected:
That's great news because it all indeed works :) Great work! |
src/device.rs
Outdated
@@ -338,7 +338,21 @@ impl<B: UsbBus> UsbDevice<'_, B> { | |||
} | |||
|
|||
(Recipient::Interface, Request::GET_INTERFACE) => { | |||
// TODO: change when alternate settings are implemented | |||
// FIXME: Reject if req.index > u8::MAX or just ignore? |
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.
For now i think you should reject interface number greater than 255.
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.
I had a quick look into the USB spec Ch. 9. I understand requests with invalid values must indeed be rejected, also for SET_INTERFACE.
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.
Exactly this. I had a look at the spec myself before mentioning it.
@Sawchord waiting for you to commit changes mentioned in the review :) Also please squash all the commits into one meaningful commit for a cleaner merge. |
Updated and squashed. @redpfire Please recheck that this still works on your project. |
Yep, still works. Nice work! |
Hey, any chance of someone merging this? Working on a cdc-ncm implementation and got stuck on the same issue. |
I couldn't resolve the merge conflicts directly here, but it looks like everything's been tested and working, so I made a PR with a minimal set of changes in #114 to get this merged. If you want to fix things directly here @Sawchord I'd also be willing to merge, although I recognize it's been 2 years since this PR was made :) |
Thanks Ryan!, I hope it will be merged soon :) |
Sorry for the late reply. I am fine with merging #114 . Thanks for moving this forward |
Hello everyone!
PR #55 implemented describing alternate settings in the
get_descriptor
function.However, handling the respective setup packets was not yet supported.
This PR is a proposal to add this functionality in a backwards compatible way.
It adds two new methods to the
UsbClass
trait,get_alt_setting
andset_alt_setting
.Their default implementations ignore the requests.
If all classes ignore the request, the crate behaves in the same manner as before.
Therefore, this PR should not require changes to
UsbClass
implementations.Please keep in mind, that I did not have the time to test this implementation yet.
I will need this functionality to implement the CDC-NCM class, but I don't know when this will happen.
Also I could not come to a conclusion, on how to handle too large interface numbers and alternate setting values.
Opinions about the naming of the functions welcome.
@mvirkkunen @redpfire @kiffie @ianrrees What do you think?