Fix sig name conflict

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',

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")

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,6 +302,7 @@ 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
             ]
 
             for mod_property_name in module_properties:
@@ -532,16 +533,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 +563,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 +610,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 +639,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 +846,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 +939,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 +956,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 +967,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 +989,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 +1107,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 +1117,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 +1152,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,

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 %}
 

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

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