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

applets: add a bi-directional bulk_speed_test applet #205

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

antoinevg
Copy link
Member

@antoinevg antoinevg commented May 23, 2023

This PR adds a new applet that is able to form a bi-directional bulk speed test.

I don't have access to SuperSpeed hardware so I thought it best to implement this separately to the bulk_in_speed_test.py applet for now.

New features added:

  • Addition of a new OUT endpoint. (Added bonus: the speed test device will now enumerate under macOS)
  • Vendor request handler to manage the switch between IN/OUT test.
  • Data sent by the IN test is now an 8-bit counter rather than zeros.
  • OUT test mirrors received data on device LED's and PMOD A.

There is one area where I am not 100% sure I'm doing the right thing in the gateware implementation and that is the logic for when a given IN or OUT test is inactive:

IN test:

https://github.com/antoinevg/luna/blob/791498f1eabb3632453770a5a3c5f7d3876e3c2a/applets/bulk_speed_test.py#L229-L236

OUT test:

https://github.com/antoinevg/luna/blob/791498f1eabb3632453770a5a3c5f7d3876e3c2a/applets/bulk_speed_test.py#L266-L270

@antoinevg antoinevg requested a review from miek May 23, 2023 09:39
@martinling
Copy link
Member

A couple of comments:

  1. I don't think the vendor requests are necessary. Since the IN endpoint generates endless data from nowhere, and the OUT endpoint accepts endless data going nowhere, there's no need to explicitly start/stop them, nor to have only one in use at once.

  2. To avoid duplicating the implemetation, it may be worth rebasing this on Move speed test gateware into library #207, which moves the implementations out of bulk_in_speed_test.py and splits them up into SuperSpeed and non-SuperSpeed versions.

@antoinevg
Copy link
Member Author

I've removed the vendor request handler without any ill-effects, tx!

Let's wait for #207 to get merged into main and then I'll update the PR to match the new code layout.

@antoinevg antoinevg force-pushed the antoinevg/bulk_speed_test branch 2 times, most recently from 808a43c to 54d489d Compare June 9, 2023 13:03
@antoinevg antoinevg closed this Jun 9, 2023
@antoinevg antoinevg force-pushed the antoinevg/bulk_speed_test branch 2 times, most recently from 54d489d to 8d3b6f7 Compare June 9, 2023 13:05
@antoinevg antoinevg reopened this Jun 9, 2023
@antoinevg
Copy link
Member Author

I've rebased the PR to take #207 into account.

Also:

  • Renamed applets/bulk_in_speed_test.py to applets/bulk_speed_test.py
  • Added an exclusion for SuperSpeed devices so that only the IN test gets performed for them.
  • Reworked the logic for bulk_speed_test.__main__ to make it a little less verbose.

@antoinevg antoinevg force-pushed the antoinevg/bulk_speed_test branch 2 times, most recently from 1772507 to 8b67e73 Compare September 6, 2023 09:58
Copy link
Member

@miek miek left a comment

Choose a reason for hiding this comment

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

This looks good to me! There's just a minor fix needed that I've added inline.

luna/gateware/applets/speed_test.py Outdated Show resolved Hide resolved
@miek miek merged commit ca18ec1 into greatscottgadgets:main Oct 10, 2023
3 checks passed
@antoinevg antoinevg deleted the antoinevg/bulk_speed_test branch April 23, 2024 09:00
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