From d9dfcdf22ef84d0ad396c9cba8fc5452bf187695 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 25 Aug 2026 20:36:55 +0000 Subject: [PATCH 01/12] Make StandardDetector a StandardReadable So that set_readable_format() works on a detector at all. read(), describe() and hints merge the registered children on top of whatever the data logics produce, and add_config_signals becomes a deprecated wrapper. The detector overrides _extra_funcs_for()/_extra_hint_sources() rather than reimplementing the verbs, so a detector with no hinted fields now reports hints as {} rather than {"fields": []}, matching StandardReadable. Split out of the runtime-readable-format work so that PR stays core-only. Assisted-by: Claude Opus 5.5 --- src/ophyd_async/core/_detector.py | 118 +++++++++++------- src/ophyd_async/epics/adcore/_detector.py | 14 ++- src/ophyd_async/sim/_blob_detector.py | 4 +- .../unit_tests/core/test_standard_detector.py | 32 ++++- .../unit_tests/fastcs/panda/test_hdf_panda.py | 4 +- 5 files changed, 124 insertions(+), 48 deletions(-) diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index ca9f756fe6..df372f856d 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -4,8 +4,9 @@ import functools import os import time +import warnings from abc import ABC, abstractmethod -from collections.abc import AsyncIterator, Awaitable, Callable, Sequence +from collections.abc import AsyncIterator, Awaitable, Callable, Iterator, Sequence from dataclasses import dataclass from enum import Enum from functools import cached_property @@ -15,9 +16,7 @@ Collectable, Flyable, HasHints, - Hints, Preparable, - Reading, Stageable, StreamAsset, Triggerable, @@ -29,8 +28,12 @@ from ophyd_async.core._log import logger from ._data_providers import ReadableDataProvider, StreamableDataProvider -from ._device import Device -from ._protocol import AsyncConfigurable, AsyncReadable +from ._readable import ( + StandardReadable, + StandardReadableFormat, + _HintedFields, + _Verb, +) from ._settings import Settings from ._signal import ( SignalDict, @@ -362,28 +365,34 @@ class _KickoffCtx: class StandardDetector( - Device, + StandardReadable, Stageable, - AsyncConfigurable, - AsyncReadable, Triggerable, Preparable, Flyable, Collectable, WritesStreamAssets, - HasHints, ): """Detector base class for step and fly scanning detectors. Aggregates trigger, arm, reading or stream logic together. + + Signals read during a step scan are registered with + [](#StandardReadable.set_readable_format), exactly as on any other + `StandardReadable`; data produced by a `DetectorDataLogic` is added on top + of those in `read()` and `describe()`. + + `read()` and `describe()` require `prepare()` to have run, and raise + otherwise, so that a descriptor can never be emitted without the detector's + data keys. `trigger()` prepares implicitly, so a step scan never has to do + it explicitly. `read_configuration()` and `describe_configuration()` have no + such requirement. """ # Logic for the detector _trigger_logic: DetectorTriggerLogic | None = None _acquire_logic: DetectorAcquireLogic | None = None _data_logics: Sequence[DetectorDataLogic] = () - # Signals to include in read_configuration - _config_signals: Sequence[SignalR] = () # Context produced by prepare, used by trigger and kickoff _prepare_ctx: _PrepareCtx | None = None # Context produced by kickoff, used by complete @@ -391,6 +400,13 @@ class StandardDetector( # The triggers that are supported by the trigger logic _supported_triggers: set[DetectorTrigger] = {DetectorTrigger.INTERNAL} + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + # Contribute to _StandardBase's fan out rather than replacing stage(), + # so the readable children registered on this detector still get staged + self._stage_funcs += (self._stage_detector,) + self._unstage_funcs += (self._unstage_detector,) + # Report the number of events for the next kickoff @cached_property def events_to_kickoff(self) -> SignalRW[int]: @@ -419,7 +435,8 @@ def add_detector_logics( # Store the triggers that are supported self._supported_triggers = _get_supported_triggers(logic) # Add the config signals it needs - self.add_config_signals(*logic.config_sigs()) + for sig in logic.config_sigs(): + self.set_readable_format(sig, StandardReadableFormat.CONFIG_SIGNAL) elif isinstance(logic, DetectorAcquireLogic): if self._acquire_logic is not None: raise RuntimeError("Detector already has acquire logic") @@ -429,12 +446,21 @@ def add_detector_logics( else: raise TypeError(f"Unknown logic type: {type(logic)}") + # Back compat - delete before 1.0 def add_config_signals(self, *signals: SignalR) -> None: """Add a signal to read_configuration(). :param sig: The signal to add """ - self._config_signals = (*self._config_signals, *signals) + warnings.warn( + DeprecationWarning( + "Use `set_readable_format(signal, StandardReadableFormat" + ".CONFIG_SIGNAL)` instead of `add_config_signals(signal)`" + ), + stacklevel=2, + ) + for signal in signals: + self.set_readable_format(signal, StandardReadableFormat.CONFIG_SIGNAL) async def get_trigger_deadtime( self, settings: Settings | None = None @@ -462,9 +488,9 @@ async def get_trigger_deadtime( return self._supported_triggers, deadtime @AsyncStatus.wrap - async def stage(self) -> None: + async def _stage_detector(self) -> None: """Make sure the detector is idle and ready to be used.""" - coros = [data_logic.stop() for data_logic in self._data_logics] + coros: list[Awaitable] = [data_logic.stop() for data_logic in self._data_logics] if self._acquire_logic: coros.append(self._acquire_logic.ensure_ready()) await asyncio.gather(*coros) @@ -626,7 +652,7 @@ async def trigger(self) -> AsyncIterator[WatcherUpdate[int]]: """Trigger a single exposure. If [`prepare()`](#StandardDetector.prepare) has not been called since - the last [`stage()`](#StandardDetector.stage), an implicit prepare is + the last `stage()`, an implicit prepare is performed. When [](#OPHYD_ASYNC_PRESERVE_DETECTOR_STATE) is `YES` [](#DetectorTriggerLogic.default_trigger_info) is called to read current hardware state; otherwise a bare [`TriggerInfo()`](#TriggerInfo) is @@ -730,22 +756,37 @@ async def complete(self): ): yield update - async def describe_configuration(self) -> dict[str, DataKey]: - return await merge_gathered_dicts( - sig.describe() for sig in self._config_signals - ) + def _extra_funcs_for(self, verb: _Verb) -> Iterator[Callable[[], Awaitable[dict]]]: + """Contribute what the data logics produce to read() and describe(). + + StandardReadable gathers these alongside the registered children, so + the verb methods themselves need no overriding. - async def read_configuration(self) -> dict[str, Reading]: - return await merge_gathered_dicts(sig.read() for sig in self._config_signals) + Raising when nothing has been prepared is deliberate, and predates + registered children being readable at all: a detector that has not been + prepared has no data keys, so a `describe()` that quietly succeeded + would emit a descriptor missing the detector's data. In practice + `trigger()` prepares implicitly, so `trigger_and_read` never reaches + this. `read_configuration()` and `describe_configuration()` are + unaffected, as this hook only feeds the data verbs. - async def describe(self) -> dict[str, DataKey]: + The cost is that the *registered children* are unreachable through + `read()`/`describe()` until then too, since this raises before the + registry is consulted. That is accepted rather than worked around: + reading an unprepared detector is already defined as an error. + """ + if verb not in (_Verb.DESCRIBE, _Verb.READ): + return ctx = error_if_none(self._prepare_ctx, "Prepare not run") - # Readable and Streamable data providers produce data during read - coros = [dp.make_datakeys() for dp in ctx.readable_data_providers] + [ - dp.make_datakeys(ctx.trigger_info.collections_per_event) - for dp in ctx.streamable_data_providers - ] - return await merge_gathered_dicts(coros) + for dp in ctx.readable_data_providers: + yield dp.make_datakeys if verb is _Verb.DESCRIBE else dp.make_readings + if verb is _Verb.DESCRIBE: + # Streamable providers describe their shape for a step scan, but + # produce their data through collect_asset_docs rather than read() + for sdp in ctx.streamable_data_providers: + yield functools.partial( + sdp.make_datakeys, ctx.trigger_info.collections_per_event + ) async def describe_collect(self) -> dict[str, DataKey]: ctx = error_if_none(self._prepare_ctx, "Prepare not run") @@ -756,18 +797,11 @@ async def describe_collect(self) -> dict[str, DataKey]: ] return await merge_gathered_dicts(coros) - @property - def hints(self) -> Hints: - fields: list[str] = [] + def _extra_hint_sources(self) -> Iterator[HasHints]: + """Contribute the data logics' hinted fields alongside the children's.""" for dl in self._data_logics: - fields.extend(dl.get_hinted_fields(self.name + dl.datakey_suffix)) - return Hints(fields=fields) - - async def read(self) -> dict[str, Reading]: - ctx = error_if_none(self._prepare_ctx, "Prepare not called") - return await merge_gathered_dicts( - dp.make_readings() for dp in ctx.readable_data_providers - ) + if fields := dl.get_hinted_fields(self.name + dl.datakey_suffix): + yield _HintedFields(fields) async def collect_asset_docs( self, index: int | None = None @@ -793,9 +827,9 @@ async def get_index(self) -> int: return min_collections_written // ctx.trigger_info.collections_per_event @AsyncStatus.wrap - async def unstage(self) -> None: + async def _unstage_detector(self) -> None: """Stop the detector and file writing.""" - coros = [data_logic.stop() for data_logic in self._data_logics] + coros: list[Awaitable] = [data_logic.stop() for data_logic in self._data_logics] if self._acquire_logic: coros.append(self._acquire_logic.ensure_stopped()) await asyncio.gather(*coros) diff --git a/src/ophyd_async/epics/adcore/_detector.py b/src/ophyd_async/epics/adcore/_detector.py index 71d500ca6c..6d91c5c7b9 100644 --- a/src/ophyd_async/epics/adcore/_detector.py +++ b/src/ophyd_async/epics/adcore/_detector.py @@ -7,6 +7,7 @@ SignalR, StandardDetector, ) +from ophyd_async.core import StandardReadableFormat as Format from ._acquire_logic import ADContAcqAcquireLogic from ._data_logic import ADWriterFactory @@ -55,9 +56,16 @@ def __init__( writer, data_logic = factory(prefix, driver, plugin_list) setattr(self, factory.writer_name, writer) self.add_detector_logics(data_logic) - self.add_config_signals( - self.driver.acquire_period, self.driver.acquire_time, *config_sigs - ) + # Register the driver's timing signals, plus anything the caller asked + # for, as configuration. Plugins are deliberately not registered here: + # a detector normally carries far more plugins than are wired into the + # chain, so opt in per plugin with set_readable_format() instead. + for signal in ( + self.driver.acquire_period, + self.driver.acquire_time, + *config_sigs, + ): + self.set_readable_format(signal, Format.CONFIG_SIGNAL) super().__init__(name=name) def get_plugin( diff --git a/src/ophyd_async/sim/_blob_detector.py b/src/ophyd_async/sim/_blob_detector.py index ed21968784..6760e088a1 100644 --- a/src/ophyd_async/sim/_blob_detector.py +++ b/src/ophyd_async/sim/_blob_detector.py @@ -1,6 +1,7 @@ from collections.abc import Sequence from ophyd_async.core import PathProvider, SignalR, StandardDetector +from ophyd_async.core import StandardReadableFormat as Format from ._blob_acquire_logic import BlobAcquireLogic from ._blob_data_logic import BlobDataLogic @@ -26,5 +27,6 @@ def __init__( path_provider=path_provider, pattern_generator=self.pattern_generator ), ) - self.add_config_signals(*config_sigs) + for signal in config_sigs: + self.set_readable_format(signal, Format.CONFIG_SIGNAL) super().__init__(name=name) diff --git a/tests/unit_tests/core/test_standard_detector.py b/tests/unit_tests/core/test_standard_detector.py index 96d449feb7..567bb9812d 100644 --- a/tests/unit_tests/core/test_standard_detector.py +++ b/tests/unit_tests/core/test_standard_detector.py @@ -25,6 +25,7 @@ soft_signal_r_and_setter, soft_signal_rw, ) +from ophyd_async.core import StandardReadableFormat as Format from ophyd_async.testing import ( assert_configuration, assert_reading, @@ -667,7 +668,7 @@ async def test_config_signals_in_describe_configuration( signal = soft_signal_rw( signal_type, initial_value=initial_value, name="test-config" ) - det.add_config_signals(signal) + det.set_readable_format(signal, Format.CONFIG_SIGNAL) await det.stage() @@ -1092,3 +1093,32 @@ async def test_data_logic_not_implemented_errors(): # stop() should not raise (has default implementation) await logic.stop() # Should pass + + +async def test_detector_readable_format_changes_at_runtime(): + # A StandardDetector is a StandardReadable, so a plugin signal can be moved + # between configuration and hinted reads without redefining the Device. + det = StandardDetector(name="det") + det.temperature, _ = soft_signal_r_and_setter(float, 20.0, name="temperature") + + name = det.temperature.name + det.set_readable_format(det.temperature, Format.CONFIG_SIGNAL) + assert set(await det.read_configuration()) == {name} + # Empty hints are {} rather than {"fields": []}, as for any StandardReadable + assert det.hints == {} + + det.set_readable_format(det.temperature, Format.HINTED_UNCACHED_SIGNAL) + assert set(await det.read_configuration()) == set() + assert det.hints == {"fields": [name]} + + det.set_readable_format(det.temperature, None) + assert set(await det.read_configuration()) == set() + assert det.hints == {} + + +async def test_detector_add_config_signals_is_deprecated(): + det = StandardDetector(name="det") + signal, _ = soft_signal_r_and_setter(float, 1.0, name="sig") + with pytest.deprecated_call(): + det.add_config_signals(signal) + assert set(await det.read_configuration()) == {signal.name} diff --git a/tests/unit_tests/fastcs/panda/test_hdf_panda.py b/tests/unit_tests/fastcs/panda/test_hdf_panda.py index 6b5e5599b1..76c423f7cc 100644 --- a/tests/unit_tests/fastcs/panda/test_hdf_panda.py +++ b/tests/unit_tests/fastcs/panda/test_hdf_panda.py @@ -187,7 +187,9 @@ async def test_flyscan_documents(hdf_panda: HDFPanda, tmp_path): "source": uri, }, } - assert hdf_panda.hints == {"fields": []} + # No hinted fields is {} rather than {"fields": []}, as for any + # StandardReadable: the detector no longer special cases this + assert hdf_panda.hints == {} # Kick it off and start it completing await hdf_panda.kickoff() status = hdf_panda.complete() From 581347a3504d4502721b7591da88c8d14668d867 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Wed, 12 Aug 2026 11:49:27 -0400 Subject: [PATCH 02/12] Make standard detector extend from standard readable. Allow for pulling read/config/describe from children automatically (cherry picked from commit 32193035d7412c81bf42b930a2e275f6f7781df3) --- src/ophyd_async/core/_detector.py | 13 ++-- .../unit_tests/core/test_standard_detector.py | 59 ++++++++++++++++++- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index df372f856d..fba36ce42d 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -145,18 +145,18 @@ class DetectorTriggerLogic: Subclasses must implement the appropriate `prepare_*` method for any trigger mode the detector supports, `get_deadtime` if it supports external - triggering, and `config_sigs` if the deadtime would vary according to + triggering, and `deadtime_sigs` if the deadtime would vary according to detector parameters. """ - def config_sigs(self) -> set[SignalR]: - """Return the signals that should appear in read_configuration.""" + def deadtime_sigs(self) -> set[SignalR]: + """Return the signals whose values are needed to compute deadtime.""" return set() def get_deadtime(self, config_values: SignalDict) -> float: """Return the deadtime in seconds for the detector. - :param config_values: the value of each signal in `config_sigs` + :param config_values: the value of each signal in `deadtime_sigs` """ raise NotImplementedError(self) @@ -434,9 +434,6 @@ def add_detector_logics( self._trigger_logic = logic # Store the triggers that are supported self._supported_triggers = _get_supported_triggers(logic) - # Add the config signals it needs - for sig in logic.config_sigs(): - self.set_readable_format(sig, StandardReadableFormat.CONFIG_SIGNAL) elif isinstance(logic, DetectorAcquireLogic): if self._acquire_logic is not None: raise RuntimeError("Detector already has acquire logic") @@ -474,7 +471,7 @@ async def get_trigger_deadtime( self._trigger_logic.get_deadtime ): config_values = SignalDict() - for sig in self._trigger_logic.config_sigs(): + for sig in self._trigger_logic.deadtime_sigs(): if settings and sig in settings: # Use value from settings if it is in there # cast to a SignalRW because settings can only contain those diff --git a/tests/unit_tests/core/test_standard_detector.py b/tests/unit_tests/core/test_standard_detector.py index 567bb9812d..342a587525 100644 --- a/tests/unit_tests/core/test_standard_detector.py +++ b/tests/unit_tests/core/test_standard_detector.py @@ -18,6 +18,7 @@ SignalDict, SignalR, StandardDetector, + StandardReadable, StreamableDataProvider, StreamResourceDataProvider, StreamResourceInfo, @@ -98,8 +99,7 @@ class DeadtimeTriggerLogic(DetectorTriggerLogic): def __init__(self, deadtime_signal: SignalR[float]): self.deadtime_signal = deadtime_signal - def config_sigs(self) -> set[SignalR]: - """Return the deadtime signal as a config signal.""" + def deadtime_sigs(self) -> set[SignalR]: return {self.deadtime_signal} def get_deadtime(self, config_values: SignalDict) -> float: @@ -1061,6 +1061,61 @@ async def test_collect_asset_docs_with_explicit_index(tmp_path): ] +async def test_child_readable_config_signals_in_describe_configuration(): + """Child StandardReadable CONFIG_SIGNALs appear in describe_configuration.""" + child = StandardReadable(name="child") + config_sig = soft_signal_rw(float, initial_value=1.5, name="child-exposure") + child.add_readables([config_sig], Format.CONFIG_SIGNAL) + + det = StandardDetector(name="det") + det.add_readables([child], Format.CHILD) + det.add_detector_logics(ReadableOnlyDataLogic()) + await det.prepare(TriggerInfo()) + + config = await det.describe_configuration() + assert "child-exposure" in config + reading = await det.read_configuration() + assert "child-exposure" in reading + assert reading["child-exposure"]["value"] == 1.5 + + +async def test_child_readable_read_signals_in_read(): + """Child StandardReadable HINTED_SIGNALs appear in read/describe.""" + child = StandardReadable(name="child") + read_sig = soft_signal_rw(int, initial_value=99, name="child-counts") + child.add_readables([read_sig], Format.HINTED_SIGNAL) + + det = StandardDetector(name="det") + det.add_readables([child], Format.CHILD) + det.add_detector_logics(ReadableOnlyDataLogic()) + await det.prepare(TriggerInfo()) + + desc = await det.describe() + assert "child-counts" in desc + assert "foo-value" in desc + + reading = await det.read() + assert "child-counts" in reading + assert reading["child-counts"]["value"] == 99 + # Data provider signal is also present + assert "foo-value" in reading + + +async def test_child_readable_hints_merged(): + """Child StandardReadable hints are merged with data logic hints.""" + child = StandardReadable(name="child") + hinted_sig = soft_signal_rw(float, name="child-intensity") + child.add_readables([hinted_sig], Format.HINTED_SIGNAL) + + det = StandardDetector(name="det") + det.add_readables([child], Format.CHILD) + det.add_detector_logics(ReadableOnlyDataLogic()) + + assert "fields" in det.hints + assert "child-intensity" in det.hints["fields"] + assert "foo-value" in det.hints["fields"] + + async def test_trigger_logic_not_implemented_errors(): """Test NotImplementedError for unimplemented DetectorTriggerLogic methods.""" logic = DetectorTriggerLogic() From ac184b8aa0cf15d649f2da578e01451a85f6d1c0 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Wed, 12 Aug 2026 11:50:14 -0400 Subject: [PATCH 03/12] Update EPICS areaDetector classes to use standard readable child components for config/read (cherry picked from commit 64f4cd32b07ae6a7d6ee23e4c494ac56f8d812f8) --- src/ophyd_async/epics/adcore/__init__.py | 2 - src/ophyd_async/epics/adcore/_data_logic.py | 17 --- src/ophyd_async/epics/adcore/_detector.py | 16 ++- src/ophyd_async/epics/adcore/_io.py | 117 ++++++++++++------ .../epics/adcore/test_data_logic.py | 28 ++--- .../unit_tests/epics/adcore/test_use_cases.py | 108 ++++++++-------- 6 files changed, 155 insertions(+), 133 deletions(-) diff --git a/src/ophyd_async/epics/adcore/__init__.py b/src/ophyd_async/epics/adcore/__init__.py index 27a079424b..be4307c00e 100644 --- a/src/ophyd_async/epics/adcore/__init__.py +++ b/src/ophyd_async/epics/adcore/__init__.py @@ -9,7 +9,6 @@ ADMultipartDataLogic, ADWriterFactory, NDArrayDescription, - PluginSignalDataLogic, ) from ._detector import AreaDetector, ContAcqDetector, plugins_with_proc from ._io import ( @@ -95,7 +94,6 @@ "ADContAcqAcquireLogic", # DataLogic "NDArrayDescription", - "PluginSignalDataLogic", "ADHDFDataLogic", "ADMultipartDataLogic", "ADWriterFactory", diff --git a/src/ophyd_async/epics/adcore/_data_logic.py b/src/ophyd_async/epics/adcore/_data_logic.py index 343d7ce219..bc11d4cc8c 100644 --- a/src/ophyd_async/epics/adcore/_data_logic.py +++ b/src/ophyd_async/epics/adcore/_data_logic.py @@ -12,7 +12,6 @@ EnableDisable, PathInfo, PathProvider, - SignalDataProvider, SignalR, StreamableDataProvider, StreamResourceDataProvider, @@ -35,22 +34,6 @@ from ._ndattribute import NDAttributeDataType, NDAttributePvDbrType -@dataclass -class PluginSignalDataLogic(DetectorDataLogic): - driver: ADBaseIO - signal: SignalR - hinted: bool = True - - async def prepare_single(self, datakey_name: str) -> SignalDataProvider: - # Need to wait for all the plugins to have finished before we can read - # the plugin signal - await self.driver.wait_for_plugins.set(True) - return SignalDataProvider(self.signal) - - def get_hinted_fields(self, datakey_name: str) -> Sequence[str]: - return [self.signal.name] if self.hinted else [] - - @dataclass class NDArrayDescription: """Signals that describe the shape and data type of an NDArray frame. diff --git a/src/ophyd_async/epics/adcore/_detector.py b/src/ophyd_async/epics/adcore/_detector.py index 6d91c5c7b9..2a1cb72b2e 100644 --- a/src/ophyd_async/epics/adcore/_detector.py +++ b/src/ophyd_async/epics/adcore/_detector.py @@ -56,15 +56,13 @@ def __init__( writer, data_logic = factory(prefix, driver, plugin_list) setattr(self, factory.writer_name, writer) self.add_detector_logics(data_logic) - # Register the driver's timing signals, plus anything the caller asked - # for, as configuration. Plugins are deliberately not registered here: - # a detector normally carries far more plugins than are wired into the - # chain, so opt in per plugin with set_readable_format() instead. - for signal in ( - self.driver.acquire_period, - self.driver.acquire_time, - *config_sigs, - ): + # The driver is always wired up, so register it whole and let ADBaseIO + # declare which of its signals are configuration. Plugins are + # deliberately *not* registered: a detector normally carries far more + # plugins than are wired into the chain, so opt in per plugin with + # `set_readable_format(det.stats, Format.CHILD)` instead. + self.set_readable_format(self.driver, Format.CHILD) + for signal in config_sigs: self.set_readable_format(signal, Format.CONFIG_SIGNAL) super().__init__(name=name) diff --git a/src/ophyd_async/epics/adcore/_io.py b/src/ophyd_async/epics/adcore/_io.py index 9278cfe706..4cd0134841 100644 --- a/src/ophyd_async/epics/adcore/_io.py +++ b/src/ophyd_async/epics/adcore/_io.py @@ -6,17 +6,15 @@ EnableDisable, SignalR, SignalRW, + StandardReadable, StrictEnum, SubsetEnum, SupersetEnum, TriggerableCommand, non_zero, ) -from ophyd_async.epics.core import ( - EpicsDevice, - EpicsOptions, - PvSuffix, -) +from ophyd_async.core import StandardReadableFormat as Format +from ophyd_async.epics.core import EpicsDevice, EpicsOptions, PvSuffix # Common classes for drivers and plugins @@ -116,7 +114,7 @@ class ADState(StrictEnum): ADBaseIOT = TypeVar("ADBaseIOT", bound="ADBaseIO") -class ADBaseIO(NDArrayBaseIO): +class ADBaseIO(StandardReadable, NDArrayBaseIO): """Base class from which areaDetector drivers are derived. This mirrors the interface provided by ADCore/db/ADBase.template. @@ -127,10 +125,14 @@ class ADBaseIO(NDArrayBaseIO): compatibility with changes made in https://github.com/areaDetector/ADCore/blob/master/RELEASE.md#asynndarraydriver-addriver """ - acquire_time: A[SignalRW[float], PvSuffix.rbv("AcquireTime")] - acquire_period: A[SignalRW[float], PvSuffix.rbv("AcquirePeriod")] - num_images: A[SignalRW[int], PvSuffix.rbv("NumImages")] - image_mode: A[SignalRW[ADImageMode], PvSuffix.rbv("ImageMode")] + acquire_time: A[SignalRW[float], PvSuffix.rbv("AcquireTime"), Format.CONFIG_SIGNAL] + acquire_period: A[ + SignalRW[float], PvSuffix.rbv("AcquirePeriod"), Format.CONFIG_SIGNAL + ] + num_images: A[SignalRW[int], PvSuffix.rbv("NumImages"), Format.CONFIG_SIGNAL] + image_mode: A[ + SignalRW[ADImageMode], PvSuffix.rbv("ImageMode"), Format.CONFIG_SIGNAL + ] detector_state: A[SignalR[ADState], PvSuffix("DetectorState_RBV")] # The following signals have been moved from NDArrayBaseIO for backwards @@ -140,11 +142,13 @@ class ADBaseIO(NDArrayBaseIO): # There is no _RBV for this one wait_for_plugins: A[SignalRW[bool], PvSuffix("WaitForPlugins")] - manufacturer: A[SignalR[str], PvSuffix("Manufacturer_RBV")] - model: A[SignalR[str], PvSuffix("Model_RBV")] - serial_number: A[SignalR[str], PvSuffix("SerialNumber_RBV")] - sdk_version: A[SignalR[str], PvSuffix("SDKVersion_RBV")] - firmware_version: A[SignalR[str], PvSuffix("FirmwareVersion_RBV")] + manufacturer: A[SignalR[str], PvSuffix("Manufacturer_RBV"), Format.CONFIG_SIGNAL] + model: A[SignalR[str], PvSuffix("Model_RBV"), Format.CONFIG_SIGNAL] + serial_number: A[SignalR[str], PvSuffix("SerialNumber_RBV"), Format.CONFIG_SIGNAL] + sdk_version: A[SignalR[str], PvSuffix("SDKVersion_RBV"), Format.CONFIG_SIGNAL] + firmware_version: A[ + SignalR[str], PvSuffix("FirmwareVersion_RBV"), Format.CONFIG_SIGNAL + ] # Classes for plugins @@ -165,13 +169,15 @@ class NDPluginBaseIO(NDArrayBaseIO): queue_size: A[SignalRW[int], PvSuffix.rbv("QueueSize")] -class NDROIIO(NDPluginBaseIO): +class NDROIIO(StandardReadable, NDPluginBaseIO): """Plugin for taking a region of an NDArray. This mirrors the interface provided by ADCore/db/NDROI.template. See HTML docs at https://areadetector.github.io/areaDetector/ADCore/NDPluginROI.html """ + min_x: A[SignalRW[int], PvSuffix.rbv("MinX")] + min_y: A[SignalRW[int], PvSuffix.rbv("MinY")] size_x: A[SignalRW[int], PvSuffix.rbv("SizeX")] size_y: A[SignalRW[int], PvSuffix.rbv("SizeY")] size_z: A[SignalRW[int], PvSuffix.rbv("SizeZ")] @@ -180,8 +186,14 @@ class NDROIIO(NDPluginBaseIO): bin_y: A[SignalRW[int], PvSuffix.rbv("BinY")] bin_z: A[SignalRW[int], PvSuffix.rbv("BinZ")] + def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): + super().__init__(prefix, with_pvi, name) + self.add_readables( + [self.min_x, self.min_y, self.size_x, self.size_y], Format.CONFIG_SIGNAL + ) + -class NDStatsIO(NDPluginBaseIO): +class NDStatsIO(StandardReadable, NDPluginBaseIO): """Plugin for computing statistics from an image or ROI within an image. This mirrors the interface provided by ADCore/db/NDStats.template. @@ -191,11 +203,24 @@ class NDStatsIO(NDPluginBaseIO): # Basic statistics compute_statistics: A[SignalRW[bool], PvSuffix.rbv("ComputeStatistics")] bgd_width: A[SignalRW[int], PvSuffix.rbv("BgdWidth")] - total: A[SignalR[float], PvSuffix("Total_RBV")] + min_value: A[SignalR[float], PvSuffix("MinValue_RBV")] + min_x: A[SignalR[float], PvSuffix("MinX_RBV")] + min_y: A[SignalR[float], PvSuffix("MinY_RBV")] + max_value: A[SignalR[float], PvSuffix("MaxValue_RBV")] + max_x: A[SignalR[float], PvSuffix("MaxX_RBV")] + max_y: A[SignalR[float], PvSuffix("MaxY_RBV")] mean_value: A[SignalR[float], PvSuffix("MeanValue_RBV")] + sigma: A[SignalR[float], PvSuffix("Sigma_RBV")] + total: A[SignalR[float], PvSuffix("Total_RBV")] + net: A[SignalR[float], PvSuffix("Net_RBV")] # Centroid statistics compute_centroid: A[SignalRW[bool], PvSuffix.rbv("ComputeCentroid")] centroid_threshold: A[SignalRW[float], PvSuffix.rbv("CentroidThreshold")] + centroid_x: A[SignalR[float], PvSuffix("CentroidX_RBV")] + centroid_y: A[SignalR[float], PvSuffix("CentroidY_RBV")] + sigma_x: A[SignalR[float], PvSuffix("SigmaX_RBV")] + sigma_y: A[SignalR[float], PvSuffix("SigmaY_RBV")] + sigma_xy: A[SignalR[float], PvSuffix("SigmaXY_RBV")] # X and Y Profiles compute_profiles: A[SignalRW[bool], PvSuffix.rbv("ComputeProfiles")] profile_size_x: A[SignalR[int], PvSuffix("ProfileSizeX_RBV")] @@ -208,27 +233,39 @@ class NDStatsIO(NDPluginBaseIO): hist_min: A[SignalRW[float], PvSuffix.rbv("HistMin")] hist_max: A[SignalRW[float], PvSuffix.rbv("HistMax")] + def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): + super().__init__(prefix, with_pvi, name) + self.add_readables( + [ + self.compute_statistics, + self.bgd_width, + self.compute_centroid, + self.centroid_threshold, + self.compute_profiles, + self.cursor_x, + self.cursor_y, + self.compute_histogram, + self.hist_size, + self.hist_min, + self.hist_max, + ], + Format.CONFIG_SIGNAL, + ) + self.add_readables( + [self.total, self.mean_value, self.min_value, self.max_value], + Format.HINTED_UNCACHED_SIGNAL, + ) + self.add_readables( + [self.min_x, self.min_y, self.max_x, self.max_y], + Format.UNCACHED_SIGNAL, + ) -class NDROIStatNIO(EpicsDevice): - """Defines the parameters for a single ROI used for statistics calculation. - Each instance represents a single ROI, with attributes for its position - (min_x, min_y) and size (size_x, size_y), as well as a name and use status. +class NDROIStatNIO(StandardReadable, EpicsDevice): + """Defines the parameters for a single ROI used for statistics calculation. This mirrors the interface provided by ADCore/db/NDROIStatN.template. See definition in ADApp/pluginSrc/NDPluginROIStat.h in https://github.com/areaDetector/ADCore. - - Attributes: - name: The name of the ROI. - use: Flag indicating whether the ROI is used. - min_x: The start X-coordinate of the ROI. - min_y: The start Y-coordinate of the ROI. - size_x: The width of the ROI. - size_y: The height of the ROI. - min_value: Minimum count value in the ROI. - max_value: Maximum count value in the ROI. - mean_value: Mean counts value in the ROI. - total: Total counts in the ROI. """ name_: A[SignalRW[str], PvSuffix("Name")] @@ -237,12 +274,22 @@ class NDROIStatNIO(EpicsDevice): min_y: A[SignalRW[int], PvSuffix.rbv("MinY")] size_x: A[SignalRW[int], PvSuffix.rbv("SizeX")] size_y: A[SignalRW[int], PvSuffix.rbv("SizeY")] - # stats min_value: A[SignalR[float], PvSuffix("MinValue_RBV")] max_value: A[SignalR[float], PvSuffix("MaxValue_RBV")] mean_value: A[SignalR[float], PvSuffix("MeanValue_RBV")] total: A[SignalR[float], PvSuffix("Total_RBV")] + def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): + super().__init__(prefix, with_pvi, name) + self.add_readables( + [self.use, self.min_x, self.min_y, self.size_x, self.size_y], + Format.CONFIG_SIGNAL, + ) + self.add_readables( + [self.min_value, self.max_value, self.mean_value, self.total], + Format.HINTED_UNCACHED_SIGNAL, + ) + class NDROIStatIO(NDPluginBaseIO): """Plugin for calculating basic statistics for multiple ROIs. diff --git a/tests/unit_tests/epics/adcore/test_data_logic.py b/tests/unit_tests/epics/adcore/test_data_logic.py index 4696f9ee98..9e940744b3 100644 --- a/tests/unit_tests/epics/adcore/test_data_logic.py +++ b/tests/unit_tests/epics/adcore/test_data_logic.py @@ -282,14 +282,12 @@ async def test_describe_different_color_modes( # Expected to give the right shape in the descriptor await hdf_det.prepare(TriggerInfo()) describe = await hdf_det.describe() - assert describe == { - "detector": { - "dtype": "array", - "dtype_numpy": " Date: Wed, 12 Aug 2026 11:50:39 -0400 Subject: [PATCH 04/12] Update implementations to move config_sigs -> deadtime_sigs for trigger logics (cherry picked from commit 404930bbdaa8648f63cdb7224ac120b6e85fa4cc) --- src/ophyd_async/epics/adaravis.py | 2 +- src/ophyd_async/epics/advimba.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ophyd_async/epics/adaravis.py b/src/ophyd_async/epics/adaravis.py index 2a0d89d176..3cbb10ed12 100644 --- a/src/ophyd_async/epics/adaravis.py +++ b/src/ophyd_async/epics/adaravis.py @@ -63,7 +63,7 @@ class AravisTriggerLogic(DetectorTriggerLogic): process_plugin: NDProcessIO | None override_deadtime: float | None = None - def config_sigs(self) -> set[SignalR]: + def deadtime_sigs(self) -> set[SignalR]: return {self.driver.model} def get_deadtime(self, config_values: SignalDict) -> float: diff --git a/src/ophyd_async/epics/advimba.py b/src/ophyd_async/epics/advimba.py index 1f74af96e4..529b98e932 100644 --- a/src/ophyd_async/epics/advimba.py +++ b/src/ophyd_async/epics/advimba.py @@ -100,7 +100,7 @@ class VimbaTriggerLogic(DetectorTriggerLogic): process_plugin: NDProcessIO | None override_deadtime: float | None = None - def config_sigs(self) -> set[SignalR]: + def deadtime_sigs(self) -> set[SignalR]: return {self.driver.model} def get_deadtime(self, config_values: SignalDict) -> float: From 6aabf27634b7a0e01f42c6c3bc8cc5882adbf858 Mon Sep 17 00:00:00 2001 From: Jakub Wlodek Date: Wed, 12 Aug 2026 12:18:11 -0400 Subject: [PATCH 05/12] Fix container test (cherry picked from commit dd36b7a64825d848f8ccf7792c3ba9d3c486399b) --- tests/container_tests/test_adsim_system.py | 58 ++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/container_tests/test_adsim_system.py b/tests/container_tests/test_adsim_system.py index f1791639f0..b9a3c3f426 100644 --- a/tests/container_tests/test_adsim_system.py +++ b/tests/container_tests/test_adsim_system.py @@ -198,10 +198,24 @@ def test_software_triggering( "data": { "adsim-driver-acquire_period": 0.005, "adsim-driver-acquire_time": 0.1, + "adsim-driver-num_images": 1, + "adsim-driver-image_mode": "Multiple", + "adsim-driver-manufacturer": "Simulated detector", + "adsim-driver-model": "Basic simulator", + "adsim-driver-serial_number": "No serial number", + "adsim-driver-sdk_version": ANY, + "adsim-driver-firmware_version": "No firmware", }, "timestamps": { "adsim-driver-acquire_period": ANY, "adsim-driver-acquire_time": ANY, + "adsim-driver-num_images": ANY, + "adsim-driver-image_mode": ANY, + "adsim-driver-manufacturer": ANY, + "adsim-driver-model": ANY, + "adsim-driver-serial_number": ANY, + "adsim-driver-sdk_version": ANY, + "adsim-driver-firmware_version": ANY, }, "data_keys": { "adsim-driver-acquire_period": { @@ -220,6 +234,50 @@ def test_software_triggering( "units": "", "precision": 3, }, + "adsim-driver-num_images": { + "dtype": "integer", + "shape": [], + "dtype_numpy": " Date: Wed, 19 Aug 2026 12:31:29 +0000 Subject: [PATCH 06/12] Opt plugins into read()/describe() explicitly in the AD use case tests #1395 registered every StandardReadable plugin as a CHILD readable in AreaDetector.__init__. That is dropped in the rebase onto #1400, for the reason @jwlodek gave on the PR himself: "what if we didn't add plugins as readables automatically when passed in at __init__, and instead explicitly required an add_readables call to include them?" and the reason @coretl gave for asking: a detector typically carries far more plugins (16 ROIs, 16 stats) than are wired into its chain, so reporting all of them gives no way to tell which values are meaningful. So the three tests that expected stats readings now say so, with set_readable_format(stat, Format.CHILD). NDStatsIO declares its own config and hinted signals, so opting the plugin in as a CHILD picks up exactly what these tests already assert. The wait_for_plugins assertions go with the AreaDetector override that set it, which both reviewers agreed was too hidden and belongs in an explicit data logic instead. Assisted-by: Claude Opus 5.5 (cherry picked from commit f56c52368a961fdb1053e888811d44a33190782d) --- tests/unit_tests/epics/adcore/test_use_cases.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/epics/adcore/test_use_cases.py b/tests/unit_tests/epics/adcore/test_use_cases.py index 9380a9d6c0..dd4e9bbf9a 100644 --- a/tests/unit_tests/epics/adcore/test_use_cases.py +++ b/tests/unit_tests/epics/adcore/test_use_cases.py @@ -2,6 +2,7 @@ import pytest +from ophyd_async.core import StandardReadableFormat as Format from ophyd_async.core import ( StaticPathProvider, TriggerInfo, @@ -24,6 +25,8 @@ async def test_step_scan_hdf_detector_with_stats_and_temp( plugins={"stats": stat}, ) temp = epics_signal_r(float, "SAMP:TEMP:RBV") + # Plugins are not registered automatically; opt this one in + det.set_readable_format(stat, Format.CHILD) ndattributes = [ adcore.NDAttributeParam( name="det-sum", @@ -46,7 +49,6 @@ async def test_step_scan_hdf_detector_with_stats_and_temp( set_mock_value(det.driver.array_size_y, 768) await det.stage() - assert await det.driver.wait_for_plugins.get_value() is False config_description = await det.describe_configuration() assert { "det-driver-acquire_period": { @@ -538,11 +540,11 @@ async def test_simdetector_with_stats_signal(): stat = adcore.NDStatsIO("PREFIX:STAT:") async with init_devices(mock=True): det = adsimdetector.SimDetector("PREFIX:", plugins={"stats": stat}) + # Plugins are not registered automatically; opt this one in + det.set_readable_format(stat, Format.CHILD) set_mock_value(stat.total, 1.8) await det.stage() - assert await det.driver.wait_for_plugins.get_value() is False await det.trigger() - assert await det.driver.wait_for_plugins.get_value() is True description = await det.describe() assert "det-stats-total" in description assert description["det-stats-total"] == { @@ -569,6 +571,8 @@ async def test_step_scan_keep_numimages( adcore.ADWriterFactory.hdf(static_path_provider), plugins={"stats": stat}, ) + # Plugins are not registered automatically; opt this one in + det.set_readable_format(stat, Format.CHILD) set_mock_value(det.driver.acquire_period, 0.1) set_mock_value(det.driver.acquire_time, 0.05) From 343b4789dbfda4449b824fc161fc8a2627ef0ce9 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 19 Aug 2026 13:46:55 +0000 Subject: [PATCH 07/12] Set WaitForPlugins from ADAcquireLogic rather than hiding it in AreaDetector _update_prepare_context override, conditional on number_of_events == 1. Both reviewers called that too hidden, and the condition was unnecessary: WaitForPlugins gates the driver's AcquireBusy going to 0 when acquisition *completes*, so it never throttles frames. A flyscan does exactly one wait_for_idle, at the end, which is precisely when the plugins should have drained. It belongs on the acquire logic because that is what owns "poke this, then check acquire is done": wait_for_idle already blocks on the acquire put callback, which is the AcquireBusy that WaitForPlugins gates, so the existing wait needs no change at all. Defaults to on, since racing is silent and the wait costs nothing when a file writer is the slowest thing in the chain anyway. Off is a constructor argument rather than a subclass, matching how driver_armed_signal already parametrises this class; a detector needing genuinely custom arm/idle logic still subclasses instead. Set from ensure_ready, not start_acquiring: the PV is latched and has no _RBV, so it is written once per scan while the driver is known idle rather than on the latency path before every arm. Stop first, then always write it, never leaving it at whatever the IOC booted with, so a scan cannot depend on who touched the PV last. ADContAcqAcquireLogic deliberately gets no equivalent: the driver is always acquiring there, so there is no AcquireBusy transition to gate. Said so on the class so it does not look like an oversight. Assisted-by: Claude Opus 5.5 (cherry picked from commit fe787ce0cbf7ce97584c429265e39ea4b4f84231) --- .../epics/adcore/_acquire_logic.py | 35 +++++++++++++++++++ .../unit_tests/epics/adcore/test_use_cases.py | 21 +++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/ophyd_async/epics/adcore/_acquire_logic.py b/src/ophyd_async/epics/adcore/_acquire_logic.py index 4d56d9a989..8371eb146b 100644 --- a/src/ophyd_async/epics/adcore/_acquire_logic.py +++ b/src/ophyd_async/epics/adcore/_acquire_logic.py @@ -16,10 +16,29 @@ class ADAcquireLogic(DetectorAcquireLogic): + """Arm and disarm an areaDetector driver. + + :param driver: The areaDetector driver to arm. + :param driver_armed_signal: + The signal to watch for the driver having armed, if it is not `acquire`. + :param wait_for_plugins: + Whether the detector should wait for its plugins to drain before it + reports acquisition as complete. `WaitForPlugins` makes the driver's + `AcquireBusy` wait for the number of queued arrays to reach zero, and + `wait_for_idle` blocks on the `acquire` put callback, so turning it on + is what makes a step scan read plugin scalars for the frame it just + took rather than the one before. Defaults to on, since racing is + silent and the wait costs nothing when a file writer is the slowest + thing in the chain anyway. Turn it off for a detector whose plugin + chain must not gate acquisition. + :param timeout: How long to wait for the driver to arm and to return to idle. + """ + def __init__( self, driver: ADBaseIO, driver_armed_signal: SignalR[bool] | None = None, + wait_for_plugins: bool = True, timeout: float = DEFAULT_TIMEOUT, ): self.driver = driver @@ -27,9 +46,19 @@ def __init__( self.driver_armed_signal = driver_armed_signal else: self.driver_armed_signal = driver.acquire + self.wait_for_plugins = wait_for_plugins self.timeout = timeout self.acquire_status: AsyncStatus | None = None + async def ensure_ready(self): + # Stop first: WaitForPlugins is latched and has no _RBV, so set it once + # per scan while the driver is known to be idle rather than on the + # latency path before every arm. Always written, never left at whatever + # the IOC happened to boot with, so a scan does not depend on who + # touched the PV last. + await self.ensure_stopped() + await self.driver.wait_for_plugins.set(self.wait_for_plugins) + async def start_acquiring(self): self.acquire_status = await set_and_wait_for_other_value( set_signal=self.driver.acquire, @@ -59,6 +88,12 @@ class ADContAcqAcquireLogic(DetectorAcquireLogic): The CB plugin must read directly from the driver, so `stage()` raises if its `NDArrayPort` is not the driver's port. + Deliberately has no `wait_for_plugins` equivalent to [](#ADAcquireLogic). + That setting gates the driver's `AcquireBusy` going to 0 at the end of an + acquisition, and here the driver is always acquiring -- there is no such + transition to wait on. `wait_for_idle` waits on the circular buffer's + capture instead. + :param driver: The continuously acquiring detector driver :param cb_plugin: The circular buffer plugin gating downstream plugins :param num_frames_to_skip: diff --git a/tests/unit_tests/epics/adcore/test_use_cases.py b/tests/unit_tests/epics/adcore/test_use_cases.py index dd4e9bbf9a..003bae71d0 100644 --- a/tests/unit_tests/epics/adcore/test_use_cases.py +++ b/tests/unit_tests/epics/adcore/test_use_cases.py @@ -49,6 +49,8 @@ async def test_step_scan_hdf_detector_with_stats_and_temp( set_mock_value(det.driver.array_size_y, 768) await det.stage() + # ensure_ready sets WaitForPlugins so read() sees this frame's scalars + assert await det.driver.wait_for_plugins.get_value() is True config_description = await det.describe_configuration() assert { "det-driver-acquire_period": { @@ -544,6 +546,7 @@ async def test_simdetector_with_stats_signal(): det.set_readable_format(stat, Format.CHILD) set_mock_value(stat.total, 1.8) await det.stage() + assert await det.driver.wait_for_plugins.get_value() is True await det.trigger() description = await det.describe() assert "det-stats-total" in description @@ -559,6 +562,24 @@ async def test_simdetector_with_stats_signal(): assert "det-stats-mean_value" in det.hints["fields"] +@pytest.mark.parametrize("wait_for_plugins", [True, False]) +async def test_acquire_logic_writes_wait_for_plugins_on_stage(wait_for_plugins: bool): + # Always written, never left at whatever the IOC booted with, so a scan + # cannot depend on who touched the PV last + driver = adcore.ADBaseIO("PREFIX:DET:") + async with init_devices(mock=True): + det = adcore.AreaDetector( + driver, + acquire_logic=adcore.ADAcquireLogic( + driver, wait_for_plugins=wait_for_plugins + ), + name="det", + ) + set_mock_value(det.driver.wait_for_plugins, not wait_for_plugins) + await det.stage() + assert await det.driver.wait_for_plugins.get_value() is wait_for_plugins + + async def test_step_scan_keep_numimages( static_path_provider: StaticPathProvider, monkeypatch: pytest.MonkeyPatch, From 776f61005b6345ca6b4f6314b5dce4776c5d3cdb Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 19 Aug 2026 14:20:09 +0000 Subject: [PATCH 08/12] Feed get_deadtime from the readable registry, dropping deadtime_sigs DetectorTriggerLogic no longer nominates the signals its deadtime depends on. get_trigger_deadtime passes the value of every signal the detector reports as configuration instead, so there is no second list to keep in step with the first: the values that determine deadtime are exactly the ones that ought to be recorded for the scan anyway. StandardReadable gains get_config_signals(), which walks the registry and recurses through CHILD-registered children. That recursion is what makes it work for areaDetector: AreaDetector registers the driver whole, and ADBaseIO declares model as CONFIG_SIGNAL, so the signal AravisTriggerLogic and VimbaTriggerLogic need arrives without either class saying anything. Both deadtime_sigs overrides are deleted. Unlike walk_config_signals it reads the registry rather than calling read_configuration(), so it does no I/O and includes read-only SignalRs. driver.model is a SignalR, so the existing helper would have dropped precisely the signal that matters. Two consequences, both real: - A logic needing a signal must ensure it is declared as configuration. If it is not, get_deadtime gets a KeyError rather than a silently fetched value. - get_trigger_deadtime now reads every configuration signal, where a logic could previously declare none and pay nothing. Reads are gathered so it stays one round trip, but it does mean the call now needs a connected device even when deadtime is a constant, so test_adpilatus::test_deadtime had to start connecting its detector. Also moves every add_readables call in adcore/_io.py into the signal annotations, which removes three __init__ methods that existed only to declare formats. The remaining registration calls elsewhere cannot follow: they take caller-supplied config_sigs or a driver passed in as a constructor argument, neither of which is declarable. Assisted-by: Claude Opus 5.5 (cherry picked from commit c239654d0491b350bca0aaba2ed62db184129f84) --- docs/how-to/implement-ad-detector.md | 9 +- docs/tutorials/implementing-detectors.md | 6 +- src/ophyd_async/core/_detector.py | 40 ++++-- src/ophyd_async/core/_readable.py | 22 +++ src/ophyd_async/epics/adaravis.py | 3 - src/ophyd_async/epics/adcore/_io.py | 130 ++++++++---------- src/ophyd_async/epics/advimba.py | 3 - .../unit_tests/core/test_standard_detector.py | 14 +- tests/unit_tests/epics/test_adpilatus.py | 11 +- 9 files changed, 133 insertions(+), 105 deletions(-) diff --git a/docs/how-to/implement-ad-detector.md b/docs/how-to/implement-ad-detector.md index 93b0e4c638..aab9795ea7 100644 --- a/docs/how-to/implement-ad-detector.md +++ b/docs/how-to/implement-ad-detector.md @@ -28,11 +28,14 @@ Implement methods for each trigger mode your detector supports: Only implement the prepare methods for trigger modes your detector actually supports. The detector will automatically report which trigger types are available based on which methods are implemented. -If the detector has configuration values that should be captured in the scan then implement: -- `config_sigs()` - Return the set of signals that should appear in read_configuration() +Configuration values that should be captured in the scan are declared on the IO class, +with [](#StandardReadableFormat.CONFIG_SIGNAL), exactly as on any other +[](#StandardReadable). The trigger logic does not nominate them. If you support external triggering you should also implement: -- `get_deadtime(config_values)` - Calculate the minimum time between exposures based on configuration values +- `get_deadtime(config_values)` - Calculate the minimum time between exposures. It is + given the value of *every* signal the detector reports as configuration, so a signal + it needs must be declared with [](#StandardReadableFormat.CONFIG_SIGNAL). To preserve hardware state in plans like [`bp.count`](#bluesky.plans.count) and [`bps.trigger_and_read`](#bluesky.plan_stubs.trigger_and_read) that call `trigger()` diff --git a/docs/tutorials/implementing-detectors.md b/docs/tutorials/implementing-detectors.md index 4fa2fe93ea..0c955d4d5d 100644 --- a/docs/tutorials/implementing-detectors.md +++ b/docs/tutorials/implementing-detectors.md @@ -199,8 +199,10 @@ If we wanted to support external triggering, we would also implement: - `prepare_level()` for external level/gate triggering (high level duration determines exposure) We could also implement: -- `get_deadtime()` to calculate the minimum time between exposures based on configuration -- `config_sigs()` to return signals that should appear in read_configuration() +- `get_deadtime()` to calculate the minimum time between exposures. It is given the value + of every signal the detector reports as configuration, so a signal it needs must be + declared with [](#StandardReadableFormat.CONFIG_SIGNAL) like on any other + [](#StandardReadable): the trigger logic does not nominate signals itself - `default_trigger_info()` to return the [](#TriggerInfo) to use when `trigger()` is called without a preceding `prepare()` (governed by [](#OPHYD_ASYNC_PRESERVE_DETECTOR_STATE)). ### `BlobAcquireLogic` diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index fba36ce42d..8c8fd1450d 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -144,19 +144,22 @@ class DetectorTriggerLogic: - Multi-exposure collection batching Subclasses must implement the appropriate `prepare_*` method for any trigger - mode the detector supports, `get_deadtime` if it supports external - triggering, and `deadtime_sigs` if the deadtime would vary according to - detector parameters. + mode the detector supports, and `get_deadtime` if it supports external + triggering. """ - def deadtime_sigs(self) -> set[SignalR]: - """Return the signals whose values are needed to compute deadtime.""" - return set() - def get_deadtime(self, config_values: SignalDict) -> float: """Return the deadtime in seconds for the detector. - :param config_values: the value of each signal in `deadtime_sigs` + :param config_values: + The value of every `Signal` the detector reports as configuration, + i.e. everything registered with + [](#StandardReadableFormat.CONFIG_SIGNAL), including those declared + on children registered as [](#StandardReadableFormat.CHILD). A + logic that needs a signal must make sure it is declared as + configuration, rather than nominating it separately: the values + that determine deadtime are exactly the ones that ought to be + recorded for the scan anyway. """ raise NotImplementedError(self) @@ -464,6 +467,12 @@ async def get_trigger_deadtime( ) -> tuple[set[DetectorTrigger], float | None]: """Get supported trigger types and deadtime for the detector. + The trigger logic is given the value of every signal this detector + reports as configuration, rather than nominating the ones it wants: + the values that determine deadtime are exactly the ones that ought to + be recorded for the scan anyway, so a second list to keep in step + earned nothing. + :param settings: Optional settings to use when getting configuration values :return: Tuple of supported trigger types and deadtime in seconds """ @@ -471,14 +480,23 @@ async def get_trigger_deadtime( self._trigger_logic.get_deadtime ): config_values = SignalDict() - for sig in self._trigger_logic.deadtime_sigs(): + to_read: list[SignalR] = [] + for sig in self.get_config_signals(): if settings and sig in settings: # Use value from settings if it is in there # cast to a SignalRW because settings can only contain those config_values[sig] = settings[cast(SignalRW, sig)] else: - # Get the value live - config_values[sig] = await sig.get_value() + to_read.append(sig) + # Read live values concurrently: this is now every configuration + # signal rather than a handful the logic named, so doing it in + # series would be one round trip per signal + for sig, value in zip( + to_read, + await asyncio.gather(*(sig.get_value() for sig in to_read)), + strict=True, + ): + config_values[sig] = value deadtime = self._trigger_logic.get_deadtime(config_values) else: deadtime = None diff --git a/src/ophyd_async/core/_readable.py b/src/ophyd_async/core/_readable.py index 7ea972fd92..b37057b539 100644 --- a/src/ophyd_async/core/_readable.py +++ b/src/ophyd_async/core/_readable.py @@ -444,6 +444,28 @@ def set_readable_format( # changing a format does not reorder `hints` self._readables[device] = format + def get_config_signals(self) -> set[SignalR]: + """Return every `Signal` contributing to `read_configuration()`. + + Recurses into children registered as + [](#StandardReadableFormat.CHILD), so a Device that registers a + sub-Device whole picks up whatever that sub-Device declares as + configuration. + + Unlike [](#walk_config_signals) this reads the registry rather than + calling `read_configuration()`, so it does no I/O and includes + read-only `SignalR`s as well as `SignalRW`s. + """ + signals: set[SignalR] = set() + for device, format in self._readables.items(): + if format is StandardReadableFormat.CONFIG_SIGNAL: + signals.add(_as_signal_r(device)) + elif format is StandardReadableFormat.CHILD and isinstance( + device, StandardReadable + ): + signals |= device.get_config_signals() + return signals + def reset_readable_formats(self) -> None: """Undo every runtime format change, back to how the class declared them. diff --git a/src/ophyd_async/epics/adaravis.py b/src/ophyd_async/epics/adaravis.py index 3cbb10ed12..26595e279c 100644 --- a/src/ophyd_async/epics/adaravis.py +++ b/src/ophyd_async/epics/adaravis.py @@ -63,9 +63,6 @@ class AravisTriggerLogic(DetectorTriggerLogic): process_plugin: NDProcessIO | None override_deadtime: float | None = None - def deadtime_sigs(self) -> set[SignalR]: - return {self.driver.model} - def get_deadtime(self, config_values: SignalDict) -> float: return get_camera_deadtime( model=config_values[self.driver.model], diff --git a/src/ophyd_async/epics/adcore/_io.py b/src/ophyd_async/epics/adcore/_io.py index 4cd0134841..65ec984eed 100644 --- a/src/ophyd_async/epics/adcore/_io.py +++ b/src/ophyd_async/epics/adcore/_io.py @@ -176,22 +176,16 @@ class NDROIIO(StandardReadable, NDPluginBaseIO): See HTML docs at https://areadetector.github.io/areaDetector/ADCore/NDPluginROI.html """ - min_x: A[SignalRW[int], PvSuffix.rbv("MinX")] - min_y: A[SignalRW[int], PvSuffix.rbv("MinY")] - size_x: A[SignalRW[int], PvSuffix.rbv("SizeX")] - size_y: A[SignalRW[int], PvSuffix.rbv("SizeY")] + min_x: A[SignalRW[int], PvSuffix.rbv("MinX"), Format.CONFIG_SIGNAL] + min_y: A[SignalRW[int], PvSuffix.rbv("MinY"), Format.CONFIG_SIGNAL] + size_x: A[SignalRW[int], PvSuffix.rbv("SizeX"), Format.CONFIG_SIGNAL] + size_y: A[SignalRW[int], PvSuffix.rbv("SizeY"), Format.CONFIG_SIGNAL] size_z: A[SignalRW[int], PvSuffix.rbv("SizeZ")] bin_x: A[SignalRW[int], PvSuffix.rbv("BinX")] bin_y: A[SignalRW[int], PvSuffix.rbv("BinY")] bin_z: A[SignalRW[int], PvSuffix.rbv("BinZ")] - def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): - super().__init__(prefix, with_pvi, name) - self.add_readables( - [self.min_x, self.min_y, self.size_x, self.size_y], Format.CONFIG_SIGNAL - ) - class NDStatsIO(StandardReadable, NDPluginBaseIO): """Plugin for computing statistics from an image or ROI within an image. @@ -201,64 +195,53 @@ class NDStatsIO(StandardReadable, NDPluginBaseIO): """ # Basic statistics - compute_statistics: A[SignalRW[bool], PvSuffix.rbv("ComputeStatistics")] - bgd_width: A[SignalRW[int], PvSuffix.rbv("BgdWidth")] - min_value: A[SignalR[float], PvSuffix("MinValue_RBV")] - min_x: A[SignalR[float], PvSuffix("MinX_RBV")] - min_y: A[SignalR[float], PvSuffix("MinY_RBV")] - max_value: A[SignalR[float], PvSuffix("MaxValue_RBV")] - max_x: A[SignalR[float], PvSuffix("MaxX_RBV")] - max_y: A[SignalR[float], PvSuffix("MaxY_RBV")] - mean_value: A[SignalR[float], PvSuffix("MeanValue_RBV")] + compute_statistics: A[ + SignalRW[bool], PvSuffix.rbv("ComputeStatistics"), Format.CONFIG_SIGNAL + ] + bgd_width: A[SignalRW[int], PvSuffix.rbv("BgdWidth"), Format.CONFIG_SIGNAL] + min_value: A[ + SignalR[float], PvSuffix("MinValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] + min_x: A[SignalR[float], PvSuffix("MinX_RBV"), Format.UNCACHED_SIGNAL] + min_y: A[SignalR[float], PvSuffix("MinY_RBV"), Format.UNCACHED_SIGNAL] + max_value: A[ + SignalR[float], PvSuffix("MaxValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] + max_x: A[SignalR[float], PvSuffix("MaxX_RBV"), Format.UNCACHED_SIGNAL] + max_y: A[SignalR[float], PvSuffix("MaxY_RBV"), Format.UNCACHED_SIGNAL] + mean_value: A[ + SignalR[float], PvSuffix("MeanValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] sigma: A[SignalR[float], PvSuffix("Sigma_RBV")] - total: A[SignalR[float], PvSuffix("Total_RBV")] + total: A[SignalR[float], PvSuffix("Total_RBV"), Format.HINTED_UNCACHED_SIGNAL] net: A[SignalR[float], PvSuffix("Net_RBV")] # Centroid statistics - compute_centroid: A[SignalRW[bool], PvSuffix.rbv("ComputeCentroid")] - centroid_threshold: A[SignalRW[float], PvSuffix.rbv("CentroidThreshold")] + compute_centroid: A[ + SignalRW[bool], PvSuffix.rbv("ComputeCentroid"), Format.CONFIG_SIGNAL + ] + centroid_threshold: A[ + SignalRW[float], PvSuffix.rbv("CentroidThreshold"), Format.CONFIG_SIGNAL + ] centroid_x: A[SignalR[float], PvSuffix("CentroidX_RBV")] centroid_y: A[SignalR[float], PvSuffix("CentroidY_RBV")] sigma_x: A[SignalR[float], PvSuffix("SigmaX_RBV")] sigma_y: A[SignalR[float], PvSuffix("SigmaY_RBV")] sigma_xy: A[SignalR[float], PvSuffix("SigmaXY_RBV")] # X and Y Profiles - compute_profiles: A[SignalRW[bool], PvSuffix.rbv("ComputeProfiles")] + compute_profiles: A[ + SignalRW[bool], PvSuffix.rbv("ComputeProfiles"), Format.CONFIG_SIGNAL + ] profile_size_x: A[SignalR[int], PvSuffix("ProfileSizeX_RBV")] profile_size_y: A[SignalR[int], PvSuffix("ProfileSizeY_RBV")] - cursor_x: A[SignalRW[int], PvSuffix.rbv("CursorX")] - cursor_y: A[SignalRW[int], PvSuffix.rbv("CursorY")] + cursor_x: A[SignalRW[int], PvSuffix.rbv("CursorX"), Format.CONFIG_SIGNAL] + cursor_y: A[SignalRW[int], PvSuffix.rbv("CursorY"), Format.CONFIG_SIGNAL] # Array Histogram - compute_histogram: A[SignalRW[bool], PvSuffix.rbv("ComputeHistogram")] - hist_size: A[SignalRW[int], PvSuffix.rbv("HistSize")] - hist_min: A[SignalRW[float], PvSuffix.rbv("HistMin")] - hist_max: A[SignalRW[float], PvSuffix.rbv("HistMax")] - - def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): - super().__init__(prefix, with_pvi, name) - self.add_readables( - [ - self.compute_statistics, - self.bgd_width, - self.compute_centroid, - self.centroid_threshold, - self.compute_profiles, - self.cursor_x, - self.cursor_y, - self.compute_histogram, - self.hist_size, - self.hist_min, - self.hist_max, - ], - Format.CONFIG_SIGNAL, - ) - self.add_readables( - [self.total, self.mean_value, self.min_value, self.max_value], - Format.HINTED_UNCACHED_SIGNAL, - ) - self.add_readables( - [self.min_x, self.min_y, self.max_x, self.max_y], - Format.UNCACHED_SIGNAL, - ) + compute_histogram: A[ + SignalRW[bool], PvSuffix.rbv("ComputeHistogram"), Format.CONFIG_SIGNAL + ] + hist_size: A[SignalRW[int], PvSuffix.rbv("HistSize"), Format.CONFIG_SIGNAL] + hist_min: A[SignalRW[float], PvSuffix.rbv("HistMin"), Format.CONFIG_SIGNAL] + hist_max: A[SignalRW[float], PvSuffix.rbv("HistMax"), Format.CONFIG_SIGNAL] class NDROIStatNIO(StandardReadable, EpicsDevice): @@ -269,26 +252,21 @@ class NDROIStatNIO(StandardReadable, EpicsDevice): """ name_: A[SignalRW[str], PvSuffix("Name")] - use: A[SignalRW[bool], PvSuffix.rbv("Use")] - min_x: A[SignalRW[int], PvSuffix.rbv("MinX")] - min_y: A[SignalRW[int], PvSuffix.rbv("MinY")] - size_x: A[SignalRW[int], PvSuffix.rbv("SizeX")] - size_y: A[SignalRW[int], PvSuffix.rbv("SizeY")] - min_value: A[SignalR[float], PvSuffix("MinValue_RBV")] - max_value: A[SignalR[float], PvSuffix("MaxValue_RBV")] - mean_value: A[SignalR[float], PvSuffix("MeanValue_RBV")] - total: A[SignalR[float], PvSuffix("Total_RBV")] - - def __init__(self, prefix: str = "", with_pvi: bool = False, name: str = ""): - super().__init__(prefix, with_pvi, name) - self.add_readables( - [self.use, self.min_x, self.min_y, self.size_x, self.size_y], - Format.CONFIG_SIGNAL, - ) - self.add_readables( - [self.min_value, self.max_value, self.mean_value, self.total], - Format.HINTED_UNCACHED_SIGNAL, - ) + use: A[SignalRW[bool], PvSuffix.rbv("Use"), Format.CONFIG_SIGNAL] + min_x: A[SignalRW[int], PvSuffix.rbv("MinX"), Format.CONFIG_SIGNAL] + min_y: A[SignalRW[int], PvSuffix.rbv("MinY"), Format.CONFIG_SIGNAL] + size_x: A[SignalRW[int], PvSuffix.rbv("SizeX"), Format.CONFIG_SIGNAL] + size_y: A[SignalRW[int], PvSuffix.rbv("SizeY"), Format.CONFIG_SIGNAL] + min_value: A[ + SignalR[float], PvSuffix("MinValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] + max_value: A[ + SignalR[float], PvSuffix("MaxValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] + mean_value: A[ + SignalR[float], PvSuffix("MeanValue_RBV"), Format.HINTED_UNCACHED_SIGNAL + ] + total: A[SignalR[float], PvSuffix("Total_RBV"), Format.HINTED_UNCACHED_SIGNAL] class NDROIStatIO(NDPluginBaseIO): diff --git a/src/ophyd_async/epics/advimba.py b/src/ophyd_async/epics/advimba.py index 529b98e932..9567ee5c30 100644 --- a/src/ophyd_async/epics/advimba.py +++ b/src/ophyd_async/epics/advimba.py @@ -100,9 +100,6 @@ class VimbaTriggerLogic(DetectorTriggerLogic): process_plugin: NDProcessIO | None override_deadtime: float | None = None - def deadtime_sigs(self) -> set[SignalR]: - return {self.driver.model} - def get_deadtime(self, config_values: SignalDict) -> float: return get_camera_deadtime( model=config_values[self.driver.model], diff --git a/tests/unit_tests/core/test_standard_detector.py b/tests/unit_tests/core/test_standard_detector.py index 342a587525..7137e0a4d3 100644 --- a/tests/unit_tests/core/test_standard_detector.py +++ b/tests/unit_tests/core/test_standard_detector.py @@ -99,11 +99,12 @@ class DeadtimeTriggerLogic(DetectorTriggerLogic): def __init__(self, deadtime_signal: SignalR[float]): self.deadtime_signal = deadtime_signal - def deadtime_sigs(self) -> set[SignalR]: - return {self.deadtime_signal} - def get_deadtime(self, config_values: SignalDict) -> float: - """Return the deadtime from the signal value.""" + """Return the deadtime from the signal value. + + The signal reaches us because the detector registers it as + CONFIG_SIGNAL, not because this logic nominates it. + """ return config_values[self.deadtime_signal] async def prepare_internal(self, num: int, livetime: float, deadtime: float): @@ -232,6 +233,10 @@ async def test_get_trigger_deadtime( det = StandardDetector() if trigger_logic: det.add_detector_logics(trigger_logic) + if isinstance(trigger_logic, DeadtimeTriggerLogic): + # A logic that needs a signal declares it as configuration; the logic + # no longer nominates signals separately + det.set_readable_format(trigger_logic.deadtime_signal, Format.CONFIG_SIGNAL) triggers, deadtime = await det.get_trigger_deadtime() assert triggers == expected_triggers assert deadtime == expected_deadtime @@ -247,6 +252,7 @@ async def test_get_trigger_deadtime_with_settings(): det.sig = deadtime_signal tl = DeadtimeTriggerLogic(deadtime_signal) det.add_detector_logics(tl) + det.set_readable_format(deadtime_signal, Format.CONFIG_SIGNAL) # Verify initial deadtime from signal triggers, deadtime = await det.get_trigger_deadtime() diff --git a/tests/unit_tests/epics/test_adpilatus.py b/tests/unit_tests/epics/test_adpilatus.py index 69d0f25020..70641c51db 100644 --- a/tests/unit_tests/epics/test_adpilatus.py +++ b/tests/unit_tests/epics/test_adpilatus.py @@ -61,9 +61,14 @@ async def test_threshold_apply_triggers_command( ) async def test_deadtime(readout_time: adpilatus.PilatusReadoutTime, tmp_path): path_provider = StaticPathProvider(StaticFilenameProvider("data"), tmp_path) - pilatus = adpilatus.PilatusDetector( - "PREFIX:", adcore.ADWriterFactory.hdf(path_provider), readout_time=readout_time - ) + # Connected because get_trigger_deadtime() now reads every configuration + # signal, even for a logic like this one whose deadtime is a constant + async with init_devices(mock=True): + pilatus = adpilatus.PilatusDetector( + "PREFIX:", + adcore.ADWriterFactory.hdf(path_provider), + readout_time=readout_time, + ) trigger_modes, deadtime = await pilatus.get_trigger_deadtime() assert trigger_modes == { DetectorTrigger.INTERNAL, From 6e0b21c44d62b62e8956086f369b925b9c9622b4 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 19 Aug 2026 14:26:04 +0000 Subject: [PATCH 09/12] Make the config signal walk a private helper rather than public API get_config_signals was added as a public method on StandardReadable in the previous commit. #1367 restructures detector logic, and whether this should be a method, a function, or something else falls out of that, so it stays a module private _config_signals() in _readable.py until then. Nothing is added to core.__all__. Taking a Device rather than a StandardReadable also simplifies the recursion: a CHILD that is not a StandardReadable contributes nothing without the caller having to check. Assisted-by: Claude Opus 5.5 (cherry picked from commit ca5fcb7680eae179c98b0419eaa0f9653b0ddebb) --- src/ophyd_async/core/_detector.py | 3 +- src/ophyd_async/core/_readable.py | 52 ++++++++++++++++++------------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index 8c8fd1450d..ddb085d57a 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -31,6 +31,7 @@ from ._readable import ( StandardReadable, StandardReadableFormat, + _config_signals, _HintedFields, _Verb, ) @@ -481,7 +482,7 @@ async def get_trigger_deadtime( ): config_values = SignalDict() to_read: list[SignalR] = [] - for sig in self.get_config_signals(): + for sig in _config_signals(self): if settings and sig in settings: # Use value from settings if it is in there # cast to a SignalRW because settings can only contain those diff --git a/src/ophyd_async/core/_readable.py b/src/ophyd_async/core/_readable.py index b37057b539..59fd6552f7 100644 --- a/src/ophyd_async/core/_readable.py +++ b/src/ophyd_async/core/_readable.py @@ -444,28 +444,6 @@ def set_readable_format( # changing a format does not reorder `hints` self._readables[device] = format - def get_config_signals(self) -> set[SignalR]: - """Return every `Signal` contributing to `read_configuration()`. - - Recurses into children registered as - [](#StandardReadableFormat.CHILD), so a Device that registers a - sub-Device whole picks up whatever that sub-Device declares as - configuration. - - Unlike [](#walk_config_signals) this reads the registry rather than - calling `read_configuration()`, so it does no I/O and includes - read-only `SignalR`s as well as `SignalRW`s. - """ - signals: set[SignalR] = set() - for device, format in self._readables.items(): - if format is StandardReadableFormat.CONFIG_SIGNAL: - signals.add(_as_signal_r(device)) - elif format is StandardReadableFormat.CHILD and isinstance( - device, StandardReadable - ): - signals |= device.get_config_signals() - return signals - def reset_readable_formats(self) -> None: """Undo every runtime format change, back to how the class declared them. @@ -541,6 +519,36 @@ def add_readables( self.set_readable_format(device, format) +def _config_signals(device: Device) -> set[SignalR]: + """Return every `Signal` contributing to a Device's `read_configuration()`. + + Recurses into children registered as [](#StandardReadableFormat.CHILD), so + a Device that registers a sub-Device whole picks up whatever that + sub-Device declares as configuration. A Device that is not a + `StandardReadable` contributes nothing. + + Unlike `walk_config_signals` this reads the registry rather than calling + `read_configuration()`, so it does no I/O and includes read-only `SignalR`s + as well as `SignalRW`s -- `ADBaseIO.model` is a `SignalR`, and it is + exactly the signal an areaDetector trigger logic needs for deadtime. + + Deliberately private and unexported: #1367 restructures detector logic, so + what the public shape of this should be is not settled yet. + """ + signals: set[SignalR] = set() + if not isinstance(device, StandardReadable): + return signals + for entry in device._readables.entries(): # noqa: SLF001 + match entry: + case (signal, Format.CONFIG_SIGNAL): + signals.add(signal) + case (child, Format.CHILD): + signals |= _config_signals(child) + case _: + pass + return signals + + #: Reserved key under which [](#store_settings) writes readable formats. Chosen #: so that it cannot be a dotted attribute path: `<` cannot start a Python #: identifier, so no attribute assignment can produce a colliding key, and From 341d0ce3e9a78d356740cd046f7ab510a4d7bb4a Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Fri, 28 Aug 2026 11:52:37 +0000 Subject: [PATCH 10/12] Delete the prepare_single data logic tier A StandardDetector is a StandardReadable now, so a source that produces one value per event is a registered signal, not a data logic: set_readable_format puts it in read() and describe() for any number of collections, where a prepare_single logic dropped itself with a warning as soon as a scan asked for more than one. That leaves the tier with nothing of its own, as its only implementation, PluginSignalDataLogic, has already gone the same way. So prepare_single, ReadableDataProvider and SignalDataProvider go rather than staying as a second way to get a scalar into an event. Also restores the two doc sections held back from the previous branch, since this is the change that makes them true, and points ADR 0012 at 0023. Assisted-by: Claude Opus 5.5 --- .../decisions/0012-detector-rewrite.md | 4 + docs/how-to/implement-ad-detector.md | 11 +- src/ophyd_async/core/__init__.py | 4 - src/ophyd_async/core/_data_providers.py | 27 +---- src/ophyd_async/core/_detector.py | 46 ++------ .../unit_tests/core/test_standard_detector.py | 102 ++++++------------ 6 files changed, 57 insertions(+), 137 deletions(-) diff --git a/docs/explanations/decisions/0012-detector-rewrite.md b/docs/explanations/decisions/0012-detector-rewrite.md index 3e1145b66b..fa5d0949d4 100644 --- a/docs/explanations/decisions/0012-detector-rewrite.md +++ b/docs/explanations/decisions/0012-detector-rewrite.md @@ -5,6 +5,10 @@ Date: 2026-01-20 Accepted +Partly superseded by [ADR 0023](0023-runtime-readable-format.md), which made +`StandardDetector` a `StandardReadable` and removed the `prepare_single` tier along with +`ReadableDataProvider`. + ## Context The original `StandardDetector` implementation had several architectural issues: diff --git a/docs/how-to/implement-ad-detector.md b/docs/how-to/implement-ad-detector.md index aab9795ea7..036919307d 100644 --- a/docs/how-to/implement-ad-detector.md +++ b/docs/how-to/implement-ad-detector.md @@ -119,12 +119,15 @@ For example, for ADAravis: The composition-based architecture makes it possible to add multiple data outputs to a detector. After creating the detector, you can call `add_detector_logics()` to add additional data sources: ### Reading stats plugins alongside file writing +A plugin scalar needs no data logic: a detector is a [](#StandardReadable), so register +the signal and it appears in `read()` alongside whatever the file writer produces. + ```python -from ophyd_async.epics.adcore import PluginSignalDataLogic +from ophyd_async.core import StandardReadableFormat as Format -det = adaravis.AravisDetector(prefix, path_provider) -# Add stats total as a readable signal in events -det.add_detector_logics(adcore.PluginSignalDataLogic(det.driver, det.stats.total)) +det = adaravis.AravisDetector(prefix, path_provider, plugins={"stats": stats}) +# Add stats total as a hinted signal in events +det.set_readable_format(det.stats.total, Format.HINTED_UNCACHED_SIGNAL) ``` ### Multiple HDF writers for different ROIs diff --git a/src/ophyd_async/core/__init__.py b/src/ophyd_async/core/__init__.py index 9022728341..3583552b48 100644 --- a/src/ophyd_async/core/__init__.py +++ b/src/ophyd_async/core/__init__.py @@ -9,8 +9,6 @@ soft_command, ) from ._data_providers import ( - ReadableDataProvider, - SignalDataProvider, StreamableDataProvider, StreamResourceDataProvider, StreamResourceInfo, @@ -306,9 +304,7 @@ def __getattr__(name): "AutoMaxIncrementingPathProvider", "UUIDFilenameProvider", # Data Providers - "ReadableDataProvider", "StreamableDataProvider", - "SignalDataProvider", "StreamResourceInfo", "StreamResourceDataProvider", # Flyer diff --git a/src/ophyd_async/core/_data_providers.py b/src/ophyd_async/core/_data_providers.py index ad2983cb59..8faa7ba069 100644 --- a/src/ophyd_async/core/_data_providers.py +++ b/src/ophyd_async/core/_data_providers.py @@ -1,39 +1,14 @@ from abc import abstractmethod from collections.abc import AsyncIterator, Sequence -from dataclasses import dataclass from typing import Any -from bluesky.protocols import Reading, StreamAsset +from bluesky.protocols import StreamAsset from event_model import ComposeStreamResource, DataKey, StreamRange from ._signal import SignalR, SignalW from ._utils import ConfinedModel -class ReadableDataProvider: - @abstractmethod - async def make_datakeys(self) -> dict[str, DataKey]: - """Return a DataKey for each Readable that produces a Reading. - - Called before the first exposure is taken. - """ - - @abstractmethod - async def make_readings(self) -> dict[str, Reading]: - """Read the Signals and return their values.""" - - -@dataclass -class SignalDataProvider(ReadableDataProvider): - signal: SignalR - - async def make_datakeys(self) -> dict[str, DataKey]: - return await self.signal.describe() - - async def make_readings(self) -> dict[str, Reading]: - return await self.signal.read(cached=False) - - class StreamableDataProvider: collections_written_signal: SignalR[int] diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index ddb085d57a..eef40e4106 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -25,9 +25,7 @@ from event_model import DataKey from pydantic import Field, NonNegativeInt, PositiveInt, computed_field -from ophyd_async.core._log import logger - -from ._data_providers import ReadableDataProvider, StreamableDataProvider +from ._data_providers import StreamableDataProvider from ._readable import ( StandardReadable, StandardReadableFormat, @@ -322,19 +320,16 @@ async def _get_collections_written( class DetectorDataLogic: """Abstract base class for detector data logic and handling. - Implementations must implement either prepare_unbounded for data sources - that work with step scans as well as flyscans, or prepare_single for those - that only work with step scans. + Implementations must override `prepare_unbounded`. A source that produces a + single value per event, like a plugin scalar, does not need a data logic at + all: register its signal with + [](#StandardReadable.set_readable_format) instead. """ #: Add this suffix to the detector name to specify the datakey. These need to be #: different for each DetectorDataLogic added to a detector datakey_suffix: str = "" - async def prepare_single(self, datakey_name: str) -> ReadableDataProvider: - """Provider can only work for a single event.""" - raise NotImplementedError(self) - async def prepare_unbounded(self, datakey_name: str) -> StreamableDataProvider: """Provider can work for an unbounded number of collections.""" raise NotImplementedError(self) @@ -354,7 +349,6 @@ async def stop(self) -> None: @dataclass class _PrepareCtx: trigger_info: TriggerInfo - readable_data_providers: Sequence[ReadableDataProvider] streamable_data_providers: Sequence[StreamableDataProvider] collections_written: int @@ -525,7 +519,6 @@ async def _update_prepare_context(self, trigger_info: TriggerInfo) -> None: == trigger_info.collections_per_event ): # Reuse the existing data providers - readable_data_providers = self._prepare_ctx.readable_data_providers streamable_data_providers = self._prepare_ctx.streamable_data_providers else: # Stop the existing providers if there is a context and make new ones @@ -534,36 +527,19 @@ async def _update_prepare_context(self, trigger_info: TriggerInfo) -> None: await data_logic.stop() # Setup the data logic for the right number of collections streamable_coros: list[Awaitable[StreamableDataProvider]] = [] - readable_coros: list[Awaitable[ReadableDataProvider]] = [] for dl in self._data_logics: - if _data_logic_supported(dl.prepare_unbounded): - streamable_coros.append( - dl.prepare_unbounded(self.name + dl.datakey_suffix) - ) - elif _data_logic_supported(dl.prepare_single): - if trigger_info.number_of_collections == 1: - readable_coros.append( - dl.prepare_single(self.name + dl.datakey_suffix) - ) - else: - logger.warning( - f"DataLogic {dl} only supports a single collection, but " - "the detector was prepared for " - f"{trigger_info.number_of_collections} collections." - ) - else: + if not _data_logic_supported(dl.prepare_unbounded): msg = f"DataLogic hasn't overridden any prepare_* methods {dl}" raise RuntimeError(msg) - streamable_data_providers, readable_data_providers = await asyncio.gather( - asyncio.gather(*streamable_coros), - asyncio.gather(*readable_coros), - ) + streamable_coros.append( + dl.prepare_unbounded(self.name + dl.datakey_suffix) + ) + streamable_data_providers = await asyncio.gather(*streamable_coros) # Stash the prepare context so we can use it in trigger/kickoff self._prepare_ctx = _PrepareCtx( trigger_info=trigger_info, streamable_data_providers=streamable_data_providers, - readable_data_providers=readable_data_providers, collections_written=await _get_collections_written( streamable_data_providers ), @@ -794,8 +770,6 @@ def _extra_funcs_for(self, verb: _Verb) -> Iterator[Callable[[], Awaitable[dict] if verb not in (_Verb.DESCRIBE, _Verb.READ): return ctx = error_if_none(self._prepare_ctx, "Prepare not run") - for dp in ctx.readable_data_providers: - yield dp.make_datakeys if verb is _Verb.DESCRIBE else dp.make_readings if verb is _Verb.DESCRIBE: # Streamable providers describe their shape for a step scan, but # produce their data through collect_asset_docs rather than read() diff --git a/tests/unit_tests/core/test_standard_detector.py b/tests/unit_tests/core/test_standard_detector.py index 7137e0a4d3..211e96f793 100644 --- a/tests/unit_tests/core/test_standard_detector.py +++ b/tests/unit_tests/core/test_standard_detector.py @@ -12,9 +12,7 @@ DetectorDataLogic, DetectorTrigger, DetectorTriggerLogic, - ReadableDataProvider, Settings, - SignalDataProvider, SignalDict, SignalR, StandardDetector, @@ -131,26 +129,14 @@ async def ensure_stopped(self): self.disarm_count += 1 -class ReadableOnlyDataLogic(DetectorDataLogic): - """Produces only readable (non-streaming) data.""" - - def __init__(self): - self.signal = soft_signal_rw(int, initial_value=42, name="foo-value") - - async def prepare_single(self, datakey_name: str) -> ReadableDataProvider: - return SignalDataProvider(self.signal) - - def get_hinted_fields(self, datakey_name: str) -> Sequence[str]: - return ["foo-value"] - - class StreamableOnlyDataLogic(DetectorDataLogic): """Produces only streamable (file-based) data.""" - def __init__(self, tmp_path): + def __init__(self, tmp_path, datakey_suffix: str = ""): self.collections_written = soft_signal_rw(int) self.stop_count = 0 self.tmp_path = tmp_path + self.datakey_suffix = datakey_suffix async def prepare_unbounded(self, datakey_name: str) -> StreamableDataProvider: resource = StreamResourceInfo( @@ -439,10 +425,10 @@ async def test_arm_logic_called_on_stage(): assert al.armed is False -async def test_describe_before_prepare_raises(): +async def test_describe_before_prepare_raises(tmp_path): """Test that describe() fails before prepare().""" det = StandardDetector() - det.add_detector_logics(ReadableOnlyDataLogic()) + det.add_detector_logics(StreamableOnlyDataLogic(tmp_path)) with pytest.raises(RuntimeError, match="Prepare not run"): await det.describe() @@ -631,23 +617,21 @@ async def test_stage_resets_state(): assert await det.events_to_kickoff.get_value() == 0 -async def test_hints_from_single_data_logic(): +async def test_hints_from_single_data_logic(tmp_path): """Test that hints come from data logic.""" - det = StandardDetector() - det.add_detector_logics(ReadableOnlyDataLogic()) + det = StandardDetector(name="bar") + det.add_detector_logics(StreamableOnlyDataLogic(tmp_path)) await det.prepare(TriggerInfo()) - assert det.hints == { - "fields": ["foo-value"] - } # ReadableOnlyDataLogic uses foo-value + assert det.hints == {"fields": ["bar"]} async def test_hints_from_multiple_data_logics(tmp_path): """Test that hints are aggregated from multiple data logics.""" det = StandardDetector(name="bar") - dl1 = ReadableOnlyDataLogic() - dl2 = StreamableOnlyDataLogic(tmp_path) + dl1 = StreamableOnlyDataLogic(tmp_path) + dl2 = StreamableOnlyDataLogic(tmp_path, datakey_suffix="-extra") det.add_detector_logics(dl1, dl2) await det.prepare(TriggerInfo()) @@ -655,7 +639,7 @@ async def test_hints_from_multiple_data_logics(tmp_path): # Should include hints from both logics hints = det.hints assert "fields" in hints - assert hints["fields"] == ["foo-value", "bar"] + assert hints["fields"] == ["bar", "bar-extra"] @pytest.mark.parametrize( @@ -701,13 +685,13 @@ async def test_config_signals_in_describe_configuration( async def test_kickoff_without_streamable_data_raises(): """Test that kickoff() without streamable data fails.""" det = StandardDetector(name="foo") - det.add_detector_logics(JustInternalTriggerLogic(), ReadableOnlyDataLogic()) + det.add_detector_logics(JustInternalTriggerLogic()) - # Single event prepare for readable-only logic + # A detector with no data logic can still be triggered for a step scan await det.prepare(TriggerInfo()) await det.trigger() # This works - # Readable-only logic doesn't support kickoff + # ...but there is nothing to collect, so it cannot be flown await det.prepare(TriggerInfo(number_of_events=5)) with pytest.raises( ValueError, match="Detector foo is not streamable, so cannot kickoff" @@ -794,13 +778,13 @@ async def test_streamable_supports_both_step_and_fly(tmp_path): async def test_read_returns_correct_values(): - """Test that read() returns values from readable providers.""" + """Test that read() returns the values of registered signals.""" det = StandardDetector(name="det") - dl = ReadableOnlyDataLogic() - det.add_detector_logics(dl) + det.counts, _ = soft_signal_r_and_setter(int, 42, name="det-counts") + det.set_readable_format(det.counts, Format.HINTED_SIGNAL) await det.trigger() - await assert_reading(det, {"foo-value": {"value": 42}}) + await assert_reading(det, {"det-counts": {"value": 42}}) async def test_detector_with_no_logics(): @@ -909,19 +893,6 @@ async def ensure_stopped(self): assert al.shutter_closed is True # unstage() must close the shutter -async def test_multiple_collections_with_single_only_logic_warns(caplog): - """Test that requesting multiple collections warns, leads to empty prepare ctx.""" - det = StandardDetector() - det.add_detector_logics(ReadableOnlyDataLogic()) - - await det.prepare(TriggerInfo(number_of_events=5)) - - assert "only supports a single collection" in caplog.text - assert det._prepare_ctx is not None - assert det._prepare_ctx.readable_data_providers == [] - assert det._prepare_ctx.streamable_data_providers == [] - - async def test_data_logic_with_no_prepare_methods_raises(): """Test error when DataLogic doesn't override any prepare methods.""" @@ -986,16 +957,18 @@ async def test_different_collections_written_raises(tmp_path): await det.kickoff() -async def test_multiple_data_logics(tmp_path): - """Test detector with multiple data logics.""" +async def test_data_logic_and_registered_signal(tmp_path): + """A registered signal and a data logic both describe(), only one collects.""" det = StandardDetector(name="det") - dl1 = ReadableOnlyDataLogic() - dl2 = StreamableOnlyDataLogic(tmp_path) - det.add_detector_logics(JustInternalTriggerLogic(), dl1, dl2) + det.counts, _ = soft_signal_r_and_setter(int, 42, name="det-counts") + det.set_readable_format(det.counts, Format.HINTED_SIGNAL) + det.add_detector_logics( + JustInternalTriggerLogic(), StreamableOnlyDataLogic(tmp_path) + ) await det.prepare(TriggerInfo()) - # Should have data from both logics + # Should have data from the data logic and the registered signal description = await det.describe() assert description == { "det": { @@ -1005,11 +978,11 @@ async def test_multiple_data_logics(tmp_path): "shape": [1, 10, 15], "source": f"file://localhost/{tmp_path.as_posix().lstrip('/')}/test.h5", }, - "foo-value": { + "det-counts": { "dtype": "integer", "dtype_numpy": " Date: Fri, 30 Oct 2026 17:14:10 +0000 Subject: [PATCH 11/12] WIP: port onto the per-verb StandardReadable registries The replayed #1403 commits target the old StandardReadable hooks (_Verb, _extra_funcs_for, _extra_hint_sources, _HintedFields), which #1400 replaced with per-verb registries. StandardDetector now adds __on_read/__on_describe/__on_hints to _read_funcs/_describe_funcs/ _hints_funcs, appended after the registered children as on the tip. _config_signals walks the typed registry entries. Tests and the eiger detector move off the deprecated add_readables/add_config_signals, the stats hints test registers stats.total as a hinted signal now that PluginSignalDataLogic is gone, and two doc links are fixed. Drafts the CHANGELOG entries for this PR. Assisted-by: Claude Opus 5.5 --- CHANGELOG.md | 5 ++ docs/reference/environment-variables.md | 2 +- src/ophyd_async/core/_detector.py | 64 +++++++++---------- src/ophyd_async/core/_readable.py | 9 +-- .../epics/adcore/_acquire_logic.py | 2 +- src/ophyd_async/fastcs/eiger/_detector.py | 3 +- .../unit_tests/core/test_standard_detector.py | 12 ++-- .../unit_tests/epics/adcore/test_detector.py | 9 ++- .../unit_tests/epics/adcore/test_use_cases.py | 1 + 9 files changed, 55 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dae311903c..63d284885b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,11 @@ - `StandardReadable` now keeps one format per registered child, keyed by the child, so registering a child again replaces its format instead of adding a second contribution, and the order of `hints` is the order children were first registered. The private `_describe_config_funcs`, `_read_config_funcs` and `_has_hints` tuples are gone: a subclass that added to them now adds a hook to `_describe_configuration_funcs`, `_read_configuration_funcs` or `_hints_funcs` (see ADR 0023). - The deprecated `ConfigSignal` and `HintedSignal` aliases are removed from `ophyd_async.core`; use `StandardReadableFormat.CONFIG_SIGNAL`, `HINTED_SIGNAL` or `HINTED_UNCACHED_SIGNAL`. `StandardReadable.add_readables()` is deprecated in favour of `set_readable_format()` on each device. - `store_settings()` writes the readable format changes of the Device tree under the reserved key ``, and `apply_settings()` resets the tree to its class formats before applying them. Settings files stored before this have no such key and leave formats alone. `retrieve_settings()` warns about and drops any other `<...>` key. +- `StandardDetector` is now a `StandardReadable`: signals read in a step scan or reported as configuration are registered with `set_readable_format()`, as on any other `StandardReadable`, and what the data logics produce is added to `read()`, `describe()` and `hints` alongside them. `add_config_signals()` is deprecated in favour of `set_readable_format(signal, StandardReadableFormat.CONFIG_SIGNAL)`. A detector with no hinted fields now reports `hints` as `{}` rather than `{"fields": []}`. +- `DetectorTriggerLogic.config_sigs()` is removed: `get_deadtime()` is given the value of every signal the detector reports as configuration, including those declared on children registered as `CHILD`, so a signal that determines deadtime must be registered as `CONFIG_SIGNAL`. +- The `prepare_single` data logic tier is removed, with `ReadableDataProvider`, `SignalDataProvider` and `adcore.PluginSignalDataLogic`. Register a plugin scalar such as `NDStatsIO.total` with `set_readable_format(signal, StandardReadableFormat.HINTED_SIGNAL)` instead. +- The areaDetector driver and plugin classes (`ADBaseIO`, `NDROIIO`, `NDStatsIO` and `NDROIStatNIO`) are now `StandardReadable`s that declare their own configuration and read signals, so registering a plugin as `CHILD` of an `AreaDetector` puts them in `read()` and `read_configuration()`. +- `ADAcquireLogic` sets `WaitForPlugins` on `stage()`, from its new `wait_for_plugins` argument (default on), rather than `AreaDetector` hiding it, so a step scan reads plugin scalars for the frame it has just taken. ### Added diff --git a/docs/reference/environment-variables.md b/docs/reference/environment-variables.md index 8ab91db8d8..78afba8c51 100644 --- a/docs/reference/environment-variables.md +++ b/docs/reference/environment-variables.md @@ -13,7 +13,7 @@ effect on ophyd-async itself. Controls the implicit prepare performed by [`StandardDetector.trigger()`](#ophyd_async.core.StandardDetector.trigger) when [`prepare()`](#ophyd_async.core.StandardDetector.prepare) has not been called since -the last [`stage()`](#ophyd_async.core.StandardDetector.stage). +the last `stage()`. | Value | Behaviour | |-------|-----------| diff --git a/src/ophyd_async/core/_detector.py b/src/ophyd_async/core/_detector.py index eef40e4106..6cf7f27a9e 100644 --- a/src/ophyd_async/core/_detector.py +++ b/src/ophyd_async/core/_detector.py @@ -15,8 +15,9 @@ from bluesky.protocols import ( Collectable, Flyable, - HasHints, + Hints, Preparable, + Reading, Stageable, StreamAsset, Triggerable, @@ -30,8 +31,6 @@ StandardReadable, StandardReadableFormat, _config_signals, - _HintedFields, - _Verb, ) from ._settings import Settings from ._signal import ( @@ -155,10 +154,7 @@ def get_deadtime(self, config_values: SignalDict) -> float: i.e. everything registered with [](#StandardReadableFormat.CONFIG_SIGNAL), including those declared on children registered as [](#StandardReadableFormat.CHILD). A - logic that needs a signal must make sure it is declared as - configuration, rather than nominating it separately: the values - that determine deadtime are exactly the ones that ought to be - recorded for the scan anyway. + logic that needs a signal must declare it as configuration. """ raise NotImplementedError(self) @@ -404,6 +400,11 @@ def __init__(self, *args, **kwargs) -> None: # so the readable children registered on this detector still get staged self._stage_funcs += (self._stage_detector,) self._unstage_funcs += (self._unstage_detector,) + # What the data logics produce, gathered alongside the registered + # children, so the verb methods themselves need no overriding + self._read_funcs += (self.__on_read,) + self._describe_funcs += (self.__on_describe,) + self._hints_funcs += (self.__on_hints,) # Report the number of events for the next kickoff @cached_property @@ -748,35 +749,30 @@ async def complete(self): ): yield update - def _extra_funcs_for(self, verb: _Verb) -> Iterator[Callable[[], Awaitable[dict]]]: - """Contribute what the data logics produce to read() and describe(). - - StandardReadable gathers these alongside the registered children, so - the verb methods themselves need no overriding. + async def __on_read(self) -> dict[str, Reading]: + """What the data logics contribute to `read()`: nothing, but checks prepare. Raising when nothing has been prepared is deliberate, and predates registered children being readable at all: a detector that has not been - prepared has no data keys, so a `describe()` that quietly succeeded - would emit a descriptor missing the detector's data. In practice - `trigger()` prepares implicitly, so `trigger_and_read` never reaches - this. `read_configuration()` and `describe_configuration()` are - unaffected, as this hook only feeds the data verbs. - - The cost is that the *registered children* are unreachable through - `read()`/`describe()` until then too, since this raises before the - registry is consulted. That is accepted rather than worked around: - reading an unprepared detector is already defined as an error. + prepared has no data keys. In practice `trigger()` prepares implicitly, + so `trigger_and_read` never reaches this. + """ + error_if_none(self._prepare_ctx, "Prepare not run") + return {} + + async def __on_describe(self) -> dict[str, DataKey]: + """What the data logics contribute to `describe()`. + + Raises when nothing has been prepared, as a `describe()` that quietly + succeeded would emit a descriptor missing the detector's data. + Streamable providers describe their shape for a step scan, but produce + their data through collect_asset_docs rather than read(). """ - if verb not in (_Verb.DESCRIBE, _Verb.READ): - return ctx = error_if_none(self._prepare_ctx, "Prepare not run") - if verb is _Verb.DESCRIBE: - # Streamable providers describe their shape for a step scan, but - # produce their data through collect_asset_docs rather than read() - for sdp in ctx.streamable_data_providers: - yield functools.partial( - sdp.make_datakeys, ctx.trigger_info.collections_per_event - ) + return await merge_gathered_dicts( + sdp.make_datakeys(ctx.trigger_info.collections_per_event) + for sdp in ctx.streamable_data_providers + ) async def describe_collect(self) -> dict[str, DataKey]: ctx = error_if_none(self._prepare_ctx, "Prepare not run") @@ -787,11 +783,11 @@ async def describe_collect(self) -> dict[str, DataKey]: ] return await merge_gathered_dicts(coros) - def _extra_hint_sources(self) -> Iterator[HasHints]: - """Contribute the data logics' hinted fields alongside the children's.""" + def __on_hints(self) -> Iterator[Hints]: + """The data logics' hinted fields, alongside the children's.""" for dl in self._data_logics: if fields := dl.get_hinted_fields(self.name + dl.datakey_suffix): - yield _HintedFields(fields) + yield {"fields": list(fields)} async def collect_asset_docs( self, index: int | None = None diff --git a/src/ophyd_async/core/_readable.py b/src/ophyd_async/core/_readable.py index 59fd6552f7..55c8e0f3a3 100644 --- a/src/ophyd_async/core/_readable.py +++ b/src/ophyd_async/core/_readable.py @@ -529,16 +529,13 @@ def _config_signals(device: Device) -> set[SignalR]: Unlike `walk_config_signals` this reads the registry rather than calling `read_configuration()`, so it does no I/O and includes read-only `SignalR`s - as well as `SignalRW`s -- `ADBaseIO.model` is a `SignalR`, and it is - exactly the signal an areaDetector trigger logic needs for deadtime. - - Deliberately private and unexported: #1367 restructures detector logic, so - what the public shape of this should be is not settled yet. + as well as `SignalRW`s (`ADBaseIO.model` is a `SignalR`, and it is exactly + the signal an areaDetector trigger logic needs for deadtime). """ signals: set[SignalR] = set() if not isinstance(device, StandardReadable): return signals - for entry in device._readables.entries(): # noqa: SLF001 + for entry in device.get_readable_formats(): match entry: case (signal, Format.CONFIG_SIGNAL): signals.add(signal) diff --git a/src/ophyd_async/epics/adcore/_acquire_logic.py b/src/ophyd_async/epics/adcore/_acquire_logic.py index 8371eb146b..6c6b18e231 100644 --- a/src/ophyd_async/epics/adcore/_acquire_logic.py +++ b/src/ophyd_async/epics/adcore/_acquire_logic.py @@ -90,7 +90,7 @@ class ADContAcqAcquireLogic(DetectorAcquireLogic): Deliberately has no `wait_for_plugins` equivalent to [](#ADAcquireLogic). That setting gates the driver's `AcquireBusy` going to 0 at the end of an - acquisition, and here the driver is always acquiring -- there is no such + acquisition, and here the driver is always acquiring, so there is no such transition to wait on. `wait_for_idle` waits on the circular buffer's capture instead. diff --git a/src/ophyd_async/fastcs/eiger/_detector.py b/src/ophyd_async/fastcs/eiger/_detector.py index 7257bde6a7..f993114c40 100644 --- a/src/ophyd_async/fastcs/eiger/_detector.py +++ b/src/ophyd_async/fastcs/eiger/_detector.py @@ -2,6 +2,7 @@ PathProvider, SignalR, StandardDetector, + StandardReadableFormat, TriggerableCommand, soft_signal_rw, ) @@ -44,6 +45,6 @@ def __init__( ), ) - self.add_config_signals(self.pixel_mask) + self.set_readable_format(self.pixel_mask, StandardReadableFormat.CONFIG_SIGNAL) super().__init__(name=name, connector=connector) diff --git a/tests/unit_tests/core/test_standard_detector.py b/tests/unit_tests/core/test_standard_detector.py index 211e96f793..5c48201c47 100644 --- a/tests/unit_tests/core/test_standard_detector.py +++ b/tests/unit_tests/core/test_standard_detector.py @@ -1044,10 +1044,10 @@ async def test_child_readable_config_signals_in_describe_configuration(): """Child StandardReadable CONFIG_SIGNALs appear in describe_configuration.""" child = StandardReadable(name="child") config_sig = soft_signal_rw(float, initial_value=1.5, name="child-exposure") - child.add_readables([config_sig], Format.CONFIG_SIGNAL) + child.set_readable_format(config_sig, Format.CONFIG_SIGNAL) det = StandardDetector(name="det") - det.add_readables([child], Format.CHILD) + det.set_readable_format(child, Format.CHILD) await det.prepare(TriggerInfo()) config = await det.describe_configuration() @@ -1061,10 +1061,10 @@ async def test_child_readable_read_signals_in_read(tmp_path): """Child StandardReadable HINTED_SIGNALs appear in read/describe.""" child = StandardReadable(name="child") read_sig = soft_signal_rw(int, initial_value=99, name="child-counts") - child.add_readables([read_sig], Format.HINTED_SIGNAL) + child.set_readable_format(read_sig, Format.HINTED_SIGNAL) det = StandardDetector(name="det") - det.add_readables([child], Format.CHILD) + det.set_readable_format(child, Format.CHILD) det.add_detector_logics(StreamableOnlyDataLogic(tmp_path)) await det.prepare(TriggerInfo()) @@ -1082,10 +1082,10 @@ async def test_child_readable_hints_merged(tmp_path): """Child StandardReadable hints are merged with data logic hints.""" child = StandardReadable(name="child") hinted_sig = soft_signal_rw(float, name="child-intensity") - child.add_readables([hinted_sig], Format.HINTED_SIGNAL) + child.set_readable_format(hinted_sig, Format.HINTED_SIGNAL) det = StandardDetector(name="det") - det.add_readables([child], Format.CHILD) + det.set_readable_format(child, Format.CHILD) det.add_detector_logics(StreamableOnlyDataLogic(tmp_path)) assert "fields" in det.hints diff --git a/tests/unit_tests/epics/adcore/test_detector.py b/tests/unit_tests/epics/adcore/test_detector.py index ac6215311d..f08d0fa592 100644 --- a/tests/unit_tests/epics/adcore/test_detector.py +++ b/tests/unit_tests/epics/adcore/test_detector.py @@ -1,6 +1,7 @@ import pytest from ophyd_async.core import ( + StandardReadableFormat, StaticPathProvider, TriggerInfo, init_devices, @@ -28,10 +29,12 @@ def test_writer_image_hints(static_path_provider: StaticPathProvider, factory, h plugins={"stats": stats}, name="det", ) - assert det.hints == {"fields": ["det-image"] if hinted else []} - det.add_detector_logics(adcore.PluginSignalDataLogic(driver, stats.total)) + # No hinted fields means no hints at all + assert det.hints == ({"fields": ["det-image"]} if hinted else {}) + det.set_readable_format(stats.total, StandardReadableFormat.HINTED_SIGNAL) + # Readable-format signals are hinted ahead of the data logics' fields assert det.hints == { - "fields": (["det-image"] if hinted else []) + ["det-stats-total"] + "fields": ["det-stats-total"] + (["det-image"] if hinted else []) } diff --git a/tests/unit_tests/epics/adcore/test_use_cases.py b/tests/unit_tests/epics/adcore/test_use_cases.py index 003bae71d0..648947b478 100644 --- a/tests/unit_tests/epics/adcore/test_use_cases.py +++ b/tests/unit_tests/epics/adcore/test_use_cases.py @@ -12,6 +12,7 @@ ) from ophyd_async.epics import adaravis, adcore, adsimdetector from ophyd_async.epics.core import epics_signal_r +from ophyd_async.testing import assert_reading async def test_step_scan_hdf_detector_with_stats_and_temp( From 3f7197ee0c298f46c574eac9609c7aaff2e8b10b Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 10 Nov 2026 11:27:43 +0000 Subject: [PATCH 12/12] WIP: how-to conclusion reports configuration from the IO classes, not the trigger logic Assisted-by: Claude Opus 5.5 --- .../decisions/0012-detector-rewrite.md | 6 +-- ...tandard-detector-is-a-standard-readable.md | 50 +++++++++++++++++++ docs/how-to/implement-ad-detector.md | 2 +- 3 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 docs/explanations/decisions/0024-standard-detector-is-a-standard-readable.md diff --git a/docs/explanations/decisions/0012-detector-rewrite.md b/docs/explanations/decisions/0012-detector-rewrite.md index fa5d0949d4..f1cc30a79a 100644 --- a/docs/explanations/decisions/0012-detector-rewrite.md +++ b/docs/explanations/decisions/0012-detector-rewrite.md @@ -5,9 +5,9 @@ Date: 2026-01-20 Accepted -Partly superseded by [ADR 0023](0023-runtime-readable-format.md), which made -`StandardDetector` a `StandardReadable` and removed the `prepare_single` tier along with -`ReadableDataProvider`. +Partly superseded by [ADR 0024](0024-standard-detector-is-a-standard-readable.md), which +made `StandardDetector` a `StandardReadable` and removed the `prepare_single` tier along +with `ReadableDataProvider`. ## Context diff --git a/docs/explanations/decisions/0024-standard-detector-is-a-standard-readable.md b/docs/explanations/decisions/0024-standard-detector-is-a-standard-readable.md new file mode 100644 index 0000000000..93f0faac31 --- /dev/null +++ b/docs/explanations/decisions/0024-standard-detector-is-a-standard-readable.md @@ -0,0 +1,50 @@ +# 24. StandardDetector is a StandardReadable + +Date: 2026-10-30 + +## Status + +Accepted + +## Context + +[ADR 0012](0012-detector-rewrite.md) gave `StandardDetector` its own ways to put a signal +in a step scan: `add_config_signals` for configuration, and the `prepare_single` data +logic tier with `ReadableDataProvider` for values read once per event. Every other Device +does both by registering the signal on a `StandardReadable`, and +[ADR 0023](0023-runtime-readable-format.md) made that registry keyed by Device and +changeable at runtime, which is the per-plugin opt-in the #1395 reviewers asked for. + +## Decision + +### `StandardDetector` inherits `StandardReadable` + +A detector's step-scan signals are registered like any other Device's. The detector does not +override `read()`, `describe()` or `hints`: each verb gathers a tuple of callables and the +detector appends its own, as `StandardFlyable` does to `_stage_funcs`, because overriding +meant re-deriving the registered children's contributions in each verb, which would drift. +`add_config_signals` becomes a deprecated wrapper. + +A trigger logic computes deadtime from all the detector's configuration signals rather than +nominating its own, as the values that determine deadtime ought to be recorded anyway. They +come from the registry, not `read_configuration()`, so read-only signals such as +`ADBaseIO.model` count. + +`AreaDetector` registers its driver but no plugin: a plugin is opted in with +`set_readable_format(plugin, StandardReadableFormat.CHILD)` and out with `None`, the +per-plugin, reversible choice the #1395 reviewers asked for. + +### The `prepare_single` data logic tier goes + +Registering a signal now gets one value per event into `read()` for any Device, which was +all [ADR 0012](0012-detector-rewrite.md)'s `prepare_single` tier did. It is deleted with its +providers rather than kept as a second way to do one thing, the one that drops itself with a +warning once a scan asks for more than one collection. + +## Consequences + +- A detector's signals can be switched between techniques, stored and restored like any + other Device's ([ADR 0023](0023-runtime-readable-format.md)). +- A signal that determines deadtime must be registered as configuration. +- Out-of-tree `prepare_single` data logics must register their signals instead; there is no + shim. diff --git a/docs/how-to/implement-ad-detector.md b/docs/how-to/implement-ad-detector.md index 036919307d..469c51ef5f 100644 --- a/docs/how-to/implement-ad-detector.md +++ b/docs/how-to/implement-ad-detector.md @@ -273,4 +273,4 @@ The detector will now support: - Step scanning with `trigger()` - Fly scanning with `kickoff()` and `complete()` - Automatic handling of file writing and StreamAsset document emission -- Configuration signal reporting based on your trigger logic +- Configuration signal reporting from the signals declared with `StandardReadableFormat.CONFIG_SIGNAL` on the IO classes