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

Add enumerations and allocator for isochronous endpoints #60

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

ianrrees
Copy link
Contributor

Aim is to resolve #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?

@dgoodlad
Copy link

dgoodlad commented Aug 3, 2022

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

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 3, 2022

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.

@dgoodlad
Copy link

dgoodlad commented Aug 3, 2022

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 :)

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 4, 2022

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.

@eldruin
Copy link
Member

eldruin commented Aug 4, 2022

Maybe @mvirkkunen can clarify here how to run these tests.

@Disasm
Copy link
Member

Disasm commented Aug 4, 2022

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

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 4, 2022

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.

@dgoodlad
Copy link

dgoodlad commented Aug 4, 2022

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 👍

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 4, 2022

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.

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 6, 2022

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

@ianrrees
Copy link
Contributor Author

ianrrees commented Aug 6, 2022

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.

dgoodlad added a commit to dgoodlad/synopsys-usb-otg that referenced this pull request Aug 10, 2022
@Cryowatt
Copy link

Cryowatt commented Mar 14, 2023

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.

@ianrrees
Copy link
Contributor Author

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.

@ryan-summers
Copy link
Member

@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

Copy link
Member

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very minor nits around unwrap/expect and input validation, but everything looks fine otherwise

src/bus.rs Show resolved Hide resolved
src/bus.rs Show resolved Hide resolved
@ianrrees
Copy link
Contributor Author

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.

@ianrrees
Copy link
Contributor Author

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.

@ryan-summers ryan-summers merged commit 46a354c into rust-embedded-community:master Mar 30, 2023
Cryowatt pushed a commit to Cryowatt/synopsys-usb-otg that referenced this pull request May 10, 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.

Isochronous endpoint support
6 participants