Merge pull request #235 from DiamondLightSource/controller-setattr

GDYendell · web-flow · commit 019ae1532d90 · 2025-10-22T14:34:27.000+01:00
Improve Controller Attribute and sub controller creation with __setattr__
diff --git a/docs/conf.py b/docs/conf.py
@@ -94,6 +94,7 @@
     ("py:class", "fastcs.logging._graylog.GraylogStaticFields"),
     ("py:class", "fastcs.logging._graylog.GraylogEnvFields"),
     ("py:obj", "fastcs.launch.build_controller_api"),
+    ("py:obj", "fastcs.transport.epics.util.controller_pv_prefix"),
     ("docutils", "fastcs.demo.controllers.TemperatureControllerSettings"),
     # TypeVar without docstrings still give warnings
     ("py:class", "fastcs.datatypes.T_Numerical"),
diff --git a/docs/snippets/dynamic.py b/docs/snippets/dynamic.py
@@ -126,7 +126,7 @@ async def initialise(self):
                 idx + 1, ramp_parameters, self._io
             )
             await ramp_controller.initialise()
-            self.register_sub_controller(f"Ramp{idx + 1:02d}", ramp_controller)
+            self.add_sub_controller(f"Ramp{idx + 1:02d}", ramp_controller)
 
         await self._connection.close()
 
diff --git a/docs/snippets/static10.py b/docs/snippets/static10.py
@@ -71,7 +71,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings):
         for index in range(1, ramp_count + 1):
             controller = TemperatureRampController(index, self._connection)
             self._ramp_controllers.append(controller)
-            self.register_sub_controller(f"R{index}", controller)
+            self.add_sub_controller(f"R{index}", controller)
 
     async def connect(self):
         await self._connection.connect(self._ip_settings)
diff --git a/docs/snippets/static11.py b/docs/snippets/static11.py
@@ -78,7 +78,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings):
         for index in range(1, ramp_count + 1):
             controller = TemperatureRampController(index, self._connection)
             self._ramp_controllers.append(controller)
-            self.register_sub_controller(f"R{index}", controller)
+            self.add_sub_controller(f"R{index}", controller)
 
     async def connect(self):
         await self._connection.connect(self._ip_settings)
diff --git a/docs/snippets/static12.py b/docs/snippets/static12.py
@@ -83,7 +83,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings):
         for index in range(1, ramp_count + 1):
             controller = TemperatureRampController(index, self._connection)
             self._ramp_controllers.append(controller)
-            self.register_sub_controller(f"R{index}", controller)
+            self.add_sub_controller(f"R{index}", controller)
 
     async def connect(self):
         await self._connection.connect(self._ip_settings)
diff --git a/docs/snippets/static13.py b/docs/snippets/static13.py
@@ -84,7 +84,7 @@ def __init__(self, ramp_count: int, settings: IPConnectionSettings):
         for index in range(1, ramp_count + 1):
             controller = TemperatureRampController(index, self._connection)
             self._ramp_controllers.append(controller)
-            self.register_sub_controller(f"R{index}", controller)
+            self.add_sub_controller(f"R{index}", controller)
 
     async def connect(self):
         await self._connection.connect(self._ip_settings)
diff --git a/src/fastcs/attributes.py b/src/fastcs/attributes.py
@@ -47,6 +47,9 @@ def __init__(
         # changing the units on an int. This should be implemented in the backend.
         self._update_datatype_callbacks: list[Callable[[DataType[T]], None]] = []
 
+        # Name to be filled in by Controller when the Attribute is bound
+        self._name = None
+
     @property
     def io_ref(self) -> AttributeIORefT:
         if self._io_ref is None:
@@ -82,8 +85,16 @@ def update_datatype(self, datatype: DataType[T]) -> None:
         for callback in self._update_datatype_callbacks:
             callback(datatype)
 
+    def set_name(self, name: list[str]):
+        if self._name:
+            raise ValueError(
+                f"Attribute is already registered with a controller as {self._name}"
+            )
+
+        self._name = name
+
     def __repr__(self):
-        return f"{self.__class__.__name__}({self._datatype})"
+        return f"{self.__class__.__name__}({self._name}, {self._datatype})"
 
 
 class AttrR(Attribute[T, AttributeIORefT]):
diff --git a/src/fastcs/controller.py b/src/fastcs/controller.py
@@ -131,18 +131,7 @@ class method and a controller instance, so that it can be called from any
 
             attr = getattr(self, attr_name, None)
             if isinstance(attr, Attribute):
-                if (
-                    attr_name in self.attributes
-                    and self.attributes[attr_name] is not attr
-                ):
-                    raise ValueError(
-                        f"`{type(self).__name__}` has conflicting attribute "
-                        f"`{attr_name}` already present in the attributes dict."
-                    )
-
-                new_attribute = deepcopy(attr)
-                setattr(self, attr_name, new_attribute)
-                self.attributes[attr_name] = new_attribute
+                setattr(self, attr_name, deepcopy(attr))
             elif isinstance(attr, UnboundPut | UnboundScan | UnboundCommand):
                 setattr(self, attr_name, attr.bind(self))
 
@@ -164,22 +153,39 @@ def _validate_io(self, ios: Sequence[AttributeIO[T, AttributeIORefT]]):
                 f"{attr.io_ref.__class__.__name__}"
             )
 
