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

Weird simulation behaviour with clock's and Instances #391

Closed
rroohhh opened this issue May 26, 2020 · 16 comments
Closed

Weird simulation behaviour with clock's and Instances #391

rroohhh opened this issue May 26, 2020 · 16 comments

Comments

@rroohhh
Copy link
Contributor

rroohhh commented May 26, 2020

I have encountered a really weird bug in pysim when using Instance in combination with ClockSignal.
The following example:

from nmigen import *
from nmigen.back.pysim import Simulator

trigger_bug = False

class Bug(Elaboratable):
    def elaborate(self, plat):
        m = Module()

        clk_source = Signal()
        clk_sink = Signal()

        m.submodules += Instance("A", o_clk=clk_source)
        m.d.comb += ClockSignal().eq(clk_source)

        if trigger_bug:
            m.submodules += Instance("B", i_clk=clk_sink)
            m.d.comb += clk_sink.eq(ClockSignal())

        counter = Signal(8)
        m.d.sync += counter.eq(counter + 1)

        return m


simulator = Simulator(Bug())
simulator.add_clock(1e-9)
with simulator.write_vcd("test.vcd", "test.gtkw"):
    simulator.run_until(1e-8, run_passive=True)

Produces this vcd output:
no_bug
Which is about what I would expect.
However if I set trigger_bug to True I get the following output:
bug
Where the counter still works, but the ClockSignal is not driven.

The behaviour doesn't change when swapping the clk_source Instance for a constant 0 assignment, it is just there to show a possible real world occurrence.

@whitequark
Copy link
Member

The behavior of Instance in pysim is somewhat unusual and I believe it's not currently documented or a part of the public API. You really shouldn't do it for now, though I can't just remove it at the moment because memories rely on it. At a later point we should decide how to make it well-defined and document it, but not yet.

@rroohhh
Copy link
Contributor Author

rroohhh commented May 26, 2020

I understand that, however I think that it should never be possible for anything that just reads the ClockSignal to change its value, even if it is a Instance. That is very unintuitive.

@whitequark
Copy link
Member

whitequark commented May 26, 2020

I agree of course. It's just a bug somewhere in the simulation code. I don't consider it a priority to fix since you can't really use Instances anyway at the moment. I've reclassified this issue as a bug.

@whitequark whitequark added bug and removed question labels May 26, 2020
@rroohhh
Copy link
Contributor Author

rroohhh commented May 26, 2020

Actually it seems like I was just bad at minimizing this example, the following snippet shows the same behaviour:

from nmigen import *
from nmigen.back.pysim import Simulator

trigger_bug = False

class Bug(Elaboratable):
    def elaborate(self, plat):
        m = Module()

        m.d.comb += ClockSignal().eq(0)

        if trigger_bug:
            a = Signal()
            m.d.comb += a.eq(ClockSignal())

        counter = Signal(8)
        m.d.sync += counter.eq(counter + 1)

        return m

simulator = Simulator(Bug())
simulator.add_clock(1e-9)
with simulator.write_vcd("test.vcd", "test.gtkw", traces=[]):
    simulator.run_until(1e-8, run_passive=True)

@rroohhh
Copy link
Contributor Author

rroohhh commented May 26, 2020

Furthermore, this does not only affect ClockSignal but also normal Signal, which makes the simulator quite hard to use with stubbed out external interfaces.
Example:

from nmigen import *
from nmigen.back.pysim import Simulator

trigger_bug = True

class Bug(Elaboratable):
    def __init__(self):
        self.a = Signal()
        self.b = Signal()

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

        m.domains.sync = ClockDomain()
        m.d.comb += self.b.eq(0)

        if trigger_bug:
            m.d.comb += self.a.eq(self.b)

        counter = Signal(8)
        m.d.sync += counter.eq(counter + 1)

        return m


dut = Bug()
simulator = Simulator(dut)
simulator.add_clock(1e-9)

def testbench():
    yield dut.b.eq(1)
    yield
    yield
    yield
    yield dut.b.eq(0)
    yield
    yield
    yield
    yield

simulator.add_sync_process(testbench)
with simulator.write_vcd("test.vcd", "test.gtkw", traces=[]):
    simulator.run_until(1e-8, run_passive=True)

This produces this, when setting trigger_bug to False:
no_bug2
And this when setting trigger_bug to True:
bug2

@whitequark
Copy link
Member

Actually it seems like I was just bad at minimizing this example, the following snippet shows the same behaviour:

Thanks for clarification, I'll look into this ASAP.

@whitequark
Copy link
Member

Ah so the problem here is that you're not allowed to override combinatorial signals in a simulation, but it's not currently checked. So this is a duplicate of #318.

