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

Fix unit tests when running on Cynthion #124

Merged
merged 5 commits into from
Nov 22, 2024
Merged

Conversation

antoinevg
Copy link
Member

@antoinevg antoinevg commented Oct 31, 2024

This PR depends on:


The fixes I made for Control IN transfers in #115 had the unfortunate side-effect of revealing a previously unknown bug in a) the handling of Control OUT and Bulk OUT transfers as well as b) resolving a long-standing ambiguity in the endpoint priming behaviour of the eptri gateware's OutFIFOInterface peripheral and c) handling an unexpected side-effect of calling set_interface on the InFIFOInterface and OutFIFOInterface's handling of the PID Data token.

There were multiple issues blocking solution of this problem:

1. EP0 was getting primed when it shouldn't be.

The first issue was that EP0 would always get re-primed after receiving control data from the host. This creates a race condition in the eptri gateware between the firing of the SETUP_PACKET interrupt after the first 8 bytes and the RECEIVE_PACKET interrupt once the data phase of the control transfer begins.

This race condition is easily avoided by only priming EP0 after receipt of a setup packet with a data stage, as we do everywhere else.

The bigger issues that lie behind this race condition are outside the scope of this PR but I do look forward to revisiting them in future.

2. Bulk endpoints are no longer primed after a ZLP is received on EP0

The second issue came about because the priming/enabling logic on eptri is not entirely optimal. The short version is that, because EP0 is now only conditionally being primed, a situation arises where an incoming ZLP on EP0 will no longer re-enable the eptri fifo's for receipt. The solution to this is to re-enable the interface without re-priming EP0.

The bigger issues that lie behind this logic are also outside the scope of this PR and are likely to be a valuable input into the re-design of the luna-soc USB Peripheral.

3. For the longest time endpoint priming behaviour has been ambiguous at best.

See greatscottgadgets/luna-soc#35 for more information.

4. set interface needs to reset PID Data Token

When a set interface request is made, the host will reset the value of the PID Data token. This would result in the Gateware device not recognizing valid packets, instead marking them as redundant. Ideally this would be taken care of in gateware but, given the bare-bones nature of the EPTRI peripheral, this needs to be handled in firmware.

See greatscottgadgets/cynthion#203 for more information.

--

Closes greatscottgadgets/cynthion#200

Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

This fixes the tests for me, thank you!

@antoinevg antoinevg force-pushed the antoinevg/fix-unittests branch from 6fe7d5e to ce479a3 Compare November 6, 2024 09:10
@antoinevg antoinevg marked this pull request as ready for review November 6, 2024 09:11
@antoinevg antoinevg requested a review from martinling November 6, 2024 09:11
@martinling
Copy link
Member

This version passes tests for me as-is, but fails when I rebase #122 on it.

See my alt-settings-broken branch for the failing version.

The working version on #122 is based on the previous version of this PR at 6fe7d5e.

…int where appropriate.

This commit also splits control and endpoint packet receiption handlers into two separate methods.
@antoinevg antoinevg marked this pull request as ready for review November 20, 2024 14:41
Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

This is passing tests reliably for me now! 🎉

I've rebased #122 on top of it, including the necessary clear_halt calls and removing the superfluous GET_INTERFACE/SET_INTERFACE handlers at the device level.

I raised some concerns on greatscottgadgets/luna-soc#35 though which might potentially affect what we want to do in the other PRs.

Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

Hold up - we still need to fix one thing here.

@antoinevg antoinevg merged commit a737fa3 into main Nov 22, 2024
1 check was pending
@antoinevg antoinevg deleted the antoinevg/fix-unittests branch November 25, 2024 08:02
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.

Facedancer unit tests fail when emulated device and tests are being run on the same host.
2 participants