-    def register_sub_controller(self, name: str, sub_controller: Controller):
+    def add_attribute(self, name, attribute: Attribute):
+        if name in self.attributes and attribute is not self.attributes[name]:
+            raise ValueError(
+                f"Cannot add attribute {name}. "
+                f"Controller {self} has has existing attribute {name}"
+            )
+        elif name in self.__sub_controller_tree.keys():
+            raise ValueError(
+                f"Cannot add attribute {name}. "
+                f"Controller {self} has existing sub controller {name}"
+            )
+
+        attribute.set_name(name)
+        self.attributes[name] = attribute
+        super().__setattr__(name, attribute)
+
+    def add_sub_controller(self, name: str, sub_controller: Controller):
         if name in self.__sub_controller_tree.keys():
             raise ValueError(
-                f"Controller {self} already has a sub controller registered as {name}"
+                f"Cannot add sub controller {name}. "
+                f"Controller {self} has existing sub controller {name}"
+            )
+        elif name in self.attributes:
+            raise ValueError(
+                f"Cannot add sub controller {name}. "
+                f"Controller {self} has existing attribute {name}"
             )
 
-        self.__sub_controller_tree[name] = sub_controller
         sub_controller.set_path(self.path + [name])
+        self.__sub_controller_tree[name] = sub_controller
+        super().__setattr__(name, sub_controller)
 
         if isinstance(sub_controller.root_attribute, Attribute):
-            if name in self.attributes:
-                raise TypeError(
-                    f"Cannot set sub controller `{name}` root attribute "
-                    f"on the parent controller `{type(self).__name__}` "
-                    f"as it already has an attribute of that name."
-                )
             self.attributes[name] = sub_controller.root_attribute
 
     def get_sub_controllers(self) -> dict[str, Controller]:
@@ -190,6 +196,14 @@ def __repr__(self):
 {type(self).__name__}({self.path}, {list(self.__sub_controller_tree.keys())})\
 """
 
+    def __setattr__(self, name, value):
+        if isinstance(value, Attribute):
+            self.add_attribute(name, value)
+        elif isinstance(value, Controller):
+            self.add_sub_controller(name, value)
+        else:
+            super().__setattr__(name, value)
+
 
 class Controller(BaseController):
     """Top-level controller for a device.
diff --git a/src/fastcs/demo/controllers.py b/src/fastcs/demo/controllers.py
@@ -82,7 +82,7 @@ def __init__(self, settings: TemperatureControllerSettings) -> None:
         for index in range(1, settings.num_ramp_controllers + 1):
             controller = TemperatureRampController(index, self.connection)
             self._ramp_controllers.append(controller)
-            self.register_sub_controller(f"R{index}", controller)
+            self.add_sub_controller(f"R{index}", controller)
 
     @command()
     async def cancel_all(self) -> None:
diff --git a/src/fastcs/transport/epics/ca/ioc.py b/src/fastcs/transport/epics/ca/ioc.py
@@ -20,6 +20,7 @@
     record_metadata_from_datatype,
 )
 from fastcs.transport.epics.options import EpicsIOCOptions
+from fastcs.transport.epics.util import controller_pv_prefix
 from fastcs.util import snake_to_pascal
 
 EPICS_MAX_NAME_LENGTH = 60
@@ -111,27 +112,26 @@ def _add_sub_controller_pvi_info(pv_prefix: str, parent: ControllerAPI):
         parent: Controller to add PVI refs for
 
     """
-    parent_pvi = ":".join([pv_prefix] + parent.path + ["PVI"])
+    parent_pvi = f"{controller_pv_prefix(pv_prefix, parent)}:PVI"
 
     for child in parent.sub_apis.values():
-        child_pvi = ":".join([pv_prefix] + child.path + ["PVI"])
+        child_pvi = f"{controller_pv_prefix(pv_prefix, child)}:PVI"
         child_name = child.path[-1].lower()
 
         _add_pvi_info(child_pvi, parent_pvi, child_name)
-
         _add_sub_controller_pvi_info(pv_prefix, child)
 
 
 def _create_and_link_attribute_pvs(
-    pv_prefix: str, root_controller_api: ControllerAPI
+    root_pv_prefix: str, root_controller_api: ControllerAPI
 ) -> None:
     for controller_api in root_controller_api.walk_api():
-        path = controller_api.path
+        pv_prefix = controller_pv_prefix(root_pv_prefix, controller_api)
+
         for attr_name, attribute in controller_api.attributes.items():
             pv_name = snake_to_pascal(attr_name)
-            _pv_prefix = ":".join([pv_prefix] + path)
-            full_pv_name_length = len(f"{_pv_prefix}:{pv_name}")
 
+            full_pv_name_length = len(f"{pv_prefix}:{pv_name}")
             if full_pv_name_length > EPICS_MAX_NAME_LENGTH:
                 attribute.enabled = False
                 print(
@@ -152,15 +152,15 @@ def _create_and_link_attribute_pvs(
                         attribute.enabled = False
                     else:
                         _create_and_link_read_pv(
-                            _pv_prefix, f"{pv_name}_RBV", attr_name, attribute
+                            pv_prefix, f"{pv_name}_RBV", attr_name, attribute
                         )
                         _create_and_link_write_pv(
-                            _pv_prefix, pv_name, attr_name, attribute
+                            pv_prefix, pv_name, attr_name, attribute
                         )
                 case AttrR():
-                    _create_and_link_read_pv(_pv_prefix, pv_name, attr_name, attribute)
+                    _create_and_link_read_pv(pv_prefix, pv_name, attr_name, attribute)
                 case AttrW():
-                    _create_and_link_write_pv(_pv_prefix, pv_name, attr_name, attribute)
+                    _create_and_link_write_pv(pv_prefix, pv_name, attr_name, attribute)
 
 
 def _create_and_link_read_pv(
@@ -224,32 +224,31 @@ async def async_write_display(value: T):
 
         record.set(cast_to_epics_type(attribute.datatype, value), process=False)
 
-    record = _make_record(
-        f"{pv_prefix}:{pv_name}", attribute, on_update=on_update, out_record=True
-    )
+    record = _make_record(pv, attribute, on_update=on_update, out_record=True)
 
     _add_attr_pvi_info(record, pv_prefix, attr_name, "w")
 
     attribute.add_write_display_callback(async_write_display)
 
 
 def _create_and_link_command_pvs(
-    pv_prefix: str, root_controller_api: ControllerAPI
+    root_pv_prefix: str, root_controller_api: ControllerAPI
 ) -> None:
     for controller_api in root_controller_api.walk_api():
-        path = controller_api.path
+        pv_prefix = controller_pv_prefix(root_pv_prefix, controller_api)
+
         for attr_name, method in controller_api.command_methods.items():
             pv_name = snake_to_pascal(attr_name)
-            _pv_prefix = ":".join([pv_prefix] + path)
-            if len(f"{_pv_prefix}:{pv_name}") > EPICS_MAX_NAME_LENGTH:
+
+            if len(f"{pv_prefix}:{pv_name}") > EPICS_MAX_NAME_LENGTH:
                 print(
                     f"Not creating PV for {attr_name} as full name would exceed"
                     f" {EPICS_MAX_NAME_LENGTH} characters"
                 )
                 method.enabled = False
             else:
                 _create_and_link_command_pv(
-                    _pv_prefix,
+                    pv_prefix,
                     pv_name,
                     attr_name,
                     method,
diff --git a/src/fastcs/transport/epics/pva/ioc.py b/src/fastcs/transport/epics/pva/ioc.py
@@ -4,6 +4,7 @@
 
 from fastcs.attributes import Attribute, AttrR, AttrRW, AttrW
 from fastcs.controller_api import ControllerAPI
+from fastcs.transport.epics.util import controller_pv_prefix
 from fastcs.util import snake_to_pascal
 
 from ._pv_handlers import (
@@ -26,12 +27,6 @@ def _attribute_to_access(attribute: Attribute) -> AccessModeType:
             raise ValueError(f"Unknown attribute type {type(attribute)}")
 
 
-def get_pv_name(pv_prefix: str, *attribute_names: str) -> str:
-    """Converts from an attribute name to a pv name."""
-    pv_formatted = ":".join([snake_to_pascal(attr) for attr in attribute_names])
-    return f"{pv_prefix}:{pv_formatted}" if pv_formatted else pv_prefix
-
-
 async def parse_attributes(
     root_pv_prefix: str, root_controller_api: ControllerAPI
 ) -> list[StaticProvider]:
@@ -40,33 +35,33 @@ async def parse_attributes(
     provider = StaticProvider(root_pv_prefix)
 
     for controller_api in root_controller_api.walk_api():
-        pv_prefix = get_pv_name(root_pv_prefix, *controller_api.path)
+        pv_prefix = controller_pv_prefix(root_pv_prefix, controller_api)
 
         pvi_tree.add_sub_device(pv_prefix, controller_api.description)
 
         for attr_name, attribute in controller_api.attributes.items():
-            pv_name = get_pv_name(pv_prefix, attr_name)
+            full_pv_name = f"{pv_prefix}:{snake_to_pascal(attr_name)}"
             match attribute:
                 case AttrRW():
                     attribute_pv = make_shared_write_pv(attribute)
                     attribute_pv_rbv = make_shared_read_pv(attribute)
-                    provider.add(pv_name, attribute_pv)
-                    provider.add(f"{pv_name}_RBV", attribute_pv_rbv)
-                    pvi_tree.add_signal(pv_name, "rw")
+                    provider.add(f"{full_pv_name}", attribute_pv)
+                    provider.add(f"{full_pv_name}_RBV", attribute_pv_rbv)
+                    pvi_tree.add_signal(f"{full_pv_name}", "rw")
                 case AttrR():
                     attribute_pv = make_shared_read_pv(attribute)
-                    provider.add(pv_name, attribute_pv)
-                    pvi_tree.add_signal(pv_name, "r")
+                    provider.add(f"{full_pv_name}", attribute_pv)
+                    pvi_tree.add_signal(f"{full_pv_name}", "r")
                 case AttrW():
                     attribute_pv = make_shared_write_pv(attribute)
-                    provider.add(pv_name, attribute_pv)
-                    pvi_tree.add_signal(pv_name, "w")
+                    provider.add(f"{full_pv_name}", attribute_pv)
+                    pvi_tree.add_signal(f"{full_pv_name}", "w")
 
         for attr_name, method in controller_api.command_methods.items():
-            pv_name = get_pv_name(pv_prefix, attr_name)
+            full_pv_name = f"{pv_prefix}:{snake_to_pascal(attr_name)}"
             command_pv = make_command_pv(method.fn)
-            provider.add(pv_name, command_pv)
-            pvi_tree.add_signal(pv_name, "x")
+            provider.add(f"{full_pv_name}", command_pv)
+            pvi_tree.add_signal(f"{full_pv_name}", "x")
 
     return [provider, pvi_tree.make_provider()]
 
diff --git a/src/fastcs/transport/epics/util.py b/src/fastcs/transport/epics/util.py
@@ -0,0 +1,6 @@
+from fastcs.controller_api import ControllerAPI
+from fastcs.util import snake_to_pascal
+
+
+def controller_pv_prefix(prefix: str, controller_api: ControllerAPI) -> str:
+    return ":".join([prefix] + [snake_to_pascal(node) for node in controller_api.path])
diff --git a/tests/assertable_controller.py b/tests/assertable_controller.py
@@ -46,7 +46,7 @@ def __init__(self) -> None:
         for index in range(1, 3):
             controller = TestSubController()
             self._sub_controllers.append(controller)
-            self.register_sub_controller(f"SubController{index:02d}", controller)
+            self.add_sub_controller(f"SubController{index:02d}", controller)
 
     initialised = False
     connected = False
diff --git a/tests/example_p4p_ioc.py b/tests/example_p4p_ioc.py
@@ -70,12 +70,9 @@ async def i(self):
 
 def run(pv_prefix="P4P_TEST_DEVICE"):
     controller = ParentController()
-    controller.register_sub_controller(
-        "Child1", ChildController(description="some sub controller")
-    )
-    controller.register_sub_controller(
-        "Child2", ChildController(description="another sub controller")
-    )
+    controller.child1 = ChildController(description="some sub controller")
+    controller.child2 = ChildController(description="another sub controller")
+
     fastcs = FastCS(
         controller, [EpicsPVATransport(pva_ioc=EpicsIOCOptions(pv_prefix=pv_prefix))]
     )
diff --git a/tests/example_softioc.py b/tests/example_softioc.py
@@ -22,7 +22,7 @@ async def d(self):
 
 def run(pv_prefix="SOFTIOC_TEST_DEVICE"):
     controller = ParentController()
-    controller.register_sub_controller("Child", ChildController())
+    controller.child = ChildController()
     fastcs = FastCS(
         controller, [EpicsCATransport(ca_ioc=EpicsIOCOptions(pv_prefix=pv_prefix))]
     )
diff --git a/tests/test_controller.py b/tests/test_controller.py
diff --git a/tests/transport/epics/pva/test_p4p.py b/tests/transport/epics/pva/test_p4p.py

Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ async def initialise(self):`
`126`	`126`	`idx + 1, ramp_parameters, self._io`
`127`	`127`	`)`
`128`	`128`	`await ramp_controller.initialise()`
`129`		`- self.register_sub_controller(f"Ramp{idx + 1:02d}", ramp_controller)`
	`129`	`+ self.add_sub_controller(f"Ramp{idx + 1:02d}", ramp_controller)`
`130`	`130`
`131`	`131`	`await self._connection.close()`
`132`	`132`