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

pysim: testbench-only signals are not placed at root level on the VCD file #561

Closed
cestrauss opened this issue Dec 13, 2020 · 3 comments · Fixed by #664
Closed

pysim: testbench-only signals are not placed at root level on the VCD file #561

cestrauss opened this issue Dec 13, 2020 · 3 comments · Fixed by #664

Comments

@cestrauss
Copy link

Consider:

from nmigen import Signal, Module
from nmigen.sim import Simulator

m = Module()
s = Signal()
t = Signal()
m.d.sync += s.eq(1)


def process():
    yield t.eq(1)


for engine in ["pysim", "cxxsim"]:
    sim = Simulator(m, engine=engine)
    sim.add_clock(1e-6)
    sim.add_sync_process(process)
    with sim.write_vcd(f"bug17.{engine}.vcd", traces=[s, t]):
        sim.run()

On PySim, both s and t are placed under the topmodule.
On CXXSim, t is at the root.

The generated GTKWave document (if any) correctly display the trace, but an already existing manually-generated document would need modification, which is inconvenient.

@whitequark
Copy link
Member

I've implemented this behavior intentionally for CXXSim; the idea is that this way, it's easier to figure out which signals come from the testbench, and which come from the netlist. I actually thought that PySim already worked that way, but it seems like I misremembered.

@cestrauss
Copy link
Author

cestrauss commented Dec 13, 2020

Sure, makes sense.
I've just checked nMigen 0.2, and its behavior does matches CXXSim, differently from the current PySim. So, it actually changed some time along the way. No harm done in changing it back, then.

@cestrauss cestrauss changed the title cxxsim: testbench-only signals are placed at root level on the VCD file pysim: testbench-only signals are not placed at root level on the VCD file Dec 13, 2020
@Lunaphied
Copy link
Contributor

Lunaphied commented Dec 16, 2021

I started working on the code required to implement this functionality (testbench signals on root level), and I realized that this might not be the most desirable behavior.

The way GTKWave handles root level signals is somewhat annoying; for one it's somewhat confusing to even recognize that there are root level signals in the first place. Selecting them if you click another module requires clicking that module again which is counterintuitive. Furthermore, you can only recursively import signals from a named hierarchy level. This means if you have a number of top-level testbench signals, you cannot simply recursively import all signals in the design (which is useful for smaller designs).

Given these patterns, I suggest we instead create our own "root" level module but give it a name such as "testbench", this has a much less obscure UX pattern. I have the code to do this already implemented, but I thought it would be worth discussing the behavior change first.

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