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

asserts.Past doesn't work correctly with DomainRenamer #372

Closed
tpwrules opened this issue Apr 28, 2020 · 3 comments
Closed

asserts.Past doesn't work correctly with DomainRenamer #372

tpwrules opened this issue Apr 28, 2020 · 3 comments
Labels

Comments

@tpwrules
Copy link
Contributor

DomainRenamers don't seem to change the domain that Past looks at. Below is a minimal example that demonstrates the problem and the correct behavior.

The Toggle module flips its internal toggle every clock cycle, then uses Past to output it delayed by one cycle. Both toggles are DomainRenamed to the "fast" domain. Toggle t1 uses Past(domain=None) and generates an incorrect output because Past samples the toggle from the sync domain; the DomainRenamer didn't change it to "fast". Toggle t2, on the other hand, uses Past(domain="fast") to ensure the sample is taken from the correct clock domain and so generates the correct output.

from nmigen import *
from nmigen.asserts import Past
from nmigen.back.pysim import Simulator, Delay

class Toggle(Elaboratable):
    def __init__(self, domain):
        self.domain = domain
        self.delayed = Signal()

    def elaborate(self, platform):
        m = Module()
        toggle = Signal()
        m.d.sync += toggle.eq(~toggle)
        m.d.sync += self.delayed.eq(Past(toggle, domain=self.domain))
        return m

m = Module()
m.submodules.t1 = t1 = DomainRenamer("fast")(Toggle(None))
m.submodules.t2 = t2 = DomainRenamer("fast")(Toggle("fast"))

sim = Simulator(m)
sim.add_clock(1e-6, domain="sync")
sim.add_clock((1/3)*1e-6, domain="fast")

with sim.write_vcd("test.vcd", "test.gtkw", traces=[t1.delayed, t2.delayed]):
    sim.run_until(10e-6, run_passive=True)

GTKWave output is shown below. The top trace is the output of t1 and the bottom is the output of t2.
image

@whitequark whitequark added the bug label Apr 29, 2020
@whitequark
Copy link
Member

whitequark commented Apr 29, 2020

I didn't investigate this in detail and I'll take you at your word that there is a bug; currently, clock/reset signals and domain renames are done via term rewriting, which is unfortunately as you're discovering now error-prone (and also quite slow).

It would probably be possible to add a workaround here but I would much rather prefer to use a different approach that systematically eliminates all similar issues, but would take more time to implement. Are you in a rush?

@tpwrules
Copy link
Contributor Author

I'm not in a rush. I just avoided using Past in this situation instead. When you change the underlying approach and close the issue, this comment is me reminding myself to go back and re-add it.

@whitequark
Copy link
Member

Past is going to be deprecated and removed (#526). Closing this issue in favor of that one.

@whitequark whitequark closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants