Skip to content

Interface definition library #2

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

Closed
wants to merge 25 commits into from
Closed

Interface definition library #2

wants to merge 25 commits into from

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Feb 16, 2023

@whitequark whitequark changed the title RFC: Interface definition library Interface definition library Feb 16, 2023
@tannewt
Copy link

tannewt commented Feb 17, 2023

How does this fit into Python's type system? Ideally it could be used for verification.

@whitequark
Copy link
Member Author

How does this fit into Python's type system? Ideally it could be used for verification.

I agree. I'm still exploring options here, so I have no definite answer for you. I am definitely going to make sure that whatever solution we end up with can fit into Python's type system.

@whitequark whitequark force-pushed the main branch 4 times, most recently from 5f6fd38 to a5a5030 Compare February 21, 2023 02:58
@josuah
Copy link

josuah commented Mar 21, 2023

This is an interesting approach!

Trying to come-up with an example: we could end-up with something like this?

input_port_template = Signature({
    "data": In[unsigned(4)],
    "vld":  In[Signal(1)],
    "rdy":  Out[Signal(1)]
})

output_port_template = Signature({
    "data": Out[unsigned(4)],
    "vld":  Out[Signal(1)],
    "rdy":  In[Signal(1)]
})

class DebugTap(Elaboratable):
    def __init__(self):
        self.i = Interface(input_port_template)
        self.o = Interface(output_port_template)
        self.debug = Interface(output_port_template)

    def elaborate(self, platform):
        m = Module()

        # pass data from input port to output port
        m.d.comb += self.o.eq(self.i)

        # also pass it to an extra debug port
        m.d.comb += self.debug.eq(self.i)

@josuah
Copy link

josuah commented Mar 21, 2023

The __getitem__ binding greatly helps making the syntax more lightweight.

Member(Flow.Out, unsinged(4)) becomes Out[unsigned(4)].

With the [] syntax, I picture In and Out being some mathematical set defined as all possible signatures, from which we would drag one in particular (they [contain] a signature).

Was it voluntary to avoid small wrapper classes?

class In(Flow):
    def __init__(self, signature):
        super().__init__(self, Flow.In, signature):

class Out(Flow):
    def __init__(self, signature):
        super().__init__(self, Flow.Out, signature):

Then they look like some wrapper around a shape, adding a direction to them.

input_port_template = Signature({
    "data": In(unsigned(4)),
    "vld":  In(Signal(1)),
    "rdy":  Out(Signal(1))
})

@galibert
Copy link

galibert commented Mar 21, 2023

Why is there both Interface and Signature? What is Interface adding?

@josuah
Copy link

josuah commented Mar 21, 2023

I understand Signatures a templates that can be used to create an Interface.

I think the use-case is to be able to define a Signature for commonplace internal ports like Wishbone, FIFOs, valid/ready, etc. and then plug multiple instances of these.

Multiple Interfaces instantiated from the same Signature are likely compatible with each other.

@whitequark
Copy link
Member Author

Yep!

@josuah
Copy link

josuah commented Mar 21, 2023

A Signature is a bit like a "class", and an Interface is a bit like an object of that class.

Maybe it is possible to have signatures defined as classes (inheriting from a Signature class), and interfaces defined as objects of these?

Something a bit like how data.Structs are made?

class WishbonePort(data.Interface):
    dat_r:  In(Signal(32))
    addr:   In(Signal(32))
    stb:    In(Signal(1))
    ack:    Out(Signal(1))
    ...

That looks good on paper, but:
Maybe that would require metaclasses.
Maybe that would be too cumbersome to use or implement.

@whitequark
Copy link
Member Author

Maybe it is possible to have signatures defined as classes (inheriting from a Signature class), and interfaces defined as objects of these?

Something a bit like how data.Structs are made?

It's simpler. Any class that has a compatible interface matches a Signature. It doesn't have to be an Interface, it just needs to have the right fields.

@galibert
Copy link

Ah, I think I'm starting to see. Signature (or its derivatives) are objects that hold the prototype of the composite port. Interface(signature) is an object instance that essentially replaces the pile-of-signals and give you pluggability. It could be an instanciation method on the signature object, that's just a matter of taste.

@josuah
Copy link

josuah commented May 9, 2023

It could be an instanciation method on the signature object, that's just a matter of taste.

Indeed, there is a possible confusion for users who would try (A):

  1. Define an Interface as a class,
  2. Instantiate this class as part of their module (i.e. from __init__).

While it would need to be (B):

  1. Define a Signature class,
  2. Instantiate a signature object out of this class (eventually inline) as part of an Interface
  3. Define an Interface class,
  4. Instantiate class taking a single Signature object, as part of a Module (i.e. from __init__).

I suppose (B) which adds more types was chosen instead of (A) for some reason?
I think I missed it, so maybe it helps with the implementation? This would be good enough of a reason for me.

