Repository: amaranth-lang/amaranth
Pull request #679 (2022-01-30 01:36:44)

sim: Fix clock phase in add_clock having to be specified in ps. 
Now more in line with the documentation, which states that the process will wait ``phase`` seconds.

Ref:
https://github.com/amaranth-lang/amaranth/blob/c83b51db6daf3b73fb2406fdeecf6ef7486bb0be/amaranth/sim/core.py#L104-L106

This was not true so far.
One had to give the clock phase wait time in ps.

codecov-commenter: # [Codecov](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) Report
> Merging [#679](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (8cccdc2) into [main](https://codecov.io/gh/amaranth-lang/amaranth/commit/c83b51db6daf3b73fb2406fdeecf6ef7486bb0be?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (c83b51d) will **decrease** coverage by `0.10%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/amaranth-lang/amaranth/pull/679/graphs/tree.svg?width=650&height=150&src=pr&token=CBuKpnBjhj&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)

```diff
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
- Coverage   81.32%   81.22%   -0.11%     
==========================================
  Files          49       49              
  Lines        6464     6465       +1     
  Branches     1526     1526              
==========================================
- Hits         5257     5251       -6     
- Misses       1015     1020       +5     
- Partials      192      194       +2     
```


| [Impacted Files](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) | Coverage Δ | |
|---|---|---|
| [amaranth/sim/core.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/679/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvc2ltL2NvcmUucHk=) | `82.82% <0.00%> (-0.85%)` | :arrow_down: |
| [amaranth/tracer.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/679/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvdHJhY2VyLnB5) | `89.18% <0.00%> (-5.41%)` | :arrow_down: |
| [amaranth/\_toolchain/cxx.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/679/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvX3Rvb2xjaGFpbi9jeHgucHk=) | `95.12% <0.00%> (-4.88%)` | :arrow_down: |
| [amaranth/\_toolchain/yosys.py](https://codecov.io/gh/amaranth-lang/amaranth/pull/679/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvX3Rvb2xjaGFpbi95b3N5cy5weQ==) | `61.01% <0.00%> (-1.70%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang). Last update [c83b51d...8cccdc2](https://codecov.io/gh/amaranth-lang/amaranth/pull/679?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang).

whitequark: cc @modwizcode 
bl0x: Illustrative example:

```python
from amaranth import *
from amaranth.sim import *

phase0_fail = Signal()
phase90_fail = Signal()
phase180_fail = Signal()
phase270_fail = Signal()
phase0_correct = Signal()
phase90_correct = Signal()
phase180_correct = Signal()
phase270_correct = Signal()

m = Module()
m.domains += ClockDomain("phase0_fail")
m.domains += ClockDomain("phase90_fail")
m.domains += ClockDomain("phase180_fail")
m.domains += ClockDomain("phase270_fail")
m.domains += ClockDomain("phase0_correct")
m.domains += ClockDomain("phase90_correct")
m.domains += ClockDomain("phase180_correct")
m.domains += ClockDomain("phase270_correct")

m.d.comb += [
        phase0_fail.eq(ClockSignal("phase0_fail")),
        phase90_fail.eq(ClockSignal("phase90_fail")),
        phase180_fail.eq(ClockSignal("phase180_fail")),
        phase270_fail.eq(ClockSignal("phase270_fail")),
        phase0_correct.eq(ClockSignal("phase0_correct")),
        phase90_correct.eq(ClockSignal("phase90_correct")),
        phase180_correct.eq(ClockSignal("phase180_correct")),
        phase270_correct.eq(ClockSignal("phase270_correct"))
        ]

sim = Simulator(m)
p = 1

# This is what is desired (?)
sim.add_clock(p, phase=0*p/4, domain="phase0_fail")
sim.add_clock(p, phase=1*p/4, domain="phase90_fail")
sim.add_clock(p, phase=2*p/4, domain="phase180_fail")
sim.add_clock(p, phase=3*p/4, domain="phase270_fail")

# This is what is needed (!)
sim.add_clock(p, phase=1e12 * (p/2 + 0*p/8), domain="phase0_correct")
sim.add_clock(p, phase=1e12 * (p/2 + 1*p/8), domain="phase90_correct")
sim.add_clock(p, phase=1e12 * (p/2 + 2*p/8), domain="phase180_correct")
sim.add_clock(p, phase=1e12 * (p/2 + 3*p/8), domain="phase270_correct")

def proc():
    for i in range(5):
        yield

sim.add_sync_process(proc, domain="phase0_fail")
with sim.write_vcd("bug_clock_phase.vcd"):
    sim.run_until(5)
```
modwizcode: Thanks! Can you add this as an actual testcase as part of the PR for regression purposes? 
bl0x: Where would that go? tests/test_sim.py?
modwizcode: Yes; that's a good place to put it. 
bl0x: On it.
bl0x: That should do it.
bl0x: @modwizcode Thanks for the review

Patch:
diff --git a/amaranth/sim/core.py b/amaranth/sim/core.py
index 5ebe8f5..f8a86bb 100644
--- a/amaranth/sim/core.py
+++ b/amaranth/sim/core.py
@@ -139,6 +139,8 @@ class Simulator:
             # to happen at a non-zero time, distinguishing it from the reset values in the waveform
             # viewer.
             phase = period // 2
+        else:
+            phase = int(phase * 1e12) + period // 2
         self._engine.add_clock_process(domain.clk, phase=phase, period=period)
         self._clocked.add(domain)
 


Test patch:
diff --git a/tests/test_sim.py b/tests/test_sim.py
index e8ffa5d..72c1764 100644
--- a/tests/test_sim.py
+++ b/tests/test_sim.py
@@ -516,6 +516,45 @@ class SimulatorIntegrationTestCase(FHDLTestCase):
                 self.assertEqual((yield self.o), 0)
             sim.add_sync_process(process)
 
+    def setUp_clock_phase(self):
+        self.m = Module()
+        self.phase0 = self.m.domains.phase0 = ClockDomain()
+        self.phase90 = self.m.domains.phase90 = ClockDomain()
+        self.phase180 = self.m.domains.phase180 = ClockDomain()
+        self.phase270 = self.m.domains.phase270 = ClockDomain()
+        self.check = self.m.domains.check = ClockDomain()
+
+        self.expected = [
+            [0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0],
+            [0, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1],
+            [0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1],
+            [0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0],
+        ]
+
+    def test_clock_phase(self):
+        self.setUp_clock_phase()
+        with self.assertSimulation(self.m) as sim:
+            period=1
+            sim.add_clock(period/8, phase=0,          domain="check")
+            sim.add_clock(period,   phase=0*period/4, domain="phase0")
+            sim.add_clock(period,   phase=1*period/4, domain="phase90")
+            sim.add_clock(period,   phase=2*period/4, domain="phase180")
+            sim.add_clock(period,   phase=3*period/4, domain="phase270")
+
+            def proc():
+                clocks = [
+                    self.phase0.clk,
+                    self.phase90.clk,
+                    self.phase180.clk,
+                    self.phase270.clk
+                ]
+                for i in range(16):
+                    yield
+                    for j, c in enumerate(clocks):
+                        self.assertEqual((yield c), self.expected[j][i])
+
+            sim.add_sync_process(proc, domain="check")
+
     def setUp_multiclock(self):
         self.sys = ClockDomain()
         self.pix = ClockDomain()