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

Python simulator hangs or throws when trying to drive reset from testbench #566

Closed
hansfbaier opened this issue Dec 25, 2020 · 9 comments
Closed

Comments

@hansfbaier
Copy link

hansfbaier commented Dec 25, 2020

I just learned that nmigen handles reset automatically, so in a simple module, I pulled out my own reset logic and tried
to use the reset logic from the clock domain. That does not seem to be a problem for verilog generation,
but for the python simulator I tried two approaches, and both failed:

  1. I get the resetsignal with ResetSignal() and then try to drive it with in the tests. This should work, because the constructors all default to the "sync" domain. But here I get the error:
File "/home/jack/riscv/litex-root/nmigen/nmigen/sim/pysim.py", line 209, in __init__
    self.curr = self.next = signal.reset
AttributeError: 'ResetSignal' object has no attribute 'reset'
  1. I explicitly create a clock domain and feed it into the simulator, but then the simulator does not seem to terminate
    reset-sim-hangs.tar.gz
@whitequark
Copy link
Member

whitequark commented Dec 25, 2020

  1. I get the resetsignal with ResetSignal() and then try to drive it with in the tests.

This part is a duplicate of #415.

@whitequark
Copy link
Member

whitequark commented Dec 25, 2020

  1. I explicitly create a clock domain and feed it into the simulator, but then the simulator does not seem to terminate

In sim.add_clock(1e-6, domain=sync), the domain argument is a clock domain name, not a clock domain object. So it should be sim.add_clock(1e-6, domain="sync") instead.

@hansfbaier
Copy link
Author

hansfbaier commented Dec 25, 2020

  1. I explicitly create a clock domain and feed it into the simulator, but then the simulator does not seem to terminate

In sim.add_clock(1e-6, domain=sync), the domain argument is a clock domain name, not a clock domain object. So it should be sim.add_clock(1e-6, domain="sync") instead.
When I do this, then I get the same error as in the first approach:
File "/home/jack/riscv/litex-root/nmigen/nmigen/sim/pysim.py", line 209, in __init__ self.curr = self.next = signal.reset AttributeError: 'ResetSignal' object has no attribute 'reset'

@whitequark
Copy link
Member

whitequark commented Dec 25, 2020

Yes. Please see #415 for a workaround.

@hansfbaier
Copy link
Author

hansfbaier commented Dec 25, 2020

  1. I explicitly create a clock domain and feed it into the simulator, but then the simulator does not seem to terminate

In sim.add_clock(1e-6, domain=sync), the domain argument is a clock domain name, not a clock domain object. So it should be sim.add_clock(1e-6, domain="sync") instead.

Also, when I wrote the code I looked it up in the source code and both the docstring and the source code seemed to suggest that passing the domain would be an option (Simulator.py):

        domain : str or ClockDomain
            Driven clock domain. If specified as a string, the domain with that name is looked up
            in the root fragment of the simulation.

@whitequark
Copy link
Member

whitequark commented Dec 25, 2020

Hm, you're right. I will look into it.

@whitequark
Copy link
Member

whitequark commented Dec 11, 2021

Also, when I wrote the code I looked it up in the source code and both the docstring and the source code seemed to suggest that passing the domain would be an option (Simulator.py):

Sorry, I was wrong about this--you can indeed pass a domain object, and it works just fine.

2. I explicitly create a clock domain and feed it into the simulator, but then the simulator does not seem to terminate

The issue with this approach is that you end up with two different clock domains called "sync" in the simulation; one automatically added by nMigen when you pass your DUT to the simulator (since the DUT does not itself define a "sync" domain), and another explicitly created by you. To avoid this, add a wrapper:

    dut = EdgeToPulse()
    wrapper = Module()
    wrapper.domains.sync = sync = ClockDomain("sync")
    sim = Simulator(wrapper)

This causes the same error as your first approach, since it's functionally identical to it.

@whitequark
Copy link
Member

whitequark commented Dec 11, 2021

I've added a warning to cover the issue in your second approach. It now shows:

edgetopulse-bench-second-approach.py:66: UserWarning: Adding a clock process that drives a clock domain object named 'sync', which is distinct from an identically named domain in the simulated design
  sim.add_clock(1e-6, domain=sync)

I consider the issue with the first approach a duplicate of #415.

@whitequark whitequark added this to the 0.3 milestone Dec 11, 2021
@hansfbaier
Copy link
Author

hansfbaier commented Dec 11, 2021

Thank you so much for looking into this! 🙏

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

No branches or pull requests

2 participants