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

Undriven signal ends up as module port #630

Closed
antonblanchard opened this issue Aug 25, 2021 · 3 comments · Fixed by #658
Closed

Undriven signal ends up as module port #630

antonblanchard opened this issue Aug 25, 2021 · 3 comments · Fixed by #658
Milestone

Comments

@antonblanchard
Copy link
Contributor

antonblanchard commented Aug 25, 2021

If a signal has no driver, it ends up as a port:

from nmigen import *
from nmigen.back import verilog

class Test(Elaboratable):
    def __init__(self):
        self.val_out = Signal()

    def elaborate(self, platform):
        m = Module()
        int_sig = Signal(8)
        m.d.comb += self.val_out.eq(int_sig)

        return m

if __name__ == "__main__":
    top = Test()
    with open("test.v", "w") as f:
        f.write(verilog.convert(top))
module top(int_sig);
  input [7:0] int_sig;
  wire val_out;
  assign val_out = int_sig[0];
endmodule

This is pretty confusing. Reporting an error here would be great.

@BracketMaster
Copy link

BracketMaster commented Aug 27, 2021

You're really supposed to provide an explicit port list iirc.

from nmigen import *
from nmigen.back import verilog

class Test(Elaboratable):
    def __init__(self):
        self.val_out = Signal()

    def elaborate(self, platform):
        m = Module()
        int_sig = Signal(8)
        m.d.comb += self.val_out.eq(int_sig)

        return m

if __name__ == "__main__":
    top = Test()
    with open("test.v", "w") as f:
        f.write(verilog.convert(top, ports=[top.val_out]))
/* Generated by Yosys 0.9+3755 (git sha1 442d19f6, clang 12.0.0 -fPIC -Os) */

(* \nmigen.hierarchy  = "top" *)
(* top =  1  *)
(* generator = "nMigen" *)
module top(val_out);
  wire [7:0] int_sig;
  output val_out;
  assign int_sig = 8'h00;
  assign val_out = 1'h0;
endmodule

@antonblanchard
Copy link
Contributor Author

antonblanchard commented Aug 28, 2021

Thanks @BracketMaster, that does fix the issue at the top level.

I think there are a couple of usability issues here:

  • If inferring ports in Verilog generation doesn't work then ports= shouldn't be an optional argument.
  • The actual issue was an undriven signal many layers deep in the hierarchy. It was confusing to see this signal in a bunch of internal module ports when looking at waveforms. I presume every undriven signal makes it's way up to the top level.

@whitequark whitequark added this to the 0.3 milestone Dec 13, 2021
@whitequark
Copy link
Member

whitequark commented Dec 13, 2021

Let's deprecate optional ports= in 0.3 and remove it in 0.4.

modwizcode added a commit to modwizcode/amaranth that referenced this issue Dec 13, 2021
modwizcode added a commit to modwizcode/amaranth that referenced this issue Dec 13, 2021
modwizcode added a commit to modwizcode/amaranth that referenced this issue Dec 13, 2021
modwizcode added a commit to modwizcode/amaranth that referenced this issue Dec 13, 2021
whitequark pushed a commit that referenced this issue Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants