Skip to content

Add stream support for isochronous endpoints. #285

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

Merged
merged 4 commits into from
Feb 28, 2025
Merged

Conversation

antoinevg
Copy link
Member

This PR depends on #278


  • Adds stream support to both IN and OUT isochronous endpoints.
  • Uses amaranth.lib.stream.Interface for the implementation instead of luna.stream.StreamInterface.
  • Introduces a new data type luna.stream.future.Packet that adds first and last signals to an Amaranth stream payload in order to support packetization.

@antoinevg antoinevg force-pushed the antoinevg/iso-streams branch 3 times, most recently from 0489601 to da2e3f3 Compare January 14, 2025 14:59
@antoinevg antoinevg force-pushed the antoinevg/iso-streams branch 3 times, most recently from 924bf87 to b6764e7 Compare February 11, 2025 08:57
@antoinevg antoinevg force-pushed the antoinevg/iso-streams branch from b6764e7 to 4cf86d9 Compare February 11, 2025 08:58
@antoinevg antoinevg force-pushed the antoinevg/iso-streams branch from 4cf86d9 to 66e46b4 Compare February 11, 2025 09:01
@antoinevg antoinevg marked this pull request as ready for review February 11, 2025 14:21
@mossmann mossmann requested a review from mndza February 11, 2025 17:10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are USBIsochronousStreamInEndpointand USBIsochronousStreamOutEndpoint implemented in different files? This is a bit inconsistent with the rest of the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd dearly love to split up the rest of the code as well, having everything in single files makes it really hard to both find and work on things.

I'm a firm believer in the "one big thing per file" principle unless it's a bunch of small things that are closely related like type definitions.

Doing a PR that does the same for the other files in luna/gateware/usb/usb2/endpoints/ should have minimal downstream impact as these are all re-exported via the top-level luna.usb2 module path, e.g.

from luna.usb2 import USBDevice, USBIsochronousInEndpoint

That said, I am open to changing this for now if we want to leave this discussion to a later time!

Copy link
Contributor

@mndza mndza left a comment

Choose a reason for hiding this comment

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

Looking great 😄

@antoinevg antoinevg merged commit f2fbdfd into main Feb 28, 2025
14 checks passed
@antoinevg antoinevg deleted the antoinevg/iso-streams branch February 28, 2025 09:39
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.

2 participants