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

IOInterface typing wrapper for lib.data signatures #726

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

piotro888
Copy link
Member

Using amaranth.lib.data Signatures with typing comes with some boilerplate and repetition :(

I came up with a class that can be used both for typing and to get amaranth Signature for use in Component constructor.

It works by crating a class with IOSignal fields declared as self. in constructor, so it can be directly for typechecking. IOSignal type derives Value with additonal shape and direction information.
IOInterface is never used globally, but is intended only for interface typing. Those signals will be filled with actual objects at super().__init__(... Component constructor (that gets Signature from generated IOInterface.signature). Nested IOInterfaces are supported.

It is a bit hacky, but what do you think?

@piotro888 piotro888 added the nice to have Could be useful, but not a top priority label Aug 29, 2024
@tilk
Copy link
Member

tilk commented Sep 4, 2024

The boilerplate reduction is nice, but this is a little bit hacky. Maybe, instead of a custom amaranth.Value subclass, typing.Annotated could be used to store the same information?

Copy link
Contributor

@lekcyjna123 lekcyjna123 left a comment

Choose a reason for hiding this comment

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

For me it looks good.

"master_wb": Out(WishboneSignature(wb_params)),
"slaves": In(WishboneSignature(wb_params)).array(num_slaves),
"master_wb": Out(WishboneInterface(wb_params).signature),
"slaves": Out(WishboneInterface(wb_params).signature).array(num_slaves),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why In was changed to Out?

]


class IOSignal(Value):
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on the meeting it would be great to have the name changed.

]


class IOSignal(Value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc string will be welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nice to have Could be useful, but not a top priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants