-
Notifications
You must be signed in to change notification settings - Fork 176
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
Prototype a new CLI #904
base: main
Are you sure you want to change the base?
Prototype a new CLI #904
Conversation
Codecov Report
@@ Coverage Diff @@
## main #904 +/- ##
==========================================
- Coverage 83.75% 83.74% -0.02%
==========================================
Files 54 54
Lines 7678 7684 +6
Branches 1867 1869 +2
==========================================
+ Hits 6431 6435 +4
- Misses 1048 1049 +1
- Partials 199 200 +1
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bac83ad
to
617fc9a
Compare
617fc9a
to
6dfed2e
Compare
amaranth_cli/__init__.py
Outdated
# Generate Verilog file. | ||
if args.verilog_file: | ||
from amaranth.back import verilog | ||
args.verilog_file.write(verilog.convert(component)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the verilog.convert
invocation should include name field (either as an input arg to main
or maybe derive it from the class name? I prefer the former slightly to avoid accidental conflicts with primitive names in the Verilog synthesizer). In projects where you generate multiple Verilog modules from Amaranth, chances are the synthesizer is not going to like them all named top
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that you'll still get duplicate names for the submodules in that case, so it's not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, per IRC, that this is a problem that needs to be solved. For the purposes of testing/finishing the refactor of my code for the time being, would you be willing to expose top-level module renaming from the CLI? It would allow me to write the FuseSoC generation in terms of the Amaranth CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try the updated version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This correctly creates the hiearchy:
pdm run amaranth generate efbutils.ufm.reader.demo:Demo -p num_leds 2 -p efb_config '{ "dev_density": "7000L", "wb_clk_freq": 12.08 }' -p ufm_config '{ "init_mem": null, "zero_mem": true, "start_page": 0, "num_pages": 2046}' verilog -v - | grep "module "
module top(rx, tx, leds);
module \top.efb (bus__adr, bus__cyc, bus__dat_r, bus__dat_w, bus__irq, bus__stb, bus__we, rst, clk, bus__ack);
module \top.por (clk, por_clk, rst);
module \top.reader (bus__data, bus__read_en, bus__valid, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__irq, efb__stb, efb__we, rst, clk, bus__addr);
module \top.reader.pagemod (clk, seq__data, seq__addr, seq__stb, seq__ack, rand__data, rand__addr, rand__read_en, rand__valid, rst);
module \top.reader.streammod (clk, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__irq, efb__stb, efb__we, stream__data, stream__addr, stream__stb, stream__ack, stream__ready, rst);
module \top.reader.streammod.seqmod (clk, ctl__cmd, ctl__data_len, ctl__done, ctl__op_len, ctl__rd__data, ctl__rd__stb, ctl__req, ctl__wr__data, ctl__xfer_is_wr, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__stb, efb__we, rst);
module \top.uart (i, data, ack, rdy, rst, clk, o);
module \top.uart.rx (rst, clk, divisor, i);
module \top.uart.tx (data, ack, rdy, rst, clk, divisor, o);
module \top.wait (rst, clk, stb);
Once I'm able to rename the top
prefix (probably via an -n [NAME]
argument to the CLI?), I can finish the refactor, and wait for other ppl to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try the updated version.
6dfed2e
to
a27f8a2
Compare
The attribute sees essentially no use and the information is much better served by putting it in the module name. In addition this means that the entire tree can be renamed simply by renaming the top module. Tools like GTKWave show the names of the instances, not the modules, so they are not affected by the longer names.
a27f8a2
to
7ff2e44
Compare
amaranth_cli/__init__.py
Outdated
raise argparse.ArgumentTypeError(f"'{qual_name}:{mod_name}' refers to an object that is not elaboratable") | ||
return obj | ||
else: | ||
raise argparse.ArgumentTypeError(f"{reference!r} is not a Python object reference") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to realize that by "object reference", the error means "your input argument fails to match a regex", rather than "Python couldn't find an object with that name" (which is what the ImportError
exception handler is for).
Perhaps this else
branch could have "expected format is path.to.mod:Obj
" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this could be improved!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to submit a PR against a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do in the coming days (my response time will be sporadic until possibly this Monday afternoon).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is done: whitequark#1
I'm quite happy with this PR at this point. But I don't want it merged before other people have tested it. Any chance we could bring it up at the next meeting to let other ppl know to try it out and report issues as a "final comment period"? |
It's still a draft in my thoughts--I just prototyped it for you. Merging it needs an RFC first. |
Demo: