diff --git a/docs/conf.py b/docs/conf.py index eac4284a3..2b758cfea 100644 --- 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 index a66d04b08..63714a71b 100644 --- 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 index fcd24e2d6..59014e0c9 100644 --- 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 index 2387025d0..78c537bf3 100644 --- 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 index 130e34cea..841a5b96a 100644 --- 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 index 7c3ae78df..370207b45 100644 --- 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 index a31a50584..ac0086f28 100644 --- 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 index 653745185..9e1fb1c7c 100755 --- 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 index 28098787c..f95a6426e 100755 --- 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 index 45bfc96dc..40e68044d 100644 --- 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,9 +224,7 @@ 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") @@ -234,14 +232,15 @@ 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 + 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" @@ -249,7 +248,7 @@ def _create_and_link_command_pvs( 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 index d17e6083b..7b8b0ef66 100644 --- 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 new file mode 100644 index 000000000..431afe4b8 --- /dev/null +++ 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 index d42c91559..0f5e3a12c 100644 --- 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 index de53fb9ad..9cd084b50 100644 --- 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 index 85e8ca9f8..8b38b1e93 100644 --- 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 index d77bd1bc2..6dceddd6c 100644 --- a/tests/test_controller.py +++ b/tests/test_controller.py @@ -2,7 +2,7 @@ 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(): @@ -10,23 +10,19 @@ def test_controller_nesting(): 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): @@ -39,22 +35,19 @@ def __init__(self): class SomeController(Controller): - annotated_attr: AttrR annotated_attr_not_defined_in_init: AttrR[int] equal_attr = AttrR(Int()) annotated_and_equal_attr: AttrR[int] = AttrR(Int()) def __init__(self, sub_controller: Controller): - self.attributes = {} + super().__init__() - self.annotated_attr = AttrR(Int()) self.attr_on_object = AttrR(Int()) self.attributes["_attributes_attr"] = AttrR(Int()) self.attributes["_attributes_attr_equal"] = self.equal_attr - super().__init__() - self.register_sub_controller("sub_controller", sub_controller) + self.sub_controller = sub_controller def test_attribute_parsing(): @@ -63,7 +56,7 @@ def test_attribute_parsing(): assert set(controller.attributes.keys()) == { "_attributes_attr", - "annotated_attr", + "attr_on_object", "_attributes_attr_equal", "annotated_and_equal_attr", "equal_attr", @@ -81,34 +74,30 @@ def test_attribute_parsing(): } -def test_attribute_in_both_class_and_get_attributes(): - class FailingController(Controller): - duplicate_attribute = AttrR(Int()) +def test_conflicting_attributes_and_controllers(): + class ConflictingController(Controller): + attr = AttrR(Int()) def __init__(self): - self.attributes = {"duplicate_attribute": AttrR(Int())} super().__init__() + self.sub_controller = Controller() + + controller = ConflictingController() + + with pytest.raises(ValueError, match=r"Cannot add attribute .* existing attribute"): + controller.attr = AttrR(Float()) # pyright: ignore[reportAttributeAccessIssue] with pytest.raises( - ValueError, - match=( - "`FailingController` has conflicting attribute `duplicate_attribute` " - "already present in the attributes dict." - ), + ValueError, match=r"Cannot add sub controller .* existing attribute" ): - FailingController() - + controller.attr = Controller() # 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 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 index 5d470a7d8..f7c338321 100644 --- 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