Just trying to spot what I might have missed.

For anyone who would prefer (A), it would likely be possible to implement (B) as part of Interface's __init__, with the Signature that never leaves Interface's __init__().

@josuah
Copy link

josuah commented May 9, 2023

Answers to my own question: "Why is Signature in addition to Interface?":

The flow!

I imagine it would be possible to have a same Signature used for an Input interface, and a separate Output interface, which are not having the same helper functions depending of their direction (i.e. self.read() or self.write()).

@whitequark
Copy link
Member Author

Yeah

@josuah
Copy link

josuah commented May 9, 2023

I am trying to summarize a typical usage for that RFC:

# defining the controller side in the signature by convention?
spi_signature = Signature({
    "cs":   Out[Signal(1)],
    "copi": Out[Signal(1)],
    "cipo": In[Signal(1)],
    "clk":  Out[Signal(1)],
})

class SpiControllerInterface(Interface):
    def __init__(self):
        super().__init__(spi_signature)

class SpiPeripheralInterface(Interface):
    def __init__(self):
        super().__init__(~spi_signature)

This looks rather minimal.

And if we add extra helpers:

# defining the controller side in the signature by convention?
spi_signature = Signature({
    "cs":   Out[Signal(1)],
    "copi": Out[Signal(1)],
    "cipo": In[Signal(1)],
    "clk":  Out[Signal(1)],
})

class SpiControllerInterface(Interface):
    def __init__(self):
        super().__init__(spi_signature)

    # for simulation:

    def read_byte(self, data):
        pass

    def write_byte(self, data):
        pass

class SpiPeripheralInterface(Interface):
    def __init__(self):
        super().__init__(~spi_signature)

    # for simulation:

    def transfer_callback(self, fn):
        self.simulation_transfer_callback = fn

Maybe it would be convenient to turn a Resource into a Signature (or rather, the other way around?) to share data formats between those.

@whitequark
Copy link
Member Author

Resources will be more or less replaced by Signatures at a later point.

@josuah
Copy link

josuah commented May 12, 2023

Here is my attempt at using Records in a way that looks like Interfaces/Signatures,
so that I can write new code ready for when they come:

class SPISignature(Record):
    def __init__(self, *, ctrl, peri):
        return super().__init__([
            ("clk",     1,  ctrl), # ctrl and peri here are to
            ("copi",    1,  ctrl), # tell who drives the signal
            ("cipo",    1,  peri),
            ("cs",      1,  ctrl),
        ])

class SPICtrlInterface(SPISignature):
    def __init__(self):
        super().__init__(ctrl=DIR_FANOUT, peri=DIR_FANIN)

    def spi_ctrl_methods(self):
        pass

class SPIPeriInterface(SPISignature):
    def __init__(self):
        super().__init__(peri=DIR_FANOUT, ctrl=DIR_FANIN)

    def spi_peri_methods(self):
        pass

@whitequark whitequark force-pushed the interfaces branch 3 times, most recently from 82ff052 to 409b1b9 Compare May 30, 2023 10:46
@jfng
Copy link
Member

jfng commented Jun 5, 2023

EDIT: answered or made obsolete by 53e5dc3.

  • Would it be possible to expose other Signal parameters to the Member constructor ? I think decoder could be useful here.

  • Will there be cases where signature.freeze() is implicitly called ? e.g. when signature.__eq__(), or Interface(signature) are called.

  • How would one attach metadata to an Interface ? The RFC mentions that Signature can be subclassed, is this an intended use case ?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I have some (very) minor comments from a read through of the changes since last week.

* the `.shape` property raise `TypeError`;
* the `.reset` property raise `TypeError`;
* the `.signature` property return `signature`.
* `member.flip()` returns a `flipped_member` where `flipped_member.flow` == `~member.flow`.
Copy link

@ghost ghost Jun 8, 2023

Choose a reason for hiding this comment

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

Suggested change
* `member.flip()` returns a `flipped_member` where `flipped_member.flow` == `~member.flow`.
* `member.flip()` returns a `flipped_member` where `flipped_member.flow == member.flow.flip()`.

* For every port member with a given `path` and `first_shape` in `first`, there is a port member with the same `path` in each of `rest` with shape `rest_shape`, where `Shape.cast(first_shape) == Shape.cast(rest_shape)`, and there are no other members in any of `rest`. In other words, the width and signedness of all of port members must be equal, but the shape-castable objects specifying the width and signedness do not have to be.
* It is expected that any mismatch in signatures will be resolved (through wrappers, mutating signature members, or otherwise) before interfaces are being connected.
* Either:
1. There is exactly one interface in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == ~rest_member.flow`.
Copy link

Choose a reason for hiding this comment

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

Suggested change
1. There is exactly one interface in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == ~rest_member.flow`.
1. There is exactly one interface in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == rest_member.flow.flip()`.

* It is expected that any mismatch in signatures will be resolved (through wrappers, mutating signature members, or otherwise) before interfaces are being connected.
* Either:
1. There is exactly one interface in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == ~rest_member.flow`.
2. There is any amount of interfaces in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == Out` and `rest_member.flow == In`.
Copy link

Choose a reason for hiding this comment

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

This could also be stated non-pairwise.

1. There is exactly one interface in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == ~rest_member.flow`.
2. There is any amount of interfaces in `rest`, and for every pair of members `first_member` and `rest_member`, `first_member.flow == Out` and `rest_member.flow == In`.

