diff --git a/src/secop_ophyd/GenNodeCode.py b/src/secop_ophyd/GenNodeCode.py index 1b409fc..2238c23 100644 --- a/src/secop_ophyd/GenNodeCode.py +++ b/src/secop_ophyd/GenNodeCode.py @@ -11,20 +11,25 @@ from frappy.client import get_datatype from frappy.datatypes import CommandType, DataType, EnumType, StructOf from jinja2 import Environment, PackageLoader, select_autoescape -from ophyd_async.core import SignalR, SignalRW +from ophyd_async.core import SignalR, SignalRW, SignalW from ophyd_async.core import StandardReadableFormat as Format from secop_ophyd.SECoPDevices import ( IGNORED_PROPS, + ParameterMemberType, ParameterType, PropertyType, + SECoPMoveableDevice, class_from_interface, ) from secop_ophyd.SECoPSignal import secop_dtype_obj_from_json from secop_ophyd.util import ( + CompositeKind, SECoPdtype, build_command_signature, + classify_datatype, command_dtype_to_annotation_str, + get_composite_members, secop_enum_name_to_python, ) @@ -222,6 +227,7 @@ def __init__(self, path: str | None = None, log=None): self.add_import("typing", "Annotated as A") self.add_import("ophyd_async.core", "SignalR") self.add_import("ophyd_async.core", "SignalRW") + self.add_import("ophyd_async.core", "SignalW") self.add_import("ophyd_async.core", "Command") self.add_import("ophyd_async.core", "TriggerableCommand") self.add_import("ophyd_async.core", "StandardReadableFormat as Format") @@ -230,6 +236,9 @@ def __init__(self, path: str | None = None, log=None): self.add_import("typing", "Any") self.add_import("numpy", "ndarray") self.add_import("secop_ophyd.SECoPDevices", "ParameterType as ParamT") + self.add_import( + "secop_ophyd.SECoPDevices", "ParameterMemberType as ParamMemberT" + ) self.add_import("secop_ophyd.SECoPDevices", "PropertyType as PropT") # Add necessary Device imports self.add_import("secop_ophyd.SECoPDevices", "SECoPDevice") @@ -425,6 +434,13 @@ def from_json_describe(self, json_data: str | dict): secop_ophyd_modclass = class_from_interface(properties) module_bases = [secop_ophyd_modclass.__name__] + # SECoPMoveableDevice is generic over the datatype of 'target' + # (see SECoPDevices.py) -- captured below, from whichever branch + # of the parameter loop resolves 'target', so the generated base + # can be parametrized as e.g. SECoPMoveableDevice[ndarray] instead + # of falling back to the unparametrized (implicitly Any) form. + movable_target_type_param: str | None = None + # Add the module class, use self reported "implementation" module property, # if not present use the module name module_class = modname @@ -535,6 +551,28 @@ def from_json_describe(self, json_data: str | dict): mod_parameters: list[ParameterAttribute] = [] + def _camel(identifier: str) -> str: + words = identifier.replace(" ", "_").replace("-", "_").split("_") + return "".join(word.capitalize() for word in words) + + def _enum_type_param( + type_param: str | None, + enum_class_name: str, + members: dict, + enum_descr: str, + ) -> str | None: + """If `type_param` is the generic 'StrictEnum', generate a + concrete named enum class for it and return that class's name + instead; otherwise return `type_param` unchanged.""" + if not (type_param and "StrictEnum" in type_param): + return type_param + + enum_cls = _build_enum_class(enum_class_name, members, enum_descr) + if enum_cls: + module_enum_classes.append(enum_cls) + return enum_class_name + return type_param + for param_name, param_data in parameters.items(): descr = self._normalize_description(param_data.get("description", "")) @@ -546,7 +584,6 @@ def from_json_describe(self, json_data: str | dict): ) else: param_descr = descr - signal_base = SignalR if param_data["readonly"] else SignalRW format = None @@ -567,40 +604,84 @@ def from_json_describe(self, json_data: str | dict): format = format or Format.HINTED_SIGNAL # Remove "StandardReadable" prefix from format for cleaner annotation - format = ( + format_str = ( str(format).removeprefix("StandardReadable") if format else None ) - datainfo = param_data.get("datainfo", {}) + raw_datatype = param_data["datatype"] + kind = classify_datatype(raw_datatype) + + if kind is CompositeKind.UNSUPPORTED: + # mirrors SECoPDeviceConnector.connect_real(): no signal is + # generated for a struct/tuple nested inside another + # struct/tuple/array. Unlike the runtime path, codegen has + # no notion of "mandatory for this instance's interface + # class" at generation time, so it always just skips -- + # the runtime path is still the one that raises for a + # genuinely mandatory parameter when actually connecting. + continue - # infer the ophyd type from secop datatype - type_param = get_type_param(param_data["datatype"]) - - # Handle StrictEnum types - generate enum class - if type_param and "StrictEnum" in type_param: - # Generate unique enum class name: - # ModuleClass + ParamName + Enum - param_name_list = ( - param_name.replace(" ", "_").replace("-", "_").split("_") - ) + if kind is CompositeKind.DECOMPOSABLE: + for member_key, member_dt in get_composite_members(raw_datatype): + member_type_param = get_type_param(member_dt) + if isinstance(member_dt, EnumType): + # an EnumType member embedded in a struct/tuple + # resolves to its member name string at runtime, + # just like a standalone top-level enum parameter + # -- so it gets its own generated enum class too + member_type_param = _enum_type_param( + member_type_param, + f"{module_class}_{_camel(param_name)}_" + f"{_camel(member_key)}_Enum", + member_dt.export_datatype().get("members", {}), + f"{param_name}.{member_key} enum for " + f"`{module_class}`.", + ) + + mod_parameters.append( + ParameterAttribute( + name=f"{param_name}_{member_key}", + type=SignalR.__name__, + type_param=member_type_param, + description=param_descr, + path_annotation=str(ParameterMemberType()), + format_annotation=format_str, + ) + ) - param_name_camel = "".join( - word.capitalize() for word in param_name_list - ) + if not param_data["readonly"]: + target_type_param = get_type_param(raw_datatype) + mod_parameters.append( + ParameterAttribute( + name=param_name, + type=SignalW.__name__, + type_param=target_type_param, + description=param_descr, + path_annotation=str(ParameterType()), + # a write-only SignalW can't carry a + # StandardReadableFormat (nothing to read) + format_annotation=None, + ) + ) + if param_name == "target": + movable_target_type_param = target_type_param + continue - enum_class_name = f"{module_class}_{param_name_camel}_Enum" + # ATOMIC: single signal, unchanged from previous behaviour + signal_base = SignalR if param_data["readonly"] else SignalRW + datainfo = param_data.get("datainfo", {}) - enum_cls = _build_enum_class( - enum_class_name, - datainfo.get("members", {}), - f"{param_name} enum for `{module_class}`.", - ) - if enum_cls: - module_enum_classes.append(enum_cls) + # infer the ophyd type from secop datatype + type_param = get_type_param(raw_datatype) + type_param = _enum_type_param( + type_param, + f"{module_class}_{_camel(param_name)}_Enum", + datainfo.get("members", {}), + f"{param_name} enum for `{module_class}`.", + ) - # Use the specific enum class name instead of generic - # StrictEnum - type_param = enum_class_name + if param_name == "target": + movable_target_type_param = type_param # Default format for parameters is CONFIG_SIGNAL @@ -611,7 +692,7 @@ def from_json_describe(self, json_data: str | dict): type_param=type_param, description=param_descr, path_annotation=str(ParameterType()), - format_annotation=format, + format_annotation=format_str, ) ) @@ -635,6 +716,19 @@ def from_json_describe(self, json_data: str | dict): ) ) + if ( + secop_ophyd_modclass is SECoPMoveableDevice + and movable_target_type_param + ): + module_bases = [ + ( + f"{base}[{movable_target_type_param}]" + if base == SECoPMoveableDevice.__name__ + else base + ) + for base in module_bases + ] + self.add_mod_class( module_cls=module_class, bases=module_bases, diff --git a/src/secop_ophyd/SECoPDevices.py b/src/secop_ophyd/SECoPDevices.py index d42a736..c891ec6 100644 --- a/src/secop_ophyd/SECoPDevices.py +++ b/src/secop_ophyd/SECoPDevices.py @@ -1,15 +1,17 @@ +import asyncio import logging import re import time as ttime import warnings from abc import abstractmethod +from collections.abc import Sequence from dataclasses import dataclass from functools import cached_property from logging import Logger from typing import Any, Dict -import bluesky.plan_stubs as bps from bluesky.protocols import ( + Location, Reading, Stoppable, Subscribable, @@ -27,8 +29,10 @@ LazyMock, MovableLogic, Signal, + SignalDatatypeT, SignalR, SignalRW, + SignalW, StandardMovable, StandardReadable, StandardReadableFormat, @@ -36,6 +40,7 @@ TriggerableCommand, observe_value, wait_for_value, + walk_devices, ) from ophyd_async.core._utils import Callback @@ -47,7 +52,15 @@ SECoPBackend, SECoPCommandBackend, ) -from secop_ophyd.util import Path +from secop_ophyd.util import ( + MAX_DEPTH, + CompositeKind, + IncompatibleSECoPDatatype, + Path, + SECoPdtype, + classify_datatype, + get_composite_members, +) # Predefined Status Codes DISABLED = 0 @@ -74,11 +87,23 @@ IGNORED_PROPS = ["meaning", "_plotly"] +TILED_MAX_NAME_LENGTH = 63 + def clean_identifier(anystring): return str(re.sub(r"\W+|^(?=\d)", "_", anystring)) +def warn_on_long_device_names(devices: Dict[str, Device]) -> None: + """Warn for every device whose name exceeds the tiled storage limit.""" + for dev in devices.values(): + if len(dev.name) > TILED_MAX_NAME_LENGTH: + warnings.warn( + f"Device name: '{dev.name}' is too long for tiled storage " + f"({len(dev.name)}>{TILED_MAX_NAME_LENGTH} chars)" + ) + + def format_assigned(device: StandardReadable, signal: SignalR) -> bool: if ( signal.describe in device._describe_funcs @@ -125,6 +150,31 @@ def __call__(self, parent: Device, child: Device): backend._secclient = parent._client +class ParameterMemberType: + """Annotation for a split struct/tuple member Signal (one field/index of + a decomposed composite parameter). Distinct from `ParameterType` so + annotation-scanning doesn't mark it AttributeType.PARAMETER -- it must + stay unset until SECoPDeviceConnector.connect_real() calls + init_member_from_introspection(), which needs to see it as MEMBER (or + unset), not PARAMETER.""" + + def __repr__(self) -> str: + """Return repr suitable for code generation in annotations.""" + return "ParamMemberT()" + + def __call__(self, parent: Device, child: Device): + if not isinstance(child, Signal): + return + + backend = child._connector.backend + + if not isinstance(backend, SECoPBackend): + return + + backend.attribute_type = AttributeType.MEMBER + backend._secclient = parent._client + + class PropertyType: """Annotation for Module Property Signals, defines the path to the property""" @@ -231,12 +281,50 @@ async def connect_real(self, device: Device, timeout: float, force_reconnect: bo # Establish connection to SEC Node await self.client.connect(3) + # deferred import to avoid a circular import (GenNodeCode imports + # from this module); imported once here rather than repeated inline + # in each branch below to avoid shadowing across loop iterations + from secop_ophyd.GenNodeCode import get_type_param + # Module Device: fill Parameters & Pproperties # (commands are done via annotated plans) if self.module: # Fill Parmeters parameter_dict = self.client.modules[self.module]["parameters"] + + # StandardMovable's readback machinery (set_name/subscribe_reading/ + # locate) unconditionally requires movable_logic.readback to be a + # working SignalR -- including right now, before any other child + # of this module has been filled below (assigning any child + # triggers a set_name() cascade that resolves movable_logic + # immediately). When 'value' is composite it is fully decomposed + # into split value_* signals below (for a tiled-safe + # read()/describe() stream), with no bare 'value' attribute -- so + # for a Movable module we additionally build+connect a private, + # undecomposed 'value' SignalR purely to satisfy that internal + # plumbing. `object.__setattr__` is used to store it without + # going through Device's own __setattr__, which would otherwise + # treat any Signal-valued attribute as a tracked child (making it + # show up in device.children()/read()/describe() -- exactly what + # decomposing 'value' is meant to avoid). + if ( + isinstance(device, SECoPMoveableDevice) + and "value" in parameter_dict + and classify_datatype(parameter_dict["value"]["datatype"]) + is CompositeKind.DECOMPOSABLE + ): + value_datatype = get_type_param(parameter_dict["value"]["datatype"]) + value_backend = SECoPBackend(None) # type: SECoPBackend + value_backend.init_parameter_from_introspection( + datatype=value_datatype, # type: ignore[arg-type] + path=self.module + ":value", + secclient=self.client, + ) + value_readback = SignalR(value_backend) + await value_readback.connect(timeout=timeout) + object.__setattr__(device, "_composite_value_readback", value_readback) + # remove ignored signals parameters = [ child @@ -247,22 +335,81 @@ async def connect_real(self, device: Device, timeout: float, force_reconnect: bo # Dertermine children that are declared but not yet filled not_filled = {unfilled for unfilled, _ in device.children()} + mandatory_parameters = getattr(device, "mandatory_parameters", []) + for param_name in parameters: - if self._auto_fill_signals or param_name in not_filled: - signal_type = ( - SignalR if parameter_dict[param_name]["readonly"] else SignalRW + raw_datatype = parameter_dict[param_name]["datatype"] + readonly = parameter_dict[param_name]["readonly"] + kind = classify_datatype(raw_datatype) + + if kind is CompositeKind.DECOMPOSABLE: + members = get_composite_members(raw_datatype) + declared = param_name in not_filled or any( + f"{param_name}_{member_key}" in not_filled + for member_key, _ in members ) - - backend = self.filler.fill_child_signal(param_name, signal_type) - - from secop_ophyd.GenNodeCode import get_type_param - - datatype = get_type_param(parameter_dict[param_name]["datatype"]) - backend.init_parameter_from_introspection( - datatype=datatype, - path=self.module + ":" + param_name, - secclient=self.client, + else: + declared = param_name in not_filled + + if not (self._auto_fill_signals or declared): + continue + + if kind is CompositeKind.UNSUPPORTED: + msg = ( + f"Parameter '{param_name}' of module '{self.module}' has a " + f"datatype nesting depth of " + f"{SECoPdtype(raw_datatype).max_depth} (struct/tuple nested " + f"inside a struct/tuple/array). Tiled & Databroker only " + f"support flat struct/tuple parameters (depth <= " + f"{MAX_DEPTH}); no signal will be created for '{param_name}'." ) + if param_name in mandatory_parameters: + raise IncompatibleSECoPDatatype(msg) + warnings.warn(msg) + continue + + if kind is CompositeKind.DECOMPOSABLE: + for member_key, member_dtype in members: + member_backend = self.filler.fill_child_signal( + f"{param_name}_{member_key}", SignalR + ) + member_backend.init_member_from_introspection( + module_name=self.module, + parent_param=param_name, + member_key=member_key, + member_datatype=member_dtype, + secclient=self.client, + ) + + if not readonly: + write_backend = self.filler.fill_child_signal( + param_name, SignalW + ) + + write_datatype = get_type_param(raw_datatype) + write_backend.init_parameter_from_introspection( + datatype=write_datatype, + path=self.module + ":" + param_name, + secclient=self.client, + ) + + if hasattr(device, "_resolved_parameters"): + device._resolved_parameters.add(param_name) + continue + + # ATOMIC: single signal, unchanged from previous behaviour + signal_type = SignalR if readonly else SignalRW + + backend = self.filler.fill_child_signal(param_name, signal_type) + + datatype = get_type_param(raw_datatype) + backend.init_parameter_from_introspection( + datatype=datatype, + path=self.module + ":" + param_name, + secclient=self.client, + ) + if hasattr(device, "_resolved_parameters"): + device._resolved_parameters.add(param_name) # Fill Properties module_property_dict = self.client.modules[self.module]["properties"] @@ -342,7 +489,7 @@ async def connect_real(self, device: Device, timeout: float, force_reconnect: bo mod_dev: SECoPDevice = getattr(device, module_name) mod_dev.set_module(module_name) - # Fill Node properties + # Fill Node propertiesdevice_filler node_property_dict = self.client.properties # remove ignored signals @@ -418,6 +565,7 @@ class SECoPDevice(StandardReadable): _logger: Logger hinted_signals: list[str] = [] + mandatory_parameters: list[str] = [] def __init__( self, @@ -443,6 +591,14 @@ def __init__( self._param_devices = {} self._node_id = sri.split(":")[0] + ":" + sri.split(":")[1] + # raw SECoP parameter names that got at least one Signal built for + # them (split members and/or a monolithic signal); used to tell + # "parameter missing from the SEC node entirely" apart from + # "parameter present but incompatible datatype" (the latter raises + # eagerly in SECoPDeviceConnector.connect_real() for mandatory + # parameters instead) + self._resolved_parameters: set[str] = set() + self._logger = setup_logging( name=f"frappy:{self._host}:{self._port}", level=loglevel, @@ -491,6 +647,8 @@ async def connect( else: self.set_name(self._module) + warn_on_long_device_names(walk_devices(self)) + @abstractmethod async def _assign_interface_formats(self): """Assign signal formats specific to this device's interface class. @@ -517,14 +675,13 @@ def assert_device_is_signalr(device: Device) -> SignalR: if not isinstance(backend, SECoPBackend): continue - param_name = backend.path_str.split(":")[-1] - if param_name == "status": - # status signals should not be assigned a format, - # but a SignalR children (this can be removed once tiled can - # hanlde composite dtypes) + if not isinstance(child, SignalR): + # write-only whole-struct/tuple SignalW: cannot be given a + # read/config format, only its split SignalR members can continue - # child is a Signal with SECoPParamBackend + # child is a Signal with a SECoPBackend (parameter, property, or + # struct/tuple member) # check if signal already has a format assigned signalr_device = assert_device_is_signalr(child) @@ -556,6 +713,45 @@ def assert_device_is_signalr(device: Device) -> SignalR: hinted_uncached_signals, StandardReadableFormat.HINTED_UNCACHED_SIGNAL ) + def _split_signals(self, parent_param: str) -> list[SignalR]: + """Return the split member Signals for a decomposed composite + parameter (empty list if `parent_param` is atomic, i.e. was not + decomposed).""" + result: list[SignalR] = [] + for _, child in self.children(): + if not isinstance(child, SignalR): + continue + backend = child._connector.backend + if ( + isinstance(backend, SECoPBackend) + and backend.attribute_type == AttributeType.MEMBER + and backend._parent_param == parent_param + ): + result.append(child) + return result + + def _assign_hinted_format( + self, param_name: str, signals: Sequence[SignalR] + ) -> None: + """Assign HINTED_SIGNAL to whichever of `signals` (either `[self.value]`/ + `[self.target]` for an atomic parameter, or its `_split_signals(...)` + for a decomposed one) don't already have a format assigned; warn about + any that do but aren't a read format.""" + unassigned = [] + for signal in signals: + if format_assigned(self, signal): + if not is_read_signal(self, signal): + warnings.warn( + f"Signal '{signal.name}' of device {self.name} has format " + f"assigned that is not compatible with {param_name}'s " + "interface class role" + ) + else: + unassigned.append(signal) + + if unassigned: + self.add_readables(unassigned, StandardReadableFormat.HINTED_SIGNAL) + class SECoPNodeDevice(SECoPDevice): @@ -655,6 +851,7 @@ class SECoPReadableDevice(SECoPDevice, Triggerable, Subscribable): """ hinted_signals: list[str] = ["value"] + mandatory_parameters: list[str] = ["value", "status"] def __init__( self, @@ -674,7 +871,9 @@ def __init__( """ self.value: SignalR - self.status: SignalR + # status is SECoP's StatusType == TupleOf(EnumType, StringType), always + # decomposed: status_0 is the status code, status_1 the status text + self.status_0: SignalR super().__init__( sri=sri, name=name, connector=connector, loglevel=loglevel, logdir=logdir @@ -683,30 +882,25 @@ def __init__( async def connect(self, mock=False, timeout=DEFAULT_TIMEOUT, force_reconnect=False): await super().connect(mock, timeout, force_reconnect) - if not hasattr(self, "value"): + if "value" not in self._resolved_parameters: raise AttributeError( - "Attribute 'value' has not been assigned," + "Parameter 'value' has not been assigned," + "but is needed for Readable interface class" ) - if not hasattr(self, "status"): + if "status" not in self._resolved_parameters: raise AttributeError( - "Attribute 'status' has not been assigned," + "Parameter 'status' has not been assigned," + "but is needed for Readable interface class" ) async def _assign_interface_formats(self): + value_signals = ( + [self.value] if hasattr(self, "value") else self._split_signals("value") + ) + self._assign_hinted_format("value", value_signals) - if format_assigned(self, self.value): - if not is_read_signal(self, self.value): - warnings.warn( - f"Signal 'value' of device {self.name} has format assigned " - + "that is not compatible with Readable interface class" - ) - else: - self.add_readables([self.value], StandardReadableFormat.HINTED_SIGNAL) - - # TODO ensure status signal must be neither config nor read format + # TODO ensure status signals must be neither config nor read format async def wait_for_idle(self): """asynchronously waits until module is IDLE again. this is helpful, @@ -715,19 +909,15 @@ async def wait_for_idle(self): self._logger.info(f"Waiting for {self.name} to be IDLE") - if self.status is None: + if self.status_0 is None: self._logger.error("Status Signal not initialized") raise Exception("status Signal not initialized") # force reading of fresh status from device - await self.status.read(False) - - async for current_stat in observe_value(self.status): - # status is has type Tuple and is therefore transported as - # structured Numpy array ('f0':statuscode;'f1':status Message) - - stat_code = current_stat["f0"] + await self.status_0.read(False) + async for stat_name in observe_value(self.status_0): + stat_code = _status_code(self.status_0, stat_name) # Module is in IDLE/WARN state if IDLE <= stat_code < BUSY: self._logger.info(f"Module {self.name} --> IDLE") @@ -745,23 +935,6 @@ async def wait_for_idle(self): self._success = False break - # TODO add timeout - def observe_status_change(self, monitored_status_code: int): - async def switch_from_status_inner(): - async for current_stat in observe_value(self.status): - # status is has type Tuple and is therefore transported as - # structured Numpy array ('f0':statuscode;'f1':status Message) - - stat_code = current_stat["f0"] - - if monitored_status_code != stat_code: - break - - def switch_from_status_factory(): - return switch_from_status_inner() - - 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") # get fresh reading of the value Parameter from the SEC Node @@ -770,7 +943,8 @@ def trigger(self) -> AsyncStatus: ) def subscribe(self, function: Callback[dict[str, Reading]]) -> None: - """Subscribe to updates in the reading""" + """Subscribe to updates in the reading. Only supported for an atomic + (non-composite) 'value' parameter.""" self.value.subscribe(function=function) def clear_sub(self, function: Callback) -> None: @@ -828,6 +1002,9 @@ class SECoPMovableLogic(MovableLogic[Any]): equals setpoint. """ + # bound to status_0 (the status code Signal, resolved to its enum member + # name, e.g. "RAMPING") -- status is SECoP's StatusType == + # TupleOf(EnumType, StringType), always decomposed status: SignalR secclient: AsyncFrappyClient module: str @@ -838,10 +1015,8 @@ async def stop(self) -> None: await self.secclient.exec_command(self.module, "stop") async def move(self, new_position: Any, timeout: TimeoutCalculator) -> None: - # status has type Tuple, transported as a structured numpy array - # ('f0': statuscode, 'f1': status message) - def _left_busy(current_stat) -> bool: - stat_code = current_stat["f0"] + def _left_busy(stat_name) -> bool: + stat_code = _status_code(self.status, stat_name) return not (BUSY <= stat_code < ERROR) self.logger.info(f"Moving {self.module} to {new_position}") @@ -856,7 +1031,7 @@ def _left_busy(current_stat) -> bool: await wait_for_value(self.status, _left_busy, timeout=timeout()) - stat_code = (await self.status.get_value())["f0"] + stat_code = _status_code(self.status, await self.status.get_value()) if stat_code >= ERROR or stat_code < IDLE: self.logger.error(f"Module {self.module} --> ERROR/DISABLED") raise RuntimeError( @@ -867,13 +1042,21 @@ def _left_busy(current_stat) -> bool: self.logger.info(f"Reached target, module {self.module} --> IDLE") -class SECoPMoveableDevice(SECoPReadableDevice, StandardMovable[Any]): +class SECoPMoveableDevice(SECoPReadableDevice, StandardMovable[SignalDatatypeT]): """ Standard movable SECoP device, corresponding to a SECoP module with the interface class "Drivable" """ hinted_signals: list[str] = ["target", "value"] + mandatory_parameters: list[str] = SECoPReadableDevice.mandatory_parameters + [ + "target" + ] + + # set (via object.__setattr__, bypassing Device's child-tracking) in + # SECoPDeviceConnector.connect_real() when 'value' is composite; None + # (the atomic-value case, where movable_logic.readback is self.value) + _composite_value_readback: SignalR | None = None # StandardMovable is @default_mock_class(InstantMovableMock), which would # otherwise also install a mock put-callback on 'target' on top of this @@ -907,38 +1090,86 @@ async def connect(self, mock=False, timeout=DEFAULT_TIMEOUT, force_reconnect=Fal await super().connect(mock, timeout, force_reconnect) - if not hasattr(self, "target"): + if "target" not in self._resolved_parameters: raise AttributeError( - "Attribute 'target' has not been assigned, " + "Parameter 'target' has not been assigned, " + "but is needed for 'Drivable' interface class!" ) + def _has_atomic_setpoint_readback(self) -> bool: + """True if both 'target' and 'value' are plain (non-composite) + parameters, i.e. self.target is a real SignalRW and self.value + exists -- the case StandardMovable's default locate()/movable_logic + usage was designed for. False if either was decomposed into split + member signals (self.target is then a write-only SignalW, and/or + self.value doesn't exist as a bare attribute).""" + return isinstance(self.target, SignalR) and hasattr(self, "value") + @cached_property def movable_logic(self) -> MovableLogic: if self._module is None: raise RuntimeError + # atomic 'value': the real signal. composite 'value': the private, + # undecomposed readback SignalR pre-connected in connect_real() -- + # StandardMovable itself (set_name/subscribe_reading/locate) requires + # movable_logic.readback to always be a working SignalR; see the + # comment at its construction site for why it isn't just self.value. + readback = ( + self.value if hasattr(self, "value") else self._composite_value_readback + ) + return SECoPMovableLogic( setpoint=self.target, - readback=self.value, - status=self.status, + readback=readback, # type: ignore[arg-type] + status=self.status_0, secclient=self._client, module=self._module, logger=self._logger, ) + async def locate(self) -> Location[Any]: + if self._has_atomic_setpoint_readback(): + return await super().locate() + + if self._module is None: + raise RuntimeError + + # 'target'/'value' were decomposed into split member signals (no + # single Signal to .get_value() on) -- bypass Signals entirely and + # read the whole structured parameter straight from the SEC node, + # mirroring the direct-client-access pattern already used by + # trigger()/SECoPMovableLogic.stop() + setpoint_reading, readback_reading = await asyncio.gather( + self._client.get_parameter(self._module, "target", trycache=True), + self._client.get_parameter(self._module, "value", trycache=True), + ) + return Location( + setpoint=setpoint_reading.value, readback=readback_reading.value + ) + def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: # set_name() can run several times before movable_logic is actually # resolvable: once before connect() (e.g. init_devices() naming devices # up front), and again mid-connect whenever a sibling/parent signal is # filled in (DeviceFiller.fill_child_signal() -> _set_device_child() - # triggers a renaming cascade down the whole tree). StandardMovable's - # set_name() needs both '_module' (set by set_module(), early in the - # parent node's connect_real()) and 'target' (only created once this - # device's own connect_real() fills its signals) to resolve - # movable_logic, so skip it and fall back to plain Device.set_name() - # until both are present; the later call does the real renaming. - if self._module is None or not hasattr(self, "target"): + # triggers a renaming cascade down the whole tree) -- including a + # rename of *this* device as a not-yet-connected child, triggered by + # a *sibling* module being filled into the parent node, well before + # this device's own connect_real() has run at all. StandardMovable's + # set_name() needs '_module' (set by set_module(), early in the + # parent node's connect_real()), 'target' (only created once this + # device's own connect_real() fills its signals), and a resolvable + # readback (either the atomic 'value' signal, or -- for composite + # 'value' -- the private _composite_value_readback connected at the + # very start of this device's own connect_real(), see there) to + # resolve movable_logic, so skip it and fall back to plain + # Device.set_name() until all three are present; a later call does + # the real renaming. + readback_ready = ( + hasattr(self, "value") or self._composite_value_readback is not None + ) + if self._module is None or not hasattr(self, "target") or not readback_ready: Device.set_name(self, name, child_name_separator=child_name_separator) return super().set_name(name, child_name_separator=child_name_separator) @@ -946,14 +1177,12 @@ def set_name(self, name: str, *, child_name_separator: str | None = None) -> Non async def _assign_interface_formats(self): await super()._assign_interface_formats() - if format_assigned(self, self.target): - if not is_read_signal(self, self.target): - warnings.warn( - f"Signal 'target' of device {self.name} has format assigned " - + "that is not compatible with Movable interface class" - ) - else: - self.add_readables([self.target], StandardReadableFormat.HINTED_SIGNAL) + target_signals = ( + [self.target] + if isinstance(self.target, SignalR) + else self._split_signals("target") + ) + self._assign_hinted_format("target", target_signals) def class_from_interface(mod_properties: dict): @@ -974,6 +1203,10 @@ def class_from_interface(mod_properties: dict): return ophyd_class +def _status_code(status_sig: SignalR, status: str) -> int: + return status_sig._connector.backend.SECoPdtype_obj.export_value(status) + + IF_CLASSES = { "Triggerable": SECoPTriggerableDevice, "Drivable": SECoPMoveableDevice, diff --git a/src/secop_ophyd/SECoPSignal.py b/src/secop_ophyd/SECoPSignal.py index 75d36b5..f0f6b8b 100644 --- a/src/secop_ophyd/SECoPSignal.py +++ b/src/secop_ophyd/SECoPSignal.py @@ -31,6 +31,7 @@ from secop_ophyd.AsyncFrappyClient import AsyncFrappyClient from secop_ophyd.util import ( + MAX_DEPTH, Path, SECoPDataKey, SECoPdtype, @@ -50,13 +51,10 @@ ) -# max depth for datatypes supported by tiled/databroker -MAX_DEPTH = 1 - - class AttributeType(StrictEnum): PARAMETER = "parameter" PROPERTY = "property" + MEMBER = "member" def _is_concrete_enum_class(datatype: Any) -> bool: @@ -180,11 +178,21 @@ async def execute(self, *args: Any, **kwargs: Any) -> Any: class SECoPBackend(SignalBackend[SignalDatatypeT]): - """Unified backend for SECoP Parameters and Properties. - - - This allows a single backend type to be used in signal_backend_factory, - with deferred initialization based on annotation metadata. + """Unified backend for SECoP Parameters, Properties, and split composite + Members (one struct field / tuple index of a decomposed "depth <= + MAX_DEPTH" StructOf/TupleOf parameter). + + This allows a single backend type to be used in signal_backend_factory + (`DeviceFiller` always constructs the same backend class for every child + Signal, regardless of what `Signal` subclass -- SignalR/SignalW/SignalRW + -- it ends up wrapped in), with deferred initialization based on + annotation metadata. + + Several MEMBER backends typically share the same underlying (module, + parent_param) SECoP parameter -- one per member. Each fetches the + parent's raw wire reading and extracts its own field; no extra network + round-trips are incurred since `AsyncFrappyClient.get_parameter(..., + trycache=True)` is backed by frappy's own per-(module, parameter) cache. """ format: StandardReadableFormat @@ -196,6 +204,10 @@ class SECoPBackend(SignalBackend[SignalDatatypeT]): SECoPdtype_obj: DataType SECoP_type_info: SECoPdtype describe_dict: dict + # MEMBER only + _parent_param: str + _member_key: str + _is_tuple_member: bool def __init__( self, @@ -302,11 +314,80 @@ def init_property_from_introspection( self.path_str = path + def init_member_from_introspection( + self, + module_name: str, + parent_param: str, + member_key: str, + member_datatype: DataType, + secclient: AsyncFrappyClient, + ) -> None: + """Bind this backend to one member (struct field / tuple index) of a + decomposed composite parameter. Fully initialized eagerly here + (rather than deferred to connect(), like _init_parameter/_init_property) + since a member's own datatype is fully known once the parent + parameter has been introspected -- mirrors SECoPCommandBackend's + single-phase init.""" + if self.attribute_type is not None: + + if secclient != self._secclient: + raise RuntimeError( + "Backend already initialized with a different SECoP client, cannot " + "re-initialize" + ) + + if self.attribute_type != AttributeType.MEMBER: + raise RuntimeError( + f"Backend already initialized as {self.attribute_type}, " + f"cannot re-initialize as MEMBER" + ) + + self.attribute_type = AttributeType.MEMBER + self._module_name = module_name + self._parent_param = parent_param + self._member_key = member_key + self._is_tuple_member = member_key.isdigit() + self._secclient = secclient + + self.SECoPdtype_obj = member_datatype + self.SECoP_type_info = SECoPdtype(member_datatype) + + # split member signals mirror the format of the parent (whole) + # parameter they were split from, e.g. a struct declared + # _signal_format="HINTED_SIGNAL" makes every one of its member + # signals HINTED_SIGNAL too + parent_description = secclient.modules[module_name]["parameters"][parent_param] + match parent_description.get("_signal_format", None): + case "HINTED_SIGNAL": + self.format = StandardReadableFormat.HINTED_SIGNAL + case "HINTED_UNCACHED_SIGNAL": + self.format = StandardReadableFormat.HINTED_UNCACHED_SIGNAL + case "UNCACHED_SIGNAL": + self.format = StandardReadableFormat.UNCACHED_SIGNAL + case _: + self.format = StandardReadableFormat.CONFIG_SIGNAL + + self.path_str = f"{module_name}:{parent_param}.{member_key}" + self.source_name = ( + self._secclient.uri + ":" + self._secclient.nodename + ":" + self.path_str + ) + + self.describe_dict = {} + self.describe_dict["source"] = self.source_name + self.describe_dict.update(self.SECoP_type_info.get_datakey()) + + if _is_concrete_enum_class(self._annotated_datatype): + self.datatype = cast(type, self._annotated_datatype) + else: + self.datatype = self.SECoP_type_info.np_datatype + def source(self, name: str, read: bool) -> str: return self._secclient.host + ":" + self._secclient.port + ":" + self.path_str async def connect(self, timeout: float): - """Connect and initialize backend (handles both parameters and properties).""" + """Connect and initialize backend (parameters/properties are + deferred-initialized here; members are already fully initialized by + init_member_from_introspection).""" await self._secclient.connect() match self.attribute_type: @@ -409,25 +490,51 @@ async def _init_property(self): self.datatype = self.SECoP_type_info.np_datatype async def put(self, value: Any | None): - """Put a value to the parameter. Properties are readonly.""" + """Put a value to the parameter. Properties and struct/tuple members + are read-only.""" if self.attribute_type == AttributeType.PROPERTY: - # Properties are readonly raise RuntimeError( f"Cannot set property '{self._attribute_name}', properties are readonly" ) + if self.attribute_type == AttributeType.MEMBER: + raise RuntimeError( + f"Cannot set '{self._parent_param}.{self._member_key}': struct/tuple " + f"member signals are read-only. Set the whole parameter " + f"'{self._parent_param}' instead." + ) + # convert to frappy compatible Format secop_val = self.SECoP_type_info.val2secop(value) await self._secclient.set_parameter(**self.get_param_path(), value=secop_val) + def _extract_member(self, raw_value: Any) -> Any: + if self._is_tuple_member: + return raw_value[int(self._member_key)] + return raw_value[self._member_key] + + def _convert_member(self, raw_member_val: Any) -> Any: + self.SECoP_type_info.update_dtype(raw_member_val) + return self.SECoP_type_info.secop2val(raw_member_val) + async def get_datakey(self, source: str) -> DataKey: """Metadata like source, dtype, shape, precision, units""" if self.attribute_type == AttributeType.PROPERTY: # Properties have static metadata return describedict_to_datakey(self.describe_dict) + if self.attribute_type == AttributeType.MEMBER: + if isinstance(self.SECoPdtype_obj, ArrayOf): + entry = await self._secclient.get_parameter( + self._module_name, self._parent_param, trycache=True + ) + self.SECoP_type_info.update_dtype(self._extract_member(entry.value)) + self.describe_dict.update(self.SECoP_type_info.get_datakey()) + + return describedict_to_datakey(self.describe_dict) + if self.SECoP_type_info._is_composite or isinstance( self.SECoPdtype_obj, ArrayOf ): @@ -443,7 +550,7 @@ async def get_datakey(self, source: str) -> DataKey: return describedict_to_datakey(self.describe_dict) async def get_reading(self) -> Reading[SignalDatatypeT]: - """Get reading, handling both parameters and properties.""" + """Get reading, handling parameters, properties, and members.""" if self.attribute_type == AttributeType.PROPERTY: # Properties have static values dataset = CacheItem( @@ -452,6 +559,19 @@ async def get_reading(self) -> Reading[SignalDatatypeT]: sec_reading = SECoPReading(entry=dataset, secop_dt=self.SECoP_type_info) return sec_reading.get_reading() + if self.attribute_type == AttributeType.MEMBER: + entry = await self._secclient.get_parameter( + self._module_name, self._parent_param, trycache=True + ) + if entry.readerror is not None: + raise entry.readerror + + member_val = self._extract_member(entry.value) + return { + "value": self._convert_member(member_val), + "timestamp": entry.timestamp, + } + else: # Parameters are fetched from SECoP dataset = await self._secclient.get_parameter( @@ -481,7 +601,38 @@ async def async_func(*args, **kwargs): return async_func - def updateItem(module, parameter, entry: CacheItem): # noqa: N802 + if self.attribute_type == AttributeType.MEMBER: + + # must be named exactly 'updateItem': frappy's + # ProxyClient.register_callback() derives the callback kind from + # cbfunc.__name__ and validates it against a fixed whitelist + # (CALLBACK_NAMES) -- any other name is rejected with a TypeError + def updateItem(module, parameter, entry: CacheItem): # noqa: N802 + member_val = self._extract_member(entry.value) + reading: Reading = { + "value": self._convert_member(member_val), + "timestamp": entry.timestamp, + } + async_callback = awaitify(callback) + + asyncio.run_coroutine_threadsafe( + async_callback(reading=reading), + self._secclient.loop, + ) + + if callback is not None: + self._secclient.register_callback( + (self._module_name, self._parent_param), updateItem + ) + else: + self._secclient.unregister_callback( + (self._module_name, self._parent_param), updateItem + ) + return + + def updateItem( # type: ignore[no-redef] + module, parameter, entry: CacheItem # noqa: N802 + ): data = SECoPReading(secop_dt=self.SECoP_type_info, entry=entry) async_callback = awaitify(callback) diff --git a/src/secop_ophyd/util.py b/src/secop_ophyd/util.py index d7365c8..affb1eb 100644 --- a/src/secop_ophyd/util.py +++ b/src/secop_ophyd/util.py @@ -6,6 +6,7 @@ import time import warnings from abc import ABC, abstractmethod +from enum import Enum from functools import reduce from itertools import chain from typing import Any, List, Union, cast @@ -71,6 +72,18 @@ class NestedRaggedArray(Exception): """The Datatype contains nested ragged arrays""" +class IncompatibleSECoPDatatype(Exception): + """The datatype cannot be represented as ophyd-async Signal(s): it is a + struct/tuple nested inside another struct/tuple/array, which is + unsupported. Raised when this happens for a parameter that is mandatory + for its module's interface class.""" + + +# max depth of struct/tuple nesting supported by tiled & databroker (and, +# for parameters, by the struct/tuple-member decomposition in SECoPDevices) +MAX_DEPTH = 1 + + def deep_get(dictionary, keys, default=None) -> dict: def get_val(obj, key, default): if isinstance(obj, dict): @@ -739,6 +752,54 @@ def update_dtype(self, input_val): self.shape = dt[2] +class CompositeKind(Enum): + """Classification of a SECoP datatype for parameter-signal construction. + + ATOMIC: scalar or array-of-scalar (incl. array-of-enum/blob/...) -- gets + a single Signal, unchanged from today's behaviour. + DECOMPOSABLE: a top-level StructOf/TupleOf whose members are all + themselves ATOMIC (nesting depth <= MAX_DEPTH) -- gets split into + one read-only Signal per member (see get_composite_members()). + UNSUPPORTED: an ArrayOf wrapping a StructOf/TupleOf, or a StructOf/TupleOf + containing a nested StructOf/TupleOf member -- cannot be represented + as flat Signals; no Signal is constructed for it. + """ + + ATOMIC = "atomic" + DECOMPOSABLE = "decomposable" + UNSUPPORTED = "unsupported" + + +def classify_datatype(datatype: DataType) -> CompositeKind: + """Classify a raw SECoP datatype for parameter-signal construction.""" + secop_dt = SECoPdtype(datatype) + + if not secop_dt._is_composite: + return CompositeKind.ATOMIC + + if isinstance(datatype, (StructOf, TupleOf)) and secop_dt.max_depth <= MAX_DEPTH: + return CompositeKind.DECOMPOSABLE + + return CompositeKind.UNSUPPORTED + + +def get_composite_members(datatype: StructOf | TupleOf) -> list[tuple[str, DataType]]: + """Return (member_key, member_datatype) pairs for a StructOf/TupleOf, in + declaration order. + + Struct members keep their SECoP member name as key. Tuple members are + keyed by their stringified 0-based index ("0", "1", ...), used as the + `_` naming postfix for split signals. + """ + if isinstance(datatype, StructOf): + return list(datatype.members.items()) + + if isinstance(datatype, TupleOf): + return [(str(idx), member) for idx, member in enumerate(datatype.members)] + + raise TypeError(f"{datatype!r} is neither StructOf nor TupleOf") + + def secop_enum_name_to_python(member_name: str) -> str: """Convert SECoP enum member name to Python identifier. diff --git a/tests/test_Node.py b/tests/test_Node.py index fb3b62c..de2f6a3 100644 --- a/tests/test_Node.py +++ b/tests/test_Node.py @@ -113,12 +113,12 @@ async def test_signal_stage_unstage_read_cached( async def test_status(cryo_sim, cryo_node_no_re: SECoPNodeDevice): cryo_dev: SECoPMoveableDevice = cryo_node_no_re.cryo - status: SignalR = cryo_dev.status + status_code: SignalR = cryo_dev.status_0 - async for current_stat in observe_value(status): - assert current_stat["f0"] == 100 + async for stat_name in observe_value(status_code): + assert stat_name == "IDLE" - if current_stat["f0"] == 100: + if stat_name == "IDLE": break diff --git a/tests/test_annotation.py b/tests/test_annotation.py index da0f199..2514f36 100644 --- a/tests/test_annotation.py +++ b/tests/test_annotation.py @@ -10,12 +10,11 @@ async def test_subset_signals_annotation(cryo_sim): from typing import Annotated as A - from numpy import ndarray from ophyd_async.core import SignalRW from ophyd_async.core import StandardReadableFormat as Format from ophyd_async.core import StrictEnum - from secop_ophyd.SECoPDevices import ParameterType + from secop_ophyd.SECoPDevices import ParameterMemberType, ParameterType class Cryostat_Mode_Enum(StrictEnum): """mode enum for `Cryostat`.""" @@ -32,7 +31,9 @@ class Cryostat(SECoPMoveableDevice): value: A[ SignalR[float], ParameterType(), Format.HINTED_SIGNAL ] # regulation temperature; Unit: (K) - status: A[SignalR[ndarray], ParameterType()] # current status of the module + # status is StatusType == TupleOf(EnumType, StringType), always split + status_0: A[SignalR[StrictEnum], ParameterMemberType()] # status code + status_1: A[SignalR[str], ParameterMemberType()] # status message target: A[ SignalRW[float], ParameterType(), Format.HINTED_SIGNAL ] # target temperature; Unit: (K) @@ -61,11 +62,12 @@ async def test_enum_annotation(cryo_sim): from typing import Annotated as A from numpy import ndarray - from ophyd_async.core import SignalR, SignalRW + from ophyd_async.core import SignalR, SignalRW, SignalW from ophyd_async.core import StandardReadableFormat as Format from ophyd_async.core import StrictEnum from secop_ophyd.SECoPDevices import ( + ParameterMemberType, ParameterType, PropertyType, SECoPMoveableDevice, @@ -94,7 +96,9 @@ class Cryostat(SECoPMoveableDevice): value: A[ SignalR[float], ParameterType(), Format.HINTED_SIGNAL ] # regulation temperature; Unit: (K) - status: A[SignalR[ndarray], ParameterType()] # current status of the module + # status is StatusType == TupleOf(EnumType, StringType), always split + status_0: A[SignalR[StrictEnum], ParameterMemberType()] # status code + status_1: A[SignalR[str], ParameterMemberType()] # status message target: A[ SignalRW[float], ParameterType(), Format.HINTED_SIGNAL ] # target temperature; Unit: (K) @@ -110,7 +114,12 @@ class Cryostat(SECoPMoveableDevice): heaterpower: A[ SignalR[float], ParameterType() ] # current heater power; Unit: (W) - pid: A[SignalRW[ndarray], ParameterType()] # regulation coefficients + # pid is TupleOf(FloatRange, FloatRange, FloatRange), writable and + # depth 1 -> split read-only members plus a write-only SignalW + pid_0: A[SignalR[float], ParameterMemberType()] # regulation coefficient 'p' + pid_1: A[SignalR[float], ParameterMemberType()] # regulation coefficient 'i' + pid_2: A[SignalR[float], ParameterMemberType()] # regulation coefficient 'd' + pid: A[SignalW[ndarray], ParameterType()] # regulation coefficients p: A[ SignalRW[float], ParameterType() ] # regulation coefficient 'p'; Unit: (%/K) diff --git a/tests/test_classgen.py b/tests/test_classgen.py index 1fa5219..da5b221 100644 --- a/tests/test_classgen.py +++ b/tests/test_classgen.py @@ -6,7 +6,7 @@ from pathlib import Path from frappy.datatypes import StructOf -from ophyd_async.core import SignalR, init_devices +from ophyd_async.core import SignalR, SignalW, init_devices from secop_ophyd.GenNodeCode import ( CommandAttribute, @@ -819,11 +819,16 @@ async def test_generated_enum_parameter_datatype_is_preserved( assert cryo_gen_code.cryo.mode.datatype is Cryostat_Mode_Enum -async def test_gen_cryo_status_not_in_cfg( +async def test_gen_cryo_status_in_cfg( clean_generated_file, cryo_sim, cryo_node_no_re: SECoPNodeDevice ): - """Test that Status signal is not marked as configuration signal but is still - instantiated.""" + """status is StatusType == TupleOf(EnumType, StringType), always split + into status_0 (code)/status_1 (message). Unlike the old monolithic + structured-array 'status' signal (which was excluded from both read() + and read_configuration() since tiled/databroker couldn't handle its + dtype), status_0/status_1 are plain atomic signals with no such + restriction, so they get the same default CONFIG_SIGNAL format as any + other un-hinted parameter.""" cryo_node_no_re.class_from_instance(clean_generated_file) @@ -832,20 +837,17 @@ async def test_gen_cryo_status_not_in_cfg( print(cryo_reading) - assert hasattr(cryo_node_no_re.cryo, "status") - assert isinstance(cryo_node_no_re.cryo.status, SignalR) + assert hasattr(cryo_node_no_re.cryo, "status_0") + assert isinstance(cryo_node_no_re.cryo.status_0, SignalR) - stat_name = cryo_node_no_re.cryo.status.name + stat_name = cryo_node_no_re.cryo.status_0.name assert ( - cryo_cfg.get(stat_name) is None - ), "Status signal should not be in configuration" - assert cryo_reading.get(stat_name) is None, "Status signal should be readable" - - # check if status signal is working - status_reding = await cryo_node_no_re.cryo.status.read() - - assert status_reding.get(stat_name) is not None, "Status signal should be readable" + cryo_cfg.get(stat_name) is not None + ), "Status signal should be in configuration" + assert ( + cryo_reading.get(stat_name) is None + ), "Status signal should not be a read signal" # Import generated class from tests.testgen.Cryo_7_frappy_demo import Cryo_7_frappy_demo # type: ignore @@ -853,27 +855,22 @@ async def test_gen_cryo_status_not_in_cfg( async with init_devices(): cryo_gen_code = Cryo_7_frappy_demo(sec_node_uri="localhost:10769") - # Status signal should still be present and functional in the generated code, even - # though it's not in the configuration - assert hasattr(cryo_gen_code.cryo, "status") - assert isinstance(cryo_gen_code.cryo.status, SignalR) + assert hasattr(cryo_gen_code.cryo, "status_0") + assert isinstance(cryo_gen_code.cryo.status_0, SignalR) cryo_cfg = await cryo_gen_code.read_configuration() cryo_reading = await cryo_gen_code.read() print(cryo_reading) - stat_name = cryo_gen_code.cryo.status.name + stat_name = cryo_gen_code.cryo.status_0.name assert ( - cryo_cfg.get(stat_name) is None - ), "Status signal should not be in configuration" - assert cryo_reading.get(stat_name) is None, "Status signal should be readable" - - # check if status signal is working - status_reding = await cryo_gen_code.cryo.status.read() - - assert status_reding.get(stat_name) is not None, "Status signal should be readable" + cryo_cfg.get(stat_name) is not None + ), "Status signal should be in configuration" + assert ( + cryo_reading.get(stat_name) is None + ), "Status signal should not be a read signal" async def test_gen_real_node( @@ -923,6 +920,45 @@ async def test_gen_real_node( ), "Enum import should be present" +async def test_gen_real_node_composite_movable_sibling_connects( + clean_generated_file, + nested_struct_sim, + nested_node_no_re: SECoPNodeDevice, +): + """Regression test: a Drivable module with a composite 'value' (no bare + 'value' attribute, only split value_* signals) that is a sibling of + other modules in a fully class-annotated *generated* node must connect + without crashing. + + create_children_from_annotations() pre-creates every declared child + device/signal -- including 'target' -- at construction time, before any + connect() call runs, purely from the generated class's annotations. So + by the time init_devices() does its first naming pass (also before + connect()), a sibling module being named can trigger this Drivable's + set_name() while 'target' already structurally exists but movable_logic + isn't actually resolvable yet (nothing has been connected). Regression + for a crash where SECoPMoveableDevice.set_name() only guarded on + hasattr(self, 'target'), not on whether the (possibly composite) readback + was ready too: AttributeError: 'NoneType' object has no attribute + 'set_name' from StandardMovable.set_name() -> movable_logic.readback. + """ + nested_node_no_re.class_from_instance(clean_generated_file) + + from tests.testgen.Ophyd_secop_frappy_demo import ( # type: ignore + Ophyd_secop_frappy_demo, + ) + + async with init_devices(): + gen_node = Ophyd_secop_frappy_demo(sec_node_uri="localhost:10771") + + struct_mod = gen_node.ophy_struct + assert not hasattr(struct_mod, "value") + assert isinstance(struct_mod.target, SignalW) + + reading = await struct_mod.value_x.read() + assert isinstance(reading[struct_mod.value_x.name]["value"], float) + + async def test_subsequent_real_nodes_with_enum( clean_generated_file, cryo_sim, @@ -956,7 +992,7 @@ async def test_subsequent_real_nodes_with_enum( assert classs_str in nested_code assert "class Cryo_7_frappy_demo(SECoPNodeDevice):" not in nested_code - assert "class Cryostat(SECoPMoveableDevice):" not in nested_code + assert "class Cryostat(SECoPMoveableDevice[float]):" not in nested_code # ===== cryo node: its own file, only its own classes ===== cryo_file = clean_generated_file / "Cryo_7_frappy_demo.py" @@ -966,7 +1002,7 @@ async def test_subsequent_real_nodes_with_enum( cryo_cls = [ "class Cryo_7_frappy_demo(SECoPNodeDevice):", - "class Cryostat(SECoPMoveableDevice):", + "class Cryostat(SECoPMoveableDevice[float]):", "class Cryostat_Mode_Enum(StrictEnum):", ] for classs_str in cryo_cls: @@ -997,8 +1033,17 @@ def test_gen_shall_mass_spec_node( assert "\n# ; Unit: (V)" not in generated_code assert "\n# ; Unit: (%)" not in generated_code + # mid_descriptor is a writable StructOf(device=Array, mass=Array) -- + # depth 1, so it's split into one read-only member Signal per struct + # field, plus a write-only SignalW at the base name for setting the + # whole struct (accepting a plain dict, see SECoPBackend.put/val2secop) + assert ( + "mid_descriptor_device: A[SignalR[ndarray], ParamMemberT()]" in generated_code + ) + assert "mid_descriptor_mass: A[SignalR[ndarray], ParamMemberT()]" in generated_code + assert "mid_descriptor: A[SignalW[ndarray], ParamT()]" in generated_code + # Intentionally multiline descriptions should be rendered as multiline comments - assert "mid_descriptor: A[SignalRW[ndarray], ParamT()]" in generated_code assert "# Example:" in generated_code assert "# {" in generated_code assert "# mass: [12,15,28,75]," in generated_code diff --git a/tests/test_device_naming.py b/tests/test_device_naming.py new file mode 100644 index 0000000..76af1fb --- /dev/null +++ b/tests/test_device_naming.py @@ -0,0 +1,29 @@ +import warnings + +import pytest +from ophyd_async.core import Device, soft_signal_rw, walk_devices + +from secop_ophyd.SECoPDevices import TILED_MAX_NAME_LENGTH, warn_on_long_device_names + + +class _ChildDevice(Device): + def __init__(self, name: str = "") -> None: + self.sig = soft_signal_rw(int) + super().__init__(name=name) + + +def test_warns_when_device_name_too_long(): + device = _ChildDevice() + device.set_name("x" * (TILED_MAX_NAME_LENGTH + 1)) + + with pytest.warns(UserWarning, match="too long for tiled storage"): + warn_on_long_device_names(walk_devices(device)) + + +def test_no_warning_when_device_name_within_limit(): + device = _ChildDevice() + device.set_name("short_name") + + with warnings.catch_warnings(): + warnings.simplefilter("error") + warn_on_long_device_names(walk_devices(device)) diff --git a/tests/test_dtype.py b/tests/test_dtype.py index 769a0b8..d2c0823 100644 --- a/tests/test_dtype.py +++ b/tests/test_dtype.py @@ -14,7 +14,13 @@ ) from frappy.lib.enum import EnumMember -from secop_ophyd.util import SECoPdtype, SECoPReading +from secop_ophyd.util import ( + CompositeKind, + SECoPdtype, + SECoPReading, + classify_datatype, + get_composite_members, +) RAGGED = True REGULAR = False @@ -328,3 +334,95 @@ def test_val2secop(start_dtype, np_input, expected_output, type_checks, ophy_val assert ( back_to_ophyd == ophy_val ), f"Back to ophyd conversion failed for {back_to_ophyd}" + + +@pytest.mark.parametrize( + "datatype,expected_kind", + [ + pytest.param(FloatRange(), CompositeKind.ATOMIC, id="atomic float"), + pytest.param(StringType(), CompositeKind.ATOMIC, id="atomic string"), + pytest.param( + ArrayOf(FloatRange(), minlen=0, maxlen=5), + CompositeKind.ATOMIC, + id="array of atomic", + ), + pytest.param( + ArrayOf(ArrayOf(FloatRange(), minlen=0, maxlen=5), minlen=0, maxlen=5), + CompositeKind.ATOMIC, + id="array of array of atomic", + ), + pytest.param( + StructOf(x=FloatRange(), y=FloatRange()), + CompositeKind.DECOMPOSABLE, + id="flat struct", + ), + pytest.param( + TupleOf(FloatRange(), StringType()), + CompositeKind.DECOMPOSABLE, + id="flat tuple", + ), + pytest.param( + StructOf( + ints=ArrayOf(IntRange(), minlen=0, maxlen=5), + label=StringType(), + ), + CompositeKind.DECOMPOSABLE, + id="struct of arrays-of-atomic is still depth 1", + ), + pytest.param( + StructOf(inner=StructOf(a=FloatRange())), + CompositeKind.UNSUPPORTED, + id="struct containing a struct", + ), + pytest.param( + StructOf(inner=TupleOf(FloatRange(), FloatRange())), + CompositeKind.UNSUPPORTED, + id="struct containing a tuple", + ), + pytest.param( + TupleOf(TupleOf(FloatRange(), FloatRange()), StringType()), + CompositeKind.UNSUPPORTED, + id="tuple containing a tuple", + ), + pytest.param( + ArrayOf(StructOf(x=FloatRange(), y=FloatRange())), + CompositeKind.UNSUPPORTED, + id="array of struct", + ), + pytest.param( + ArrayOf(TupleOf(FloatRange(), FloatRange())), + CompositeKind.UNSUPPORTED, + id="array of tuple", + ), + ], +) +def test_classify_datatype(datatype, expected_kind): + assert classify_datatype(datatype) is expected_kind + + +def test_get_composite_members_struct(): + dt = StructOf(x=FloatRange(), y=FloatRange(), color=StringType()) + + members = get_composite_members(dt) + + assert [key for key, _ in members] == ["x", "y", "color"] + assert [member_dt for _, member_dt in members] == [ + dt.members["x"], + dt.members["y"], + dt.members["color"], + ] + + +def test_get_composite_members_tuple(): + float_dt, str_dt = FloatRange(), StringType() + dt = TupleOf(float_dt, str_dt) + + members = get_composite_members(dt) + + assert [key for key, _ in members] == ["0", "1"] + assert [member_dt for _, member_dt in members] == [float_dt, str_dt] + + +def test_get_composite_members_rejects_non_composite(): + with pytest.raises(TypeError): + get_composite_members(FloatRange()) diff --git a/tests/test_nested.py b/tests/test_nested.py index 907ac12..2cc153d 100644 --- a/tests/test_nested.py +++ b/tests/test_nested.py @@ -1,8 +1,10 @@ # mypy: disable-error-code="attr-defined" import numpy as np -from ophyd_async.core import SignalR, SignalRW +import pytest +from ophyd_async.core import SignalR, SignalW from secop_ophyd.SECoPDevices import SECoPNodeDevice, SECoPReadableDevice +from secop_ophyd.util import IncompatibleSECoPDatatype async def test_nested_connect(nested_struct_sim, nested_node_no_re: SECoPNodeDevice): @@ -15,111 +17,117 @@ async def test_tuple_dev(nested_struct_sim): await ophy_struct.connect() - status_sig: SignalR = ophy_struct.status + # status is StatusType == TupleOf(EnumType, StringType), always split + # into a status_0 (code, resolved to its enum member name) / status_1 + # (message, str) pair + status_code_sig: SignalR = ophy_struct.status_0 + status_text_sig: SignalR = ophy_struct.status_1 - reading = await status_sig.read() + code_reading = await status_code_sig.read() + text_reading = await status_text_sig.read() - reading_val = reading[status_sig.name]["value"] + stat_code = code_reading[status_code_sig.name]["value"] + stat_text = text_reading[status_text_sig.name]["value"] - await status_sig.describe() + await status_code_sig.describe() - stat0 = reading_val["f0"] - stat1 = reading_val["f1"] - - assert stat0.item() == 300 # isinstance(stat0, int) - - assert isinstance(stat1.item(), str) + assert isinstance(stat_code, str) + assert isinstance(stat_text, str) async def test_struct_dev(nested_struct_sim): ophy_struct = SECoPReadableDevice("localhost:10771:ophy_struct") await ophy_struct.connect() - nested_struct_sig: SignalR = ophy_struct.nested_struct - await nested_struct_sig.read() + # value is StructOf(x, y, z, color), depth 1 -> split into one + # read-only Signal per member, no monolithic 'value' Signal + assert not hasattr(ophy_struct, "value") + value_x_sig: SignalR = ophy_struct.value_x + value_color_sig: SignalR = ophy_struct.value_color -async def test_nested_dtype_str_signal_generation( - nested_struct_sim, nested_node_no_re: SECoPNodeDevice -): - struct_mod = nested_node_no_re.ophy_struct - - target: SignalRW = struct_mod.target + x_reading = await value_x_sig.read() + color_reading = await value_color_sig.read() - reading = await target.read() + assert isinstance(x_reading[value_x_sig.name]["value"], float) + assert isinstance(color_reading[value_color_sig.name]["value"], str) - descr_reading = await target.describe() - descr = descr_reading.get(target.name) - val = reading.get(target.name)["value"] +async def test_unsupported_nested_struct_warns(nested_struct_sim): + # nested_struct contains a StructOf member (pos_struct) and a TupleOf + # member (tupl) -- nesting depth 2, unsupported. It isn't mandatory for + # the Readable interface class, so it should only warn and be skipped, + # not raise. + with pytest.warns(UserWarning, match="nested_struct"): + ophy_struct = SECoPReadableDevice("localhost:10771:ophy_struct") + await ophy_struct.connect() - assert isinstance(val, np.ndarray) - assert descr["dtype"] == "array" - assert "struct" in descr["SECOP_datainfo"] + assert not hasattr(ophy_struct, "nested_struct") -async def test_nested_dtype_set_str_struct( +async def test_nested_dtype_str_signal_generation( nested_struct_sim, nested_node_no_re: SECoPNodeDevice ): struct_mod = nested_node_no_re.ophy_struct - target: SignalRW = struct_mod.target - - reading = await target.read() - - val = reading.get(target.name)["value"] - - val["x"] = 20 - val["y"] = 30 - val["z"] = 40 - val["color"] = "yellow" + # target is writable and composite (StructOf) -> a write-only SignalW + # at the base name, no read/describe capability there + target = struct_mod.target + assert isinstance(target, SignalW) + assert not isinstance(target, SignalR) - stat = target.set(val) + target_x: SignalR = struct_mod.target_x + target_color: SignalR = struct_mod.target_color - await stat + reading = await target_x.read() + descr_reading = await target_x.describe() - reading = await target.read() + descr = descr_reading.get(target_x.name) + val = reading.get(target_x.name)["value"] - val = reading.get(target.name)["value"] + assert isinstance(val, float) + assert descr["dtype"] == "number" - assert val["x"] == 20 - assert val["y"] == 30 - assert val["z"] == 40 - assert val["color"] == "yellow" - assert isinstance(val, np.ndarray) + color_reading = await target_color.read() + assert isinstance(color_reading.get(target_color.name)["value"], str) -async def test_nested_dtype_set_str_tuple( +async def test_nested_dtype_set_str_struct( nested_struct_sim, nested_node_no_re: SECoPNodeDevice ): struct_mod = nested_node_no_re.ophy_struct - tuple_param: SignalRW = struct_mod.tuple_param + target: SignalW = struct_mod.target - reading = await tuple_param.read() + # SignalW.put() accepts a plain dict for a StructOf parameter (frappy's + # StructOf.validate() coerces it), no need to build a numpy struct array + await target.set({"x": 20, "y": 30, "z": 40, "color": "yellow"}) - val = reading.get(tuple_param.name)["value"] + assert (await struct_mod.target_x.get_value()) == 20 + assert (await struct_mod.target_y.get_value()) == 30 + assert (await struct_mod.target_z.get_value()) == 40 + assert (await struct_mod.target_color.get_value()) == "yellow" - assert val["f0"] == 5 - assert val["f1"] == 5 - assert val["f2"] == 5 - assert val["f3"] == "green" - tuple_val = (50, 20, 30, "blue") - - stat = tuple_param.set(tuple_val) +async def test_nested_dtype_set_str_tuple( + nested_struct_sim, nested_node_no_re: SECoPNodeDevice +): + struct_mod = nested_node_no_re.ophy_struct - await stat + tuple_param: SignalW = struct_mod.tuple_param - reading = await tuple_param.read() + assert (await struct_mod.tuple_param_0.get_value()) == 5 + assert (await struct_mod.tuple_param_1.get_value()) == 5 + assert (await struct_mod.tuple_param_2.get_value()) == 5 + assert (await struct_mod.tuple_param_3.get_value()) == "green" - val = reading.get(tuple_param.name)["value"] + # SignalW.put() also accepts a plain tuple for a TupleOf parameter + await tuple_param.set((50, 20, 30, "blue")) - assert val["f0"] == 50 - assert val["f1"] == 20 - assert val["f2"] == 30 - assert val["f3"] == "blue" - assert isinstance(val, np.ndarray) + assert (await struct_mod.tuple_param_0.get_value()) == 50 + assert (await struct_mod.tuple_param_1.get_value()) == 20 + assert (await struct_mod.tuple_param_2.get_value()) == 30 + assert (await struct_mod.tuple_param_3.get_value()) == "blue" async def test_nested_struct_of_arrays( @@ -127,41 +135,85 @@ async def test_nested_struct_of_arrays( ): str_of_arr_mod: SECoPReadableDevice = nested_node_no_re.struct_of_arrays - reading = await str_of_arr_mod.read() + # value is StructOf(ints=Array, strings=Array, floats=Array), readonly + # -> split, no monolithic 'value' Signal + assert not hasattr(str_of_arr_mod, "value") - val = reading[str_of_arr_mod.value.name]["value"] + # a struct member that is itself an ArrayOf(atomic) behaves exactly like + # any other standalone ArrayOf parameter (see test_primitive_arrays.py) + # -- a plain tuple/list, not a numpy array (numpy wrapping only ever + # applied to the *structured*-dtype whole-parameter value, not to a + # plain array field extracted from it) + ints_val = await str_of_arr_mod.value_ints.get_value() + assert len(ints_val) == 5 + assert all(isinstance(v, int) for v in ints_val) - assert isinstance(val, np.ndarray) + # Write testing: writable_strct_of_arr is the same shape but writable + # -> a SignalW at the base name, plus split read-only member Signals + rw_str_of_arr: SignalW = str_of_arr_mod.writable_strct_of_arr + assert not isinstance(rw_str_of_arr, SignalR) - # Write testing - rw_str_of_arr: SignalRW = str_of_arr_mod.writable_strct_of_arr + old_ints = np.array(await str_of_arr_mod.writable_strct_of_arr_ints.get_value()) + old_strings = await str_of_arr_mod.writable_strct_of_arr_strings.get_value() + old_floats = np.array(await str_of_arr_mod.writable_strct_of_arr_floats.get_value()) - rw_reading = await rw_str_of_arr.read() + await rw_str_of_arr.set( + { + "ints": old_ints + 20, + "strings": old_strings, + "floats": old_floats + 0.2, + } + ) - rw_val: np.ndarray = rw_reading[rw_str_of_arr.name]["value"] + new_ints = await str_of_arr_mod.writable_strct_of_arr_ints.get_value() + new_floats = await str_of_arr_mod.writable_strct_of_arr_floats.get_value() - rw_old = rw_val.copy() + assert np.equal(new_ints, old_ints + 20).all() + assert np.allclose(new_floats, old_floats + 0.2) - rw_val["ints"] += 20 - rw_val["floats"] += 0.2 - await rw_str_of_arr.set(rw_val) +# TODO Nested Arrays (2D) uniform and ragged - rw_reading = await rw_str_of_arr.read() - rw_val = rw_reading[rw_str_of_arr.name]["value"] +async def test_hinted_signal(nested_struct_sim, nested_node_no_re: SECoPNodeDevice): + str_of_arr_mod: SECoPReadableDevice = nested_node_no_re.struct_of_arrays - assert np.equal(rw_val["ints"], rw_old["ints"] + 20).all() + reading = await str_of_arr_mod.read() - assert np.equal(rw_val["floats"], rw_old["floats"] + 0.2).all() + # value (3 members) + writable_strct_of_arr (3 members), all hinted; + # status_0/status_1 are CONFIG_SIGNAL, not part of read() + assert len(reading) == 6 -# TODO Nested Arrays (2D) uniform and ragged +async def test_mandatory_composite_incompatible_raises(nested_struct_sim): + from secop_ophyd.SECoPDevices import SECoPMoveableDevice + # ophy_struct is a Drivable whose 'value'/'target'/'status' are all + # depth-1 (compatible); nested_struct (depth 2, unsupported) is not + # mandatory there, so connecting normally must succeed. + moveable = SECoPMoveableDevice("localhost:10771:ophy_struct") + with pytest.warns(UserWarning, match="nested_struct"): + await moveable.connect() -async def test_hinted_signal(nested_struct_sim, nested_node_no_re: SECoPNodeDevice): - str_of_arr_mod: SECoPReadableDevice = nested_node_no_re.struct_of_arrays + assert isinstance(moveable.target, SignalW) - reading = await str_of_arr_mod.read() - assert len(reading) == 2 +async def test_mandatory_incompatible_datatype_raises_exception( + nested_struct_sim, monkeypatch +): + # nested_struct (StructOf containing a nested StructOf/TupleOf member) is + # genuinely unsupported (nesting depth > MAX_DEPTH). It isn't normally + # mandatory for the Readable interface class -- see + # test_unsupported_nested_struct_warns above, which only warns -- so + # temporarily mark it mandatory (the same way 'value'/'status' already + # are) to prove SECoPDeviceConnector.connect_real() raises + # IncompatibleSECoPDatatype instead of warning in that case. + monkeypatch.setattr( + SECoPReadableDevice, + "mandatory_parameters", + ["value", "status", "nested_struct"], + ) + + ophy_struct = SECoPReadableDevice("localhost:10771:ophy_struct") + with pytest.raises(IncompatibleSECoPDatatype, match="nested_struct"): + await ophy_struct.connect()