From 4de07a21a05fb0d44936f7879dc2b904efa97505 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Mon, 26 Feb 2024 16:09:50 +0100
Subject: [PATCH 1/8] Add an RFC for async testbench functions.

---
 text/0036-async-testbench-functions.md | 166 +++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 text/0036-async-testbench-functions.md

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
new file mode 100644
index 0000000..dbec6f8
--- /dev/null
+++ b/text/0036-async-testbench-functions.md
@@ -0,0 +1,166 @@
+- Start Date: (fill me in with today's date, YYYY-MM-DD)
+- RFC PR: [amaranth-lang/rfcs#0036](https://github.com/amaranth-lang/rfcs/pull/0036)
+- Amaranth Issue: [amaranth-lang/amaranth#0000](https://github.com/amaranth-lang/amaranth/issues/0000)
+
+# Async testbench functions
+
+## Summary
+[summary]: #summary
+
+Introduce an improved simulator testbench interface using `async`/`await` style coroutines.
+
+## Motivation
+[motivation]: #motivation
+
+For the purpose of writing a testbench, an `async` function will read more naturally than a generator function, especially when calling subfunctions/methods.
+
+A more expressive way to specify trigger/wait conditions allows the condition checking to be offloaded to the simulator engine, only returning control to the testbench process when it has work to do.
+
+Passing a simulator context to the testbench function provides a convenient place to gather all simulator operations.
+
+Having `.get()` and `.set()` methods provides a convenient way for value castables to implement these in a type-specific manner.
+
+## Guide-level explanation
+[guide-level-explanation]: #guide-level-explanation
+
+As an example, let's consider a simple stream interface with `valid`, `ready` and `data` members.
+On the interface class, we can then implement `.send()` and `.recv()` methods like this:
+
+```python
+class StreamInterface(PureInterface):
+    async def recv(self, sim):
+        await self.ready.set(1)
+        await sim.tick().until(self.valid)
+
+        value = await self.data.get()
+
+        await sim.tick()
+        await self.ready.set(0)
+
+        return value
+
+    async def send(self, sim, value):
+        await self.data.set(value)
+
+        await self.valid.set(1)
+        await sim.tick().until(self.ready)
+
+        await sim.tick()
+        await self.valid.set(0)
+```
+
+`sim.tick()` replaces the existing `Tick()`. It returns a trigger object that either can be awaited directly, or made conditional through `.until()`.
+
+Using this stream interface, let's consider a colorspace converter accepting a stream of RGB values and outputting a stream of YUV values:
+
+```python
+class RGBToYUVConverter(Component):
+    input: In(StreamSignature(RGB888))
+    output: Out(StreamSignature(YUV888))
+```
+
+A testbench could then look like this:
+
+```python
+async def test_rgb(sim, r, g, b):
+    rgb = {'r': r, 'g': g, 'b': b}
+    await dut.input.send(sim, rgb)
+    yuv = await dut.output.recv(sim)
+
+    print(rgb, yuv)
+
+async def testbench(sim):
+    await test_rgb(sim, 0, 0, 0)
+    await test_rgb(sim, 255, 0, 0)
+    await test_rgb(sim, 0, 255, 0)
+    await test_rgb(sim, 0, 0, 255)
+    await test_rgb(sim, 255, 255, 255)
+```
+
+Since `.send()` and `.recv()` invokes `.get()` and `.set()` that a value castable (here `data.View`) can implement in a suitable manner, it is general enough to work for streams with arbitrary shapes.
+
+`Tick()` and `Delay()` are replaced by `sim.tick()` and `sim.delay()` respectively.
+In addition, `sim.changed()` is introduced that allows creating triggers from arbitrary signals.
+These all return a trigger object that can be made conditional through `.until()`.
+
+`Active()` and `Passive()` are replaced by an `passive=False` keyword argument to `.add_process()` and `.add_testbench()`.
+To mark a passive testbench temporarily active, `sim.active()` is introduced, which is used as a context manager:
+
+```python
+async def packet_reader(sim, stream):
+    while True
+        # Wait until stream has valid data.
+        await sim.tick().until(stream.valid)
+
+        # Go active to ensure simulation doesn't end in the middle of a packet.
+        async with sim.active():
+            packet = await stream.read_packet()
+            print('Received packet:', packet.hex(' '))
+```
+
+## Reference-level explanation
+[reference-level-explanation]: #reference-level-explanation
+
+The following `Simulator` methods have their signatures updated:
+
+* `add_process(process, *, passive=False)`
+* `add_testbench(process, *, passive=False)`
+
+The new optional named argument `passive` registers the testbench as passive when true.
+
+Both methods are updated to accept an async function passed as `process`. When the function passed to `process` accepts an argument named `sim`, it will be passed a simulator context.
+
+The simulator context have the following methods:
+- `delay(interval=None)`
+  - Return a trigger object for advancing simulation by `interval` seconds.
+- `tick(domain="sync")`
+  - Return a trigger object for advancing simulation by one tick of `domain`.
+- `changed(signal, value=None)`
+  - Return a trigger object for advancing simulation until `signal` is changed to `value`. `None` is a wildcard and will trigger on any change.
+- `active()`
+  - Return a context manager that temporarily marks the testbench as active for the duration.
+- `time()`
+  - Return the current simulation time.
+
+A trigger object has the following methods:
+- `until(condition)`
+  - Repeat the trigger until `condition` is true. If `condition` is initially true, `await` will return immediately without advancing simulation.
+
+`Value`, `data.View` and `enum.EnumView` have `.get()` and `.set()` methods added.
+
+`Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
+
+## Drawbacks
+[drawbacks]: #drawbacks
+
+Reserves two new names on `Value` and value castables. Increase in API surface area and complexity. Churn.
+
+## Rationale and alternatives
+[rationale-and-alternatives]: #rationale-and-alternatives
+
+- Do nothing. Keep the existing interface, add `Changed()` alongside `Delay()` and `Tick()`, use `yield from` when calling functions.
+
+- Don't introduce `.get()` and `.set()`. Instead require a value castable and the return value of its `.eq()` to be awaitable so `await value` and `await value.eq(foo)` is possible.
+
+
+## Prior art
+[prior-art]: #prior-art
+
+Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutines.html) that originally used generator based coroutines have also moved to `async`/`await` style coroutines.
+
+## Unresolved questions
+[unresolved-questions]: #unresolved-questions
+
+- It should be possible to combine triggers, e.g. when we have a set of signals and are waiting for either of them to change.
+  Simulating combinational logic with `add_process` would be one use case for this.
+  Simulating sync logic with async reset could be another.
+  What would be a good syntax to combine triggers?
+- Is there any other functionality that's natural to have on the simulator context?
+- Is there any other functionality that's natural to have on the trigger object?
+  - Maybe a way to skip a given number of triggers? We still lack a way to say «advance by n cycles».
+- Bikeshed all the names.
+
+## Future possibilities
+[future-possibilities]: #future-possibilities
+
+Add simulation helpers in the manner of `.send()` and `.recv()` to standard interfaces where it makes sense.

From 54e6a605a5b9dc9e6f92ef2157d3069f1a8e5bc4 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Sun, 3 Mar 2024 23:04:38 +0100
Subject: [PATCH 2/8] RFC #36: Updated to address feedback.

---
 text/0036-async-testbench-functions.md | 47 +++++++++++++++++---------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index dbec6f8..b1b9a16 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -18,7 +18,7 @@ A more expressive way to specify trigger/wait conditions allows the condition ch
 
 Passing a simulator context to the testbench function provides a convenient place to gather all simulator operations.
 
-Having `.get()` and `.set()` methods provides a convenient way for value castables to implement these in a type-specific manner.
+~~Having `.get()` and `.set()` methods provides a convenient way for value castables to implement these in a type-specific manner.~~
 
 ## Guide-level explanation
 [guide-level-explanation]: #guide-level-explanation
@@ -29,26 +29,28 @@ On the interface class, we can then implement `.send()` and `.recv()` methods li
 ```python
 class StreamInterface(PureInterface):
     async def recv(self, sim):
-        await self.ready.set(1)
+        await sim.set(self.ready, 1)
         await sim.tick().until(self.valid)
 
-        value = await self.data.get()
+        value = await sim.get(self.data)
 
         await sim.tick()
-        await self.ready.set(0)
+        await sim.set(self.ready, 0)
 
         return value
 
     async def send(self, sim, value):
-        await self.data.set(value)
+        await sim.set(self.data, value)
 
-        await self.valid.set(1)
+        await sim.set(self.valid, 1)
         await sim.tick().until(self.ready)
 
         await sim.tick()
-        await self.valid.set(0)
+        await sim.set(self.valid, 0)
 ```
 
+`await sim.get()` and `await sim.set()` replaces the existing operations `yield signal` and `yield signal.eq()` respectively.
+
 `sim.tick()` replaces the existing `Tick()`. It returns a trigger object that either can be awaited directly, or made conditional through `.until()`.
 
 Using this stream interface, let's consider a colorspace converter accepting a stream of RGB values and outputting a stream of YUV values:
@@ -77,7 +79,7 @@ async def testbench(sim):
     await test_rgb(sim, 255, 255, 255)
 ```
 
-Since `.send()` and `.recv()` invokes `.get()` and `.set()` that a value castable (here `data.View`) can implement in a suitable manner, it is general enough to work for streams with arbitrary shapes.
+Since `.send()` and `.recv()` invokes `sim.get()` and `sim.set()` that in turn will invoke the appropriate value conversions for a value castable (here `data.View`), it is general enough to work for streams with arbitrary shapes.
 
 `Tick()` and `Delay()` are replaced by `sim.tick()` and `sim.delay()` respectively.
 In addition, `sim.changed()` is introduced that allows creating triggers from arbitrary signals.
@@ -108,13 +110,21 @@ The following `Simulator` methods have their signatures updated:
 
 The new optional named argument `passive` registers the testbench as passive when true.
 
-Both methods are updated to accept an async function passed as `process`. When the function passed to `process` accepts an argument named `sim`, it will be passed a simulator context.
+Both methods are updated to accept an async function passed as `process`.
+The async function must accept a named argument `sim`, which will be passed a simulator context.
 
 The simulator context have the following methods:
-- `delay(interval=None)`
+- `get(signal)`
+  - Returns the value of `signal` when awaited.
+    When `signal` is a value-castable, the value will be converted through `.from_bits()`. (Pending RFC #51)
+- `set(signal, value)`
+  - Set `signal` to `value` when awaited.
+    When `signal` is a value-castable, the value will be converted through `.const()`.
+- `delay(interval)`
   - Return a trigger object for advancing simulation by `interval` seconds.
-- `tick(domain="sync")`
+- `tick(domain="sync", *, context=None)`
   - Return a trigger object for advancing simulation by one tick of `domain`.
+    When an elaboratable is passed to `context`, `domain` will be resolved from its perspective.
 - `changed(signal, value=None)`
   - Return a trigger object for advancing simulation until `signal` is changed to `value`. `None` is a wildcard and will trigger on any change.
 - `active()`
@@ -124,24 +134,27 @@ The simulator context have the following methods:
 
 A trigger object has the following methods:
 - `until(condition)`
-  - Repeat the trigger until `condition` is true. If `condition` is initially true, `await` will return immediately without advancing simulation.
+  - Repeat the trigger until `condition` is true.
+    `condition` is an arbitrary Amaranth expression.
+    If `condition` is initially true, `await` will return immediately without advancing simulation.
 
-`Value`, `data.View` and `enum.EnumView` have `.get()` and `.set()` methods added.
+~~`Value`, `data.View` and `enum.EnumView` have `.get()` and `.set()` methods added.~~
 
 `Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
 
 ## Drawbacks
 [drawbacks]: #drawbacks
 
-Reserves two new names on `Value` and value castables. Increase in API surface area and complexity. Churn.
+-  ~~Reserves two new names on `Value` and value castables~~
+- Increase in API surface area and complexity.
+- Churn.
 
 ## Rationale and alternatives
 [rationale-and-alternatives]: #rationale-and-alternatives
 
 - Do nothing. Keep the existing interface, add `Changed()` alongside `Delay()` and `Tick()`, use `yield from` when calling functions.
 
-- Don't introduce `.get()` and `.set()`. Instead require a value castable and the return value of its `.eq()` to be awaitable so `await value` and `await value.eq(foo)` is possible.
-
+- ~~Don't introduce `.get()` and `.set()`. Instead require a value castable and the return value of its `.eq()` to be awaitable so `await value` and `await value.eq(foo)` is possible.~~
 
 ## Prior art
 [prior-art]: #prior-art
@@ -156,9 +169,11 @@ Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutine
   Simulating sync logic with async reset could be another.
   What would be a good syntax to combine triggers?
 - Is there any other functionality that's natural to have on the simulator context?
+  - (@wanda-phi) `sim.memory_read(memory, address)`, `sim.memory_write(memory, address, value[, mask])`?
 - Is there any other functionality that's natural to have on the trigger object?
   - Maybe a way to skip a given number of triggers? We still lack a way to say «advance by n cycles».
 - Bikeshed all the names.
+  - (@whitequark) We should consider different naming for `active`/`passive`.
 
 ## Future possibilities
 [future-possibilities]: #future-possibilities

From ea266842f3fe1077409b9c5e7c16cf2e1253fd44 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Fri, 8 Mar 2024 23:34:22 +0100
Subject: [PATCH 3/8] RFC #36: Updated to address more feedback.

---
 text/0036-async-testbench-functions.md | 134 +++++++++++++++++--------
 1 file changed, 94 insertions(+), 40 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index b1b9a16..7856b7b 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -18,8 +18,6 @@ A more expressive way to specify trigger/wait conditions allows the condition ch
 
 Passing a simulator context to the testbench function provides a convenient place to gather all simulator operations.
 
-~~Having `.get()` and `.set()` methods provides a convenient way for value castables to implement these in a type-specific manner.~~
-
 ## Guide-level explanation
 [guide-level-explanation]: #guide-level-explanation
 
@@ -53,6 +51,10 @@ class StreamInterface(PureInterface):
 
 `sim.tick()` replaces the existing `Tick()`. It returns a trigger object that either can be awaited directly, or made conditional through `.until()`.
 
+> **Note**
+> This simplified example does not include any way of specifying the clock domain of the interface and as such is only directly applicable to single domain simulations.
+> A way to attach clock domain information to interfaces is desireable, but out of scope for this RFC.
+
 Using this stream interface, let's consider a colorspace converter accepting a stream of RGB values and outputting a stream of YUV values:
 
 ```python
@@ -82,11 +84,12 @@ async def testbench(sim):
 Since `.send()` and `.recv()` invokes `sim.get()` and `sim.set()` that in turn will invoke the appropriate value conversions for a value castable (here `data.View`), it is general enough to work for streams with arbitrary shapes.
 
 `Tick()` and `Delay()` are replaced by `sim.tick()` and `sim.delay()` respectively.
-In addition, `sim.changed()` is introduced that allows creating triggers from arbitrary signals.
+In addition, `sim.changed()` and `sim.edge()` is introduced that allows creating triggers from arbitrary signals.
 These all return a trigger object that can be made conditional through `.until()`.
 
-`Active()` and `Passive()` are replaced by an `passive=False` keyword argument to `.add_process()` and `.add_testbench()`.
-To mark a passive testbench temporarily active, `sim.active()` is introduced, which is used as a context manager:
+`Active()` and `Passive()` are replaced by an `background=False` keyword argument to `.add_testbench()`.
+Processes created through `.add_process()` are always created as background processes.
+To allow a background process to ensure an operation is finished before end of simulation, `sim.critical()` is introduced, which is used as a context manager:
 
 ```python
 async def packet_reader(sim, stream):
@@ -94,58 +97,118 @@ async def packet_reader(sim, stream):
         # Wait until stream has valid data.
         await sim.tick().until(stream.valid)
 
-        # Go active to ensure simulation doesn't end in the middle of a packet.
-        async with sim.active():
+        # Ensure simulation doesn't end in the middle of a packet.
+        async with sim.critical():
             packet = await stream.read_packet()
             print('Received packet:', packet.hex(' '))
 ```
 
+When a trigger object is awaited, it'll return the value(s) of the trigger(s), and it can also be used as an async generator to repeatedly await the same trigger.
+Multiple triggers can be combined.
+Consider the following examples:
+
+Combinational adder as a process:
+```python
+a = Signal(); b = Signal(); o = Signal()
+async def adder(sim):
+    async for a_val, b_val in sim.changed(a, b):
+        await sim.set(o, a_val + b_val)
+sim.add_process(adder)
+```
+
+DDR IO buffer as a process:
+```python
+o = Signal(2); pin = Signal()
+async def ddr_buffer(sim):
+    while True: # could be extended to pre-capture next `o` on posedge
+        await sim.negedge()
+        await sim.set(pin, o[0])
+        await sim.posedge()
+        await sim.set(pin, o[1])
+sim.add_process(ddr_buffer)
+```
+
+Flop with configurable edge reset and posedge clock as a process:
+```python
+clk = Signal(); rst = Signal(); d = Signal(); q = Signal()
+def dff(rst_edge):
+    async def process(sim):
+        async for clk_val, rst_val in sim.posedge(clk).edge(rst, rst_edge):
+            await sim.set(q, 0 if rst_val == rst_edge else await sim.get(d))
+    return process
+sim.add_process(dff(rst_edge=0))
+```
+
 ## Reference-level explanation
 [reference-level-explanation]: #reference-level-explanation
 
 The following `Simulator` methods have their signatures updated:
 
-* `add_process(process, *, passive=False)`
-* `add_testbench(process, *, passive=False)`
+* `add_process(process)`
+* `add_testbench(process, *, background=False)`
 
-The new optional named argument `passive` registers the testbench as passive when true.
+The new optional named argument `background` registers the testbench as a background process when true.
 
 Both methods are updated to accept an async function passed as `process`.
-The async function must accept a named argument `sim`, which will be passed a simulator context.
+The async function must accept an argument `sim`, which will be passed a simulator context.
+(Argument name is just convention, will be passed positionally.)
 
 The simulator context have the following methods:
-- `get(signal)`
-  - Returns the value of `signal` when awaited.
-    When `signal` is a value-castable, the value will be converted through `.from_bits()`. (Pending RFC #51)
-- `set(signal, value)`
-  - Set `signal` to `value` when awaited.
-    When `signal` is a value-castable, the value will be converted through `.const()`.
+- `get(expr: Value) -> int`
+- `get(expr: ValueCastable) -> any`
+  - Returns the value of `expr` when awaited.
+    When `expr` is a value-castable, the value will be converted through `.from_bits()`.
+- `set(expr: Value, value: ConstLike)`
+- `set(expr: ValueCastable, value: any)`
+  - Set `expr` to `value` when awaited.
+    When `expr` is a value-castable, the value will be converted through `.const()`.
+- `memory_read(instance: MemoryInstance, address)`
+  - Read the value from `address` in `instance` when awaited.
+- `memory_write(instance: MemoryInstance, address, value, mask=None)`
+  - Write `value` to `address` in `instance` when awaited. If `mask` is given, only the corresponding bits are written.
 - `delay(interval)`
-  - Return a trigger object for advancing simulation by `interval` seconds.
+  - Create a trigger object for advancing simulation by `interval` seconds.
 - `tick(domain="sync", *, context=None)`
-  - Return a trigger object for advancing simulation by one tick of `domain`.
+  - Create a trigger object for advancing simulation by one tick of `domain`.
     When an elaboratable is passed to `context`, `domain` will be resolved from its perspective.
-- `changed(signal, value=None)`
-  - Return a trigger object for advancing simulation until `signal` is changed to `value`. `None` is a wildcard and will trigger on any change.
-- `active()`
-  - Return a context manager that temporarily marks the testbench as active for the duration.
-- `time()`
-  - Return the current simulation time.
+  - If `domain` is asynchronously reset while this is being awaited, `AsyncReset` is raised.
+- `changed(*signals)`
+  - Create a trigger object for advancing simulation until any signal in `signals` changes.
+- `edge(signal, value)`
+  - Create a trigger object for advancing simulation until `signal` is changed to `value`.
+    `signal` must be a 1-bit signal or a 1-bit slice of a signal.
+- `posedge(signal)`
+- `negedge(signal)`
+  - Aliases for `edge(signal, 1)` and `edge(signal, 0)` respectively.
+  
+  `signal` is changed to `value`. `None` is a wildcard and will trigger on any change.
+- `critical()`
+  - Return a context manager that ensures simulation won't terminate in the middle of the enclosed scope.
 
 A trigger object has the following methods:
+- `__await__()`
+  - Advance simulation and return the value(s) of the trigger(s).
+    - `delay` and `tick` triggers return `True` when they are hit, otherwise `False`.
+    - `changed` and `edge` triggers return the current value of the signals they are monitoring.
+- `__aiter__()`
+  - Return an async generator that repeatedly invokes `__await__()` and yields the returned values.
+- `delay(interval)`
+- `tick(domain="sync", *, context=None)`
+- `changed(*signals)`
+- `edge(signal, value)`
+- `posedge(signal)`
+- `negedge(signal)`
+  - Create a new trigger object by copying the current object and appending another trigger.
 - `until(condition)`
   - Repeat the trigger until `condition` is true.
     `condition` is an arbitrary Amaranth expression.
     If `condition` is initially true, `await` will return immediately without advancing simulation.
 
-~~`Value`, `data.View` and `enum.EnumView` have `.get()` and `.set()` methods added.~~
-
 `Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
 
 ## Drawbacks
 [drawbacks]: #drawbacks
 
--  ~~Reserves two new names on `Value` and value castables~~
 - Increase in API surface area and complexity.
 - Churn.
 
@@ -154,8 +217,6 @@ A trigger object has the following methods:
 
 - Do nothing. Keep the existing interface, add `Changed()` alongside `Delay()` and `Tick()`, use `yield from` when calling functions.
 
-- ~~Don't introduce `.get()` and `.set()`. Instead require a value castable and the return value of its `.eq()` to be awaitable so `await value` and `await value.eq(foo)` is possible.~~
-
 ## Prior art
 [prior-art]: #prior-art
 
@@ -164,18 +225,11 @@ Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutine
 ## Unresolved questions
 [unresolved-questions]: #unresolved-questions
 
-- It should be possible to combine triggers, e.g. when we have a set of signals and are waiting for either of them to change.
-  Simulating combinational logic with `add_process` would be one use case for this.
-  Simulating sync logic with async reset could be another.
-  What would be a good syntax to combine triggers?
-- Is there any other functionality that's natural to have on the simulator context?
-  - (@wanda-phi) `sim.memory_read(memory, address)`, `sim.memory_write(memory, address, value[, mask])`?
-- Is there any other functionality that's natural to have on the trigger object?
-  - Maybe a way to skip a given number of triggers? We still lack a way to say «advance by n cycles».
 - Bikeshed all the names.
-  - (@whitequark) We should consider different naming for `active`/`passive`.
 
 ## Future possibilities
 [future-possibilities]: #future-possibilities
 
-Add simulation helpers in the manner of `.send()` and `.recv()` to standard interfaces where it makes sense.
+- Add simulation helpers in the manner of `.send()` and `.recv()` to standard interfaces where it makes sense.
+- There is a desire for a `sim.time()` method that returns the current simulation time, but it needs a suitable return type to represent seconds with femtosecond resolution and that is out of the scope for this RFC.
+- We ought to have a way to skip a given number of triggers, so that we can tell the simulation engine to e.g. «advance by n cycles».

From ae7edf3d6db815c9dd9b23262e9abac14be910b8 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Mon, 11 Mar 2024 01:46:08 +0100
Subject: [PATCH 4/8] RFC #36: Clarifications and minor changes.

---
 text/0036-async-testbench-functions.md | 31 +++++++++++++-------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index 7856b7b..6adac34 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -93,7 +93,7 @@ To allow a background process to ensure an operation is finished before end of s
 
 ```python
 async def packet_reader(sim, stream):
-    while True
+    while True:
         # Wait until stream has valid data.
         await sim.tick().until(stream.valid)
 
@@ -133,8 +133,8 @@ Flop with configurable edge reset and posedge clock as a process:
 clk = Signal(); rst = Signal(); d = Signal(); q = Signal()
 def dff(rst_edge):
     async def process(sim):
-        async for clk_val, rst_val in sim.posedge(clk).edge(rst, rst_edge):
-            await sim.set(q, 0 if rst_val == rst_edge else await sim.get(d))
+        async for clk_hit, rst_hit in sim.posedge(clk).edge(rst, rst_edge):
+            await sim.set(q, 0 if rst_hit else await sim.get(d))
     return process
 sim.add_process(dff(rst_edge=0))
 ```
@@ -162,16 +162,16 @@ The simulator context have the following methods:
 - `set(expr: ValueCastable, value: any)`
   - Set `expr` to `value` when awaited.
     When `expr` is a value-castable, the value will be converted through `.const()`.
-- `memory_read(instance: MemoryInstance, address)`
+- `memory_read(instance: MemoryInstance, address: int)`
   - Read the value from `address` in `instance` when awaited.
-- `memory_write(instance: MemoryInstance, address, value, mask=None)`
+- `memory_write(instance: MemoryInstance, address: int, value: int, mask:int = None)`
   - Write `value` to `address` in `instance` when awaited. If `mask` is given, only the corresponding bits are written.
-- `delay(interval)`
+- `delay(interval: float)`
   - Create a trigger object for advancing simulation by `interval` seconds.
 - `tick(domain="sync", *, context=None)`
-  - Create a trigger object for advancing simulation by one tick of `domain`.
+  - Create a trigger object for advancing simulation until the next active edge of the `domain` clock.
     When an elaboratable is passed to `context`, `domain` will be resolved from its perspective.
-  - If `domain` is asynchronously reset while this is being awaited, `AsyncReset` is raised.
+  - If `domain` is asynchronously reset while this is being awaited, `amaranth.sim.AsyncReset` is raised.
 - `changed(*signals)`
   - Create a trigger object for advancing simulation until any signal in `signals` changes.
 - `edge(signal, value)`
@@ -180,19 +180,18 @@ The simulator context have the following methods:
 - `posedge(signal)`
 - `negedge(signal)`
   - Aliases for `edge(signal, 1)` and `edge(signal, 0)` respectively.
-  
-  `signal` is changed to `value`. `None` is a wildcard and will trigger on any change.
 - `critical()`
-  - Return a context manager that ensures simulation won't terminate in the middle of the enclosed scope.
+  - Context manager.
+    If the current process is a background process, `async with sim.critical():` makes it a non-background process for the duration of the statement.
 
 A trigger object has the following methods:
 - `__await__()`
   - Advance simulation and return the value(s) of the trigger(s).
-    - `delay` and `tick` triggers return `True` when they are hit, otherwise `False`.
-    - `changed` and `edge` triggers return the current value of the signals they are monitoring.
+    - `delay`, `tick` and `edge` triggers return `True` when they are hit, otherwise `False`.
+    - `changed` triggers return the current value of the signals they are monitoring.
 - `__aiter__()`
-  - Return an async generator that repeatedly invokes `__await__()` and yields the returned values.
-- `delay(interval)`
+  - Return an async generator that is equivalent to repeatedly awaiting the trigger object in an infinite loop.
+- `delay(interval: float)`
 - `tick(domain="sync", *, context=None)`
 - `changed(*signals)`
 - `edge(signal, value)`
@@ -203,6 +202,7 @@ A trigger object has the following methods:
   - Repeat the trigger until `condition` is true.
     `condition` is an arbitrary Amaranth expression.
     If `condition` is initially true, `await` will return immediately without advancing simulation.
+    The return value is an unspecified awaitable with `await` as the only defined operation.
 
 `Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
 
@@ -226,6 +226,7 @@ Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutine
 [unresolved-questions]: #unresolved-questions
 
 - Bikeshed all the names.
+  - (@whitequark) Should we go for `posedge` (Verilog convention) or `pos_edge` (Python convention)?
 
 ## Future possibilities
 [future-possibilities]: #future-possibilities

From f9fe446372d44434b20ad65f9832f0b30a1d299a Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Mon, 11 Mar 2024 17:17:01 +0100
Subject: [PATCH 5/8] RFC #36: More clarifications. Split trigger objects into
 domain trigger objects and combinable trigger objects.

---
 text/0036-async-testbench-functions.md | 110 +++++++++++++++----------
 1 file changed, 68 insertions(+), 42 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index 6adac34..21bb7aa 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -22,29 +22,28 @@ Passing a simulator context to the testbench function provides a convenient plac
 [guide-level-explanation]: #guide-level-explanation
 
 As an example, let's consider a simple stream interface with `valid`, `ready` and `data` members.
-On the interface class, we can then implement `.send()` and `.recv()` methods like this:
+We can then implement `stream_send()` and `stream_recv()` functions like this:
 
 ```python
-class StreamInterface(PureInterface):
-    async def recv(self, sim):
-        await sim.set(self.ready, 1)
-        await sim.tick().until(self.valid)
+async def stream_recv(sim, stream):
+    await sim.set(stream.ready, 1)
+    await sim.tick().until(stream.valid)
 
-        value = await sim.get(self.data)
+    value = await sim.get(stream.data)
 
-        await sim.tick()
-        await sim.set(self.ready, 0)
+    await sim.tick()
+    await sim.set(stream.ready, 0)
 
-        return value
+    return value
 
-    async def send(self, sim, value):
-        await sim.set(self.data, value)
+async def stream_send(sim, stream, value):
+    await sim.set(stream.data, value)
 
-        await sim.set(self.valid, 1)
-        await sim.tick().until(self.ready)
+    await sim.set(stream.valid, 1)
+    await sim.tick().until(stream.ready)
 
-        await sim.tick()
-        await sim.set(self.valid, 0)
+    await sim.tick()
+    await sim.set(stream.valid, 0)
 ```
 
 `await sim.get()` and `await sim.set()` replaces the existing operations `yield signal` and `yield signal.eq()` respectively.
@@ -68,8 +67,8 @@ A testbench could then look like this:
 ```python
 async def test_rgb(sim, r, g, b):
     rgb = {'r': r, 'g': g, 'b': b}
-    await dut.input.send(sim, rgb)
-    yuv = await dut.output.recv(sim)
+    await stream_send(sim, dut.input, rgb)
+    yuv = await stream_recv(sim, dut.output)
 
     print(rgb, yuv)
 
@@ -81,11 +80,14 @@ async def testbench(sim):
     await test_rgb(sim, 255, 255, 255)
 ```
 
-Since `.send()` and `.recv()` invokes `sim.get()` and `sim.set()` that in turn will invoke the appropriate value conversions for a value castable (here `data.View`), it is general enough to work for streams with arbitrary shapes.
+Since `stream_send()` and `stream_recv()` invokes `sim.get()` and `sim.set()` that in turn will invoke the appropriate value conversions for a value castable (here `data.View`), it is general enough to work for streams with arbitrary shapes.
 
 `Tick()` and `Delay()` are replaced by `sim.tick()` and `sim.delay()` respectively.
 In addition, `sim.changed()` and `sim.edge()` is introduced that allows creating triggers from arbitrary signals.
-These all return a trigger object that can be made conditional through `.until()`.
+
+`sim.tick()` return a domain trigger object that can be made conditional through `.until()` or repeated through `.repeat()`.
+
+`sim.delay()`, `sim.changed()` and `sim.edge()` return a combinable trigger object that can be used to add additional triggers.
 
 `Active()` and `Passive()` are replaced by an `background=False` keyword argument to `.add_testbench()`.
 Processes created through `.add_process()` are always created as background processes.
@@ -103,7 +105,7 @@ async def packet_reader(sim, stream):
             print('Received packet:', packet.hex(' '))
 ```
 
-When a trigger object is awaited, it'll return the value(s) of the trigger(s), and it can also be used as an async generator to repeatedly await the same trigger.
+When a combinable trigger object is awaited, it'll return the value(s) of the trigger(s), and it can also be used as an async generator to repeatedly await the same trigger.
 Multiple triggers can be combined.
 Consider the following examples:
 
@@ -147,36 +149,45 @@ The following `Simulator` methods have their signatures updated:
 * `add_process(process)`
 * `add_testbench(process, *, background=False)`
 
-The new optional named argument `background` registers the testbench as a background process when true.
-
 Both methods are updated to accept an async function passed as `process`.
 The async function must accept an argument `sim`, which will be passed a simulator context.
 (Argument name is just convention, will be passed positionally.)
 
-The simulator context have the following methods:
+The new optional named argument `background` registers the testbench as a background process when true.
+Processes created through `add_process` are always registered as background processes (except when registering legacy non-async generator functions).
+
+The simulator context has the following properties and methods:
+- `process_type`
+  - Property, `ProcessType`, indicates the type of the current process.
+    This property allows a generic simulation helper function to assert that it's being run under the appropriate process type.
 - `get(expr: Value) -> int`
 - `get(expr: ValueCastable) -> any`
   - Returns the value of `expr` when awaited.
-    When `expr` is a value-castable, the value will be converted through `.from_bits()`.
+    When `expr` is a value-castable, and its `shape()` is a `ShapeCastable`, the value will be converted through the shape's `.from_bits()`.
+    Otherwise, a plain integer is returned.
 - `set(expr: Value, value: ConstLike)`
 - `set(expr: ValueCastable, value: any)`
   - Set `expr` to `value` when awaited.
-    When `expr` is a value-castable, the value will be converted through `.const()`.
-- `memory_read(instance: MemoryInstance, address: int)`
+    When `expr` is a value-castable, and its `shape()` is a `ShapeCastable`, the value will be converted through the shape's `.const()`.
+    Otherwise, it must be a const-castable `ValueLike`.
+- `memory_read(instance: MemoryIdentity, address: int)`
   - Read the value from `address` in `instance` when awaited.
-- `memory_write(instance: MemoryInstance, address: int, value: int, mask:int = None)`
+- `memory_write(instance: MemoryIdentity, address: int, value: int, mask:int = None)`
   - Write `value` to `address` in `instance` when awaited. If `mask` is given, only the corresponding bits are written.
-- `delay(interval: float)`
-  - Create a trigger object for advancing simulation by `interval` seconds.
+    Like `MemoryInstance`, these two functions are an internal interface that will be usually only used via `lib.Memory`.
+    It comes without a stability guarantee.
 - `tick(domain="sync", *, context=None)`
-  - Create a trigger object for advancing simulation until the next active edge of the `domain` clock.
+  - Create a domain trigger object for advancing simulation until the next active edge of the `domain` clock.
     When an elaboratable is passed to `context`, `domain` will be resolved from its perspective.
   - If `domain` is asynchronously reset while this is being awaited, `amaranth.sim.AsyncReset` is raised.
+- `delay(interval: float)`
+  - Create a combinable trigger object for advancing simulation by `interval` seconds.
 - `changed(*signals)`
-  - Create a trigger object for advancing simulation until any signal in `signals` changes.
+  - Create a combinable trigger object for advancing simulation until any signal in `signals` changes.
 - `edge(signal, value)`
-  - Create a trigger object for advancing simulation until `signal` is changed to `value`.
+  - Create a combinable trigger object for advancing simulation until `signal` is changed to `value`.
     `signal` must be a 1-bit signal or a 1-bit slice of a signal.
+    `value` is a boolean where true indicates rising edge and false indicates falling edge.
 - `posedge(signal)`
 - `negedge(signal)`
   - Aliases for `edge(signal, 1)` and `edge(signal, 0)` respectively.
@@ -184,25 +195,40 @@ The simulator context have the following methods:
   - Context manager.
     If the current process is a background process, `async with sim.critical():` makes it a non-background process for the duration of the statement.
 
-A trigger object has the following methods:
+The `ProcessType` enum has the following members:
+  - `BikeshedProcess`
+    - The current process is a process added by `add_process`.
+  - `BikeshedTestbench`
+    - The current process is a testbench process added by `add_testbench`.
+
+A domain trigger object has the following methods:
+- `__await__()`
+  - Advance simulation. No value is returned.
+- `until(condition)`
+  - Repeat the trigger until `condition` is true.
+    `condition` is an arbitrary Amaranth expression.
+    If `condition` is initially true, `await` will return immediately without advancing simulation.
+    The return value is an unspecified awaitable with `await` as the only defined operation.
+- `repeat(times: int)`
+  - Repeat the trigger `times` times.
+    The return value is an unspecified awaitable with `await` as the only defined operation.
+
+A combinable trigger object has the following methods:
 - `__await__()`
   - Advance simulation and return the value(s) of the trigger(s).
-    - `delay`, `tick` and `edge` triggers return `True` when they are hit, otherwise `False`.
+    - `delay` and `edge` triggers return `True` when they are hit, otherwise `False`.
     - `changed` triggers return the current value of the signals they are monitoring.
+    - At least one of the triggers hit will be reflected in the return value.
+      In case of multiple triggers occuring at the same time step, it is unspecified which of these will show up in the return value beyond “at least one”.
 - `__aiter__()`
   - Return an async generator that is equivalent to repeatedly awaiting the trigger object in an infinite loop.
 - `delay(interval: float)`
-- `tick(domain="sync", *, context=None)`
 - `changed(*signals)`
 - `edge(signal, value)`
 - `posedge(signal)`
 - `negedge(signal)`
   - Create a new trigger object by copying the current object and appending another trigger.
-- `until(condition)`
-  - Repeat the trigger until `condition` is true.
-    `condition` is an arbitrary Amaranth expression.
-    If `condition` is initially true, `await` will return immediately without advancing simulation.
-    The return value is an unspecified awaitable with `await` as the only defined operation.
+  - Awaiting the returned trigger object pauses the process until the first of the combined triggers hit, i.e. the triggers are combined using OR semantics.
 
 `Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
 
@@ -231,6 +257,6 @@ Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutine
 ## Future possibilities
 [future-possibilities]: #future-possibilities
 
-- Add simulation helpers in the manner of `.send()` and `.recv()` to standard interfaces where it makes sense.
+- Add simulation helper methods to standard interfaces where it makes sense.
+  - This includes `lib.memory.Memory`.
 - There is a desire for a `sim.time()` method that returns the current simulation time, but it needs a suitable return type to represent seconds with femtosecond resolution and that is out of the scope for this RFC.
-- We ought to have a way to skip a given number of triggers, so that we can tell the simulation engine to e.g. «advance by n cycles».

From 5468750d51e1e45ca61686ebfd8cf123c1af4a78 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Fri, 15 Mar 2024 21:29:47 +0100
Subject: [PATCH 6/8] RFC #36: More clarifications. Add `testbench_helper`
 decorator.

---
 text/0036-async-testbench-functions.md | 44 +++++++++++++++++---------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index 21bb7aa..f66e30c 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -25,6 +25,7 @@ As an example, let's consider a simple stream interface with `valid`, `ready` an
 We can then implement `stream_send()` and `stream_recv()` functions like this:
 
 ```python
+@testbench_helper
 async def stream_recv(sim, stream):
     await sim.set(stream.ready, 1)
     await sim.tick().until(stream.valid)
@@ -36,6 +37,7 @@ async def stream_recv(sim, stream):
 
     return value
 
+@testbench_helper
 async def stream_send(sim, stream, value):
     await sim.set(stream.data, value)
 
@@ -50,6 +52,8 @@ async def stream_send(sim, stream, value):
 
 `sim.tick()` replaces the existing `Tick()`. It returns a trigger object that either can be awaited directly, or made conditional through `.until()`.
 
+The `testbench_helper` decorator indicates that this function is only designed to be called from testbench processes and will raise an exception if called elsewhere.
+
 > **Note**
 > This simplified example does not include any way of specifying the clock domain of the interface and as such is only directly applicable to single domain simulations.
 > A way to attach clock domain information to interfaces is desireable, but out of scope for this RFC.
@@ -156,10 +160,7 @@ The async function must accept an argument `sim`, which will be passed a simulat
 The new optional named argument `background` registers the testbench as a background process when true.
 Processes created through `add_process` are always registered as background processes (except when registering legacy non-async generator functions).
 
-The simulator context has the following properties and methods:
-- `process_type`
-  - Property, `ProcessType`, indicates the type of the current process.
-    This property allows a generic simulation helper function to assert that it's being run under the appropriate process type.
+The simulator context has the following methods:
 - `get(expr: Value) -> int`
 - `get(expr: ValueCastable) -> any`
   - Returns the value of `expr` when awaited.
@@ -184,10 +185,10 @@ The simulator context has the following properties and methods:
   - Create a combinable trigger object for advancing simulation by `interval` seconds.
 - `changed(*signals)`
   - Create a combinable trigger object for advancing simulation until any signal in `signals` changes.
-- `edge(signal, value)`
+- `edge(signal, value: int)`
   - Create a combinable trigger object for advancing simulation until `signal` is changed to `value`.
     `signal` must be a 1-bit signal or a 1-bit slice of a signal.
-    `value` is a boolean where true indicates rising edge and false indicates falling edge.
+    Valid values for `value` are `1` for rising edge and `0` for falling edge.
 - `posedge(signal)`
 - `negedge(signal)`
   - Aliases for `edge(signal, 1)` and `edge(signal, 0)` respectively.
@@ -195,13 +196,7 @@ The simulator context has the following properties and methods:
   - Context manager.
     If the current process is a background process, `async with sim.critical():` makes it a non-background process for the duration of the statement.
 
-The `ProcessType` enum has the following members:
-  - `BikeshedProcess`
-    - The current process is a process added by `add_process`.
-  - `BikeshedTestbench`
-    - The current process is a testbench process added by `add_testbench`.
-
-A domain trigger object has the following methods:
+A domain trigger object is immutable and has the following methods:
 - `__await__()`
   - Advance simulation. No value is returned.
 - `until(condition)`
@@ -209,11 +204,26 @@ A domain trigger object has the following methods:
     `condition` is an arbitrary Amaranth expression.
     If `condition` is initially true, `await` will return immediately without advancing simulation.
     The return value is an unspecified awaitable with `await` as the only defined operation.
+    It is only awaitable once and awaiting it returns no value.
+  - Example implementation:
+    ```python
+    async def until(self, condition):
+      while not await self._sim.get(condition):
+        await self
+    ```
 - `repeat(times: int)`
   - Repeat the trigger `times` times.
+    Valid values are `times >= 0`.
     The return value is an unspecified awaitable with `await` as the only defined operation.
-
-A combinable trigger object has the following methods:
+    It is only awaitable once and awaiting it returns no value.
+  - Example implementation:
+    ```python
+    async def repeat(self, times):
+      for _ in range(times):
+        await self
+    ```
+
+A combinable trigger object is immutable and has the following methods:
 - `__await__()`
   - Advance simulation and return the value(s) of the trigger(s).
     - `delay` and `edge` triggers return `True` when they are hit, otherwise `False`.
@@ -230,6 +240,10 @@ A combinable trigger object has the following methods:
   - Create a new trigger object by copying the current object and appending another trigger.
   - Awaiting the returned trigger object pauses the process until the first of the combined triggers hit, i.e. the triggers are combined using OR semantics.
 
+To ensure testbench helper functions are only called from a testbench process, the `amaranth.sim.testbench_helper` decorator is added.
+The function wrapper expects the first positional argument (or second, after `self` or `cls` if decorating a method/classmethod) to be a simulator context, and will raise `TypeError` if not.
+If the function is called outside a testbench process, an exception will be raised.
+
 `Tick()`, `Delay()`, `Active()` and `Passive()` as well as the ability to pass generator coroutines as `process` are deprecated and removed in a future version.
 
 ## Drawbacks

From b13d1925a7c93965f6846191c6453b5c52d6f6c3 Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Sun, 17 Mar 2024 19:43:23 +0100
Subject: [PATCH 7/8] RFC #36: Fix example.

---
 text/0036-async-testbench-functions.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index f66e30c..201fed5 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -124,12 +124,12 @@ sim.add_process(adder)
 
 DDR IO buffer as a process:
 ```python
-o = Signal(2); pin = Signal()
+clk = Signal(); o = Signal(2); pin = Signal()
 async def ddr_buffer(sim):
     while True: # could be extended to pre-capture next `o` on posedge
-        await sim.negedge()
+        await sim.negedge(clk)
         await sim.set(pin, o[0])
-        await sim.posedge()
+        await sim.posedge(clk)
         await sim.set(pin, o[1])
 sim.add_process(ddr_buffer)
 ```

From 09771a08b37f3b16029d181e758606f0b64c28cb Mon Sep 17 00:00:00 2001
From: Vegard Storheil Eriksen <zyp@jvnv.net>
Date: Mon, 18 Mar 2024 19:50:58 +0100
Subject: [PATCH 8/8] RFC #36: Final changes.

---
 text/0036-async-testbench-functions.md | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/text/0036-async-testbench-functions.md b/text/0036-async-testbench-functions.md
index 201fed5..d853c13 100644
--- a/text/0036-async-testbench-functions.md
+++ b/text/0036-async-testbench-functions.md
@@ -1,6 +1,6 @@
-- Start Date: (fill me in with today's date, YYYY-MM-DD)
-- RFC PR: [amaranth-lang/rfcs#0036](https://github.com/amaranth-lang/rfcs/pull/0036)
-- Amaranth Issue: [amaranth-lang/amaranth#0000](https://github.com/amaranth-lang/amaranth/issues/0000)
+- Start Date: 2024-03-18
+- RFC PR: [amaranth-lang/rfcs#36](https://github.com/amaranth-lang/rfcs/pull/36)
+- Amaranth Issue: [amaranth-lang/amaranth#1213](https://github.com/amaranth-lang/amaranth/issues/1213)
 
 # Async testbench functions
 
@@ -265,8 +265,7 @@ Other python libraries like [cocotb](https://docs.cocotb.org/en/stable/coroutine
 ## Unresolved questions
 [unresolved-questions]: #unresolved-questions
 
-- Bikeshed all the names.
-  - (@whitequark) Should we go for `posedge` (Verilog convention) or `pos_edge` (Python convention)?
+None.
 
 ## Future possibilities
 [future-possibilities]: #future-possibilities