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

ComponentInterface typing wrapper for lib.wiring interfaces #42

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

piotro888
Copy link
Member

@piotro888 piotro888 commented Jan 18, 2025

Redesign of concept from Coreblocks PR kuznia-rdzeni/coreblocks#726.
Class that can be used both for typing and to get amaranth Signature for use in Component constructor.

@piotro888
Copy link
Member Author

Improvement from previous version: now separate FlippedComponentInterface is used to implement flips cleanly, without using copy and getting is_flipped under try/catch.

@piotro888 piotro888 marked this pull request as ready for review January 18, 2025 16:16
@piotro888
Copy link
Member Author

amaranth-stubs commit needs to be changed before merge

@tilk
Copy link
Member

tilk commented Jan 18, 2025

Can this handle arbitrary ShapeLikes as members?

@piotro888
Copy link
Member Author

Yes, tested here for int:


and is ShapeLike in type stubs.

I can additionally include some ShapeCastable field in testbench.

@piotro888
Copy link
Member Author

piotro888 commented Jan 18, 2025

Turns out there was an unnecessary early Shape.cast that broken View wrapping trick around Signal, and Layouts were flattened. Fixed now and added to tests.
Thanks!

@piotro888 piotro888 marked this pull request as draft January 18, 2025 17:03
@tilk
Copy link
Member

tilk commented Jan 18, 2025

This ComponentSignal feels hacky, which could be fragile with future Amaranth updates. Is it possible to implement it as a ShapeCastable?

@piotro888
Copy link
Member Author

No, it needs to be type-equivalent to Signal.
I'm now thinking of making AbstractSignal instead.

@piotro888
Copy link
Member Author

There is a problem with typing of ComponentSignal.shape(). It is required from View that it returns Shape, but it is not true from lib.data fields that use a hack to wrap around Signal and return ShapeLike/ShapeCastable? instead.

AbstractSignal with shape changed to always return Shape | ShapeCastable (without Value inheritance) should solve this issue, but loosen typing a bit, but I don't think that Shape ShapeCastable distinction is that critical in calling shape() and I don't see other way to do it.

@piotro888
Copy link
Member Author

I think this problem is not addressed in current amaranth-stubs: Signal(ArrayLayout(2, 3)).shape() == ArrayLayout(2, 2)), but according to stubs it must be Shape.
It is true for Amaranth Signal, but not one extended with lib.data

@tilk
Copy link
Member

tilk commented Jan 18, 2025

I think this problem is not addressed in current amaranth-stubs: Signal(ArrayLayout(2, 3)).shape() == ArrayLayout(2, 2)), but according to stubs it must be Shape.

This is not true. Signal(ArrayLayout(2, 3)) is not a Signal, it's a View.

@piotro888
Copy link
Member Author

This is not true. Signal(ArrayLayout(2, 3)) is not a Signal, it's a View.

How this is described in stubs? Maybe similar method could be used here?
I know about Signal -> View conversion, but didn't expect it to be possible to define in stubs.

@tilk
Copy link
Member

tilk commented Jan 18, 2025

This is not true. Signal(ArrayLayout(2, 3)) is not a Signal, it's a View.

How this is described in stubs?

See _SignalMeta: https://github.com/kuznia-rdzeni/amaranth-stubs/blob/d62b68241fa1881244470e63931ceb635ac91044/amaranth-stubs/hdl/_ast.pyi#L497

(By the way, there is a slight problem in the stubs here - the overload for ShapeLike should be for ShapeLikes which are not ShapeCastable. This leads to misleading inferred types when developing components which are generic in arbitrary ShapeLikes - like current MemoryBank.)

Maybe similar method could be used here?

Maybe. But please consider again if ValueCastable, ShapeCastable or something like this could be used here. It would be preferable to use standard Amaranth methods for customizing the language.

@tilk
Copy link
Member

tilk commented Jan 19, 2025

Another thought: maybe handling everything at type level via typing.Annotated could work for this?

@piotro888 piotro888 marked this pull request as ready for review January 21, 2025 10:31
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