From e5a938087d87a6c9f206916e5261b3768737dbf2 Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Wed, 18 Feb 2026 16:15:24 +0100 Subject: [PATCH 1/4] fix ignored props issue and renam internal atrrs to avoid conflicts with signal names --- src/secop_ophyd/SECoPDevices.py | 103 +++++++++++++++++--------------- tests/conftest.py | 2 +- 2 files changed, 55 insertions(+), 50 deletions(-) diff --git a/src/secop_ophyd/SECoPDevices.py b/src/secop_ophyd/SECoPDevices.py index d404ca3..785221f 100644 --- a/src/secop_ophyd/SECoPDevices.py +++ b/src/secop_ophyd/SECoPDevices.py @@ -205,10 +205,10 @@ def __init__( else: raise RuntimeError(f"Invalid SECoP resource identifier: {sri}") - if SECoPDevice.clients.get(self.node_id) is None: + if SECoPDevice._clients.get(self.node_id) is None: raise RuntimeError(f"No AsyncFrappyClient for URI {sri} exists") - self.client: AsyncFrappyClient = SECoPDevice.clients[self.node_id] + self.client: AsyncFrappyClient = SECoPDevice._clients[self.node_id] def set_module(self, module_name: str): if self.sri.count(":") != 1: @@ -302,8 +302,11 @@ async def connect_real(self, device: Device, timeout: float, force_reconnect: bo child for child in module_property_dict.keys() if child not in self.filler.ignored_signals + and child not in IGNORED_PROPS ] + print(module_properties) + for mod_property_name in module_properties: if self._auto_fill_signals or mod_property_name in not_filled: @@ -532,16 +535,16 @@ def collect(self) -> Iterator[PartialEvent]: class SECoPDevice(StandardReadable): - clients: Dict[str, AsyncFrappyClient] = {} + _clients: Dict[str, AsyncFrappyClient] = {} - node_id: str - sri: str - host: str - port: str - module: str | None - mod_prop_devices: Dict[str, SignalR] - param_devices: Dict[str, Any] - logger: Logger + _node_id: str + _sri: str + _host: str + _port: str + _module: str | None + _mod_prop_devices: Dict[str, SignalR] + _param_devices: Dict[str, Any] + _logger: Logger hinted_signals: list[str] = [] @@ -562,40 +565,40 @@ def __init__( loglevel = connector.loglevel logdir = connector.logdir - self.sri = sri - self.host = sri.split(":")[0] - self.port = sri.split(":")[1] - self.mod_prop_devices = {} - self.param_devices = {} - self.node_id = sri.split(":")[0] + ":" + sri.split(":")[1] + self._sri = sri + self._host = sri.split(":")[0] + self._port = sri.split(":")[1] + self._mod_prop_devices = {} + self._param_devices = {} + self._node_id = sri.split(":")[0] + ":" + sri.split(":")[1] - self.logger = setup_logging( - name=f"frappy:{self.host}:{self.port}", + self._logger = setup_logging( + name=f"frappy:{self._host}:{self._port}", level=loglevel, log_dir=logdir, ) - self.module = None + self._module = None if len(sri.split(":")) > 2: - self.module = sri.split(":")[2] + self._module = sri.split(":")[2] - if SECoPDevice.clients.get(self.node_id) is None: - SECoPDevice.clients[self.node_id] = AsyncFrappyClient( - host=self.host, port=self.port, log=self.logger + if SECoPDevice._clients.get(self._node_id) is None: + SECoPDevice._clients[self._node_id] = AsyncFrappyClient( + host=self._host, port=self._port, log=self._logger ) connector = connector or SECoPDeviceConnector(sri=sri) - self._client: AsyncFrappyClient = SECoPDevice.clients[self.node_id] + self._client: AsyncFrappyClient = SECoPDevice._clients[self._node_id] super().__init__(name=name, connector=connector) def set_module(self, module_name: str): - if self.module is not None: + if self._module is not None: raise RuntimeError("Module can only be set if it was not already set") - self.module = module_name - self.sri = self.sri + ":" + module_name + self._module = module_name + self._sri = self._sri + ":" + module_name self._connector.set_module(module_name) @@ -609,13 +612,13 @@ async def connect( # Establish connection to SEC Node await self._client.connect(3) - if self.module: - module_desc = self._client.modules[self.module] + if self._module: + module_desc = self._client.modules[self._module] # Initialize Command Devices for command, _ in module_desc["commands"].items(): # generate new root path - cmd_path = Path(parameter_name=command, module_name=self.module) + cmd_path = Path(parameter_name=command, module_name=self._module) cmd_dev_name = command + "_CMD" setattr( self, @@ -638,11 +641,11 @@ async def connect( await super().connect(mock, timeout, force_reconnect) - if self.module is None: + if self._module is None: # set device name from equipment id property self.set_name(self._client.properties[EQUIPMENT_ID].replace(".", "-")) else: - self.set_name(self.module) + self.set_name(self._module) def generate_cmd_plan( self, @@ -845,7 +848,7 @@ def class_from_instance(self, path_to_module: str | None = None): description = self._client.client.request("describe")[2] # parse genClass file if already present - genCode = GenNodeCode(path=path_to_module, log=self.logger) + genCode = GenNodeCode(path=path_to_module, log=self._logger) genCode.from_json_describe(description) @@ -938,10 +941,10 @@ async def wait_for_idle(self): for running commands that are not done immediately """ - self.logger.info(f"Waiting for {self.name} to be IDLE") + self._logger.info(f"Waiting for {self.name} to be IDLE") if self.status is None: - self.logger.error("Status Signal not initialized") + self._logger.error("Status Signal not initialized") raise Exception("status Signal not initialized") # force reading of fresh status from device @@ -955,7 +958,7 @@ async def wait_for_idle(self): # Module is in IDLE/WARN state if IDLE <= stat_code < BUSY: - self.logger.info(f"Module {self.name} --> IDLE") + self._logger.info(f"Module {self.name} --> IDLE") break if hasattr(self, "_stopped"): @@ -966,7 +969,7 @@ async def wait_for_idle(self): # Error State or DISABLED if hasattr(self, "_success"): if stat_code >= ERROR or stat_code < IDLE: - self.logger.error(f"Module {self.name} --> ERROR/DISABLED") + self._logger.error(f"Module {self.name} --> ERROR/DISABLED") self._success = False break @@ -988,10 +991,10 @@ def switch_from_status_factory(): yield from bps.wait_for([switch_from_status_factory]) def trigger(self) -> AsyncStatus: - self.logger.info(f"Triggering {self.name}: read fresh data from device") + self._logger.info(f"Triggering {self.name}: read fresh data from device") # get fresh reading of the value Parameter from the SEC Node return AsyncStatus( - awaitable=self._client.get_parameter(self.module, "value", trycache=False) + awaitable=self._client.get_parameter(self._module, "value", trycache=False) ) def subscribe(self, function: Callback[dict[str, Reading]]) -> None: @@ -1106,7 +1109,7 @@ async def _move(self, new_target): self._stopped = False await self.target.set(new_target) - self.logger.info(f"Moving {self.name} to {new_target}") + self._logger.info(f"Moving {self.name} to {new_target}") # force reading of status from device await self.status.read(False) @@ -1116,26 +1119,28 @@ async def _move(self, new_target): stat_code = current_stat["f0"] if self._stopped is True: - self.logger.info( + self._logger.info( f"Move of {self.name} to {new_target} was stopped STOPPED" ) break # Error State or DISABLED if stat_code >= ERROR or stat_code < IDLE: - self.logger.error(f"Module {self.name} --> ERROR/DISABLED") + self._logger.error(f"Module {self.name} --> ERROR/DISABLED") self._success = False break # Module is in IDLE/WARN state if IDLE <= stat_code < BUSY: - self.logger.info(f"Reached Target Module {self.name} --> IDLE") + self._logger.info(f"Reached Target Module {self.name} --> IDLE") break # TODO other status transitions if not self._success: - self.logger.error(f"Move of {self.name} to {new_target} was not successful") + self._logger.error( + f"Move of {self.name} to {new_target} was not successful" + ) async def stop(self, success=True): """Calls stop command on the SEC Node module @@ -1149,15 +1154,15 @@ async def stop(self, success=True): self._success = success if not success: - self.logger.info(f"Stopping {self.name} success={success}") - await self._client.exec_command(self.module, "stop") + self._logger.info(f"Stopping {self.name} success={success}") + await self._client.exec_command(self._module, "stop") self._stopped = True async def locate(self) -> Location: # return current location of the device (setpoint and readback). # Only locally cached values are returned - setpoint = await self._client.get_parameter(self.module, "target", True) - readback = await self._client.get_parameter(self.module, "value", True) + setpoint = await self._client.get_parameter(self._module, "target", True) + readback = await self._client.get_parameter(self._module, "value", True) location: Location = { "setpoint": setpoint.value, diff --git a/tests/conftest.py b/tests/conftest.py index baf9027..0f7641c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -27,7 +27,7 @@ def cleanup_secop_clients(): # After each test, clear the cached clients from secop_ophyd.SECoPDevices import SECoPDevice - SECoPDevice.clients.clear() + SECoPDevice._clients.clear() @pytest.fixture From 8a27472a0114f28474e3683b41a2bc0f401b3c4e Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Wed, 18 Feb 2026 16:23:21 +0100 Subject: [PATCH 2/4] remove print --- src/secop_ophyd/SECoPDevices.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/secop_ophyd/SECoPDevices.py b/src/secop_ophyd/SECoPDevices.py index 785221f..4a1bbcd 100644 --- a/src/secop_ophyd/SECoPDevices.py +++ b/src/secop_ophyd/SECoPDevices.py @@ -305,8 +305,6 @@ async def connect_real(self, device: Device, timeout: float, force_reconnect: bo and child not in IGNORED_PROPS ] - print(module_properties) - for mod_property_name in module_properties: if self._auto_fill_signals or mod_property_name in not_filled: From 2b8a61a49e31f6f6c660e3c48dc0ac70df12adce Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Wed, 18 Feb 2026 16:38:19 +0100 Subject: [PATCH 3/4] moved from abstract methods to planstubs --- src/secop_ophyd/GenNodeCode.py | 3 +- .../templates/generated_classes.py.jinja2 | 5 ++- tests/test_classgen.py | 35 +++++++++++++++++-- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/secop_ophyd/GenNodeCode.py b/src/secop_ophyd/GenNodeCode.py index fdaef66..831c4d0 100644 --- a/src/secop_ophyd/GenNodeCode.py +++ b/src/secop_ophyd/GenNodeCode.py @@ -190,8 +190,7 @@ def __init__(self, path: str | None = None, log=None): self.inline_comment_threshold: int = 120 self.comment_wrap_width: int = 100 - # Required imports for abstract classes - self.add_import("abc", "abstractmethod") + # Required imports for generated classes self.add_import("typing", "Annotated as A") self.add_import("ophyd_async.core", "SignalR") self.add_import("ophyd_async.core", "SignalRW") diff --git a/src/secop_ophyd/templates/generated_classes.py.jinja2 b/src/secop_ophyd/templates/generated_classes.py.jinja2 index 9911c92..100045d 100644 --- a/src/secop_ophyd/templates/generated_classes.py.jinja2 +++ b/src/secop_ophyd/templates/generated_classes.py.jinja2 @@ -66,9 +66,12 @@ class {{ module_cls.name }}({{ module_cls.bases | join(', ') }}): # SECoP Commands wrapped as Bluesky Plans: {%- for method in module_cls.methods %} - @abstractmethod def {{ method.name }}{{ method.signature }}: """{{ method.description }}""" + + raise RuntimeError( + "Command plan not bound yet. Call connect()/init_devices() before using commands." + ) {%- endfor %} {%- endif %} diff --git a/tests/test_classgen.py b/tests/test_classgen.py index 0e8d77b..7086401 100644 --- a/tests/test_classgen.py +++ b/tests/test_classgen.py @@ -1,5 +1,6 @@ """Simple test to verify GenNodeCode refactoring works.""" +import inspect import sys from pathlib import Path @@ -39,6 +40,35 @@ def test_extract_descriptions_from_source_is_token_safe(): assert "local" not in descriptions +def test_generated_command_methods_are_concrete(tmp_path: Path): + """Generated command wrappers must be concrete so devices are instantiable.""" + gen_code = GenNodeCode(path=str(tmp_path), log=None) + + def command_plan_no_arg(self, wait_for_idle: bool = False): + pass + + gen_code.add_mod_class( + module_cls="CommandTestModule", + bases=["SECoPReadableDevice"], + parameters=[], + properties=[], + cmd_plans=[ + Method( + cmd_name="factory_reset", + description="Reset module", + cmd_sign=inspect.signature(command_plan_no_arg), + ) + ], + description="test module", + ) + + generated_code = gen_code.generate_code() + + assert "def factory_reset" in generated_code + assert "@abstractmethod" not in generated_code + assert "raise RuntimeError(" in generated_code + + def test_basic_functionality(clean_generated_file): """Test basic GenNodeCode functionality.""" print("Testing GenNodeCode refactored implementation...") @@ -122,7 +152,6 @@ def sample_method(self, value: int) -> str: print("=" * 60) # Verify code contains expected elements - assert "from abc import abstractmethod" in code assert "class TestModule(SECoPDevice):" in code assert "temperature: A[SignalR[float], ParamT()]" in code assert "count: A[SignalRW[int], ParamT()]" in code @@ -554,8 +583,8 @@ async def test_gen_real_node( # The ophy_struct module has a test_cmd command assert "def test_cmd" in generated_code, "test_cmd plan should be generated" assert ( - "@abstractmethod" in generated_code - ), "Command methods should be marked as abstract" + "@abstractmethod" not in generated_code + ), "Command methods should be concrete so generated classes are instantiable" # ===== Assertions for generated enum classes ===== # Enum classes should be generated for enum parameters From 4d12b3578a5620d5d2c9904491bd1e31fa8d79e5 Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Wed, 18 Feb 2026 17:02:43 +0100 Subject: [PATCH 4/4] moved codegen dependencies from dev to main --- pyproject.toml | 8 ++++---- uv.lock | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index eb53838..29a6de1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,10 @@ description = "An Interface between bluesky and SECoP, using ophyd and frappy-cl dependencies = [ 'ophyd-async >= 0.10.0', - 'frappy-core == 0.20.4' + 'frappy-core == 0.20.4', + 'black >= 23.0.0', + 'Jinja2 >= 3.1.6', + 'autoflake >= 2.3.2', ] @@ -24,9 +27,6 @@ dev = [ 'numpy', 'isort', 'pytest', - 'black', - 'jinja2', - 'autoflake', 'pep8-naming', 'mypy', 'wheel', diff --git a/uv.lock b/uv.lock index 0945cfe..0ab2b19 100644 --- a/uv.lock +++ b/uv.lock @@ -2561,19 +2561,19 @@ wheels = [ name = "secop-ophyd" source = { editable = "." } dependencies = [ + { name = "autoflake" }, + { name = "black" }, { name = "frappy-core" }, + { name = "jinja2" }, { name = "ophyd-async" }, ] [package.dev-dependencies] dev = [ - { name = "autoflake" }, - { name = "black" }, { name = "bluesky" }, { name = "cycler" }, { name = "ipykernel" }, { name = "isort" }, - { name = "jinja2" }, { name = "matplotlib" }, { name = "mlzlog" }, { name = "mypy" }, @@ -2602,19 +2602,19 @@ dev = [ [package.metadata] requires-dist = [ + { name = "autoflake", specifier = ">=2.3.2" }, + { name = "black", specifier = ">=23.0.0" }, { name = "frappy-core", specifier = "==0.20.4" }, + { name = "jinja2", specifier = ">=3.1.6" }, { name = "ophyd-async", specifier = ">=0.10.0" }, ] [package.metadata.requires-dev] dev = [ - { name = "autoflake" }, - { name = "black" }, { name = "bluesky" }, { name = "cycler" }, { name = "ipykernel" }, { name = "isort" }, - { name = "jinja2" }, { name = "matplotlib" }, { name = "mlzlog" }, { name = "mypy" },