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

examples.boilerplate: modify to use port groups #663

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

isabelburgos
Copy link
Contributor

Follow up to #599 - replacement of pads with ports.
Added an "example" pin to demonstrate get_port_groups syntax, although this feels clunky compared to the previous boilerplate which had no pins.

@classmethod
def add_build_arguments(cls, parser, access):
super().add_build_arguments(parser, access)

for pin in cls.__pins:
access.add_pin_argument(parser, pin, default=True)
access.add_pin_argument(parser, "example", default=True)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call it something like data instead?

@isabelburgos
Copy link
Contributor Author

This is an idea for an expanded boilerplate applet that shows how to use in and out fifos and ports; feel free to ignore and we can go back to the previous commit and just use "data" instead of "example".

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

I like the direction, let's flesh it out some, I think it could be really helpful to newcomers.

## Connect BoilerplateModule.in_stream to BoilerplateSubtarget.out_fifo
boilerplate.in_stream.payload.eq(self.out_fifo.r_data),
boilerplate.in_stream.valid.eq(self.out_fifo.r_rdy),
self.out_fifo.r_en.eq(boilerplate.in_stream.ready),
Copy link
Member

Choose a reason for hiding this comment

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

I think you should be able to do these via wiring.connect too, but I may not have upstreamed that patch yet...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my latest version, I used the "interface adaptation" pattern between the in/out fifo signals to a stream.Signature. But I see now that amaranth.lib.fifo implements streams, so out_fifo.r_stream and in_fifo.w_stream should just work.

Copy link
Member

Choose a reason for hiding this comment

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

yep!


## Instantiate IO buffers for pins/ports
m.submodules.data_buffer = data_buffer = io.Buffer("i", args.port_data)
m.submodules.enable_buffer = enable_buffer = io.Buffer("o", args.port_enable)
Copy link
Member

Choose a reason for hiding this comment

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

Conceptually these should live in the BoilerplateModule which I'd probably call BoilerplateCore. So the core would do the port manipulation and the subtarget would set up the transfers and stuff like that, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That separation of functions makes sense to me. A more verbose option would be to create a BoilerplateOutput module to handle the ports, and then leave BoilerplateCore empty as a placeholder for "core" logic.

Copy link
Member

Choose a reason for hiding this comment

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

That seems excessive for the sample applet. This all will change some time not too far away anyway.

@isabelburgos isabelburgos force-pushed the isabelburgos/boilerplate-padless branch from 8b6fc89 to 05b4346 Compare August 11, 2024 02:03
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