From 58d7a697df210d7fff60593c642fdebecc007ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois=20Nguyen?= Date: Fri, 17 May 2024 15:28:13 +0200 Subject: [PATCH] Remove .check_parameters() method from signatures. Since RFC 38, the signature of a component can only be assigned within its constructor. It is no longer useful to decouple the validation of signature parameters from its constructor. --- amaranth_soc/csr/bus.py | 72 ++++++++---------------------------- amaranth_soc/csr/reg.py | 40 ++++++-------------- amaranth_soc/event.py | 33 ++++------------- amaranth_soc/wishbone/bus.py | 59 ++++++++--------------------- tests/test_csr_bus.py | 5 +-- tests/test_wishbone_bus.py | 19 ---------- 6 files changed, 51 insertions(+), 177 deletions(-) diff --git a/amaranth_soc/csr/bus.py b/amaranth_soc/csr/bus.py index 5e4aaf2..87b9da3 100644 --- a/amaranth_soc/csr/bus.py +++ b/amaranth_soc/csr/bus.py @@ -49,13 +49,17 @@ class Signature(wiring.Signature): w_stb : Signal() Write strobe. Registers should update their value or perform the write side effect when this strobe is asserted. - - Raises - ------ - See :meth:`Element.Signature.check_parameters`. """ def __init__(self, width, access): - self.check_parameters(width, access) + if not isinstance(width, int) or width < 0: + raise TypeError(f"Width must be a non-negative integer, not {width!r}") + # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message + # instead of cls.__qualname__. + # Element.Access(access) + try: + Element.Access(access) + except ValueError as e: + raise ValueError(f"{access!r} is not a valid Element.Access") from e self._width = width self._access = Element.Access(access) @@ -81,27 +85,6 @@ def width(self): def access(self): return self._access - @classmethod - def check_parameters(cls, width, access): - """Validate signature parameters. - - Raises - ------ - :exc:`TypeError` - If ``width`` is not an integer greater than or equal to 0. - :exc:`ValueError` - If ``access`` is not a member of :class:`Element.Access`. - """ - if not isinstance(width, int) or width < 0: - raise TypeError(f"Width must be a non-negative integer, not {width!r}") - # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message - # instead of cls.__qualname__. - # Element.Access(access) - try: - Element.Access(access) - except ValueError as e: - raise ValueError(f"{access!r} is not a valid Element.Access") from e - def create(self, *, path=None, src_loc_at=0): """Create a compatible interface. @@ -139,10 +122,6 @@ def __repr__(self): Register access mode. path : iter(:class:`str`) Path to this CSR interface. Optional. See :class:`wiring.PureInterface`. - - Raises - ------ - See :meth:`Element.Signature.check_parameters` """ def __init__(self, width, access, *, path=None, src_loc_at=0): super().__init__(Element.Signature(width=width, access=access), path=path, src_loc_at=1 + src_loc_at) @@ -188,13 +167,12 @@ class Signature(wiring.Signature): to the register and causes write side effects to be performed (if any). If ``addr`` points to any chunk of a register, latches ``w_data`` to the captured value. Otherwise, does nothing. - - Raises - ------ - See :meth:`Signature.check_parameters`. """ def __init__(self, *, addr_width, data_width): - self.check_parameters(addr_width=addr_width, data_width=data_width) + if not isinstance(addr_width, int) or addr_width <= 0: + raise TypeError(f"Address width must be a positive integer, not {addr_width!r}") + if not isinstance(data_width, int) or data_width <= 0: + raise TypeError(f"Data width must be a positive integer, not {data_width!r}") self._addr_width = addr_width self._data_width = data_width @@ -216,22 +194,6 @@ def addr_width(self): def data_width(self): return self._data_width - @classmethod - def check_parameters(cls, *, addr_width, data_width): - """Validate signature parameters. - - Raises - ------ - :exc:`TypeError` - If ``addr_width`` is not an integer greater than 0. - :exc:`TypeError` - If ``data_width`` is not an integer greater than 0. - """ - if not isinstance(addr_width, int) or addr_width <= 0: - raise TypeError(f"Address width must be a positive integer, not {addr_width!r}") - if not isinstance(data_width, int) or data_width <= 0: - raise TypeError(f"Data width must be a positive integer, not {data_width!r}") - def create(self, *, path=None, src_loc_at=0): """Create a compatible interface. @@ -288,14 +250,10 @@ class Interface(wiring.PureInterface): ---------- memory_map: :class:`MemoryMap` Memory map of the bus. Optional. - - Raises - ------ - See :meth:`Signature.check_parameters`. """ def __init__(self, *, addr_width, data_width, path=None, src_loc_at=0): - sig = Signature(addr_width=addr_width, data_width=data_width) - super().__init__(sig, path=path, src_loc_at=1 + src_loc_at) + super().__init__(Signature(addr_width=addr_width, data_width=data_width), + path=path, src_loc_at=1 + src_loc_at) self._memory_map = None @property diff --git a/amaranth_soc/csr/reg.py b/amaranth_soc/csr/reg.py index 7263fcd..66d4dfe 100644 --- a/amaranth_soc/csr/reg.py +++ b/amaranth_soc/csr/reg.py @@ -51,13 +51,20 @@ class Signature(wiring.Signature): w_stb : Signal() Write strobe. Fields should update their value or perform the write side effect when this strobe is asserted. - - Raises - ------ - See :meth:`FieldPort.Signature.check_parameters`. """ def __init__(self, shape, access): - self.check_parameters(shape, access) + try: + Shape.cast(shape) + except TypeError as e: + raise TypeError(f"Field shape must be a shape-castable object, not {shape!r}") from e + # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message + # instead of cls.__qualname__. + # FieldPort.Access(access) + try: + FieldPort.Access(access) + except ValueError as e: + raise ValueError(f"{access!r} is not a valid FieldPort.Access") from e + self._shape = Shape.cast(shape) self._access = FieldPort.Access(access) @@ -76,29 +83,6 @@ def shape(self): def access(self): return self._access - @classmethod - def check_parameters(cls, shape, access): - """Validate signature parameters. - - Raises - ------ - :exc:`TypeError` - If ``shape`` is not a shape-castable object. - :exc:`ValueError` - If ``access`` is not a member of :class:`FieldPort.Access`. - """ - try: - Shape.cast(shape) - except TypeError as e: - raise TypeError(f"Field shape must be a shape-castable object, not {shape!r}") from e - # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message - # instead of cls.__qualname__. - # FieldPort.Access(access) - try: - FieldPort.Access(access) - except ValueError as e: - raise ValueError(f"{access!r} is not a valid FieldPort.Access") from e - def create(self, *, path=None, src_loc_at=0): """Create a compatible interface. diff --git a/amaranth_soc/event.py b/amaranth_soc/event.py index 979309f..67a92f2 100644 --- a/amaranth_soc/event.py +++ b/amaranth_soc/event.py @@ -27,13 +27,16 @@ class Signature(wiring.Signature): Input line. Sampled in order to detect an event. trg : Signal() Event trigger. Asserted when an event occurs, according to the trigger mode. - - Raises - ------ - See :meth:`Source.Signature.check_parameters`. """ def __init__(self, *, trigger="level"): - self.check_parameters(trigger=trigger) + # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message + # instead of cls.__qualname__. + # Source.Trigger(trigger) + try: + Source.Trigger(trigger) + except ValueError as e: + raise ValueError(f"{trigger!r} is not a valid Source.Trigger") from e + super().__init__({ "i": Out(1), "trg": In(1), @@ -44,22 +47,6 @@ def __init__(self, *, trigger="level"): def trigger(self): return self._trigger - def check_parameters(cls, *, trigger): - """Validate signature parameters. - - Raises - ------ - :exc:`ValueError` - If ``trigger`` is not a member of :class:`Source.Trigger`. - """ - # TODO(py3.9): Remove this. Python 3.8 and below use cls.__name__ in the error message - # instead of cls.__qualname__. - # Source.Trigger(trigger) - try: - Source.Trigger(trigger) - except ValueError as e: - raise ValueError(f"{trigger!r} is not a valid Source.Trigger") from e - def create(self, *, path=None, src_loc_at=0): """Create a compatible interface. @@ -94,10 +81,6 @@ def __repr__(self): ---------- event_map : :class:`EventMap` A collection of event sources. - - Raises - ------ - See :meth:`Source.Signature.check_parameters`. """ def __init__(self, *, trigger="level", path=None, src_loc_at=0): super().__init__(Source.Signature(trigger=trigger), path=path, src_loc_at=1 + src_loc_at) diff --git a/amaranth_soc/wishbone/bus.py b/amaranth_soc/wishbone/bus.py index 9ec71cb..393fca4 100644 --- a/amaranth_soc/wishbone/bus.py +++ b/amaranth_soc/wishbone/bus.py @@ -46,12 +46,10 @@ class Signature(wiring.Signature): ---------- addr_width : int Width of the address signal. - data_width : int + data_width : ``8``, ``16``, ``32`` or ``64`` Width of the data signals ("port size" in Wishbone terminology). - One of 8, 16, 32, 64. - granularity : int + granularity : ``8``, ``16``, ``32``, ``64`` or ``None`` Granularity of select signals ("port granularity" in Wishbone terminology). - One of 8, 16, 32, 64. Optional. If ``None`` (by default), the granularity is equal to ``data_width``. features : iter(:class:`Feature`) Selects additional signals that will be a part of this interface. @@ -94,16 +92,22 @@ class Signature(wiring.Signature): Optional. Corresponds to Wishbone signal ``CTI_O`` (initiator) or ``CTI_I`` (target). bte : Signal() Optional. Corresponds to Wishbone signal ``BTE_O`` (initiator) or ``BTE_I`` (target). - - Raises - ------ - See :meth:`Signature.check_parameters`. """ def __init__(self, *, addr_width, data_width, granularity=None, features=frozenset()): if granularity is None: - granularity = data_width - self.check_parameters(addr_width=addr_width, data_width=data_width, granularity=granularity, - features=features) + granularity = data_width + + if not isinstance(addr_width, int) or addr_width < 0: + raise TypeError(f"Address width must be a non-negative integer, not {addr_width!r}") + if data_width not in (8, 16, 32, 64): + raise ValueError(f"Data width must be one of 8, 16, 32, 64, not {data_width!r}") + if granularity not in (8, 16, 32, 64): + raise ValueError(f"Granularity must be one of 8, 16, 32, 64, not {granularity!r}") + if granularity > data_width: + raise ValueError(f"Granularity {granularity} may not be greater than data width " + f"{data_width}") + for feature in features: + Feature(feature) # raises ValueError if feature is invalid self._addr_width = addr_width self._data_width = data_width @@ -150,35 +154,6 @@ def granularity(self): def features(self): return self._features - @classmethod - def check_parameters(cls, *, addr_width, data_width, granularity, features): - """Validate signature parameters. - - Raises - ------ - :exc:`TypeError` - If ``addr_width`` is not an integer greater than or equal to 0. - :exc:`ValueError` - If ``data_width`` is not 8, 16, 32 or 64. - :exc:`ValueError` - If ``granularity`` is not 8, 16, 32 or 64. - :exc:`ValueError` - If ``granularity`` is greater than ``data_width``. - :exc:`ValueError` - If ``features`` contains other values than :class:`Feature` members. - """ - if not isinstance(addr_width, int) or addr_width < 0: - raise TypeError(f"Address width must be a non-negative integer, not {addr_width!r}") - if data_width not in (8, 16, 32, 64): - raise ValueError(f"Data width must be one of 8, 16, 32, 64, not {data_width!r}") - if granularity not in (8, 16, 32, 64): - raise ValueError(f"Granularity must be one of 8, 16, 32, 64, not {granularity!r}") - if granularity > data_width: - raise ValueError(f"Granularity {granularity} may not be greater than data width " - f"{data_width}") - for feature in features: - Feature(feature) # raises ValueError if feature is invalid - def create(self, *, path=None, src_loc_at=0): """Create a compatible interface. @@ -232,10 +207,6 @@ class Interface(wiring.PureInterface): ---------- memory_map: :class:`MemoryMap` Memory map of the bus. Optional. - - Raises - ------ - See :meth:`Signature.check_parameters`. """ def __init__(self, *, addr_width, data_width, granularity=None, features=frozenset(), path=None, src_loc_at=0): diff --git a/tests/test_csr_bus.py b/tests/test_csr_bus.py index cab9f08..3039a8a 100644 --- a/tests/test_csr_bus.py +++ b/tests/test_csr_bus.py @@ -131,14 +131,11 @@ def test_wrong_addr_width(self): with self.assertRaisesRegex(TypeError, r"Address width must be a positive integer, not -1"): csr.Signature(addr_width=-1, data_width=8) - with self.assertRaisesRegex(TypeError, - r"Address width must be a positive integer, not -1"): - csr.Signature.check_parameters(addr_width=-1, data_width=8) def test_wrong_data_width(self): with self.assertRaisesRegex(TypeError, r"Data width must be a positive integer, not -1"): - csr.Signature.check_parameters(addr_width=16, data_width=-1) + csr.Signature(addr_width=16, data_width=-1) class InterfaceTestCase(unittest.TestCase): diff --git a/tests/test_wishbone_bus.py b/tests/test_wishbone_bus.py index cdd14d8..8c8ad5f 100644 --- a/tests/test_wishbone_bus.py +++ b/tests/test_wishbone_bus.py @@ -106,44 +106,25 @@ def test_wrong_addr_width(self): with self.assertRaisesRegex(TypeError, r"Address width must be a non-negative integer, not -1"): wishbone.Signature(addr_width=-1, data_width=8) - with self.assertRaisesRegex(TypeError, - r"Address width must be a non-negative integer, not -1"): - wishbone.Signature.check_parameters(addr_width=-1, data_width=8, granularity=8, - features=()) def test_wrong_data_width(self): with self.assertRaisesRegex(ValueError, r"Data width must be one of 8, 16, 32, 64, not 7"): wishbone.Signature(addr_width=0, data_width=7) - with self.assertRaisesRegex(ValueError, - r"Data width must be one of 8, 16, 32, 64, not 7"): - wishbone.Signature.check_parameters(addr_width=0, data_width=7, granularity=7, - features=()) def test_wrong_granularity(self): with self.assertRaisesRegex(ValueError, r"Granularity must be one of 8, 16, 32, 64, not 7"): wishbone.Signature(addr_width=0, data_width=32, granularity=7) - with self.assertRaisesRegex(ValueError, - r"Granularity must be one of 8, 16, 32, 64, not 7"): - wishbone.Signature.check_parameters(addr_width=0, data_width=32, granularity=7, - features=()) def test_wrong_granularity_wide(self): with self.assertRaisesRegex(ValueError, r"Granularity 32 may not be greater than data width 8"): wishbone.Signature(addr_width=0, data_width=8, granularity=32) - with self.assertRaisesRegex(ValueError, - r"Granularity 32 may not be greater than data width 8"): - wishbone.Signature.check_parameters(addr_width=0, data_width=8, granularity=32, - features=()) def test_wrong_features(self): with self.assertRaisesRegex(ValueError, r"'foo' is not a valid Feature"): wishbone.Signature(addr_width=0, data_width=8, features={"foo"}) - with self.assertRaisesRegex(ValueError, r"'foo' is not a valid Feature"): - wishbone.Signature.check_parameters(addr_width=0, data_width=8, granularity=8, - features={"foo"}) class InterfaceTestCase(unittest.TestCase):