Conversation
Under USE_USB_FIQ, a periodic split transaction's first Complete-Split is scheduled 1 microframe after its Start-Split ACK instead of the 2 microframes used everywhere else, since the FIQ's low latency normally makes that safe. Some Full/Low-Speed hub/device combinations don't respond in time for that tighter schedule and see persistent transaction errors on their interrupt endpoint(s). Add USE_USB_FIQ_PERIODIC_SPLIT_COMPATIBILITY to sysconfig.h, which widens dwhciframeschedper.cpp's Complete-Split offset back to 2 microframes to match the non-FIQ timing, leaving the rest of USE_USB_FIQ's behaviour untouched.
Previously, CUSBHIDDevice::CompletionRoutine() gave up on a report endpoint (bRestart = FALSE) after a single non-frame-overrun USB error, so one transient glitch would permanently stop polling that endpoint. Add m_nErrorCount and m_nLastErrorTicksHZ to CUSBHIDDevice, and only give up after MaxConsecutiveErrors (50) errors in a row. A run resets to zero on any successful completion, and also decays if at least ErrorDecayMs (2000ms) has passed since the last error, so an idle endpoint's isolated errors don't accumulate toward the limit just because it rarely completes successfully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am investigating USB transaction errors in BMC64 on Raspberry Pi 3 using Keyrah V3.
The affected device is an Individual Computers Keyrah V3: a full-speed composite HID device behind the Pi 3 onboard LAN9514 high-speed hub. It exposes six interrupt-IN HID endpoints, all normally with bInterval = 1.
I have been working with them based on the original Keyrah BMC64 issue created here (sorry it's a very long thread):
randyrossi/bmc64#235
They have a thread on their forum describing the problems as well:
https://forum.icomp.de/index.php?thread/4525-how-to-use-keyrah-v3-with-bmc64-and-other-circle-based-projects/
A separate independent joystick issue has also popped up in BMC64 that has the same recurring USBErrorTransaction pattern:
randyrossi/bmc64#371
What I discovered:
With high-speed USB and USE_USB_FIQ enabled (as BMC64 is configured), Circle reports recurring USBErrorTransaction failures in the situations listed above.
After a lot of research and testing it was looking like it was a problem with high-speed-to-full-speed split-transactions. I ran two tests that helped me narrow down the problem:
This lead me to the code below in CDWHCIFrameSchedulerPeriodic::CompleteSplit(), where there is a difference for the FIQ path as it schedules the first Complete-Split one microframe offset, and two microframes when it's off, after a successful Start-Split: See here: https://github.com/rsta2/circle/blob/develop/lib/usb/dwhciframeschedper.cpp#L91
As a test I created a USE_USB_FIQ enabled build with 2 set as the m_usFrameOffset instead of 1, and then all the errors went away! :)
According to what I could understand, with the help of AI, changing this value to 2 in the FIQ path adds one extra microframe (125 microseconds) of scheduling delay, but otherwise should be ok.
I wanted to run this by you as it's deep in the USB code and I'm not an expert.
Fixes
Main fix
This PR added a new optional setting
USE_USB_FIQ_PERIODIC_SPLIT_COMPATIBILITY, which by default is off, but when defined sets the microframe offset to 2. I was not sure if this should be a permant change so I created this optional define.It is in the commit "Add periodic-split timing compatibility option"
Additional hardening
While I was investigating this problem I also found that a usb connection was dropped and wouldn't reconnect or retry if just a single error was reported. To try and make the system more tolerant to some errors, and retry and stay connected I added code to only drop the connection after multiple errors.
This enhancment is in the commit "Tolerate transient HID report errors". I think this makes sense to include in Circle but please let me know if you would like me to keep this as a patch in BMC64.
Testing
To reproduce the problem I creates a mock Keyrah V3 firmware that can be installed on a Raspberry Pi and connected to a Pi and driven over UART with telnet. It is available in the main repo if you want to use it for testing:
https://github.com/randyrossi/bmc64/tree/master/tools/keyrah_test
In addition I worked with the developer @mrdudz from Keyrah to test the device on an actual Keyrah unit. The fix has been confirmed working on actual hardware (see issue thread at the top).
I have also only tested on Pi0, Pi2 and Pi3 which BMC64 is targeted at.