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

Add PacketStream protocol. #85

Merged
merged 63 commits into from
Dec 13, 2024
Merged

Add PacketStream protocol. #85

merged 63 commits into from
Dec 13, 2024

Conversation

rowanG077
Copy link
Member

@rowanG077 rowanG077 commented Jul 3, 2024

Todos:

  • Test haddock and make nicer
  • Copy over tests
  • Make top-level PacketStream module that exports all sub-modules
  • Top-level module should hide inner modules
  • Add forceResetSanity to async fifo
  • Remove code duplication in the tests

@rowanG077 rowanG077 force-pushed the packetstream branch 3 times, most recently from 0669c0c to ee9e485 Compare July 3, 2024 14:35
@bgamari
Copy link
Collaborator

bgamari commented Aug 2, 2024

The motivation here is a bit unclear to me. The documentation says

Provides the PacketStream protocol, a simple streaming protocol for transferring packets of data between components

However the "data" in question appears to be only bytes. Presumably there is a specific use-case in mind here but it is not clear to me what it is; network packets, perhaps?

It seems to me like ideally protocols in clash-protocols should be polymorphic in the type of the data that they transfer. Afterall, I wouldn't expect packets of bytes to be terribly prevalent in the "interior" of most designs. Unless you are building actual networking hardware, I would imagine you would typically want to parse the packet into a more structured form somewhere near the "edge" of the design.

@t-wallet
Copy link
Collaborator

t-wallet commented Aug 2, 2024

The motivation here is a bit unclear to me. The documentation says

Provides the PacketStream protocol, a simple streaming protocol for transferring packets of data between components

However the "data" in question appears to be only bytes. Presumably there is a specific use-case in mind here but it is not clear to me what it is; network packets, perhaps?

It seems to me like ideally protocols in clash-protocols should be polymorphic in the type of the data that they transfer. Afterall, I wouldn't expect packets of bytes to be terribly prevalent in the "interior" of most designs. Unless you are building actual networking hardware, I would imagine you would typically want to parse the packet into a more structured form somewhere near the "edge" of the design.

This protocol was indeed specifically created for a hardware-based MAC/IP/UDP stack (which will soon be coming to clash-cores). However, it is just a simpler version of AXI Stream with stricter metadata invariants. You can use it for any other purpose that needs high-speed data streaming, like video streaming. This PR also provides several circuits which are able to read these unstructured bytes into arbitrary types.

@rowanG077
Copy link
Member Author

rowanG077 commented Aug 2, 2024

@bgamari It was discussed with @lmbollen whether to make the payload polymorphic. In theory there isn't anything that currently really looks at the payload so it would not be super hard to do add a type variable for the payload.

But in my experience working with packetized streams there usually is not a sane way to have an interior type. You are almost certainly only dealing with fragments, these will generally not have a clear structure at the scope of a single cycle. The structure comes from the fragments together.

Do you have some concrete idea how you see a packet stream packet with a non-byte oriented payload? I'm not against the idea but just don't see the advantage.

@martijnbastiaan
Copy link
Member

But in my experience working with packetized streams there usually is not a sane way to have an interior type.

This is my experience too. In particular, any protocol that has byte enables (including the interface of block RAMs) are a pain to deal with if they're not having bits/bytes/bitvector as an interior type.

Still, ADTs are the killer feature of Clash IMO, so perhaps we should also offer API compatible versions without byte enables.

@t-wallet t-wallet force-pushed the packetstream branch 2 times, most recently from 4d8758d to a38eadc Compare August 30, 2024 16:21
@t-wallet t-wallet force-pushed the packetstream branch 2 times, most recently from 97b5f56 to a90007a Compare October 18, 2024 14:11
* Support zero-byte transfers
* Add utility that merges trailing zero-byte transfers
* Make PacketStream test framework more flexible
Now allows users to:
- Set the distribution of abort generation themselves;
- Choose whether to generate zero-byte packets;
- Choose whether to generate trailing zero-byte transfers.
* Converters: remove expensive operations
`mod` and `*` are removed.
Reverted back to the old version where all M2S outputs are registered, but now also handles arbitrary metadata and output data widths bigger than 1 that divide the input data width. Zero-byte transfers are always preserved. This new version reduces the LUT resource usage by half compared to the old registered version, because of a mux with 1 selector bit (optimal) being inferred by synthesizers for the next state computation, instead of a mux with 2 selector bits.
Now correctly drops packets that are too big to fit in the content FIFO in 'Backpressure' mode.
Forcing bytes in PacketStream _data to be 0x00 turned out to be too restrictive. Allowing them to be undefined makes a lot of components more efficient: depacketizers, packetizers, dropTail, and upConverter. It also strengthens the test framework as we can now force un-enabled bytes to throw an error when evaluated.
Adds a generic component which enforces packets to be a certain length,
which is extracted from the metadata. Packets that are too small are
aborted. This is useful for higher-level network protocols such as IP
and UDP.
- Removed dangerous leZeroIsZero (could cause type checker to break)
- Added source argument to nullByte
- Improved documentation of unsafeAbortOnBackpressureC
- Remove use of bitCoerce if only pack or unpack is required
- Removed M2SNoMeta in favour of PacketStreamM2S n ()
Now tests the behaviour of the metadata transformer functions.
packetArbiterC now reuses Df.CollectMode. A NoSkip mode is added.
A new PR will be opened for these.
@t-wallet t-wallet merged commit ec001d4 into main Dec 13, 2024
8 checks passed
@t-wallet t-wallet deleted the packetstream branch December 13, 2024 15:59
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.

8 participants