Tidy BaseController API

GDYendell · GDYendell · commit 284b2e5e97b0 · 2025-11-14T14:20:47.000Z
Remove direct access to `attributes` in preference for `add_attribute`
Improve docstrings
Change method ordering
diff --git a/docs/snippets/dynamic.py b/docs/snippets/dynamic.py
@@ -99,7 +99,8 @@ def __init__(
         super().__init__(f"Ramp{index}", ios=[io])
 
     async def initialise(self):
-        self.attributes.update(create_attributes(self._parameters))
+        for name, attribute in create_attributes(self._parameters).items():
+            self.add_attribute(name, attribute)
 
 
 class TemperatureController(Controller):
@@ -119,7 +120,9 @@ async def initialise(self):
         api = json.loads((await self._connection.send_query("API?\r\n")).strip("\r\n"))
 
         ramps_api = api.pop("Ramps")
-        self.attributes.update(create_attributes(api))
+
+        for name, attribute in create_attributes(api).items():
+            self.add_attribute(name, attribute)
 
         for idx, ramp_parameters in enumerate(ramps_api):
             ramp_controller = TemperatureRampController(
diff --git a/src/fastcs/control_system.py b/src/fastcs/control_system.py
@@ -84,8 +84,7 @@ def _stop_scan_tasks(self):
 
     async def serve(self, interactive: bool = True) -> None:
         await self._controller.initialise()
-        self._controller.validate_hinted_attributes()
-        self._controller.connect_attribute_ios()
+        self._controller.post_initialise()
 
         self.controller_api = build_controller_api(self._controller)
         self._scan_coros, self._initial_coros = (
diff --git a/src/fastcs/controller.py b/src/fastcs/controller.py
@@ -13,12 +13,20 @@
 
 
 class BaseController(Tracer):
-    """Base class for controller."""
+    """Base class for controllers
 
-    #: Attributes passed from the device at runtime.
-    attributes: dict[str, Attribute]
-    root_attribute: Attribute | None = None
+    Instances of this class can be loaded into FastCS to expose its Attributes to
+    the transport layer, which can then perform a specific function such as generating a
+    UI or creating parameters for a control system.
+
+    This class is public for type hinting purposes, but should not be inherited to
+    implement device drivers. Use either ``Controller`` or ``ControllerVector`` instead.
+
+    """
 
+    # These class attributes can be overridden on child classes to define default
+    # behaviour of instantiated controllers
+    root_attribute: Attribute | None = None
     description: str | None = None
 
     def __init__(
@@ -29,17 +37,17 @@ def __init__(
     ) -> None:
         super().__init__()
 
-        if (
-            description is not None
-        ):  # Use the argument over the one class defined description.
+        if description is not None:
+            # Use the argument over the one class defined description.
             self.description = description
 
-        if not hasattr(self, "attributes"):
-            self.attributes = {}
         self._path: list[str] = path or []
-        self.__sub_controller_tree: dict[str, BaseController] = {}
 
+        # Internal state that should not be accessed directly by base classes
+        self.__attributes: dict[str, Attribute] = {}
+        self.__sub_controllers: dict[str, BaseController] = {}
         self.__hinted_attributes = self._parse_attribute_type_hints()
+
         self._bind_attrs()
 
         ios = ios or []
@@ -62,11 +70,73 @@ def _parse_attribute_type_hints(
 
         return hinted_attributes
 
+    def _bind_attrs(self) -> None:
+        """Search for Attributes and Methods to bind them to this instance.
+
+        This method will search the attributes of this controller class to bind them to
+        this specific instance. For Attributes, this is just a case of copying and
+        re-assigning to ``self`` to make it unique across multiple instances of this
+        controller class. For Methods, this requires creating a bound method from a
+        class method and a controller instance, so that it can be called from any
+        context with the controller instance passed as the ``self`` argument.
+
+        """
+        # Lazy import to avoid circular references
+        from fastcs.cs_methods import UnboundCommand, UnboundScan
+
+        # Using a dictionary instead of a set to maintain order.
+        class_dir = {key: None for key in dir(type(self)) if not key.startswith("_")}
+        class_type_hints = {
+            key: value
+            for key, value in get_type_hints(type(self)).items()
+            if not key.startswith("_")
+        }
+
+        for attr_name in {**class_dir, **class_type_hints}:
+            if attr_name == "root_attribute":
+                continue
+
+            attr = getattr(self, attr_name, None)
+            if isinstance(attr, Attribute):
+                setattr(self, attr_name, deepcopy(attr))
+            elif isinstance(attr, UnboundScan | UnboundCommand):
+                setattr(self, attr_name, attr.bind(self))
+
+    def _validate_io(self, ios: Sequence[AttributeIO[T, AttributeIORefT]]):
+        """Validate that there is exactly one AttributeIO class registered to the
+        controller for each type of AttributeIORef belonging to the attributes of the
+        controller"""
+        for ref_type, count in Counter([io.ref_type for io in ios]).items():
+            if count > 1:
+                raise RuntimeError(
+                    f"More than one AttributeIO class handles {ref_type.__name__}"
+                )
+
+    def __repr__(self):
+        name = self.__class__.__name__
+        path = ".".join(self.path) or None
+        sub_controllers = list(self.sub_controllers.keys()) or None
+
+        return f"{name}(path={path}, sub_controllers={sub_controllers})"
+
+    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)
+
     async def initialise(self):
-        """Hook to dynamically add attributes before building the API"""
+        """Hook for subclasses to dynamically add attributes before building the API"""
         pass
 
-    def validate_hinted_attributes(self):
+    def post_initialise(self):
+        """Hook to call after all attributes added, before serving the application"""
+        self._validate_hinted_attributes()
+        self._connect_attribute_ios()
+
+    def _validate_hinted_attributes(self):
         """Validate ``Attribute`` type-hints were introspected during initialisation"""
         for name in self.__hinted_attributes:
             attr = getattr(self, name, None)
@@ -77,11 +147,11 @@ def validate_hinted_attributes(self):
                 )
 
         for subcontroller in self.sub_controllers.values():
-            subcontroller.validate_hinted_attributes()  # noqa: SLF001
+            subcontroller._validate_hinted_attributes()  # noqa: SLF001
 
-    def connect_attribute_ios(self) -> None:
+    def _connect_attribute_ios(self) -> None:
         """Connect ``Attribute`` callbacks to ``AttributeIO``s"""
-        for attr in self.attributes.values():
+        for attr in self.__attributes.values():
             ref = attr.io_ref if attr.has_io_ref() else None
             if ref is None:
                 continue
@@ -99,7 +169,7 @@ def connect_attribute_ios(self) -> None:
                 attr.set_update_callback(io.update)
 
         for controller in self.sub_controllers.values():
-            controller.connect_attribute_ios()
+            controller._connect_attribute_ios()  # noqa: SLF001
 
     @property
     def path(self) -> list[str]:
@@ -111,57 +181,15 @@ def set_path(self, path: list[str]):
             raise ValueError(f"sub controller is already registered under {self.path}")
 
         self._path = path
-        for attribute in self.attributes.values():
+        for attribute in self.__attributes.values():
             attribute.set_path(path)
 
-    def _bind_attrs(self) -> None:
-        """Search for `Attributes` and `Methods` to bind them to this instance.
-
-        This method will search the attributes of this controller class to bind them to
-        this specific instance. For `Attribute`s, this is just a case of copying and
-        re-assigning to `self` to make it unique across multiple instances of this
-        controller class. For `Method`s, this requires creating a bound method from a
-        class method and a controller instance, so that it can be called from any
-        context with the controller instance passed as the `self` argument.
-
-        """
-        # Lazy import to avoid circular references
-        from fastcs.cs_methods import UnboundCommand, UnboundScan
-
-        # Using a dictionary instead of a set to maintain order.
-        class_dir = {key: None for key in dir(type(self)) if not key.startswith("_")}
-        class_type_hints = {
-            key: value
-            for key, value in get_type_hints(type(self)).items()
-            if not key.startswith("_")
-        }
-
-        for attr_name in {**class_dir, **class_type_hints}:
-            if attr_name == "root_attribute":
-                continue
-
-            attr = getattr(self, attr_name, None)
-            if isinstance(attr, Attribute):
-                setattr(self, attr_name, deepcopy(attr))
-            elif isinstance(attr, UnboundScan | UnboundCommand):
-                setattr(self, attr_name, attr.bind(self))
-
-    def _validate_io(self, ios: Sequence[AttributeIO[T, AttributeIORefT]]):
-        """Validate that there is exactly one AttributeIO class registered to the
-        controller for each type of AttributeIORef belonging to the attributes of the
-        controller"""
-        for ref_type, count in Counter([io.ref_type for io in ios]).items():
-            if count > 1:
-                raise RuntimeError(
-                    f"More than one AttributeIO class handles {ref_type.__name__}"
-                )
-
     def add_attribute(self, name, attr: Attribute):
-        if name in self.attributes:
+        if name in self.__attributes:
             raise ValueError(
                 f"Cannot add attribute {attr}. "
                 f"Controller {self} has has existing attribute {name}: "
-                f"{self.attributes[name]}"
+                f"{self.__attributes[name]}"
             )
         elif name in self.__hinted_attributes:
             attr_class, attr_dtype = self.__hinted_attributes[name]
@@ -178,67 +206,50 @@ def add_attribute(self, name, attr: Attribute):
                     f"Expected '{attr_dtype.__name__}', "
                     f"got '{attr.datatype.dtype.__name__}'."
                 )
-        elif name in self.__sub_controller_tree.keys():
+        elif name in self.__sub_controllers.keys():
             raise ValueError(
                 f"Cannot add attribute {attr}. "
                 f"Controller {self} has existing sub controller {name}: "
-                f"{self.__sub_controller_tree[name]}"
+                f"{self.__sub_controllers[name]}"
             )
 
         attr.set_name(name)
         attr.set_path(self.path)
-        self.attributes[name] = attr
+        self.__attributes[name] = attr
         super().__setattr__(name, attr)
 
+    @property
+    def attributes(self) -> dict[str, Attribute]:
+        return self.__attributes
+
     def add_sub_controller(self, name: str, sub_controller: BaseController):
-        if name in self.__sub_controller_tree.keys():
+        if name in self.__sub_controllers.keys():
             raise ValueError(
                 f"Cannot add sub controller {sub_controller}. "
                 f"Controller {self} has existing sub controller {name}: "
-                f"{self.__sub_controller_tree[name]}"
+                f"{self.__sub_controllers[name]}"
             )
-        elif name in self.attributes:
+        elif name in self.__attributes:
             raise ValueError(
                 f"Cannot add sub controller {sub_controller}. "
                 f"Controller {self} has existing attribute {name}: "
-                f"{self.attributes[name]}"
+                f"{self.__attributes[name]}"
             )
 
         sub_controller.set_path(self.path + [name])
-        self.__sub_controller_tree[name] = sub_controller
+        self.__sub_controllers[name] = sub_controller
         super().__setattr__(name, sub_controller)
 
         if isinstance(sub_controller.root_attribute, Attribute):
-            self.attributes[name] = sub_controller.root_attribute
+            self.__attributes[name] = sub_controller.root_attribute
 
     @property
     def sub_controllers(self) -> dict[str, BaseController]:
-        return self.__sub_controller_tree
-
-    def __repr__(self):
-        name = self.__class__.__name__
-        path = ".".join(self.path) or None
-        sub_controllers = list(self.sub_controllers.keys()) or None
-
-        return f"{name}(path={path}, sub_controllers={sub_controllers})"
-
-    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)
+        return self.__sub_controllers
 
 
 class Controller(BaseController):
-    """Top-level controller for a device.
-
-    This is the primary class for implementing device support in FastCS. Instances of
-    this class can be loaded into a FastCS to expose its ``Attribute``s to the transport
-    layer, which can then perform a specific function with the set of ``Attributes``,
-    such as generating a UI or creating parameters for a control system.
-    """
+    """Controller containing Attributes and named sub Controllers"""
 
     def __init__(
         self,
@@ -263,8 +274,12 @@ async def disconnect(self) -> None:
 
 
 class ControllerVector(MutableMapping[int, Controller], BaseController):
-    """A controller with a collection of identical sub controllers distinguished
-    by a numeric value"""
+    """Controller containing Attributes and indexed sub Controllers
+
+    The sub controllers registered with this Controller should be instances of the same
+    Controller type, distinguished only by an integer index. The indexes do not need
+    to be continiguous.
+    """
 
     def __init__(
         self,
diff --git a/tests/test_attribute.py b/tests/test_attribute.py
@@ -76,10 +76,10 @@ class MissingIOController(Controller):
 
     with pytest.raises(ValueError, match="does not have an AttributeIO to handle"):
         controller = MissingIOController()
-        controller.connect_attribute_ios()
+        controller._connect_attribute_ios()
 
     await c.initialise()
-    c.connect_attribute_ios()
+    c._connect_attribute_ios()
     await c.my_attr.bind_update_callback()()
 
 
@@ -218,7 +218,7 @@ async def initialise(self):
 
     c = DemoParameterController(ios=[DemoParameterAttributeIO()])
     await c.initialise()
-    c.connect_attribute_ios()
+    c._connect_attribute_ios()
     await c.ro_int_parameter.bind_update_callback()()
     assert c.ro_int_parameter.get() == 10
     await c.ro_int_parameter.bind_update_callback()()
@@ -239,7 +239,7 @@ class MyController(Controller):
         match="MyController does not have an AttributeIO to handle AttributeIORef",
     ):
         c = MyController()
-        c.connect_attribute_ios()
+        c._connect_attribute_ios()
 
     class SimpleAttributeIO(AttributeIO[T, AttributeIORef]):
         async def update(self, attr):
@@ -259,7 +259,7 @@ async def update(self, attr):
     assert c.base_class_ref.has_io_ref()
 
     await c.initialise()
-    c.connect_attribute_ios()
+    c._connect_attribute_ios()
 
     # There is a difference between providing an AttributeIO for the default
     # AttributeIORef class and not specifying the io_ref for an Attribute
@@ -278,7 +278,7 @@ async def update(self, attr):
     c2 = MyController(ios=[SimpleAttributeIO()])
 
     await c2.initialise()
-    c2.connect_attribute_ios()
+    c2._connect_attribute_ios()
 
     assert c2.base_class_ref.get() == 0
     await c2.base_class_ref.bind_update_callback()()
diff --git a/tests/test_control_system.py b/tests/test_control_system.py
@@ -36,7 +36,7 @@ class MyTestController(Controller):
         def __init__(self):
             super().__init__(description="Controller for testing")
 
-            self.attributes["attr2"] = AttrRW(Int())
+            self.attr2 = AttrRW(Int())
 
         @command()
         async def do_nothing(self):
diff --git a/tests/test_controller.py b/tests/test_controller.py
