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

Error accessing ResetSignal() #415

Open
GuzTech opened this issue Jun 29, 2020 · 6 comments
Open

Error accessing ResetSignal() #415

GuzTech opened this issue Jun 29, 2020 · 6 comments

Comments

@GuzTech
Copy link

GuzTech commented Jun 29, 2020

I want to toggle the reset signal of a module in simulation, but I get an error that do not quite understand. Here is a simple example:

from nmigen import *
from nmigen.back import pysim

class Issue(Elaboratable):
	def __init__(self):
		self.rst = ResetSignal()
		self.a = Signal()

	def elaborate(self, platform):
		m = Module()
		
		with m.If(self.rst):
			m.d.sync += self.a.eq(1)
		
		return m

if __name__ == "__main__":
	dut = Issue()
	sim = pysim.Simulator(Issue())
	sim.add_clock(1.0 / 12e6)

	def test():
		yield dut.rst.eq(1)
		yield
		yield dut.rst.eq(0)
		yield

	sim.add_sync_process(test())

	with sim.write_vcd("issue.vcd", "issue.gtkw", traces=[dut.rst]):
		sim.run()

This gives me the error AttributeError: 'ResetSignal' object has no attribute 'name'.
If I change it to traces=[dut.a], then I get the error AttributeError: 'ResetSignal' object has no attribute 'reset'.

@GuzTech
Copy link
Author

GuzTech commented Jun 29, 2020

Btw, here's the full output when I run python issue.py with traces=[dut.a]:

issue.py:28: DeprecationWarning: instead of generators, use generator functions as processes; this allows the simulator to be repeatedly reset
  sim.add_sync_process(test())
Traceback (most recent call last):
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 309, in get_signal
    return self.indexes[signal]
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/hdl/ast.py", line 1442, in __getitem__
    return self._storage[key]
KeyError: <nmigen.hdl.ast.SignalKey (rst sync)>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 239, in for_signal
    return self.signals[signal]
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/hdl/ast.py", line 1442, in __getitem__
    return self._storage[key]
