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

Even when we get an empty "status change" interrupt from the hub, schedule another interrupt poll #2016

Merged
merged 3 commits into from
May 29, 2023

Conversation

ipopov
Copy link
Contributor

@ipopov ipopov commented Apr 9, 2023

Describe the PR
Even when we get an empty "status change" interrupt from a hub, schedule another interrupt poll.

During enumeration, when there are multiple devices attached to the hub as it's plugged into the Pi Pico, enumeration hangs, because we get a "status change" callback with value zero. With this patch, we retry several times on "zero" status change callbacks, until eventually we succeed.

Additional context
This is the cheapo hub that exhibits this behavior, but I assume it's not the only one: https://www.amazon.com/gp/product/B083RQMC7S.

While debugging this, I consulted the implementation in the Linux kernel. There, hub setup explicitly checks each port individually, before starting to depend on "status change" interrupts: https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/hub.c#L1133. We probably should do something like that here, but it's a much bigger change.

@morio
Copy link

morio commented Apr 14, 2023

With #define CFG_TUSB_DEBUG 3 this breaks connecting to an USB keyboard, it is an old Apple M7803. I attached the log with ipopov's pull request code and the previous commit in his repo, hash: 28817a715024c82b78f9b9d19f37a72771870759

The error with ipopov's code comes up as Queue EP 81 with 1 bytes ... usbh_edpt_xfer_with_callback 781: ASSERT FAILED

ipopov's code works find with #define CFG_TUSB_DEBUG 0

I was hoping this pull request would fix an issue with an unbranded two port hub that gets a Data seq error whenever I plug a Logitech B100 mouse into it. If I plug the mouse into another hub downstream, like the one on the Apple M7803 keyboard, the mouse works.

I think this issue has been reproduced here: #1997 (comment)

prev-commit.log
ipopov-commit.log

@aperezbios
Copy link

@hathach ping, is this by chance on your radar as something in need of improvement? There has been no further discussion since @morio wrote about this a couple of weeks ago.

@hathach
Copy link
Owner

hathach commented May 4, 2023

will review this soon, just come back from vacation and there is quite a bit of work I have to do first.

ipopov and others added 2 commits May 29, 2023 13:18
…edule another interrupt poll.

During enumeration, when there are multiple devices attached to the
hub as it's plugged into the Pi Pico, enumeration hangs, because we
get a "status change" callback with value zero. With this patch, we
retry several times on "zero" status change callbacks, until
eventually we succeed.

This is the cheapo hub that exhibits this behavior, but I assume it's
not the only one: https://www.amazon.com/gp/product/B083RQMC7S.

While debugging this, I consulted the implementation in the Linux
kernel. There, hub setup explicitly checks each port individually,
before starting to depend on "status change" interrupts:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/hub.c#L1133.
We probably should do something like that here, but it's a much bigger
change.
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

thank you for your PR, I slightly move the check for status_change not zero to make it clearer. Will merge when ci passed.

@hathach hathach merged commit ddc029c into hathach:master May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants