usb-device
usb-device copied to clipboard
rust-embedded-community
Add enumerations and allocator for isochronous endpoints
Aim is to resolve https://github.com/mvirkkunen/usb-device/issues/33, however as @lkolbly points out it doesn't add testing support. @mvirkkunen is that OK with you, or should I look in to adding test support? I expect any practical use of isochronous endpoints to not use the current endpoint buffering arrangement (the copy_to_nonoverlapping() takes a fair bit of processor time), so a test that transfers data in/out of an isochronous endpoint would currently be a bit artificial.
This will also need a minor version bump, assuming we're following semver.
@sibiryakov - any comments?
@ianrrees I've been working on support on STM32 chips over in stm32-rs/synopsys-usb-otg#29. Did you ever get feedback on this? Is the only real blocker testing support?
Hi @dgoodlad, from across the Tasman! I've not had more feedback about this, but it's looking like I will be interested in it for some USB audio work sometime in the next several months...
Haven't had a chance to look in to testing support; it seems like something that would be helpful but perhaps a simplified "unit test" approach would be easy without needing to implement isochronous transfers in the test harness.
I'll aim to update the PR today anyway.
That'd be amazing! I'm unfamiliar with how the test harness etc works here, so got a bit stuck as to how to approach it myself.
I've been hacking on some USB audio stuff over here, including the basics of USB Audio Class 2. That's what I've been using to drive out support in the other linked PR, so I know that it at least works for isochronous IN endpoints.
In that work, it did become clear that it would be good to have some extra events on the usb bus & device to allow implementers to handle incomplete transfers properly. If this one gets merged I'd be happy to follow up with a PR suggesting changes that felt somewhat ergonomic when I was hacking away :)
I've rebased on to current master, though didn't test it against my project (would require a fair bit of work). That said, it's not a very big change and I can't see any reason that the rebase would've introduced new problems. ed: I see defmt support has been added in the meantime, will aim to fix CI checks for that tomorrow
Also looked a little at testing, but I must admit my memory is a bit fuzzy about the details. Since this PR is really just about setting the right attributes bits in endpoint descriptors, test coverage for this PR doesn't actually require doing a transfer. Isochronous transfers were the main sticking point in implementing tests, from memory. At least at the time, rusb didn't support libusb's asynchronous interface, which is needed to do isochronous transfers. So, it might not be that hard to add tests to this PR, if we're OK with not actually testing isochronous transfers?
How does the testing work at an organisational level? It looks like actually running the tests requires someone to manually run them on real hardware.
Currently tests can be executed manually by flashing firmware with test_class from this repo to a device and running cargo test with the device connected to USB.
Test class code for STM32F446: https://github.com/Disasm/usb-otg-workspace/blob/master/example-f446ze-board/examples/test_class.rs
Looking at this again with a fresh set of eyes (and more Rust experience), I'm not sure that the new types are actually necessary and I don't like the tuple-style enum... Please hold.
Looking at this again with a fresh set of eyes (and more Rust experience), I'm not sure that the new types are actually necessary and I don't like the tuple-style enum... Please hold.
I like this a lot better 👍
Sweet, I'll try to bang together a test setup this weekend (it'll be SAMD-based) and add test coverage for this.
Testing isochronous transfers thoroughly might actually be a hard problem, since they're not supposed to be reliable, and TBH I think it's a bit outside the scope of this crate anyway.
I've made a start at testing, but think there might be something wrong with either the ATSAMD HAL or my hardware - haven't managed to get this board to enumerate... Might try later today with a board that uses a different chip that I know the HAL USB stuff works with, but have some IRL things to attend to.
Work-in-progress is here:
https://github.com/ianrrees/test-usb-device
Ok, new tests are implemented and confirmed to pass on ATSAMD21 against this branch of the atsamd HAL.
I'll open up an issue to discuss that test-usb-device repo - maybe it's something that we should have in this project or an adjacent one.
Is there something else that needs to be done here to get this merged? Even if testing is a blocker, I'd like to see this shipped as an unstable feature at the very least.
This PR adds test coverage that I'd say is on par with the rest of usb-device. My vague memory is that in August we wound up talking about a general level-up in USB testing, but that's a bit beyond the scope of this PR. LMK if there's anything I can do to help this along, but it'll be a while before I'll have a significant amount of time for work on Rust USB stuff.
@ianrrees If you have this tested and somewhat operational, I'm fine merging this as-is without the additional testing. Just let me know if it's in a usable state
Some very minor nits around unwrap/expect and input validation, but everything looks fine otherwise
Thanks! I'll try to make time for this in the next few days. It's been a while since I've had anything to do with this stuff so just want to get re-oriented before responding.
Have rebased on to current master and touched up a few formatting/comment issue. I'm in favour of merging this as-is, and capturing those other improvements in the bug tracker as I do think they'd be nice to have eventually.