Repository navigation
Conversation
A Device that both moves and flies (Motor) had to expose the same logic object under two property names. Each mix-in now declares `logic` with its own required type, so one implementation satisfies both and the type checker verifies it against each mix-in independently -- the intersection is expressed by the logic class's own MRO. Also fixes the abstractness of the declaration. `functools.cached_property` does not forward `__isabstractmethod__`, so `@cached_property` over `@abstractmethod` registered nothing: `StandardMovable.__abstractmethods__` was empty and a missing logic surfaced as `AttributeError: 'NoneType' object has no attribute 'readback'` from inside `Device.__init__`. The new `abstract_cached_property` sets the flag on the descriptor so the metaclass collects it, giving a TypeError at instantiation instead. `StandardFlyable` is consequently abstract, so `FlyableLogic.with_device` builds a concrete `_EphemeralFlyable` to attach the logic to. `movable_logic` and `flyable_logic` remain as deprecated properties. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit e98799b3877b7c55f2835177a763fca6105887c2)
Fixes bluesky#1394. `add_readables` baked bound methods into four tuples, discarding which device each contribution came from. Nothing could then be found, removed or re-formatted, so there was no equivalent of ophyd v1's runtime `kind`. The registry is now a tuple of `(device, format)` pairs and the callables for each verb are derived on demand, which makes `set_readable_format(dev, format)` a whole-tuple swap. Passing `None` drops the device entirely. Staging is derived the same way, so a format changed between runs is picked up by the next `stage()`. `add_readables` stays as a thin wrapper, so the 85 in-tree call sites and all downstream Devices are unaffected. It has "set" semantics now: registering the same device twice replaces rather than duplicates. The deprecated `ConfigSignal`/`HintedSignal` markers announce themselves by comparing equal to their target, which relied on the `match` running at registration; formats are normalised to real enum members on the way in so the warning still fires there rather than on first read(). Tests that asserted on the private tuples are rewritten onto the public verbs, per the testing conventions in CLAUDE.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 1a43d10ddf0af8d02062fb77958041774c8c5cb2)
Which signals are hinted, config or omitted is now part of a Device's
state, so it needs to be storable alongside signal values. A beamline that
switches technique wants to save "energy is config" and "energy is hinted"
as two profiles and load whichever applies.
Stored as {path of StandardReadable: {path of child: format}}, using the
same dotted attribute paths as store_settings, with "" for the root. Values
are plain strings so the yaml stays readable and hand-editable.
apply_readable_formats replaces the registered children of each named
StandardReadable rather than merging into them, so loading a profile drops
what the previous one registered instead of accumulating the union. It
resolves every path before changing anything, so a bad path cannot leave
the tree half applied.
Kept out of store_settings: values and formats are stored under separate
names, since they change on different cadences.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit fb74d300537f3965b5eb4545632dea29251fde8e)
Adds ADR 0021 for keying the readable registry by Device, covering the designs that were rejected (chain parsing, one pluggable registry for formats and logic objects both), and ADR 0022 for the single `logic` property, which supersedes part of ADR 0017. Numbering starts at 21 to leave 20 for the unmerged ADR in bluesky#1367. Adds a how-to for changing format at runtime and for saving/restoring sets of formats as technique profiles. `tox -e docs` already failed on main with three warnings. Retargeting the write-a-flyer literalinclude at `Motor.logic` fixes one of them; the other two are TypeVars documented on StandardFlyable in bluesky#1359, added to the existing nitpick ignore list so the build is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit f0ddc99d9755a2457ebf477c1f4f76f67b29e439)
Values and formats are applied on the same cadence -- a technique change wants both -- so they belong in one file and one apply_settings call rather than two of each. Settings gains a readable_formats attribute, so one retrieve_settings returns the whole stored state. Values stay flat, formats go under "<READABLE_FORMATS>". That key cannot collide with a dotted attribute path, because "<" cannot begin a Python identifier, so no store-time guard is needed; and unlike "*FORMATS*" it is not a YAML indicator, so it needs no quoting. "<ROOT>" replaces "" as the root owner for the same reason, and because an empty-string key makes PyYAML emit its hard-to-read "? '' :" explicit key form. "<DEVICE_NAMES>" is reserved but unwritten so storing names later needs no migration. No version marker: a file written before formats existed simply lacks the key, which under replacement semantics correctly means "clear no owner". Each part being optional also makes a hand-written formats-only profile work, switching technique without writing a value to hardware. Settings.partition copies formats to both halves. apply_panda_settings applies only the halves and never the original Settings, so putting them on one half would let that plan silently decide whether formats were restored. Applying them twice touches no hardware and is idempotent. apply_settings applies formats after values, so a failed write leaves the registry alone. store_readable_formats and retrieve_readable_formats are removed; they only existed earlier in this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 586f79e8ac40b5a48ccb74734957c35f300bdc48)
TangoTestDevice is a StandardReadable that registers nothing, so
walk_readable_formats returns {"<ROOT>": {}} for it -- one owner with no
children, which means "this owner registers nothing" and has to be stored
so that applying the file clears it.
The EPICS golden files are untouched because EpicsTestCaDevice is not a
StandardReadable, so no formats key is written for it at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 2f764b259bf78732aa1bcf5c25d391ad6a1a29fb)
An owner entry with no children means "this owner registers nothing", so it
has to be stored -- otherwise applying a profile that registers nothing
would silently leave the previous profile's registrations in place. That is
what made the tango golden file need the key.
The distinction was only exercised by a system test needing a live Tango
device server, so both shapes are now unit tested: a StandardReadable that
registers nothing stores {"<ROOT>": {}} and applying it clears, while a
Device with no StandardReadable in its tree stores no key at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 1681595e843acad13129aa299d2042f1f5e11349)
…rge formats Detector-side changes from the original commit move to the StandardDetector group, so this PR stays core-only.
get_readable_format(device) was only ever used from tests, and readable_children() returned an iterator of pairs that every caller immediately wanted to look up by device. One get_readable_formats() -> dict[Device, StandardReadableFormat] covers both: a Device that does not contribute is simply absent, so .get(child) replaces the singular accessor, and the dict keeps registration order. Device.__eq__ is object identity and DeviceVector / DeviceMap hash by id, so the dict is identity keyed exactly like the tuple it is built from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 3517dbcc1f1854463ef97953df618d89054e5b74)
…rning
Settings.__or__ merged formats with `formats |= other.readable_formats`,
a dict update keyed by owner, so the whole owner entry was replaced:
{"<ROOT>": {a: CONFIG, b: CONFIG}} | {"<ROOT>": {a: HINTED}} dropped b.
Values merge per signal and apply_readable_formats merges per child --
deliberately, so a stored file never silently unregisters a signal added
since it was written -- so __or__ must merge at the same granularity.
Two things fell out of writing the test. __or__ rejected `other` being a
Settings for the *same* Device, because _is_in_device is really "is a
strict child of", which made the whole formats path unreachable. And a
Settings for a Device *below* this one has paths relative to itself, so
<ROOT> and every dotted path would come to mean something else once
merged; re-rooting them is not implemented as nothing needs it, so that
now raises rather than silently attaching them to the wrong Device.
Settings.__or__ has no callers in tree and had no tests either way.
Also covers walk_readable_formats' warning for a readable registered
outside the tree (bluesky#1402) -- the branch whose message says the value is
not stored either, which nothing exercised.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 1e9cc9f3ce8783c2bbc54358ed1586dac035e266)
Five small things from review, no behaviour change except the reserved key handling: - retrieve_settings copied the dict from the provider before popping the reserved keys out of it, so a provider that caches what it returns no longer loses the formats on a second retrieve. - DEVICE_NAMES_KEY is gone. It was exported in core.__all__ but nothing wrote it and only a defensive pop read it, so it was public API for a feature that does not exist. retrieve_settings now drops *every* <RESERVED> key rather than the ones it knows: no path can look like that, so a key written by a newer version is ignored instead of being reported as an unknown signal name, and adding one later needs neither a migration nor a constant reserved up front. Import directionality forbids plan_stubs reaching into core._readable, so keeping the constant module private was not an option anyway. - walk_readable_formats walked the tree twice, once to build the path map and once to invert it. - _signals_to_stage had the same condition written out twice; the two staging formats are now a named frozenset. - The warning stacklevel is left at 2 deliberately: it correctly blames the caller of walk_readable_formats, which is the public entry point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit fa2b8b280841d2c6a761004c10a06db24df19f78)
set_readable_format rebuilt the whole tuple on every call, scanning it for the device first, so add_readables of n children was O(n^2): 2000 signals cost 77ms at construction. A dict makes it O(1) per call and linear overall -- 2000 signals now cost 13ms, 4000 cost 25ms. Keying by Device is identity keying, which is what a registry of children needs. Device does not override __eq__; DeviceVector and DeviceMap do inherit a value based __eq__ from Mapping, but they hash by id(), which is injective over live objects, so two distinct devices never share a hash and __eq__ is never reached. It is a cached_property rather than an attribute assigned in __init__, matching Device._child_devices: add_children_as_readables and set_readable_format are routinely called *before* super().__init__() (AreaDetector does exactly this), so anything __init__ assigned would discard them. One behaviour change: re-formatting an already registered child now keeps its position instead of moving it to the end, so changing a format no longer reorders hints. Both behaviours are new in this PR, so nothing downstream depends on the old one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit a6d2aa8c31acd3dcd3dd77140dec5d864a175165)
@oliwenmandiamond's concern was that a class declaring sig_a as hinted, then having that changed at runtime, makes the class definition misleading, and proposed either locking class-declared formats or a separate StandardDynamicReadable. Both were rejected: static-by-default blocks bluesky#1394's own motivating case, a monochromator whose energy is *declared in the class* as config and which one technique wants hinted. @jwlodek made the same call from the v1 side, where changing kind at runtime is used extensively and behaviour differences cost NSLS-II. This is their counter-proposal instead: put the Device back to what its class declared, so a plan that retunes one can undo it without recording what it changed. The baseline is sealed by a metaclass rather than at the end of StandardReadable.__init__. Formats can be declared in three places: annotations, applied inside Device.__init__; registration a subclass does before its super().__init__(), which is what all 24 in-tree Devices do; and registration it does after, which nothing in tree does but which is legal. type.__call__ runs the whole __init__ chain before returning and is the only point after all three -- an end-of-__init__ snapshot was measured to miss the third, and the symptom would have been reset_readable_formats silently dropping a child the class declared. Device already has a metaclass (_ProtocolMeta, from the HasName Protocol), so this adds no new machinery. Per Device rather than per tree, pairing with set_readable_format rather than apply_readable_formats, and the baseline is the class declaration rather than the last applied settings file -- so it discards a stored technique too. Both documented, with a one-liner for the tree case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit e798387ad9a37033ecfe8f87adac928f01ab2d96)
…etector Three review points on the readable-format docs: - ADR 0017 only had a backward reference from 0022; add the forward one, so a reader landing on 0017 learns that movable_logic is gone. - The note on reset_readable_formats said the baseline was "the class declaration, not the last file you loaded", which does not say what it affects. Say plainly that it discards formats that came from a stored settings file. - The stored-file how-to named the constant while the yaml above it showed the string. Name the string, and link the constant once. Also move the two sections that describe a StandardDetector inheriting StandardReadable -- one in ADR 0021, one in the how-to -- out of this branch. Nothing here makes that true yet; they belong with the change that does.
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.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng read/config/describe from children automatically (cherry picked from commit 3219303)
…onents for config/read (cherry picked from commit 64f4cd3)
…er logics (cherry picked from commit 404930b)
(cherry picked from commit c80dc36)
(cherry picked from commit dd36b7a)
bluesky#1395 registered every StandardReadable plugin as a CHILD readable in AreaDetector.__init__. That is dropped in the rebase onto bluesky#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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit f56c523)
…etector bluesky#1395 set driver.wait_for_plugins from an 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 left 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit fe787ce)
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 -- 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit c239654)
get_config_signals was added as a public method on StandardReadable in the previous commit. bluesky#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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit ca5fcb7)
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 -- 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 0021.
bluesky#1248, bluesky#888, bluesky#1309 and bluesky#1364 all bottom out in the same two gaps: a data logic is told nothing about timing or extent at prepare time, and StandardDetector claims WritesStreamAssets unconditionally so the bluesky bundler will never call collect_pages on it. Record the design for a third `prepare_bounded` tier that closes both. It is one tier rather than two changes because the devices that cannot write files (MeasComp CTR-08, SIS3820 scaler, TetrAmm) and the ones that want rate-based chunking (Xspress, PandA) both need to be told what the scan looks like before it starts. Document the CTR-08 explicitly. It is not an areaDetector, and it is the case that puts pressure on the trigger/acquire/data split: one control both starts acquisition and erases the buffer, and one PV is both the exposure count and the buffer size. An areaDetector hides this by acquiring and capturing on separate PVs. The split holds -- it is a split of concerns, not of hardware -- but it forces the progress baseline to be stated rather than derived, since a buffer erased by the acquire control is erased after the data logic has been prepared. Decided ahead of any code so the tier design can be agreed on its own. (cherry picked from commit 97b72c6)
add_detector_logics dispatched with an if/elif chain, so an object that was both an AcquireLogic and a DataLogic registered as the first match only and had its other role silently dropped. A device whose concerns all live on one control is a natural candidate for a single combined logic object, so catch the ambiguity and direct the author to pass separate objects instead. Refs ADR 0020. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 6fe39e2)
Adds prepare_bounded as a third data-logic tier alongside prepare_single and prepare_unbounded, returning a PageableDataProvider for data held in a finite buffer that must be sized before acquiring (a scaler MCA, an areaDetector stats time series). Such a provider emits event pages; read() derives a step-scan reading from the single-event page the buffer holds. Tier selection is capability-only: a data logic whose tier cannot serve the requested number of collections is dropped with a warning rather than raising, generalising bluesky#1364 to all three tiers. This replaces the previous hard error when a single-only logic met multiple collections. Bounded providers are never reused, so trigger() re-prepares them each point, which re-arms the buffer; kickoff() does not, so a fly scan arms once and accumulates across kickoffs. A bounded buffer's progress baseline is therefore 0 in trigger() and read live in kickoff(). Refs ADR 0020. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 7fa515d)
A TriggerInfo livetime of 0 means "use whatever the detector currently has set", so forwarding it to a data logic that sizes chunks or a buffer by the exposure period would hand it a period of 0. Resolve it in prepare(), after the trigger logic has been prepared (so the hardware holds the values), by reading the current livetime and deadtime back via default_trigger_info(). trigger_info_from_num_images now reads acquire_time and acquire_period alongside num_images so AD detectors report their real exposure period. Detectors whose exposure is unset (0) are unaffected. Refs ADR 0020. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 1c68afe)
StandardDetector no longer inherits WritesStreamAssets. A detector writes stream assets or emits event pages depending on which data logics it carries, never both, and the bluesky bundler treats the two protocols as mutually exclusive. collect_asset_docs and collect_pages are now served by __getattr__ only when a data logic supporting that tier is present, so the bundler's structural isinstance check matches exactly the one that applies. add_detector_logics raises if a detector mixes bounded and unbounded data logics, the one combination that would expose both. A file writer that also wants stats should carry them in the file as NDAttributes. Refs ADR 0020. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d67ec04)
The single-event tier went with the previous branch, so ADR 0020 and the detector tutorial should never have described it: a scalar per event is a registered signal now. Also trims the comments that argued with themselves rather than saying what the code does.
DetectorDataLogic had one prepare_* method per tier, each of which both described the data and did the writes to make it happen. That forces the detector to arm hardware to find out what it would get: a provider it then decides not to use has to be stopped again, which is a wasted erase on a stats time series and impossible to justify once the choice depends on what the providers actually produce. Split them. make_data_provider() describes what this logic would produce and must not start anything; start() does the writes, and is called only for the providers the detector settles on. Which tier a logic is becomes the type of the provider it returns, so _DataLogicTier, the override detection and _tier_can_serve all go, and a logic sits a scan out by returning None -- a finite buffer asked for an unbounded scan, or a plugin that is switched off. That is silent rather than warned, since a detector may carry more logics than any one scan uses. Only knowable at prepare, so which collect verb the detector exposes moves there too, recomputed each time so the verb that no longer applies is removed. Mixing the two kinds still raises, but now after asking and before starting, so nothing is armed when it does. OdinDataLogic is the one that cannot describe without starting -- its frame shape is only readable once the file processor is writing -- so it keeps its writes in make_data_provider. Safe while it is never the logic a detector drops. ADHDFDataLogic and StatsTimeSeriesDataLogic gain an enable_callbacks flag, defaulting to today's force-on, which when False follows whatever the plugin is set to and produces nothing when it is off. That is the switch a detector needs to fall back from its file writer to its stats time series between runs.
The detector had its own prepare/kickoff/complete, threading its own state between them and guarding each stage on whether a context was None, so a verb called too early failed with an AttributeError on None. StandardFlyable already does all of that, for every other flyer in the tree. To satisfy it, the trigger, acquire and data logics become children of one DetectorLogic, which a detector builds in __init__ and returns from logic; the orchestration moves with them and the logic holds no back-reference, so ADR 0017 still holds unqualified. StandardDetector itself takes no __init__ args, since a Standard* class that did would interfere with multiple inheritance, and with_logics() builds both logic and Device for an ad-hoc detector. What the logic cannot get from its own fields is the datakey namespace, pushed in by set_name, and which collect verb the Device should expose, done with a required callback it is constructed with and called from prepare -- because only then is it known what the data logics will actually produce. That also lets carrying a file writer and a finite buffer stop being an error when the file covers every key the buffer would produce: the durable copy wins and the buffer sits the scan out, unarmed. Uncovered keys still raise, naming them. Progress reporting would otherwise have been lost, since complete() only produced WatcherUpdates for a MovableLogic; WatchableFlyableLogic lets a logic whose progress is neither a readback nor a setpoint yield its own. events_to_kickoff and multi-kickoff go with this: nothing drove them, the RunEngine completes what it kicked off, and every other flyer already worked that way. The providers stay on the logic rather than in the fly context, because a step scan reuses them across points and the RunEngine collects after complete() has returned; trigger() re-arms only what needs it, so a step scan does not repeat the trigger logic's puts. See ADR 0023.
…FlyableLogic A whole extra base class to inherit was a lot of machinery for what is a property of one method's return value. on_complete now returns either an Awaitable[None] or an AsyncIterator[WatcherUpdate], and complete() looks at what it got: an iterator goes straight to the watchers, anything else is awaited.
…ogic DetectorLogic no longer takes a publish_collect_methods callback: the Device binds whichever collect verb its data logics turn out to need, in prepare() and trigger(), reading it from the logic's prepared data. The logic therefore holds nothing that points back at its Device. That makes a DetectorLogic constructible on its own, so an ad-hoc detector is DetectorLogic(*logics).with_device(name) exactly as for any other FlyableLogic, and StandardDetector.with_logics goes.
external="" fails event-model's descriptor schema (it must match '^[A-Z]+:?'), so any real scan reading a stats time series raised ValidationError when the descriptor was composed. Nothing caught it because the tests called the detector's verbs directly. Drive the two step/fly scan cases through the RunEngine instead, which is where a descriptor gets validated. That also pins the order these have to go in: stage() resets the detector, so a prepare() before it is discarded and the scan silently falls back to a default TriggerInfo.
The stats system test has never got past connect in CI. The IOC reports
TSAcquireMode's choices as ('Fixed length', 'Circ. buffer'), and has no
TS:TSNumPoints_RBV; the per-statistic arrays belong to NDStats itself, at
its own prefix, rather than to the embedded time series plugin under TS:.
Taken from the PVs in example-services' generated NDStats OPI, which is
the IOC CI runs against - worth a check against a live IOC.
The test's plan also prepared before bp.count staged the detector, which
discards the prepare.
The bounded-tier ADR was written first but lands third, so 0020-0022 read in an order no branch introduces them in. Number them the way the commits arrive - readable formats, one logic property, then the bounded tier - and turn the "ADR 00NN" prose references between them into links. The bounded-tier ADR is also rewritten: shorter, and without the per-kickoff sizing and _update_prepare_context internals that no longer exist. What it said about exposing the collect verbs now lives in 0023, next to the decision itself.
_stage_funcs/_unstage_funcs are gathered internally, so wrapping each one in an AsyncStatus only to reach for its .task is noise. Type them as Callable[[], Awaitable[None]] and let the registrants be plain coroutine functions. Type on_complete as returning a Coroutine rather than any Awaitable, which is what AsyncStatus wants, so the _awaited shim goes with it. Note on FlyableLogic.stop why it deliberately shares a name with MovableLogic.stop: a motor record is both moved and flown, and one stop serves both. Assisted-by: Claude Opus 5
The two tests either patched the logic's stop() with an AsyncMock or called it directly. Fold them into one parametrized test that goes through the public unstage(), covering both the running and idle cases. Assisted-by: Claude Opus 5
self.logics was never read after __init__ sorted the objects into their roles, so it was a second copy of state that could only go stale. Assisted-by: Claude Opus 5
NDWriterPluginT was a second TypeVar with the same bound as the NDPluginBaseIOT already declared in _io.py, so it needed its own nitpick_ignore entry for nothing. Assisted-by: Claude Opus 5
A DetectorDataLogic stashed what it worked out on itself, in a _to_start attribute, for start() to pick up. But the detector asks every logic what it would make and then starts only the ones it settles on, so a logic that is passed over is left holding a plan for a scan it never served. Give DetectorDataLogic a DataCtxT and have make_data_provider return the context alongside its provider, exactly as FlyableLogic threads its own between on_prepare, on_kickoff and on_complete. The detector hands it straight back to start(), so a logic keeps no state of its own. Two things ride on the same signature change: - flush_period moves from ADHDFDataLogic's __init__ to TriggerInfo, so the scan says how often it wants data rather than the device being built to a fixed rate. It sizes the HDF chunk as before, and now also drives PandaHDFDataLogic's flush period signal, which was hardcoded 0. - StatsTimeSeriesProvider becomes a concrete EventPageDataProvider in core: nothing in it was areaDetector-specific once the datakey dtype comes from the signal's datatype rather than a hardcoded <f8. Its pages now carry numpy arrays rather than lists; event-model does not type those, but they round trip, so that is flagged for an upstream fix rather than converted at every page. Assisted-by: Claude Opus 5
read(), describe() and their configuration counterparts each derived their callables through a _funcs_for(verb) switch, and a subclass with data of its own contributed by overriding _extra_funcs_for(verb) — one hook serving four verbs, which every caller then had to filter. Give each verb its own tuple of contributors, exactly as stage() already gathers _stage_funcs. StandardReadable puts the registry's contribution in each, as _formatted_read, _formatted_describe and so on, which hold the format table their verb cares about and nothing else. StandardDetector appends what its data logics produce rather than overriding a hook. The _Verb enum, _funcs_for, both _extra_* hooks and _UncachedRead all go; everything in a tuple runs in parallel. add_readables is deprecated in favour of set_readable_format. Since ADR 0020 gave it "set" semantics it has not been adding anything, and its name says otherwise. Assisted-by: Claude Opus 5
Review asked whether a single CtxT is right or whether prepare and kickoff should have their own. Writing down the answer so it is not asked a third time. Assisted-by: Claude Opus 5
_HintsFromName and _HintedFields existed only to wrap a list of field names in something with a .hints property, so the merge loop could read it back off again. Have the hint funcs yield Hints directly and both adapters go, along with the `name` neither of them had anything useful to put in — the merge error now names the Device doing the merging, which is the one the caller asked. Assisted-by: Claude Opus 5
The verb was recomputed after every trigger() on the grounds that a re-armed bounded provider might sit the rest of the scan out. It cannot: on_trigger re-prepares the data from the same TriggerInfo prepare() used, so both cache-key comparisons match and _rearm_bounded runs, which passes streamable providers through untouched and re-makes bounded ones from byte-identical arguments. Nor is there a consumer -- the bound verbs exist for the bundler's isinstance checks on the fly path, and a step scan reads through _data_read, which consults data.pageable directly. With one call site left, _publish_collect_methods is inlined into prepare(), and its docstring becomes a comment saying what the bundler change now in progress upstream will let us delete: the binding goes and _collect_asset_docs/_collect_pages become plain public methods. Assisted-by: Claude Opus 5
on_trigger re-prepared the data unconditionally, so the first point of a step scan armed the buffer twice: prepare() made and started the provider, then on_trigger stopped it and made and started it again. For an NDStats time series that is TSAcquire 1, 0, 1 plus four duplicated config writes before a frame arrives -- harmless against an empty buffer, but a visible glitch on the arming control, and with external triggering prepare() has already started acquiring, so the gap can drop frames. The re-arm is still needed from the second point on, where the buffer holds the previous point's data. _FlyCtx carries a needs_rearm flag saying which case this is: prepare() leaves it False, and each trigger sets it. Assisted-by: Claude Opus 5
_wait_for_collections was handed a single initial_collections_written and seeded every provider from it. That is only sound because _drop_shadowed bans one detector serving both a finite buffer and a file writer, which would be at different counts -- an invariant enforced three methods away from the code relying on it. Put the number on _DetectorData, next to the pageable/streamable split it depends on, and say there what would have to change if the ban were ever lifted. on_trigger and on_kickoff each record it under their own rule (a re-armed buffer starts at zero; a fly scan reads live because buffers accumulate across the kickoffs of one prepare) and _wait_for_collections reads it back, so it loses a parameter. _FlyCtx.kickoff_collections_written goes with it, along with its "Kickoff not run" guard: StandardFlyable.complete() already refuses to run before kickoff, with a better message. Assisted-by: Claude Opus 5
on_trigger armed unconditionally, so an externally triggered detector was armed once by prepare() and then again on every point of a step scan, restarting an acquisition that was already sitting waiting on its trigger source. on_prepare and on_kickoff have always split on the trigger type; on_trigger is now the same, and matches what DetectorAcquireLogic .start_acquiring's own docstring already claimed: "called from prepare() for external-trigger modes, and from kickoff() / trigger() for internal". Carried over from main, where trigger() armed unconditionally too. No test covered trigger() in an external mode; test_trigger_arms_detector becomes test_trigger_arms_only_an_internally_triggered_detector, parametrized over all three modes and asserting the detector ends up armed exactly once either way. Assisted-by: Claude Opus 5
…flict resolution Assisted-by: Claude Sonnet 5.5
…e stack APIs Eiger uses set_readable_format instead of the deprecated add_config_signals; test_writer_image_hints drops add_detector_logics and PluginSignalDataLogic, which the stack removes. Assisted-by: Claude Sonnet 5.5
coretl
force-pushed
the
detector-stack-4-flyable
branch
from
October 8, 2026 14:44
ee22fb1 to
90c9dcd
Compare
They were dropped silently, so a typo such as <READABLE_FORMAT> lost the formats without any sign. Still not a NameError, as a newer version may write keys this one does not understand. Assisted-by: Claude Opus 5.5
…ative A settings file used to record every registered format, which pinned the class defaults of the day into it and, as applying merged, could not return from one technique to another. Store only the changes since construction, with a dropped child as null, and reset the tree to its class defaults before applying them. A file with no formats key still leaves formats alone; an empty one now restores the defaults. Child paths are now relative to their owner rather than to the root, so the owner is not repeated on every line. Registrations of objects outside their owner's subtree (dodal's Reference and __init__ argument cases) are outside the stored tree too, so are no less storable, and as __init__ registrations they are defaults that are never written. The fixtures go back to main's formatting plus the one new line. Assisted-by: Claude Opus 5.5
… them Assisted-by: Claude Opus 5.5
…tively The readable registry becomes a MutableMapping[Device, Format] whose __setitem__ validates the entry, as Settings does, and whose entries() yields typed (device, format) tuples for the verbs to match on. Enable pyright's reportMatchNotExhaustive so a new format cannot be silently ignored by a match. _config_signals moves next to StandardDetector and uses the public get_readable_formats(). Assisted-by: Claude Opus 5.5
Settings retrieval needs the formats that differ from construction. Expose that as a method rather than reaching into _default_readables from a module helper. Assisted-by: Claude Opus 5.5
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, for CI only. Supersedes nothing, closes nothing, and is not for review as a
single PR — it is bluesky#1400 + bluesky#1403 + bluesky#1367 + the StandardFlyable rework built as one linear
branch so CI can exercise the union. The intended split is four PRs at the cut points
below, each of which is green locally on its own.
Replaces bluesky#1408, which was pushed from a token that no longer has write
access to that repo. Same branch line, same purpose. It is raised inside this fork
because the token cannot open a cross-fork PR either; base is
coretl:main, whose mergebase with this branch is exactly the branch point, so the diff is only the 57 commits
below. None of bluesky#1400, bluesky#1395, bluesky#1403 or bluesky#1367 has been touched.
The four cut points
detector-stack-1-readable-formatsStandardReadableFormat, load/save, onelogicproperty, ADRs 0021+0022. No detector.Fixes #1394detector-stack-2-detector-readableStandardDetectoris aStandardReadable; AD IO classes declare their own formats; theprepare_singledata logic tier goes. Supersedes bluesky#1395/bluesky#1403detector-stack-3-bounded-tierDetectorDataLogictier, onemake_data_provider/startsplit, period plumbing, stats TS, ADR 0020.Fixes #1309 #1364detector-stack-4-flyableStandardDetectorinheritsStandardFlyable, ADR 0023, and this round's review fixesThe tip passes all four
toxenvs. G1 and G3 are stale relative to the tip and stillneed re-cutting before they become PRs — the last two review rounds landed on G4 only.
What changed since bluesky#1408
@coretl's second review round, all actioned on the tip.
A data logic threads a context instead of stashing state on itself.
DetectorDataLogicgains aDataCtxT;make_data_providerreturns(provider, ctx)andstart(ctx)takes it back, the wayFlyableLogicalready threads prepare → kickoff →complete. The detector asks every logic what it would make and then starts only the ones it
settles on, so a logic that stored its plan in a
_to_startattribute was left holding aplan for a scan it never served. Every
_to_start/_to_openin tree is gone.flush_periodmoves from device construction toTriggerInfo. It was anADHDFDataLogic.__init__argument, so how often a file flushed was a property of how theDevice was built rather than of the scan being run. As a
TriggerInfofield the scan asksfor it: it sizes the HDF chunk from the frame period as before, and now also drives
PandaHDFDataLogic's flush-period signal, which was hardcoded to 0. 0 still means "leavethe hardware as it is".
StatsTimeSeriesProviderbecomes a concreteEventPageDataProviderin core. Nothing init was areaDetector-specific once the datakey dtype comes from the signal's datatype rather
than a hardcoded
<f8. It takes(data, collections_written_signal, timestamps)and servesany device that fills fixed-length arrays as it acquires — a scaler's MCA arrays as much as
a stats time series. Its pages now carry numpy arrays rather than lists;
event-modeldoesnot type those, but they round trip through the bundler, so it is a
type: ignorewith anote to fix upstream rather than a conversion on every page.
Each readable verb gathers a tuple, like
stage()does.read(),describe()andtheir configuration counterparts derived their callables through one
_funcs_for(verb)switch, and a subclass with data of its own contributed by overriding
_extra_funcs_for(verb)— one hook serving four verbs, which every caller then had tofilter. Each verb now has its own tuple of contributors, exactly as
stage()gathers_stage_funcs.StandardReadableputs the registry's contribution in each, as_formatted_read/_formatted_describe/…, which hold only the format table their own verbcares about;
StandardDetectorappends what its data logics produce._Verb,_funcs_for,both
_extra_*hooks and_UncachedReadare deleted. This closes the_extra_funcs_forprivate-API leak listed as deferred on bluesky#1408.
Hints go the same way, and lose two adapter classes.
_HintsFromNameand_HintedFieldsexisted only to wrap a list of field names in something with a.hintsproperty so the merge loop could read it straight back off. The hint funcs yield
Hintsdirectly and both classes go, along with the
nameneither had anything useful to put in— the merge error now names the Device doing the merging, which is the one the caller
asked.
add_readablesis deprecated in favour ofset_readable_format. Since ADR 0020 gave it"set" semantics it has not been adding anything, and its name says otherwise. It still
works, with a
DeprecationWarning.Smaller items.
_stage_funcs/_unstage_funcsare typedCallable[[], Awaitable[None]]rather than wrapping each hook in an
AsyncStatusonly to reach for its.task;on_completeis typed as returning aCoroutine, which is whatAsyncStatuswants, so the_awaitedshim goes;DetectorLogicno longer keeps thelogicstuple it was handed, whichwas a second copy of state that could only go stale;
NDWriterPluginTis replaced by theNDPluginBaseIOTalready declared in_io.py, with the same bound.One review item came back "no". It asked whether
FlyableLogicshould splitCtxTintoPrepareCtxTandKickoffCtxT. Splitting would make three| Nonefields non-optional,but costs a second dataclass per flyer (or inheritance between two), a third parameter at
every declaration site — the PandA logics would read
FlyableLogic[SeqTableInfo, None, None]— and buysStandardFlyablenothing, since the context it holds changes type partway through the lifecycle and cannot be typed as one or the other regardless. The
| Noneis doing useful work: it says this appears at kickoff. Reasoning is written into ADR 0023
so it is not asked a third time.
FlyableLogic.stop()stays. It was deleted as a name collision withMovableLogic.stop()and put back on review: a motor record is both moved and flown, andone
stoplegitimately serves both. It now carries a comment saying so.Behaviour changes
Everything listed on bluesky#1408 still applies, plus:
TriggerInfogainsflush_period, defaulting to 0.DetectorDataLogic.make_data_providergains aflush_periodargument and returns(provider, ctx);starttakes that ctx. Out-of-tree data logics must be updated.ADWriterFactory.hdf(..., flush_period=...)is removed; set it on theTriggerInfo.StatsTimeSeriesProvideris renamed and moved toophyd_async.core.EventPageDataProvider.StandardReadable.add_readableswarns; useset_readable_format.StandardReadable._extra_funcs_forand_extra_hint_sourcesare removed — a subclassappends to
_read_funcs/_describe_funcs/_hint_funcsinstead.Found along the way
Carried over from bluesky#1408 and still true:
61ad2d204deletesPageableDataProvider.make_readingswithout updating
test_stats_time_series.py, which still calls it; the branch is red atthat commit and green again at the next. Reproducible on
detector-tiers-impl.docs/how-to/implement-ad-detector.mddocumentedPluginSignalDataLogicafter it hadbeen deleted. Now a
set_readable_formatcall.c239654d0converts the IO classes to
Format.CONFIG_SIGNALannotations, andfe787ce0cmovesWaitForPluginsout ofAreaDetector.64f4cd32bnarrowsNDROIIO.size_x/y/zfromSignalRWtoSignalR. ROI size is settable on a realNDPluginROI and the narrowing is unrelated to that commit's purpose, so they were kept
SignalRWwith only the format annotation applied.Deferred
what it actually produced, so a detector cannot declare both and choose per scan. Until
that is fixed and released,
_publish_collect_methodsbinds the right verb as an instanceattribute; ADR 0023 records the intent to delete it.
suffixes so the shadowing holds by construction rather than by the user remembering a plan
stub, and giving
Settingsenough provenance that storing a mode does not freeze computedvalues into the file.
AI Disclaimer
The code and this description were generated with AI and reviewed/tweaked by the author.