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

Fixing processing order of EP0 (control) transactions #41

Conversation

ryan-summers
Copy link
Member

This PR fixes an issue with the processing order of multiple EP0 events.

Specifically, if both EP0-IN and EP0-OUT events have occurred, we have to process EP0-IN first so that we update the ControlState properly. Otherwise, we inadvertently process the EP0-OUT status phase as a spurious OUT transfer (instead of the STATUS phase as expected).

This only occurs when both EP0-OUT and EP0-IN transactions occur at the same time, which occurs when polling is completed at a lower-than-continuous rate.

After this fix, this crate can be used in debug configuration reliably as well.

@mvirkkunen
Copy link
Collaborator

Hmm, yes, I believe processing them in this order will be more reliable because the SETUP is handled second, and that always resets everything back to working order. I wonder if we should also add ep_out.unstall() into ControlPipe when SETUP is received, just in case the IN handling code still ends up stalling it for some reason?

@ryan-summers
Copy link
Member Author

ryan-summers commented Sep 8, 2020

I wonder if we should also add ep_out.unstall() into ControlPipe when SETUP is received, just in case the IN handling code still ends up stalling it for some reason?

That doesn't sound like a bad approach to me - I assume we would only unstall() when we correctly parse a setup packet.
Edit: I've now added this

I think it's also safe to remove the compiler warning when compiling for debug. Edit: I dont think this is still in master.

@mvirkkunen mvirkkunen merged commit b6db3d6 into rust-embedded-community:master Sep 22, 2020
@mvirkkunen
Copy link
Collaborator

Thanks! My tests pass for this so I'll release a new version with these changes. This should make things more reliable in the future.

@ryan-summers ryan-summers deleted the rs/periodic-poll-bug/race-condition-fix branch September 23, 2020 17:30
Disasm referenced this pull request in stm32-rs/synopsys-usb-otg Nov 27, 2021
newAM added a commit to newAM/stm32h7xx-hal that referenced this pull request Dec 28, 2021
bors bot added a commit to stm32-rs/stm32h7xx-hal that referenced this pull request Jan 3, 2022
236: Migrate to Rust 2021 as part of the public testing r=richardeoin a=hargoniX

Migration to Rust 2021 as part of the public testing.

We did actually catch one ICE bug with this \o/: rust-lang/rust#87426

305: remove release mode requirement from USB examples r=richardeoin a=newAM

This was fixed in usb-device: rust-embedded-community/usb-device#41

Co-authored-by: Henrik Böving <[email protected]>
Co-authored-by: Richard Meadows <[email protected]>
Co-authored-by: Alex Martens <[email protected]>
richardeoin pushed a commit to richardeoin/stm32h7xx-hal that referenced this pull request Jan 4, 2022
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.

2 participants