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

hps_accel/stream: Update stream API #238

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Conversation

alanvgreen
Copy link
Collaborator

Defines a new Stream API

  • remove distinction between Source and Sink
  • make a StreamDescription class that can be used to document whether
    the Stream endpoint ignores the handshaking rules.

def cast(obj, src_loc_at=0, ignores_ready=False, ignores_valid=False):
if isinstance(obj, StreamDefinition):
return StreamDefinition(
paylad_type=obj.payload_type,
Copy link
Collaborator

Choose a reason for hiding this comment

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

paylad -> payload

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. More test cases needed, clearly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new version of this function is used in the normal path, so this is now tested - at least cursorily tested.

Copy link
Collaborator

@danc86 danc86 left a comment

Choose a reason for hiding this comment

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

Very interesting.

I think the Litex name streams.Endpoint might be better than Stream. I think of the "stream" as really the abstract movement of data over the signals between two "endpoints" which have been connected.

@alanvgreen
Copy link
Collaborator Author

I'm very much up in the air with naming. I somewhat agree with your objection to Stream, but I don't think Endpoint is quite right either - when connected, two Endpoints disappear in gateware, so they're no longer endpoints, they're actually the stream.

I also want to note that, in some situations, three "Endpoints" are connected in a single Stream A==B==C. It doesn't seem right to call B an Endpoint in this situation.

Happy to discuss further.

Defines a new Stream API
- remove distinction between Source and Sink
- make a StreamDescription class that can be used to document whether
  the Stream endpoint ignores the handshaking rules.

These changes were made in light of comments on
amaranth-lang/amaranth#317

Signed-off-by: Alan Green <[email protected]>
Changes needed to work with Stream API

Signed-off-by: Alan Green <[email protected]>
Recasts API, to use Endpoint and PayloadDefinition instead of Stream and
StreamDefinition. Also removes ignores_ready and ignores_valid
parameters.

Signed-off-by: Alan Green <[email protected]>
Copy link
Collaborator

@danc86 danc86 left a comment

Choose a reason for hiding this comment

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

I was wondering about the purpose for ignores_ready and ignores_valid. I guess the idea is to have the other end raise an exception if it doesn't like being connected to streams which behave that way? But since we don't have any like that, there's no need for it now?

@alanvgreen
Copy link
Collaborator Author

I was wondering about the purpose for ignores_ready and ignores_valid. I guess the idea is to have the other end raise an exception if it doesn't like being connected to streams which behave that way?

Yes, and also formal verification. I don't think this API was a good direction to take. There's a bit more thinking documented at amaranth-lang/amaranth#317 (comment).

But since we don't have any like that, there's no need for it now?

I'm not sure the value outweighs the impact on the API. Even if we wanted something like this, there are probably better ways to it.

@alanvgreen alanvgreen merged commit d4c5b1d into google:main Aug 27, 2021
@alanvgreen alanvgreen deleted the streams-again branch August 27, 2021 00:16
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