KeyError: <nmigen.hdl.ast.SignalKey (rst sync)>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "issue.py", line 31, in <module>
    sim.run()
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 1086, in run
    while self.step():
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 1075, in step
    self._settle()
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 1063, in _settle
    while self._delta():
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 1053, in _delta
    process.run()
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 912, in run
    self.coroutine.throw(exn)
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 974, in wrapper
    yield from process()
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 949, in wrapper
    yield from process
  File "issue.py", line 23, in test
    yield dut.rst.eq(1)
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 861, in run
    exec(_StatementCompiler.compile(self.eval_context, command),
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 681, in compile
    output_indexes = [context.get_signal(signal) for signal in stmt._lhs_signals()]
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 681, in <listcomp>
    output_indexes = [context.get_signal(signal) for signal in stmt._lhs_signals()]
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 312, in get_signal
    self.slots.append(self.state.for_signal(signal))
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 241, in for_signal
    signal_state = _SignalState(signal, self.pending)
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 190, in __init__
    self.reset()
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 193, in reset
    self.curr = self.next = self.signal.reset
AttributeError: 'ResetSignal' object has no attribute 'reset'

And when I run it with traces=[dut.a] like in the OP, then this is the output:

  sim.add_sync_process(test())
Traceback (most recent call last):
  File "issue.py", line 30, in <module>
    with sim.write_vcd("issue.vcd", "issue.gtkw", traces=[dut.rst]):
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 1121, in write_vcd
    waveform_writer = _VCDWaveformWriter(self._signal_names,
  File "/home/oguz286/.local/lib/python3.8/site-packages/nmigen/back/pysim.py", line 96, in __init__
    trace_names[trace] = {("top", trace.name)}
AttributeError: 'ResetSignal' object has no attribute 'name'

@bhansconnect
Copy link

bhansconnect commented Nov 30, 2020

Are there any workarounds for this, or can simulations just never toggle reset currently?

@rroohhh
Copy link
Contributor

rroohhh commented Nov 30, 2020

@bhansconnect you can do something like this:

from nmigen import *
from nmigen.back import pysim

class Issue(Elaboratable):
    def __init__(self):
        self.rst = Signal()
        self.a = Signal()

    def elaborate(self, platform):
        m = Module()

        m.d.comb += ResetSignal().eq(self.rst)

        with m.If(self.rst):
            m.d.sync += self.a.eq(1)

        return m

if __name__ == "__main__":
    dut = Issue()
    sim = pysim.Simulator(Issue())
    sim.add_clock(1.0 / 12e6)

    def test():
        yield dut.rst.eq(1)
        yield
        yield dut.rst.eq(0)
        yield

    sim.add_sync_process(test)

    with sim.write_vcd("issue.vcd", "issue.gtkw", traces=[dut.rst]):
        sim.run()

@bhansconnect
Copy link

bhansconnect commented Dec 1, 2020

That does work, thanks.

@hansfbaier
Copy link

hansfbaier commented Dec 26, 2020

@bhansconnect, @GuzTech
The above code will generate a driver-driver conflict, because m.d.sync += self.a.eq(1) will try to drive a high, while the reset logic generated by nmigen will try to drive a low, and you will get this in the simulator:
driver-driver-conflict
The proper nmigen way to have your signal reset to 1 would be to tell nmigen's internal reset logic to set it to one and
then you can skip your if statement (of course then nothing is left and if we don't want a to be it's reset value forever, we give it a different level):

from nmigen import Elaboratable, Module, ResetSignal, Signal
from nmigen.sim import Simulator
from nmigen.cli import main

class Issue(Elaboratable):
    def __init__(self):
        self.rst = Signal()
        self.a = Signal(reset=1)

    def elaborate(self, platform):
        m = Module()
        m.d.comb += ResetSignal().eq(self.rst)
        m.d.sync += self.a.eq(0)

        return m

if __name__ == "__main__":
    dut = Issue()
    simulate = True
    if simulate:
        sim = Simulator(dut)
        sim.add_clock(1.0 / 12e6)

        def test():
            yield
            yield dut.rst.eq(1)
            yield
            yield dut.rst.eq(0)
            yield

        sim.add_sync_process(test)

        with sim.write_vcd("issue.vcd", "issue.gtkw", traces=[dut.a, dut.rst]):
            sim.run()
    else:
        main(dut, ports=[dut.a])

Which will get you:
2020-12-26_10-14
Having said that, while the simulator seems to look nice,
the generated verilog has some serious issues, which will probably make it unuseable:

module top(clk, rst, a);
  reg \initial  = 0;
  output a;
  reg a = 1'h1;
  reg \a$next ;
  input clk;
  output rst; // rst became an output!
  wire \rst$1 ;

  always @(posedge clk)
    a <= \a$next ;

  always @* begin
    if (\initial ) begin end
    \a$next  = 1'h0;
    casez (rst)
      1'h1:
          \a$next  = 1'h1;
    endcase
  end

  assign \rst$1  = 1'h0;
  assign rst = 1'h0; // rst is hardwired to 0
endmodule

The most noteable here is that rst became an output, generating a driver-driver conflict with the connected module which is actually supposed to drive rst.

@hansfbaier
Copy link

hansfbaier commented Dec 26, 2020

@bhansconnect, @GuzTech
So it looks like the only viable path would appear be to decouple a from nmigen's internally generated reset logic:

from nmigen import Elaboratable, Module, ResetSignal, Signal
from nmigen.sim import Simulator
from nmigen.cli import main

class Issue(Elaboratable):
    def __init__(self):
        self.reset = Signal()
        self.a = Signal(reset=1, reset_less=True)

    def elaborate(self, platform):
        m = Module()

        with m.If(self.reset):
            m.d.sync += self.a.eq(1)
        with m.Else():
            m.d.sync += self.a.eq(0)

        return m

but while this works in the simulator as above, the verilog file shows:
image
Note that the reset input must not be named rst because this is auto generated by nmigen and will cause rst to turn into an output in the verilog.
Now the verilog looks like:

module top(reset, clk, rst, a);
    reg \initial  = 0;
    output a;
    reg a = 1'h1;
    reg \a$next ;
    input clk;
    input reset;
    input rst;

  always @(posedge clk)
    a <= \a$next ;

  always @* begin
    if (\initial ) begin end
    casez (reset)
      /* src = "issue-fix.py:13" */
      1'h1:
          \a$next  = 1'h1;
      /* src = "issue-fix.py:15" */
      default:
          \a$next  = 1'h0;
    endcase
  end
endmodule

which looks useable, with the wart that rst now is a no-op here and might as well have been omitted, which is not possible, because it is autogenerated by nmigen.

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

No branches or pull requests

5 participants