In both of the cases (1) and (2), the function, for each pair of an input port and an output of port with the same path, the function connects these as follows:
Copy link

Choose a reason for hiding this comment

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

It is worth nothing that, depending on the CSS in use, (1) and (2) here may not resemble the list item numbers above. Here they're rendered i. and ii. since they're second-level list items. It may be clearer to say "In both of these cases", and/or to indent this text and the code block at the same level as the sublist as follows:


...

  • Either:

    1. There is exactly one interface in rest, and for every pair of members first_member and rest_member, first_member.flow == rest_member.flow.flip().
    2. There is any amount of interfaces in rest, and for every pair of members first_member and rest_member, first_member.flow == Out and rest_member.flow == In.

    In both of these cases, the function, for each pair of an input port and an output of port with the same path, the function connects these as follows:

    m.d.comb += output_port.eq(input_port)

Drawbacks

...


In both of the cases (1) and (2), the function, for each pair of an input port and an output of port with the same path, the function connects these as follows:
```python
m.d.comb += output_port.eq(input_port)
Copy link

Choose a reason for hiding this comment

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

I think the overloading of "out"/"output" and "in"/"input" in this illustrative code block has great potential to confuse — e.g. it's the output stream of SequenceSource (which is described in the source with the member Out[16]) from the above example which would be the input_port here, and it's the input stream of the NumberSink (described with In[16] in its signature) which is the output_port.

In the context of knowing only fan-out is supported (as noted later in § Future work), this makes sense, but in the context of case (2) saying "if you have multiple rest interfaces, all their members must be Flow.In", it may read as if there would be multiple input_ports and thus some kind of fan-in instead.


- Do nothing. `Record` will continue to be used alongside the continued proliferation of ad-hoc implementations of similar functionality, and continue to impair the use of Amaranth components together.

- Replace the `interface.lib.module.connect` free function with a function `Interface.connect` or a function `amaranth.hdl.dsl.Module.connect`.
Copy link

Choose a reason for hiding this comment

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

Suggested change
- Replace the `interface.lib.module.connect` free function with a function `Interface.connect` or a function `amaranth.hdl.dsl.Module.connect`.
- Replace the `amaranth.lib.module.connect` free function with a function `Interface.connect` or a function `amaranth.hdl.dsl.Module.connect`.

Also, this RFC no longer proposes an Interface class, and Signature.connect wouldn't work.

@whitequark whitequark force-pushed the interfaces branch 2 times, most recently from 78f8c38 to 503ab88 Compare June 16, 2023 06:54
whitequark added a commit to whitequark/amaranth that referenced this pull request Aug 19, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Aug 19, 2023
@whitequark whitequark added the area:core RFC affecting APIs in amaranth-lang/amaranth label Aug 21, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Aug 21, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Aug 21, 2023
@whitequark whitequark removed the meta:nominated Nominated for discussion on the next relevant meeting label Aug 21, 2023
whitequark added a commit that referenced this pull request Aug 22, 2023
@whitequark whitequark closed this Aug 22, 2023
@whitequark whitequark deleted the interfaces branch August 22, 2023 06:15
@whitequark
Copy link
Member Author

We have discussed this RFC on the 2023-08-21 weekly meeting. The disposition was to merge.

github-merge-queue bot pushed a commit to amaranth-lang/amaranth that referenced this pull request Aug 22, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Aug 23, 2023
whitequark added a commit that referenced this pull request Aug 29, 2023
whitequark added a commit that referenced this pull request Aug 29, 2023
whitequark added a commit to whitequark/amaranth-rfcs that referenced this pull request Aug 31, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Sep 1, 2023
whitequark added a commit that referenced this pull request Sep 4, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Sep 25, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Sep 26, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Nov 26, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 1, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 4, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 5, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 5, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 5, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 11, 2023
whitequark added a commit to whitequark/amaranth that referenced this pull request Dec 11, 2023
github-merge-queue bot pushed a commit to amaranth-lang/amaranth that referenced this pull request Dec 11, 2023
jfng added a commit to jfng/amaranth-rfcs that referenced this pull request Jan 5, 2024
whitequark added a commit that referenced this pull request Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core RFC affecting APIs in amaranth-lang/amaranth
Development

Successfully merging this pull request may close these issues.

5 participants