Separate `Record` into `PackedStruct` and `Interface` components

[awygle]

Record.connect is very rarely useful, but looks broadly useful. It should be deprecated (and ideally replaced with something better).

[awygle]

Both connect and the direction functionality are used in the test suite (obviously). How would you like to deal with that? Put a with warnings.catch_warnings around them (perhaps with checking we got the right warning), as is done for the AST warnings? Or is there more that needs to be done?

[whitequark]

with warnings.catch_warnings is fine; nMigen does have some tests for deprecation warnings but not many of them, so either direction is OK.

[awygle]

After further discussion both on PR #368 and on IRC, the current thinking is that we'll split Record into two components - a PackedStruct component, representing the idea of multiple Values packed together into a larger Value, and an Interface component, representing the idea of multiple signals with defined directionality which can be connected together. Record would be deprecated during the 0.4 development cycle and removed at the time of the 0.5 release.

[rroohhh]

How should the user interface of PackedStruct and Interface look like; how does one create such a value?

Currently I can think of atleast three different ways:

1. Using class definitions, similar to how dataclasses look:

@packed_struct
class AxiProtectionType:
    priviledged: unsigned(1)
    secure: unsigned(1)
    is_instruction: unsigned(1)

def stream_type(payload):
    @interface
    class Stream:
        ready: SinkToSource(unsigned(1))
        valid: SourceToSink(unsigned(1))
        last: SourceToSink(unsigned(1))
        payload: SourceToSink(payload)
    return Stream

The main disadvantage I see with this method is the need to dynamically generate the classes in case the shape of some part should be parametrized. Also it might not be obvious in which order the bits are layed out. Finally it is more difficult to generically transform something declared like this (like for example reversing the order of the fields).

2. Using a layout DSL like Record currently does:

AxiProtectionTypeLayout = [("priviledged",  1), ("secure", 1), ("is_instruction", 1)]

def stream_layout(payload_layout):
    return [
        ("ready", sink_to_source(1)), 
        ("valid", source_to_sink(1)),  
        ("last", source_to_sink(1)), 
        ("payload", source_to_sink(payload_layout)))
    ]

This would work the same way as the layout specification of Record, but atleast written this way, it lacks the structure the other approaches have. For example one can not see at a glance, if a layout description is for a Interface or a PackedStruct.

3. Adhoc based on fields present on a object:

class AxiProtectionType(PackedStruct):
    def __init__(self):
        self.priviledged = Signal(1)
        self.secure = Signal(1)
        self.is_instruction = Signal(1)

class Stream(Interface):
    def __init__(self, payload: Value | ValueCastable):
        self.ready = SinkToSource(Signal(1))
        self.valid = SourceToSink(Signal(1))
        self.last = SourceToSink(Signal(1))
        self.payload = SourceToSink(payload)

This is very adhoc and would probably need some kind of filtering of the instance variables based on type.

How does a user use a PackedStruct or a Interface

For PackedStruct some time ago @whitequark suggested only allowing access to the fields via __getitem__ or a additional namespace similar to m.d.<domain>. With the introduction of ValueCastable however I don't think this restriction is necessary any longer.

For Interface I currently envision something like this:

class A(Elaboratable):
    def __init__(self, input_stream):
        self.input_stream = input_stream
        self.output = Interface.like(input_stream).source_side()
        self.packet_counter_stream = Stream(Signal(32)).source_side()

    def elaborate(self, platform):
        m = Module()
        packet_counter = Signal(32)
        m.d.comb += self.output.connect(self.input_stream)
        m.d.comb += self.packet_counter_stream.payload.eq(packet_counter)
        with m.If(self.input_stream.ready & self.input_stream.valid):
            m.d.sync += packet_counter.eq(packet_counter + 1)
            m.d.sync += self.packet_counter_stream.valid.eq(1)
        with m.ElseIf(self.packet_counter_stream.ready & self.packet_counter_stream.valid):
            m.d.sync += self.packet_counter_stream.valid.eq(0)

(Yes this example is stupid and broken)

What do you all think?

[anuejn]

To the first question, I highly prefer option 1) or 3) since these result in working autocompletion, while 2) cant be understood by my IDE.

[whitequark]

This has been designed and is now being implemented in RFCs 1 and 2:

• https://github.com/amaranth-lang/amaranth/issues/748
• https://github.com/amaranth-lang/amaranth/issues/872