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

Adding USB tracing, fixing further enumeration issues #142

Merged
merged 7 commits into from
Feb 27, 2024

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Feb 26, 2024

This PR does a few things:

  1. Adds in a new logging feature to make it easy to trace USB debug issues in the future (I've written this a few times, and figured it would be useful as a general feature instead)
  2. Removed the set_error() calls when processing a SETUP packet. We are not allowed to error in this state and should simply ignore the SETUP packet instead.
  3. Changed the handling of EP0-IN-complete tokens. We were being pretty jumpy with getting these tokens in unexpected states, however this is simply an indication that a packet transmission completed. We may have updated state internally and lost track of what we sent. As such, I've updated it such that we just ignore these tokens if we don't need them in our current state instead of stalling the control pipe, as this isn't really an error.

This effectively reverts #41, as further updates to EP0 control pipe handling make it unnecessary. Reversion of this PR prevents us from sending more data in a control transfer than is expected, since the host may have initiated the STATUS phase of the transfer already.

@ryan-summers ryan-summers changed the title Feature/usb tracing Adding USB tracing, fixing further enumeration issues Feb 26, 2024
@ryan-summers
Copy link
Member Author

@eldruin / @thejpster / @mvirkkunen I've tested this with 2 STM devices (Booster and Stabilizer, an STM32H7 and an STM32F4) successfully, and it reportedly fixed one of our users issues where the device was not enumerating properly (ref quartiq/stabilizer#817), so I think it'd be beneficial to get this landed and re-released as a potential 0.3.1 :)

@jannic
Copy link
Contributor

jannic commented Feb 26, 2024

I successfully updated one of the rp2040 examples (https://github.com/rp-rs/rp-hal-boards/blob/773ee73bfdff66d080dda528bfd03db845b75967/boards/rp-pico/examples/pico_usb_twitchy_mouse.rs) to use this version of usb-device. That probably doesn't test many code paths, and I only have linux hosts to test it with, but at least this makes sure that your changes don't break USB on the rp2040 completely.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks fine to me but I do not know how it works :)
You can decide whether to wait for an opinion from @mvirkkunen.

@ryan-summers
Copy link
Member Author

I'm going to merge this for now and aim for releasing soon to give some feedback period from anyone wanting to test it out. Thanks for taking a look everyone, I know USB can be extremely opaque - I'm no expert myself.

@ryan-summers ryan-summers merged commit fbf903e into master Feb 27, 2024
3 checks passed
@ryan-summers ryan-summers deleted the feature/usb-tracing branch February 27, 2024 15:41
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.

3 participants