Repository: amaranth-lang/amaranth
Pull request #891 (2023-09-01 04:36:32)

lib.io: make `Pin` an interface object
Tracking #879.

The directions of signals in `Pin` make it convenient to use a pin signature in a component, such as in:

```python
class LEDDriver(Component):
    pins: Out(Signature({"o": Out(1)}))
    # same as `Out(Pin(1, dir="o").signature)`

led_driver = LEDDriver()
connect(led_driver.pins, platform.request("led"))
````

The `platform.request` call, correspondingly, returns a flipped `Pin` object.

whitequark: We have [discussed](https://libera.irclog.whitequark.org/amaranth-lang/2023-09-04#1693847414-1693848200;) this PR on the 2023-09-04 weekly meeting. The decision was to **merge**.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) Report
> Merging [#891](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (f3ecba2) into [main](https://app.codecov.io/gh/amaranth-lang/amaranth/commit/f135226a79fddf5caf894030ac7f49995774c3a7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) (f135226) will **decrease** coverage by `0.13%`.
> Report is 2 commits behind head on main.
> The diff coverage is `93.75%`.

```diff
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   83.91%   83.78%   -0.13%     
==========================================
  Files          54       54              
  Lines        7621     7674      +53     
  Branches     1847     1867      +20     
==========================================
+ Hits         6395     6430      +35     
- Misses       1029     1045      +16     
- Partials      197      199       +2     
```


| [Files Changed](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) | Coverage Δ | |
|---|---|---|
| [amaranth/lib/io.py](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvbGliL2lvLnB5) | `80.00% <90.00%> (+4.39%)` | :arrow_up: |
| [amaranth/build/res.py](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvYnVpbGQvcmVzLnB5) | `83.24% <100.00%> (+0.18%)` | :arrow_up: |
| [amaranth/hdl/\_rec.py](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang#diff-YW1hcmFudGgvaGRsL19yZWMucHk=) | `93.51% <100.00%> (+0.10%)` | :arrow_up: |

... and [3 files with indirect coverage changes](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/891/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang)


Patch:
diff --git a/amaranth/build/res.py b/amaranth/build/res.py
index 41a3004..0202953 100644
--- a/amaranth/build/res.py
+++ b/amaranth/build/res.py
@@ -122,29 +122,36 @@ class ResourceManager:
                     fields[sub.name] = resolve(sub, dir[sub.name], xdr[sub.name],
                                                name="{}__{}".format(name, sub.name),
                                                attrs={**attrs, **sub.attrs})
-                return Record([
+                rec = Record([
                     (f_name, f.layout) for (f_name, f) in fields.items()
                 ], fields=fields, name=name)
+                rec.signature = wiring.Signature({
+                    f_name: wiring.Out(f.signature) for (f_name, f) in fields.items()
+                })
+                return rec
 
             elif isinstance(resource.ios[0], (Pins, DiffPairs)):
                 phys = resource.ios[0]
+                # The flow is `In` below regardless of requested pin direction. The flow should
+                # never be used as it's not used internally and anyone using `dir="-"` should
+                # ignore it as well.
                 if isinstance(phys, Pins):
                     phys_names = phys.names
-                    port = Record([("io", len(phys))], name=name)
+                    port = wiring.Signature({"io": wiring.In(len(phys))}).create(path=(name,))
                 if isinstance(phys, DiffPairs):
                     phys_names = []
-                    record_fields = []
+                    members = {}
                     if not self.should_skip_port_component(None, attrs, "p"):
                         phys_names += phys.p.names
-                        record_fields.append(("p", len(phys)))
+                        members["p"] = wiring.In(len(phys))
                     if not self.should_skip_port_component(None, attrs, "n"):
                         phys_names += phys.n.names
-                        record_fields.append(("n", len(phys)))
-                    port = Record(record_fields, name=name)
+                        members["n"] = wiring.In(len(phys))
+                    port = wiring.Signature(members).create(path=(name,))
                 if dir == "-":
                     pin = None
                 else:
-                    pin = Pin(len(phys), dir, xdr=xdr, name=name)
+                    pin = wiring.flipped(Pin(len(phys), dir, xdr=xdr, name=name))
 
                 for phys_name in phys_names:
                     if phys_name in self._phys_reqd:
diff --git a/amaranth/hdl/_rec.py b/amaranth/hdl/_rec.py
index 2033ed0..2e88c58 100644
--- a/amaranth/hdl/_rec.py
+++ b/amaranth/hdl/_rec.py
@@ -7,6 +7,7 @@ from functools import reduce, wraps
 from .. import tracer
 from .._utils import union
 from .ast import *
+from ..lib import wiring
 
 
 __all__ = ["Direction", "DIR_NONE", "DIR_FANOUT", "DIR_FANIN", "Layout", "Record"]
@@ -132,6 +133,8 @@ class Record(ValueCastable):
             if fields is not None and field_name in fields:
                 field = fields[field_name]
                 if isinstance(field_shape, Layout):
+                    if isinstance(field, wiring.FlippedInterface):
+                        field = wiring.flipped(field)
                     assert isinstance(field, Record) and field_shape == field.layout
                 else:
                     assert isinstance(field, Signal) and Shape.cast(field_shape) == field.shape()
diff --git a/amaranth/lib/io.py b/amaranth/lib/io.py
index 2bb3615..3983794 100644
--- a/amaranth/lib/io.py
+++ b/amaranth/lib/io.py
@@ -4,18 +4,13 @@ from .. import *
 with warnings.catch_warnings():
     warnings.filterwarnings(action="ignore", category=DeprecationWarning)
     from ..hdl.rec import *
+from ..lib.wiring import In, Out, Signature
 
 
 __all__ = ["pin_layout", "Pin"]
 
 
-def pin_layout(width, dir, xdr=0):
-    """
-    Layout of the platform interface of a pin or several pins, which may be used inside
-    user-defined records.
-
-    See :class:`Pin` for details.
-    """
+def _pin_signature(width, dir, xdr=0):
     if not isinstance(width, int) or width < 0:
         raise TypeError("Width must be a non-negative integer, not {!r}"
                         .format(width))
@@ -26,29 +21,42 @@ def pin_layout(width, dir, xdr=0):
         raise TypeError("Gearing ratio must be a non-negative integer, not {!r}"
                         .format(xdr))
 
-    fields = []
+    members = {}
     if dir in ("i", "io"):
         if xdr > 0:
-            fields.append(("i_clk", 1))
+            members["i_clk"] = In(1)
         if xdr > 2:
-            fields.append(("i_fclk", 1))
+            members["i_fclk"] = In(1)
         if xdr in (0, 1):
-            fields.append(("i", width))
+            members["i"] = In(width)
         else:
             for n in range(xdr):
-                fields.append(("i{}".format(n), width))
+                members["i{}".format(n)] = In(width)
     if dir in ("o", "oe", "io"):
         if xdr > 0:
-            fields.append(("o_clk", 1))
+            members["o_clk"] = Out(1)
         if xdr > 2:
-            fields.append(("o_fclk", 1))
+            members["o_fclk"] = Out(1)
         if xdr in (0, 1):
-            fields.append(("o", width))
+            members["o"] = Out(width)
         else:
             for n in range(xdr):
-                fields.append(("o{}".format(n), width))
+                members["o{}".format(n)] = Out(width)
     if dir in ("oe", "io"):
-        fields.append(("oe", 1))
+        members["oe"] = In(1)
+    return Signature(members)
+
+
+def pin_layout(width, dir, xdr=0):
+    """
+    Layout of the platform interface of a pin or several pins, which may be used inside
+    user-defined records.
+
+    See :class:`Pin` for details.
+    """
+    fields = []
+    for name, member in _pin_signature(width, dir, xdr).members.items():
+        fields.append((name, member.shape))
     return Layout(fields)
 
 
@@ -118,3 +126,7 @@ class Pin(Record):
 
         super().__init__(pin_layout(self.width, self.dir, self.xdr),
                          name=name, src_loc_at=src_loc_at + 1)
+
+    @property
+    def signature(self):
+        return _pin_signature(self.width, self.dir, self.xdr)


Test patch:
diff --git a/tests/test_build_res.py b/tests/test_build_res.py
index 4acd55c..b017c6e 100644
--- a/tests/test_build_res.py
+++ b/tests/test_build_res.py
@@ -6,6 +6,7 @@ from amaranth import *
 with warnings.catch_warnings():
     warnings.filterwarnings(action="ignore", category=DeprecationWarning)
     from amaranth.hdl.rec import *
+from amaranth.lib.wiring import *
 from amaranth.lib.io import *
 from amaranth.build.dsl import *
 from amaranth.build.res import *
@@ -62,7 +63,7 @@ class ResourceManagerTestCase(FHDLTestCase):
         r = self.cm.lookup("user_led", 0)
         user_led = self.cm.request("user_led", 0)
 
-        self.assertIsInstance(user_led, Pin)
+        self.assertIsInstance(flipped(user_led), Pin)
         self.assertEqual(user_led.name, "user_led_0")
         self.assertEqual(user_led.width, 1)
         self.assertEqual(user_led.dir, "o")
@@ -91,11 +92,11 @@ class ResourceManagerTestCase(FHDLTestCase):
         self.assertEqual(ports[1].width, 1)
 
         scl_info, sda_info = self.cm.iter_single_ended_pins()
-        self.assertIs(scl_info[0], i2c.scl)
+        self.assertIs(flipped(scl_info[0]), i2c.scl)
         self.assertIs(scl_info[1].io, scl)
         self.assertEqual(scl_info[2], {})
         self.assertEqual(scl_info[3], False)
-        self.assertIs(sda_info[0], i2c.sda)
+        self.assertIs(flipped(sda_info[0]), i2c.sda)
         self.assertIs(sda_info[1].io, sda)
 
         self.assertEqual(list(self.cm.iter_port_constraints()), [
@@ -105,7 +106,7 @@ class ResourceManagerTestCase(FHDLTestCase):
 
     def test_request_diffpairs(self):
         clk100 = self.cm.request("clk100", 0)
-        self.assertIsInstance(clk100, Pin)
+        self.assertIsInstance(flipped(clk100), Pin)
         self.assertEqual(clk100.dir, "i")
         self.assertEqual(clk100.width, 1)
 
@@ -155,7 +156,6 @@ class ResourceManagerTestCase(FHDLTestCase):
 
     def test_request_raw(self):
         clk50 = self.cm.request("clk50", 0, dir="-")
-        self.assertIsInstance(clk50, Record)
         self.assertIsInstance(clk50.io, Signal)
 
         ports = list(self.cm.iter_ports())
@@ -164,7 +164,6 @@ class ResourceManagerTestCase(FHDLTestCase):
 
     def test_request_raw_diffpairs(self):
         clk100 = self.cm.request("clk100", 0, dir="-")
-        self.assertIsInstance(clk100, Record)
         self.assertIsInstance(clk100.p, Signal)
         self.assertIsInstance(clk100.n, Signal)