-
Notifications
You must be signed in to change notification settings - Fork 173
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
fsm_state changes mid cycle #439
Comments
Greetings. I'm Cesar, from libre-SOC. I'd like to point to the previous discussion on https://bugs.libre-soc.org/show_bug.cgi?id=417#c13, where the issue first arose. I managed to trim the test case by removing the shift state. Since only two states remain, the next logical step is to replace the FSM with a one-bit register. """Simple Handshake Test
1) p_ready_o is asserted on the initial ("Idle") state, otherwise it keeps low.
2) n_valid_o is asserted on the final ("Done") state, otherwise it keeps low.
3) The FSM stays in the Idle state while p_valid_i is low, otherwise
it goes to Done.
4) The FSM stays in the Done state while n_ready_i is low, otherwise
it goes back to Idle.
"""
from nmigen import Elaboratable, Signal, Module
cxxsim = True
if cxxsim:
from nmigen.sim.cxxsim import Simulator, Settle
else:
from nmigen.sim.pysim import Simulator, Settle
class Handshake(Elaboratable):
def __init__(self):
self.p_valid_i = Signal()
self.p_ready_o = Signal()
self.n_ready_i = Signal()
self.n_valid_o = Signal()
def elaborate(self, platform):
m = Module()
with m.FSM():
with m.State("IDLE"):
m.d.comb += self.p_ready_o.eq(1)
with m.If(self.p_valid_i):
m.next = "DONE"
with m.State("DONE"):
m.d.comb += self.n_valid_o.eq(1)
with m.If(self.n_ready_i):
m.next = "IDLE"
return m
def __iter__(self):
yield self.p_valid_i
yield self.p_ready_o
yield self.n_ready_i
yield self.n_valid_o
def ports(self):
return list(self)
def test_handshake():
m = Module()
m.submodules.hsk = dut = Handshake()
sim = Simulator(m)
sim.add_clock(1e-6)
def send():
yield
yield
yield
yield
yield
yield
yield
# assert p_valid_i
yield dut.p_valid_i.eq(1)
yield
print("set up signals")
# wait for p_ready_o to be asserted
ready_o = yield dut.p_ready_o
print("ready_o", ready_o)
while not (yield dut.p_ready_o):
ready_o = yield dut.p_ready_o
print("ready_o", ready_o)
yield
print("done ready check")
# negate p_valid_i
yield dut.p_valid_i.eq(0)
print("done send")
def receive():
yield
# signal readiness to receive data
yield dut.n_ready_i.eq(1)
yield
# wait for n_valid_o to be asserted
valid_o = yield dut.n_valid_o
print(" valid_o", valid_o)
while not (yield dut.n_valid_o):
valid_o = yield dut.n_valid_o
print(" valid_o", valid_o)
yield
# negate n_ready_i
yield dut.n_ready_i.eq(0)
print(" done receive")
def producer():
print("start of producer")
yield from send()
print("end of producer")
def consumer():
yield
# the consumer is not in step with the producer, but the
# order of the results are preserved
# 3 << 4 = 48
print(" start of receiver")
yield from receive()
print(" end of receiver")
sim.add_sync_process(producer)
sim.add_sync_process(consumer)
sim_writer = sim.write_vcd("handshake.vcd")
with sim_writer:
sim.run()
if __name__ == "__main__":
test_handshake() |
Thanks Cesar. BracketMaster is Yehowshua BTW. |
This looks much better, thank you! Though it seems like it could be minimized a bit further without much effort? |
On https://bugs.libre-soc.org/show_bug.cgi?id=417#c15, Luke wrote:
To which, I replied:
On the other hand, the behavior of cxxsim is consistent with it doing an implicit Settle() after a yield. The following test demonstrates it: from nmigen import Elaboratable, Signal, Module
from nmigen.sim.cxxsim import Simulator as CxxSimulator
from nmigen.sim.pysim import Simulator as PySimulator
class SamplePoint(Elaboratable):
"""Just a simple one-bit register"""
def __init__(self):
self.data_i = Signal()
self.data_o = Signal()
def elaborate(self, _):
m = Module()
m.d.sync += self.data_o.eq(self.data_i)
return m
def ports(self):
return [self.data_i, self.data_o]
def test_sample_point(sim_type):
print("Testing", sim_type)
if sim_type == "cxxsim":
simulator = CxxSimulator
else:
simulator = PySimulator
m = Module()
m.submodules.sp = dut = SamplePoint()
sim = simulator(m)
sim.add_clock(1e-6)
def process():
# present data to register input, to be latched at the next clock
# rising edge
yield dut.data_i.eq(1)
yield
# at this point, just after the clock rising edge, the register
# should still hold its previous (reset) value
assert (yield dut.data_o) == 0
sim.add_sync_process(process)
sim_writer = sim.write_vcd(f"{sim_type}.vcd")
with sim_writer:
sim.run()
print("PASS")
if __name__ == "__main__":
test_sample_point("pysim")
test_sample_point("cxxsim") |
This narrows it down enough. Thanks for your effort, I'll fix this soon. |
This issue is now fixed in the |
Greetings. After a pull of the cxxsim branch (commit 1f8ba74), it looks like the register is not being updated, at all, even after a Settle() or a yield. I modified the previous test case, to check for this. from nmigen import Elaboratable, Signal, Module
from nmigen.sim.cxxsim import Simulator as CxxSimulator
from nmigen.sim.cxxsim import Settle as CxxSettle
from nmigen.sim.pysim import Simulator as PySimulator
from nmigen.sim.pysim import Settle as PySettle
class SamplePoint(Elaboratable):
"""Just a simple one-bit register"""
def __init__(self):
self.data_i = Signal()
self.data_o = Signal()
def elaborate(self, _):
m = Module()
m.d.sync += self.data_o.eq(self.data_i)
return m
def ports(self):
return [self.data_i, self.data_o]
def test_sample_point(sim_type):
print("Testing", sim_type)
if sim_type == "cxxsim":
simulator = CxxSimulator
settle = CxxSettle
else:
simulator = PySimulator
settle = PySettle
m = Module()
m.submodules.sp = dut = SamplePoint()
sim = simulator(m)
sim.add_clock(1e-6)
def process():
# present data to register input, to be latched at the next clock
# rising edge
yield dut.data_i.eq(1)
yield
# at this point, just after the clock rising edge, the register
# should still hold its previous (reset) value
assert (yield dut.data_o) == 0
print("Check reset value: PASS")
yield settle()
# now we should see it
assert (yield dut.data_o) == 1
print("Check latched value: PASS")
sim.add_sync_process(process)
sim_writer = sim.write_vcd(f"{sim_type}.vcd")
with sim_writer:
sim.run()
if __name__ == "__main__":
test_sample_point("pysim")
test_sample_point("cxxsim") |
Same result on the just rebased branch (060ad25). |
Commit 7ca1477 fixes the problem where registers change mid-cycle. However, the second problem (as described in #455 (comment)) also arises here, so the code is just as broken. |
I've pushed a low-performance workaround to the |
Sorry, with the cxxsim branch checked out a 0caa57e, I still see the earlier issue. It seems to me, that the behavior of cxxsim is consistent with it doing an implicit Settle() after the clock rises, but before the Python process is run. I managed to further reduce the test case. See below: from nmigen import Signal, Module
from nmigen.sim.cxxsim import Simulator
m = Module()
o = Signal()
m.d.sync += o.eq(1)
def process():
assert (yield o) == 0
sim = Simulator(m)
sim.add_clock(1e-6)
sim.add_sync_process(process)
sim_writer = sim.write_vcd("bug-439.vcd")
with sim_writer:
sim.run() |
Thanks for the investigation! The reduced testcase is very helpful. Regarding the implicit |
@cestrauss Please take another look. I completely reworked the integration layer and I believe cxxsim should no longer exhibit this class of issue. |
You'll need the very latest Yosys to use the current |
I installed Yosys master (commit c66d1dfad). |
Here you go. from nmigen import Signal, Module
from nmigen.sim import Simulator, Tick
m = Module()
r = Signal()
o = Signal()
m.d.sync += r.eq(1)
m.d.comb += o.eq(r & 1)
# m.d.comb += o.eq(r)
def process():
# (yield r)
yield Tick()
# (yield r)
yield Tick()
sim = Simulator(m, engine="cxxsim")
sim.add_clock(1e-6)
sim.add_process(process)
sim_writer = sim.write_vcd("bug-439.vcd")
with sim_writer:
sim.run() It generates the VCD below.
|
Thanks for reducing this. This is actually a very subtle issue in CXXRTL itself more so than the cxxsim integration code. This can be shown by eliminating everything but the clock process: from nmigen import Signal, Module
from nmigen.sim import Simulator, Tick
m = Module()
r = Signal()
o = Signal()
m.d.sync += r.eq(1)
m.d.comb += o.eq(r & 1)
sim = Simulator(m, engine="cxxsim")
sim.add_clock(1e-6)
sim_writer = sim.write_vcd("bug-439.vcd")
with sim_writer:
sim.run_until(2e-6, run_passive=True) I'll need to investigate how to fix this. Most likely, it will be necessary to change static scheduling of combinatorial nodes connected to registers. |
As a temporary workaround, try applying this patch: diff --git a/nmigen/sim/cxxsim.py b/nmigen/sim/cxxsim.py
index 753c72d..1ad91f1 100644
--- a/nmigen/sim/cxxsim.py
+++ b/nmigen/sim/cxxsim.py
@@ -71,6 +71,7 @@ class _CxxRTLProcess(BaseProcess):
def run(self):
self.cxxlib.eval(self.handle)
+ self.runnable = True
class _CxxSimulation(BaseSimulation): I think it won't cover every case, but it might get you further with your overall design. |
It works!
|
Right. I did check that on my end. What I'm more interested is you experimenting with your overall design moreso than the extracted test cases. |
I ran an integration test, which should give good coverage of the whole project. It did output the following:
Then, it spent some time compiling a 11MB sim.cc file, generating a 15MB sim.so. After one hour, with no further output, I interrupted it, giving the following stack trace:
I guess there is more reducing for me to do. |
Greetings. I managed to reduce the cxxsim hang, above, to the following: from nmigen import Signal, Module
from nmigen.sim import Simulator
m = Module()
s = Signal(33)
def process():
yield s.eq(1)
print("end of process")
sim = Simulator(m, engine="cxxsim")
sim.add_process(process)
sim.run() I guess we tended to use 32 bits or less in our own experiments, which resulted a lack of coverage for wider signals. |
Just to clarify:
|
@cestrauss I believe this is actually a logic bug, not a ctypes bug. Should be easy to fix. |
Please try again. |
The hanging is gone, thanks. |
Here you go: from nmigen import Signal, Module
from nmigen.sim import Simulator
m = Module()
s = Signal(reset=1)
def process():
assert((yield s) == 1)
sim = Simulator(m, engine="cxxsim")
sim.add_process(process)
sim.run() On pysim, signal "s" is high, even when not driven. |
Please try again. |
Not quite there yet: from nmigen import Signal, Module
from nmigen.sim import Simulator
m = Module()
s = Signal(reset=1)
t = Signal()
m.d.comb += t.eq(s)
def process():
assert((yield s) == 1)
sim = Simulator(m, engine="cxxsim")
sim.add_process(process)
sim.run() |
Interesting--that's a completely unrelated bug. I'll have to look into it as the cause is clear but the fix is not obvious. |
@cestrauss Please evaluate the following temporary solution: diff --git a/nmigen/back/rtlil.py b/nmigen/back/rtlil.py
index 916605e..de9e5e2 100644
--- a/nmigen/back/rtlil.py
+++ b/nmigen/back/rtlil.py
@@ -333,6 +333,10 @@ class _ValueCompilerState:
for value in signal._enum_class:
attrs["enum_value_{:0{}b}".format(value.value, signal.width)] = value.name
+ # XXX
+ if signal not in self.driven:
+ attrs["init"] = signal.reset
+
wire_curr = self.rtlil.wire(width=signal.width, name=wire_name,
port_id=port_id, port_kind=port_kind,
attrs=attrs, src=src(signal.src_loc)) It cannot be applied as-is, but it should unblock you to do further verification. |
Indeed. Appreciated. |
With the above patch, I'm getting the following error on some designs: By the way, the patch interferes with bounded model check, which is a minor inconvenience. I get: |
Not necessary; I approximately know the cause. I'll look into it. |
@cestrauss Minimized with this patch and command
|
@cestrauss |
@cestrauss Okay, should be fixed properly now. |
Confirmed fixed, thanks. |
Closing this issue now; it's been three unrelated bugs already, so any new issues that arise should probably be reported on their own. That said, does everything work for you now, or have you not tested the full design yet?
Yup, I wrote it exactly for cases like this. |
Sure. |
This "minimal" example is kind of long, but you said you[Whitequark] think you know what buy we're hitting.
I'll keep this example as is for now and try to trim it later.
The text was updated successfully, but these errors were encountered: