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

Separate Record into PackedStruct and Interface components #342

Closed
awygle opened this issue Mar 29, 2020 · 6 comments
Closed

Separate Record into PackedStruct and Interface components #342

awygle opened this issue Mar 29, 2020 · 6 comments
Labels
Milestone

Comments

@awygle
Copy link
Contributor

awygle commented Mar 29, 2020

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

@awygle
Copy link
Contributor Author

awygle commented Apr 24, 2020

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
Copy link
Member

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
Copy link
Contributor Author

awygle commented Jul 9, 2020

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.

@awygle awygle changed the title Deprecate Record.connect Separate Record into PackedStruct and Interface components Jul 20, 2020
@whitequark whitequark added this to the 0.4 milestone Dec 12, 2020
@rroohhh
Copy link
Contributor

rroohhh commented May 31, 2021

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).

  1. 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.

  1. 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
Copy link
Contributor

anuejn commented Jun 1, 2021

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants