Allow binding sub controller to Controller via __setattr__

GDYendell · GDYendell · commit 1fd8830629cd · 2025-10-22T13:00:54.000Z
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/controller.py b/src/fastcs/controller.py
@@ -169,22 +169,23 @@ def add_attribute(self, name, attribute: Attribute):
         self.attributes[name] = attribute
         super().__setattr__(name, attribute)
 
-    def register_sub_controller(self, name: str, sub_controller: Controller):
+    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]:
@@ -198,6 +199,8 @@ def __repr__(self):
     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)
 
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
@@ -114,7 +114,7 @@ def _add_sub_controller_pvi_info(pv_prefix: str, parent: ControllerAPI):
     parent_pvi = ":".join([pv_prefix] + parent.path + ["PVI"])
 
     for child in parent.sub_apis.values():
-        child_pvi = ":".join([pv_prefix] + child.path + ["PVI"])
+        child_pvi = ":".join([pv_prefix] + _controller_pv_prefix(child.path) + ["PVI"])
         child_name = child.path[-1].lower()
 
         _add_pvi_info(child_pvi, parent_pvi, child_name)
@@ -123,14 +123,14 @@ def _add_sub_controller_pvi_info(pv_prefix: str, parent: ControllerAPI):
 
 
 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
         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}")
+            pv_prefix = ":".join([root_pv_prefix] + _controller_pv_prefix(path))
+            full_pv_name_length = len(f"{pv_prefix}:{pv_name}")
 
             if full_pv_name_length > EPICS_MAX_NAME_LENGTH:
                 attribute.enabled = False
@@ -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(
@@ -234,22 +234,22 @@ async def async_write_display(value: T):
 
 
 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
         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:
+            pv_prefix = ":".join([root_pv_prefix] + _controller_pv_prefix(path))
+            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,
@@ -302,3 +302,7 @@ def _add_attr_pvi_info(
             }
         },
     )
+
+
+def _controller_pv_prefix(controller_path: list[str]) -> list[str]:
+    return [snake_to_pascal(node) for node in controller_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
@@ -2,31 +2,27 @@
 
 from fastcs.attributes import AttrR
 from fastcs.controller import Controller
-from fastcs.datatypes import Int
+from fastcs.datatypes import Float, Int
 
 
 def test_controller_nesting():
     controller = Controller()
     sub_controller = Controller()
     sub_sub_controller = Controller()
 
-    controller.register_sub_controller("a", sub_controller)
-    sub_controller.register_sub_controller("b", sub_sub_controller)
+    controller.a = sub_controller
+    sub_controller.b = sub_sub_controller
 
     assert sub_controller.path == ["a"]
     assert sub_sub_controller.path == ["a", "b"]
     assert controller.get_sub_controllers() == {"a": sub_controller}
     assert sub_controller.get_sub_controllers() == {"b": sub_sub_controller}
 
-    with pytest.raises(
-        ValueError, match=r"Controller .* already has a sub controller registered as .*"
-    ):
-        controller.register_sub_controller("a", Controller())
+    with pytest.raises(ValueError, match=r"existing sub controller"):
+        controller.a = Controller()
 
-    with pytest.raises(
-        ValueError, match=r"sub controller is already registered under .*"
-    ):
-        controller.register_sub_controller("c", sub_controller)
+    with pytest.raises(ValueError, match=r"already registered"):
+        controller.c = sub_controller
 
 
 class SomeSubController(Controller):
@@ -51,7 +47,7 @@ def __init__(self, sub_controller: Controller):
         self.attributes["_attributes_attr"] = AttrR(Int())
         self.attributes["_attributes_attr_equal"] = self.equal_attr
 
-        self.register_sub_controller("sub_controller", sub_controller)
+        self.sub_controller = sub_controller
 
 
 def test_attribute_parsing():
@@ -78,28 +74,30 @@ def test_attribute_parsing():
     }
 
 
-def test_attribute_in_both_class_and_get_attributes_fails():
-    class FailingController(Controller):
-        duplicate_attribute = AttrR(Int())
+def test_conflicting_attributes_and_controllers():
+    class ConflictingController(Controller):
+        attr = AttrR(Int())
 
         def __init__(self):
             super().__init__()
-            self.duplicate_attribute = AttrR(Int())
+            self.sub_controller = Controller()
 
-    with pytest.raises(ValueError, match="existing attribute"):
-        FailingController()
+    controller = ConflictingController()
 
+    with pytest.raises(ValueError, match=r"Cannot add attribute .* existing attribute"):
+        controller.attr = AttrR(Float())  # pyright: ignore[reportAttributeAccessIssue]
 
-def test_root_attribute():
-    class FailingController(SomeController):
-        sub_controller = AttrR(Int())
+    with pytest.raises(
+        ValueError, match=r"Cannot add sub controller .* existing attribute"
+    ):
+        controller.attr = Controller()  # pyright: ignore[reportAttributeAccessIssue]
+
+    with pytest.raises(
+        ValueError, match=r"Cannot add sub controller .* existing sub controller"
+    ):
+        controller.sub_controller = Controller()
 
     with pytest.raises(
-        TypeError,
-        match=(
-            "Cannot set sub controller `sub_controller` root attribute "
-            "on the parent controller `FailingController` as it already "
-            "has an attribute of that name."
-        ),
+        ValueError, match=r"Cannot add attribute .* existing sub controller"
     ):
-        FailingController(SomeSubController())
+        controller.sub_controller = AttrR(Int())  # pyright: ignore[reportAttributeAccessIssue]
diff --git a/tests/transport/epics/pva/test_p4p.py b/tests/transport/epics/pva/test_p4p.py
@@ -287,27 +287,28 @@ class SomeController(Controller):
         another_attr_0: AttrRW = AttrRW(Int())
         another_attr_1000: AttrRW = AttrRW(Int())
         a_third_attr: AttrW = AttrW(Int())
-        child_attribute_same_name: AttrR = AttrR(Int())
 
     controller = SomeController()
 
     sub_controller = ChildController()
-    controller.register_sub_controller("Child0", sub_controller)
-    sub_controller.register_sub_controller("ChildChild", ChildChildController())
-    sub_controller = ChildController()
-    controller.register_sub_controller("Child1", sub_controller)
-    sub_controller.register_sub_controller("ChildChild", ChildChildController())
+    controller.child0 = sub_controller
+    sub_controller.child_child = ChildChildController()
+
     sub_controller = ChildController()
-    controller.register_sub_controller("Child2", sub_controller)
-    sub_controller.register_sub_controller("ChildChild", ChildChildController())
+    controller.child1 = sub_controller
+    sub_controller.child_child = ChildChildController()
+
     sub_controller = ChildController()
-    controller.register_sub_controller("another_child", sub_controller)
-    sub_controller.register_sub_controller("ChildChild", ChildChildController())
+    controller.child2 = sub_controller
+    sub_controller.child_child = ChildChildController()
+
     sub_controller = ChildController()
-    controller.register_sub_controller("AdditionalChild", sub_controller)
-    sub_controller.register_sub_controller("ChildChild", ChildChildController())
+    controller.another_child = sub_controller
+    sub_controller.child_child = ChildChildController()
+
     sub_controller = ChildController()
-    controller.register_sub_controller("child_attribute_same_name", sub_controller)
+    controller.additional_child = sub_controller
+    sub_controller.child_child = ChildChildController()
 
     pv_prefix = str(uuid4())
     fastcs = make_fastcs(pv_prefix, controller)
@@ -359,10 +360,6 @@ class SomeController(Controller):
                         "v2": f"{pv_prefix}:Child2:PVI",
                     }
                 },
-                "child_attribute_same_name": {
-                    "d": f"{pv_prefix}:ChildAttributeSameName:PVI",
-                    "r": f"{pv_prefix}:ChildAttributeSameName",
-                },
             },
         }
         assert len(child_controller_pvi) == 1

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`