Implement get and set alternate settings

[Sawchord]

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 and set_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?

[redpfire]

This looks promising. I will have to write an example implementation to your functionality to test if it even works as it should.

[redpfire]

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:

dfu-util 0.10

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2020 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Found DFU: [41ca:2137] ver=0010, devnum=13, cfg=1, intf=0, path="3-5", alt=0, name="DFU Bootloader 0.2.3", serial="8971842209015648"

As you can see it only lists alt=0 and nothing else while i have two alts in my config: https://github.com/redpfire/dfu-boot/blob/c56c1fcc6f33c3d02767a5a0acd00264b8ca73cb/src/config.rs#L14

pub(crate) const ALT_SETTINGS: usize = 2;
pub(crate) const ALT_STRS: &'static [&'static str] = &[concat!("DFU Bootloader ", env!("CARGO_PKG_VERSION")), "TEST"];

[mvirkkunen]

@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 interface_alt multiple times) and they must not change while the device is configured, so a "current setting" value shouldn't be used in that method.

[Sawchord]

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 get_configuration_descriptors and describe the interface comm_if with the different alt settings.

I also want to stress, that you need to check, whether interface matches your comm_if, even though you only use one interface for portability:

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. I know this is a drawback of the API design, but I could not come up with a better way.

[redpfire]

Oh okay, that makes much more sense. Will correct this and hopefully it will work now :)

@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 interface_alt multiple times) and they must not change while the device is configured, so a "current setting" value shouldn't be used in that method.

[redpfire]

Okay, calling interface_alt for every alt setting indeed worked. I also implemented an interface check. https://github.com/redpfire/dfu-boot/blob/739e0a7e665c1e1aa270dfdba34e1721fa6aa96e/src/dfu.rs#L203

Output from dfu-util is as expected:

dfu-util 0.10

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2020 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Found DFU: [41ca:2137] ver=0010, devnum=7, cfg=1, intf=0, path="3-5", alt=1, name="TEST", serial="8971842209015648"
Found DFU: [41ca:2137] ver=0010, devnum=7, cfg=1, intf=0, path="3-5", alt=0, name="DFU Bootloader 0.2.3", serial="8971842209015648"

That's great news because it all indeed works :) Great work!

[redpfire]

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

[Sawchord]

Updated and squashed. Also bumped version number.

@redpfire Please recheck that this still works on your project.

[redpfire]

Yep, still works. Nice work!

[maor1993]

Hey, any chance of someone merging this? Working on a cdc-ncm implementation and got stuck on the same issue.

[ryan-summers]

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 https://github.com/rust-embedded-community/usb-device/pull/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 :)

[maor1993]

Thanks Ryan!, I hope it will be merged soon :)

[Sawchord]

Sorry for the late reply. I am fine with merging #114 . Thanks for moving this forward