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

MAX3421E NAK retry handling next frame #2543

Merged
merged 19 commits into from
Apr 2, 2024
Merged

Conversation

IngHK
Copy link

@IngHK IngHK commented Mar 26, 2024

This PR is intended to fix the problem as discussed in #2510 / issue #2541
The change means that in the case of a NAK, the transfer will only be repeated in the next frame (triggered by frame IRQ).
The 1st commit of PR is a first draft (not for merging) as a basis for further discussion and testing.

@IngHK
Copy link
Author

IngHK commented Mar 31, 2024

I finished work now and It's ready for review and merge.
It can handle up to 10 retries per frame adjustable by CFG_TUH_MAX3421_MAX_ATTEMPS_PER_FRAME.
With value zero it's compatible with endless retries as before.

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.

Perfect, thank you very much of the PR. Though I made some changes to

  • always en-force max NAK with default = 1. IMO, this should work best for most people. We can increase to 2,3 should it has issue with throughput in the future
  • add tuh_configure() cfgid to allow changing this behavior. This can only be used to change other max3421 behavior such as interrupt level polarity, edge in the future.
  • add ep's hxfr

tested and confirmed it only send 1 token per frame, previously it would send around 6 IN token for the bulkd endpoint

image

@hathach hathach merged commit 2265bfe into hathach:master Apr 2, 2024
49 checks passed
@IngHK
Copy link
Author

IngHK commented Apr 2, 2024

@hathach It doesn't work if cfg->max3421.max_nak is the maximum value.
Then it's running endless, because ep->state remains EP_STATE_ATTEMPT_MAX and that flags pending.
We really need an additional state "suspend", if the last attempt is NAKed.
I'll fix it soon...

@hathach
Copy link
Owner

hathach commented Apr 2, 2024

That is ok max attempt can also be inferred as no nak limit as well. To be honest a usb frame will probably hold less than 10 retries due to back and forth spi transactions

PS: though we could add a check in the tuh_configure() to limit it to EP_STATE_ATTEMPT_MAX-EP_STATE_ATTEMPT_1

@IngHK
Copy link
Author

IngHK commented Apr 2, 2024

@hathach I think, it's a little bit confusing, when last "attempt" is not a real attempt but instead used as suspend state.
On assembler level is it the same, but it's not easy to understand the code.
If you like I would create a commit with TU_VERIFY

@hathach hathach mentioned this pull request Apr 2, 2024
@hathach
Copy link
Owner

hathach commented Apr 2, 2024

thanks, I already made an PR to limit it #2565 #2566

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