@whitequark whitequark added duplicate and removed bug labels May 26, 2020
@rroohhh
Copy link
Contributor Author

rroohhh commented May 27, 2020

Ah of course, sorry for not connecting the dots.

I encounter this problem mostly in a situation like this:

a = Signal()
b = Signal()

m.d.comb += b.eq(a)

Where a is a port (and would outside of simulation be connected to an Instance). Due to how the code is structured I can only get easy access to the b Signal, so thats the one driven by the simulation.
Maybe this is too niche, but for my situation a way to get the driver of the b Signal, so that I can drive that one instead would be very helpful. Is something like that possible (I only need the simple case of straight assignments)?

@whitequark
Copy link
Member

Is something like that possible (I only need the simple case of straight assignments)?

No, unfortunately not. How is your code structured?

@rroohhh
Copy link
Contributor Author

rroohhh commented May 27, 2020

Well this ties back to Instances. I want to simulate AXI slaves that are connected to the ARM processor in an Zynq, which is done by connecting them to the PS7 blackbox.

To make it easy to create AXI slave all over the design hierarchy, my platform gives access to a PS7 singleton and more interesting a AXI interconnect singleton.
This is for example used to create registers that are readable / writable over AXI from all over the design hierarchy.
As a workaround it is of course possible to just disable the part that connects the PS7 Instance to the signals.

However the same problem also happens for example for clockdomains generated by a PLL. The PLL is obviously also a Instance so that one has to be stubbed out aswell.

So the main problem is I would have to disconnect every Instance when simulating.

Now that I have written all this out I have noticed that I already don't use Instance directly, but use a wrapper to make it easier to use, so it should be trivial to just put code that disconnects the Instance for simulations in there.

So I guess sorry for the noise.

(The code lives here https://github.com/apertus-open-source-cinema/nmigen-gateware if you actually want to see how its structured, but it probably breaks nmigen's intended usage in many ways :)

@whitequark
Copy link
Member

I want to simulate AXI slaves that are connected to the ARM processor in an Zynq, which is done by connecting them to the PS7 blackbox.

Oh I see! Yeah, that's not currently handled well at all, but we already have two tracking issues for those: #113 and #308 (#308 isn't quite about PS7 but it's similar; we might want to open another issue for exposing Zynq-like things in particular).

Maybe you could comment there with your experience and desires? Then the design that eventually emerges would be guided by those.

Now that I have written all this out I have noticed that I already don't use Instance directly, but use a wrapper to make it easier to use, so it should be trivial to just put code that disconnects the Instance for simulations in there.

Yep that's what I would suggest as a workaround.

but it probably breaks nmigen's intended usage in many ways

Wouldn't that mean that nmigen's intended usage is wrong? Or at the very least, underdocumented or unfinished. Anyway, the main thing I see is that you're using your own CSR and SoC abstractions instead of nmigen-soc.

@whitequark
Copy link
Member

I'm going to close this now in favor of the two issues I mentioned.

@rroohhh
Copy link
Contributor Author

rroohhh commented May 27, 2020

Wouldn't that mean that nmigen's intended usage is wrong? Or at the very least, underdocumented or unfinished.

Maybe, currently for example I don't see a easy way to to what SocPlatform in combination with this is doing without what feels like poking at nmigen internals a bit too much.

Anyway, the main thing I see is that you're using your own CSR and SoC abstractions instead of nmigen-soc.

Yeah at the time it didn't feel like using nmigen-soc would be a great help, but I have to think more about why exactly again.

@whitequark
Copy link
Member

Maybe, currently for example I don't see a easy way to to what SocPlatform in combination with this is doing without what feels like poking at nmigen internals a bit too much.

Ah interesting, I'll remember to take a look at that. Right now I'm pretty busy with cxxrtl/cxxsim but SoC stuff is quite high on the list.

Yeah at the time it didn't feel like using nmigen-soc would be a great help, but I have to think more about why exactly again.

Nice, looking forward to hearing your thoughts on that.

@anuejn
Copy link
Contributor

anuejn commented May 28, 2020

Maybe this is too niche, but for my situation a way to get the driver of the b Signal, so that I can drive that one instead would be very helpful. Is something like that possible (I only need the simple case of straight assignments)?

another useful way to handle this would be a simulator command, that disconnects a signal from all signals that drive it in the design to be able to drive it from the testbench. This was also a thing that I a few times when debugging a core and stubbing half of its functionality out.

@whitequark
Copy link
Member

That would be pretty challenging to add on simulator level, especially if parity of pysim and cxxsim is desired. It might be a lot easier to add on code transformation level though.

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

No branches or pull requests

3 participants