From b289e4077f5a6d73c1ac7f5781699feaa7669aca Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:19:28 +0000 Subject: [PATCH 01/16] docs: ADR 13 - declarative/procedural split and ControllerFiller --- ...-procedural-split-and-controller-filler.md | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md diff --git a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md new file mode 100644 index 000000000..9e2578865 --- /dev/null +++ b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md @@ -0,0 +1,158 @@ +# 13. Declarative/Procedural Split and ControllerFiller + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +FastCS currently has two mechanisms for declaring the shape of a `Controller`: + +1. **Class-scope `Attribute` instances** (`ramp_rate = AttrRW(Float(), io_ref=...)` + assigned directly in the class body). `BaseController._bind_attrs` + (`src/fastcs/controllers/base_controller.py`) walks the MRO, finds these, and + `deepcopy`s each one onto the instance so that multiple instances of the same + `Controller` subclass do not share mutable state. +2. **Bare type hints** (`frames: AttrRW[int]`) validated, not created, by + `HintedAttribute` (`src/fastcs/attributes/hinted_attribute.py`) via + `_find_type_hints`/`_validate_type_hints`. The actual `Attribute` must be + constructed and assigned by the developer, normally in an `initialise()` + override that introspects a device. + +ophyd-async has the equivalent split (`Device` class-body hints vs. `__init__` +procedural construction, see `docs/explanations/declarative-vs-procedural.md`), +but only one mechanism for the declarative half: hints always *create* children, +provisioned by a `DeviceConnector`-owned `DeviceFiller` +(`ophyd_async/core/_device_filler.py`) either immediately or later via +connect-time introspection. There is no ophyd-async equivalent of FastCS's +class-scope instance style, and there cannot be — a `Signal`'s backend depends on +which `DeviceConnector` the owning `Device` is constructed with, so the backend +cannot be known until connect/construction time chooses the connector. + +Our own downstream drivers show why the FastCS class-scope-instance mechanism is +already the minority case in practice, not the norm: + +- `fastcs-eiger` mixes class-body instances (`trigger_exposure = AttrRW(Float())`) + with bare hints filled by REST-API introspection in `initialise()` + (`eiger_detector_controller.py`) — i.e. it already wants one unified mechanism. +- `fastcs-secop`, `fastcs-PandABlocks`, and `fastcs-catio`'s dynamic path build + **all** of their attributes from wire/YAML-derived data at `initialise()` time; + none of them use class-scope instances at all. + +The `deepcopy` half of `_bind_attrs` exists solely to make class-scope instances +safe to reuse across `Controller` instances. It is fragile (IO objects, bound +callbacks, and connections do not always survive a deepcopy cleanly) and costs +construction time on every instantiation, for a feature none of our real-world +introspecting drivers use. + +## Decision + +Adopt a single declarative mechanism, matching ophyd-async: **class body = +bare type hints only; instance scope = procedural construction.** Concretely: + +- Remove class-scope `Attribute` **instances** entirely. `AttrRW(Float(), + io=...)` may no longer be assigned directly in a class body. +- Remove the deepcopy half of `_bind_attrs`. Method binding for `@command`/ + `@scan` (the `UnboundCommand`/`UnboundScan` machinery) is unaffected and + stays, since it does not require deepcopy — see decision 14 (`@attr_rw` + decorator sugar). +- Remove `HintedAttribute` and `_validate_type_hints`/`_validate_hinted_*` as + a *separate* validation-only pass. Their job — "this hinted child must + exist with the right type after initialisation" — is subsumed into the new + `ControllerFiller`. +- Introduce `ControllerFiller`, a direct structural port of ophyd-async's + `DeviceFiller`. It scans class-body type hints (`AttrR/W/RW[T]`, + `Command[P, T]` — see [ADR 15](0015-typed-commands.md), nested `Controller` + / `ControllerVector[T]`), creates children **unfilled**, and tracks + filled/unfilled state per child. `check_filled(source)` raises, listing by + name, anything a `Controller`'s `initialise()` promised via a hint but did + not provision. +- `ControllerFiller` yields `(child, extras)` for each created child — the + `extras` being anything else found in an `Annotated[...]` hint — so that + protocol libraries (a future SCPI package, for example) can define their + own extras vocabulary the same way ophyd-async's `PvSuffix`/`TangoPolling` + do. Core FastCS defines **no** extras vocabulary for 1.0 (decision 3 of + #388). +- The refined rule from decision 14 of #388: *class body = declarations + + decorated behaviour; instance scope = construction with data.* This keeps + `@command`/`@scan`, and the new `@attr_r`/`@attr_rw` sugar, as class-body + citizens, since none of them require per-instance deepcopy — they bind a + method to `self` at construction time instead. + +Example, before and after: + +```python +# Before: class-scope instance, deepcopy'd per-instance +class TemperatureRampController(Controller): + start = AttrRW(Int(), io_ref=TemperatureControllerAttributeIORef(name="S")) + +# After: bare hint, filled procedurally +class TemperatureRampController(Controller): + start: AttrRW[int] + + def __init__(self, index: int, conn: IPConnection) -> None: + super().__init__() + suffix = f"{index:02d}" + self.start = AttrRW(Int(), io=TempIO(conn, "S", suffix)) +``` + +Introspecting controllers (`fastcs-eiger`, `fastcs-secop`, +`fastcs-PandABlocks`, `fastcs-catio`'s dynamic path) keep working exactly as +today's `initialise()` + `add_attribute` pattern, but the fully-dynamic case +where the *set* of attributes is not known until a network round-trip +completes (`fastcs-PandABlocks`, `fastcs-secop`) needs `ControllerFiller` to +support filling children that were never hinted at all — mirroring +`DeviceFiller.fill_child_signal`'s "no annotation existed, introspection +added an undeclared attribute" path. This is a harder requirement than most +of ophyd-async's own connectors exercise (PVI and Tango both fill *some* +undeclared children, but FastCS's dynamic drivers may have **zero** static +hints and still need to build a full attribute tree from nothing) and is +called out below as an open question. + +## Consequences + +- Every existing driver using class-scope `Attribute` instances needs + migration to bare hints + `__init__`/`initialise()` construction — see the + Example 1 (`DRAFT: Example 1 — IORef temperature controller`) and Example + 2 (`DRAFT: Example 2 — introspectable Eiger-style controller`) sub-issues + of #388, and the corresponding downstream repo work. +- `Controller.__init__` no longer needs to run `_bind_attrs`, simplifying + construction and removing a source of deepcopy-related bugs. +- Static typing improves: a bare hint `frames: AttrRW[int]` is exactly the + type a type checker sees, with no deepcopy step that could plausibly + change it. +- `ControllerFiller` becomes a new stable, documented surface — see + [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) for how + it interacts with the stable `ControllerAPI` surface consumed by the + embedded ophyd-async connector. + +## Open questions + +1. Does `ControllerFiller` need to support "no hints exist at all — build the + entire attribute tree from introspected data" (the `fastcs-PandABlocks` + and `fastcs-secop` case), or is some minimal static shape (even just a + marker on the `Controller` subclass) always required? `DeviceFiller` has + no precedent for the fully-hint-free case. +2. `fastcs-catio`'s dynamic path builds whole controller *classes* at runtime + via `type(...)` from YAML definitions, before any instance (and hence any + `ControllerFiller`) exists. Is that pattern still supported, unsupported, + or does it need to move to instance-level dynamic attribute construction + under the new model? +3. `fastcs-eiger`'s `OdinController.initialise()` constructs new attributes + that reference sibling sub-controllers' attributes, assuming those + sub-controllers already exist. Does `ControllerFiller` impose an + ordering/dependency mechanism between sibling children, or is this left + as an `initialise()` implementation detail (call `super().initialise()` + first)? +4. Should `check_filled` be able to distinguish "this hinted child is + optional" (ophyd-async's `Optional[X]` convention), or does FastCS treat + every hint as required for 1.0? +5. Exact `ControllerFiller` method names/signatures are left to the + prototype — should they mirror `DeviceFiller`'s names 1:1 + (`fill_child_signal` → `fill_child_attribute`?) for discoverability by + developers who know both libraries, or diverge where FastCS's vocabulary + (`Attribute` vs `Signal`) differs? From 710433cdbd3f3ad75ef101c106684a50fa818d78 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:20:22 +0000 Subject: [PATCH 02/16] docs: ADR 14 - AttributeIO R/W/RW rework, remove AttributeIORef --- .../decisions/0014-attribute-io-rw-rework.md | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 docs/explanations/decisions/0014-attribute-io-rw-rework.md diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md new file mode 100644 index 000000000..7d731a366 --- /dev/null +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -0,0 +1,186 @@ +# 14. AttributeIO R/W/RW Rework and Removal of AttributeIORef + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), +[ADR 9](0009-handler-to-attribute-io-pattern.md), [ADR 12](0012-attribute-io-naming-convention.md) + +## Status + +Proposed + +## Context + +[ADR 9](0009-handler-to-attribute-io-pattern.md) split the old `Handler` +pattern into `AttributeIO` (behaviour, one instance per `Controller`, +shared across attributes) and `AttributeIORef` (per-attribute resource +specification, dispatched to the right `AttributeIO` by type at +`_connect_attribute_ios` time). Its sole structural justification was that +class-scope `Attribute` instances are created before `__init__` runs, so +they cannot close over a live connection — the `AttributeIORef` only needed +to carry inert data (a register name, a URI) until the matching +`AttributeIO` was found by type at `post_initialise()`. + +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md) removes +class-scope `Attribute` instances entirely. Every attribute is now +constructed procedurally, in `__init__` or `initialise()`, where a live +connection is already in scope. The situation `AttributeIORef` was invented +to solve no longer exists. + +We also verified downstream that nothing outside FastCS consumes `io_ref` or +the IO registry directly — every consumer bottoms out in +`attr.set_on_put_callback(io.send)` / `attr.set_update_callback(io.update)` +(`base_controller.py:_connect_attribute_ios`), i.e. the ref/registry split is +a dispatch layer over callbacks that already exist as plain methods. + +The dispatch-by-type registry has real costs in our downstream drivers: + +- `fastcs-catio` registers **three** separate `AttributeIO`/`AttributeIORef` + pairs on one `Controller` (`ios=[poll_io, symbol_io, coe_io]`) purely so + each attribute's `io_ref` type can select the right one at + `_connect_attribute_ios` time — indirection that a direct `io=` argument + removes outright. +- `fastcs-secop` needs a private escape hatch, + `attr._call_sync_setpoint_callbacks`, to push setpoint echoes from its + `send()` implementation, because the current `AttributeIO.send` signature + has no sanctioned way to do this — flagged in-code as pending a public API. +- `fastcs-PandABlocks`'s `UnitsIO.send` mutates a *sibling* attribute's + datatype (`attribute_to_scale.update_datatype(...)`), reaching outside the + attribute it was invoked for — a pattern the new IO shape should not make + harder, even though it stays an edge case. + +## Decision + +Replace `AttributeIO`/`AttributeIORef` with three focused, per-attribute IO +base classes with abstract `update`/`send` methods, passed as a single `io=` +constructor argument: + +```python +class ReadIO(Generic[DType_T], ABC): + def __init__(self, update_period: float | None = None): ... + + @abstractmethod + async def update(self, attr: AttrR[DType_T]) -> None: ... + + +class WriteIO(Generic[DType_T], ABC): + @abstractmethod + async def send(self, attr: AttrW[DType_T], value: DType_T) -> None: ... + + +class ReadWriteIO(ReadIO[DType_T], WriteIO[DType_T], ABC): ... +``` + +(Working names per #388; exact naming — `ReadIO`/`WriteIO`/`ReadWriteIO` vs. +`AttrRIO`/`AttrWIO`/`AttrRWIO` — is an open question below and in +[ADR 17](0017-naming-pass.md).) + +- `AttrR(dt, io: ReadIO[DType_T] | None)`, `AttrW(dt, io: WriteIO[DType_T] | + None)`, `AttrRW(dt, io: ReadWriteIO[DType_T] | None)`. Passing a + read-only IO to an `AttrRW` is a **static** type error, not a runtime + `_validate_io` check — the abstract methods force a subclass to implement + the right surface for the `Attr` flavour it is attached to. +- `update_period` moves onto `ReadIO` — it describes the IO's polling + behaviour, not a property of the attribute. `Controller.create_api_and_tasks` + schedules from `attr.io.update_period` instead of pattern-matching on + `AttributeIORef` (`control_system.py`/`controller.py`'s + `case AttrR(_io_ref=AttributeIORef(update_period=update_period))` becomes a + direct attribute access). +- **Delete:** `AttributeIORef`, the `ios=` constructor kwarg on + `BaseController`/`Controller`/`ControllerVector`, `_validate_io`, + `_connect_attribute_ios`, `_attribute_ref_io_map`, `__init_subclass__`'s + generic-arg sniffing in `AttributeIO`, and the second TypeVar — + `Attribute[DType_T, AttributeIORefT]` collapses to `Attribute[DType_T]`, + making `AttrRW[float]` structurally isomorphic to ophyd-async's + `SignalRW[float]`. +- `io=None` keeps today's soft-attribute behaviour: `AttrRW` self-wires + setpoint→readback via `_internal_update`, and the sync-setpoint machinery + is unaffected. This remains the analogue of ophyd-async's + `soft_signal_rw`. +- A concrete `CallbackReadIO`/`CallbackWriteIO` pair ships in core as an + escape hatch for one-off attributes, mirroring `soft_command` — e.g. + `CallbackReadIO(update=cb, update_period=0.2)` — without requiring a full + subclass. + +Migration is mechanical for the common case (an old `AttributeIO` subclass +absorbs its `AttributeIORef`'s fields into its own `__init__` and is +constructed once per attribute instead of once per controller): + +```python +# Before (ADR 9 shape) +class TempIORef(AttributeIORef): + name: str + +class TempIO(AttributeIO[float, TempIORef]): + async def update(self, attr: AttrR[float, TempIORef]) -> None: + resp = await self._conn.send_query(f"{attr.io_ref.name}?\r\n") + await attr.update(float(resp)) + +ramp_rate = AttrRW(Float(), io_ref=TempIORef(name="R")) +# ... elsewhere: Controller(ios=[TempIO(conn)]) + +# After +class TempIO(ReadWriteIO[float]): + def __init__(self, conn: IPConnection, name: str, update_period=0.2): + super().__init__(update_period=update_period) + self._conn, self._name = conn, name + + async def update(self, attr: AttrR[float]) -> None: + resp = await self._conn.send_query(f"{self._name}?\r\n") + await attr.update(float(resp)) + + async def send(self, attr: AttrW[float], value: float) -> None: + await self._conn.send_command(f"{self._name}={value}\r\n") + +self.ramp_rate = AttrRW(Float(), io=TempIO(conn, "R")) +``` + +`fastcs-catio`'s three-IO-per-controller pattern becomes three IO +*instances*, one per relevant attribute, with no registry needed at all. +`fastcs-secop`'s private `_call_sync_setpoint_callbacks` call is replaced by +a public method on `AttrW`/`ReadWriteIO` — exact shape is an open question. + +## Consequences + +- Every driver that declared `AttributeIORef` subclasses must migrate them + into `AttributeIO.__init__` fields — see the affected §9 files in the + sub-issues of #388 (`attributes/`, `controllers/base_controller.py`, + `controllers/controller.py`) and the corresponding downstream repo issues. +- `Attribute` loses its second generic parameter, simplifying every type + hint in downstream code (`AttrR[float, MyRef]` → `AttrR[float]`). +- Access-mode compatibility between an `Attr` and its `io=` argument is + caught by the type checker instead of at runtime in `_validate_io` — + earlier feedback for driver authors, at the cost of losing the runtime + "no AttributeIO registered for this ref type" error message; a + misconfigured `io=None` on an attribute that needed IO now simply behaves + as a soft attribute rather than raising loudly. Whether this needs a + runtime check as well (e.g. in `post_initialise`) is an open question. +- [ADR 12](0012-attribute-io-naming-convention.md)'s guidance (subclass to + get a shorter driver-local name) still applies to the new `ReadIO`/ + `WriteIO`/`ReadWriteIO` names. + +## Open questions + +1. Final class names: `ReadIO`/`WriteIO`/`ReadWriteIO` vs. `AttrRIO`/ + `AttrWIO`/`AttrRWIO` (mirroring the `Attr` family) vs. something else + entirely — see [ADR 17](0017-naming-pass.md). +2. What is the public replacement for `fastcs-secop`'s + `_call_sync_setpoint_callbacks` workaround? Does `WriteIO.send` get an + optional `sync_setpoint` callback argument, or does `AttrW.put` grow a + public method IO authors can call from `send`? +3. Should there be a runtime check (e.g. at `post_initialise`) that + catches "read-only IO passed to a write-capable `Attr`" for cases the + static type checker cannot see (e.g. an `Any`-typed IO built + dynamically, as in `fastcs-secop`'s and `fastcs-PandABlocks`'s + introspection-driven construction)? Both of those drivers build + attributes and their IO from runtime data where static checking cannot + help. +4. `fastcs-PandABlocks`'s `UnitsIO.send` mutates a sibling attribute's + datatype and `fastcs-catio` recovers per-attribute metadata via + `attribute.io_ref` from *outside* the attribute's own `send`/`update` + (`panda_controller.py:_coerce_value_to_panda_type`). With `io_ref` + removed, what is the sanctioned way to recover an attribute's IO-specific + metadata (e.g. `attr.io` becoming a public, typed property)? +5. Do we ship `CallbackReadIO`/`CallbackWriteIO` in `fastcs` core for 1.0, or + leave the "no subclass needed" one-off case entirely to driver authors + using `io=None` plus manual `set_update_callback`? From 3ce16b99ee6160de47f47ce1cdd5a846825bc7d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:20:55 +0000 Subject: [PATCH 03/16] docs: ADR 15 - typed commands --- .../decisions/0015-typed-commands.md | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 docs/explanations/decisions/0015-typed-commands.md diff --git a/docs/explanations/decisions/0015-typed-commands.md b/docs/explanations/decisions/0015-typed-commands.md new file mode 100644 index 000000000..a3d6c9b37 --- /dev/null +++ b/docs/explanations/decisions/0015-typed-commands.md @@ -0,0 +1,102 @@ +# 15. Typed Commands + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +FastCS `Command` (`src/fastcs/methods/command.py`) is void/void only: +`Method._validate` requires zero parameters and `None`/empty return type. +`UnboundCommand.bind` produces a `Command` wrapping a zero-arg, no-return +async callable. `Method.__init__` already captures the full +`inspect.Signature` of the wrapped function (`method.py:21`), but `Command`'s +own `_validate` throws that signature away by rejecting anything with +parameters. + +ophyd-async's equivalent, `Command[P, T]` (`ophyd_async/core/_command.py`), +carries a real parameter and return type, exposed via `CommandBackend.signature` +and `CommandBackend.execute(*args, **kwargs) -> T`. `TriggerableCommand = +Command[[], None]` is the void/void case, expressed as a special case of the +general one rather than the only case. + +This gap already shows in our downstream drivers rather than being +speculative: `fastcs-secop` builds command arguments and results dynamically +from SECoP's wire `datainfo` (`_controllers.py:102-110`) — a genuinely typed +(if dynamically-typed) command surface that FastCS's void/void `Command` +cannot represent today, forcing `fastcs-secop` to route command arguments +through attributes on a dedicated `SecopCommandController` instead of a +single typed call. + +## Decision + +Lift the zero-arg/no-return restriction in `Method`/`Command._validate`, and +introduce `Command[P, T]` generic over parameters and return type, keeping +the already-captured `inspect.Signature` as the public surface — +`ControllerAPI` exposes it directly, mirroring `CommandBackend.signature`. + +Transport capability is declared, not assumed uniform: + +- **Tango, REST, GraphQL, and the embedded ophyd-async connector** serve + typed commands fully — arguments and return value round-trip through + each protocol's native typed-call mechanism. +- **EPICS CA/PVA** stay void/void at the wire level (there is no PV + representation of "call with these typed arguments, get this typed + return" that doesn't already exist as separate attributes). They **skip + typed commands with a warning** at start-up rather than failing to serve + the controller at all. A command the user explicitly declares as typed + *and* forces to be served over an EPICS-only transport is a hard error — + matching the existing ophyd-async connector's behaviour, which errors + rather than silently drops when a `Device` requires a capability its + connector cannot provide. + +```python +class Ramp(Controller): + move_to: Command[[float], None] # typed: not served over CA/PVA + stop: Command[[], None] # void/void: served everywhere +``` + +## Consequences + +- `Command.__call__` gains real `*args`/`**kwargs` forwarding instead of a + bare `await self.fn()`; `UnboundCommand.bind` needs the same treatment. +- `ControllerAPI` (or its per-command entries) needs to expose the + signature to transports, so each transport can decide serve-fully / + serve-with-warning / hard-error per decision above. +- EPICS transports (`transports/epics/ca`, `transports/epics/pva`) need a + capability check at controller-API-build time, producing a startup-time + warning log rather than a runtime failure per typed-command call. +- `fastcs-secop`'s dynamically-typed command args/results likely still need + `Command[Any, Any]` or a per-instance generated type, since SECoP's + `datainfo` is only known at connect time — full static typing of command + signatures is not achievable for introspection-driven drivers, only for + statically-declared ones. This mirrors the same "hint vs. no-hint" tension + as [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). +- Command args/return values need datatype validation analogous to + `Attribute`'s `DataType.validate` — whether they reuse the `DataType` + family directly or a separate mechanism is an open question. + +## Open questions + +1. Do command arguments/return values validate through the same `DataType` + family attributes use, or is a separate (lighter-weight, since there's no + "current value" to cache) validation path introduced? +2. For `fastcs-secop`-style dynamically-typed commands, what's the + recommended pattern — `Command[Any, Any]` with manual validation inside + the handler, or a documented way to construct a `Command[P, T]` with `P`/ + `T` determined at runtime (which conflicts with normal generic typing)? +3. Exactly what should the EPICS skip-with-warning message say, and where — + at controller construction, at `post_initialise`, or lazily the first + time a typed command is looked up by the transport? +4. Should typed commands support partial typing (e.g. typed arguments but + void return, or vice versa), or is it all-or-nothing relative to + `Command[[], None]`? +5. Does the REST/GraphQL/Tango serialisation of complex argument/return + types (numpy arrays, `Enum`, `Table`) reuse existing `DataType` + serialisation code from attributes, and if so does that argue for + sharing more machinery between `Attribute` and `Command` than they do + today? From 6d6a1919cc33d1bc0ef75b6ad1de867af922befb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:21:33 +0000 Subject: [PATCH 04/16] docs: ADR 16 - setpoint cache, native timestamps, ControllerRunner --- ...-cache-timestamps-and-controller-runner.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md diff --git a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md new file mode 100644 index 000000000..3c20a85bd --- /dev/null +++ b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md @@ -0,0 +1,112 @@ +# 16. AttrW Setpoint Cache, Native Timestamps, and ControllerRunner + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +Three related gaps block a clean embedded ophyd-async connector +(see [ADR 19](0019-embedded-ophyd-async-connector.md)) and are useful to all +transports independently of embedding: + +1. **No cached setpoint.** `AttrW.put` (`src/fastcs/attributes/attr_w.py`) + applies a setpoint via `_on_put_callback` but does not retain it anywhere + queryable. ophyd-async's `SignalBackend.get_setpoint()` — needed for + `locate()` — has no FastCS equivalent to read from. +2. **No FastCS-native timestamps.** `AttrR.update` (`attr_r.py`) stamps + nothing; individual transports each do their own thing (EPICS records + get a timestamp from the record subsystem, Tango pushes are unstamped). + An embedded connector currently has no choice but to stamp receive-time + only, which is a real information loss versus what the underlying device + protocol may already provide (Tango event timestamps, EPICS record + timestamps at source). +3. **No documented, extracted runtime.** `FastCS.serve` (`control_system.py`) + inlines the full controller lifecycle — `initialise()` → + `post_initialise()` → `create_api_and_tasks()` → `connect()` → initial + coroutines → scan tasks — as private logic inside the `serve` coroutine. + There is no standalone object an embedding connector can start/stop + without also pulling in `FastCS`'s transport-serving and interactive-shell + concerns. + +Decision 13 of #388 requires this lifecycle, plus `ControllerAPI` and the +attribute/command runtime methods, to be formalised as fastcs-core's single +documented "stable interface" that the ophyd-async connector is restricted +to using — no reaching into `BaseController` internals. + +## Decision + +**Setpoint cache:** `AttrW` gains an internally-tracked last-applied +setpoint, exposed via a public getter (name TBD — see open questions), +updated whenever `put` is called, independent of whether the underlying +`send` succeeds. This is available to all transports (not just the embedded +connector) as a "what did we last ask for" query distinct from `AttrR.get()` +("what did we last read back"). + +**Native timestamps (+ severity):** `AttrR.update` accepts an optional +timestamp (and, where meaningful, severity) alongside the value, defaulting +to current time if not supplied by the caller. This is FastCS-native, not +EPICS-specific — Tango event pushes and other IO can supply a device-side +timestamp through the same path a `ReadIO.update` call already uses. The +embedded connector stamps receive-time only as an interim measure until this +lands, per decision 10 of #388 — this is 1.0 scope, not a follow-up. + +**ControllerRunner:** Extract the controller lifecycle currently inlined in +`FastCS.serve` into a standalone `ControllerRunner` (or equivalent +`Controller.serve()`/`Controller.stop()` API), independent of the +transport-serving and interactive-shell logic that stays in `FastCS`/ +`control_system.py`. `FastCS.serve` becomes a thin caller of +`ControllerRunner` plus transport wiring. The runner owns: + +- Running `initialise()`/`post_initialise()`/`create_api_and_tasks()` once. +- Running `connect()` and the initial coroutines. +- Starting/stopping the periodic scan tasks. +- Being **idempotent** — safe to call start again after a stop, since the + embedded connector's `connect_real` may run more than once across + reconnects (see [ADR 19](0019-embedded-ophyd-async-connector.md)). + +This, together with `ControllerAPI` and the attribute/command runtime +methods (`AttrR.get`/`add_on_update_callback`, `AttrW.put` + cached +setpoint, `Attribute.datatype`/`access_mode`/`description`/`group`), becomes +the documented stable surface referenced by decision 13 of #388. + +## Consequences + +- `FastCS.serve` shrinks to transport orchestration; the controller + lifecycle it currently inlines becomes independently testable and + reusable without instantiating a `FastCS` object or any `Transport`. +- Every `ReadIO.update` implementation *may* supply a timestamp/severity, + but existing IO that does not is unaffected — defaults to current time, + severity unset. +- Transports gain access to a real setpoint distinct from the readback + value; whether EPICS/Tango/REST/GraphQL surface this as new fields is + transport-specific follow-up work, not part of this ADR. +- The embedded ophyd-async connector becomes buildable against a documented, + narrow surface instead of `BaseController` internals — see + [ADR 19](0019-embedded-ophyd-async-connector.md). + +## Open questions + +1. Setpoint cache accessor name and shape — `AttrW.setpoint` property, + `AttrW.get_setpoint()` method (mirroring `SignalBackend.get_setpoint()`), + or folded into `AttrW.put`'s return value? +2. Timestamp/severity type — reuse a existing convention (e.g. + `ophyd_async`/bluesky's `Reading`/event-model shape) or define a + FastCS-native pair? Decision 12 of #388 already aligns numeric limits + naming with event-model `Limits` — should timestamps/severity follow the + same alignment for consistency? +3. Severity: what are the FastCS-native severity levels, and do they map + 1:1 to EPICS alarm severities, or is EPICS's severity model transport- + specific with FastCS defining its own smaller/different vocabulary? +4. Exact `ControllerRunner` API shape — a class with `start()`/`stop()`, or + `async` context-manager semantics (`async with runner:`)? The embedded + connector needs idempotent start across reconnects; does the chosen shape + make idempotency the caller's responsibility or the runner's? +5. Does `ControllerRunner` own reconnect logic (calling `Controller.reconnect()` + on scan-task failure, as `Controller._create_periodic_scan_coro` does + today), or does that stay controller-specific and out of the runner's + documented surface? From b8707b04f2255142258ee52fc3add64bb7f2a104 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:22:14 +0000 Subject: [PATCH 05/16] docs: ADR 17 - naming pass (precision, Limits, Array1D/Table hints) --- .../decisions/0017-naming-pass.md | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 docs/explanations/decisions/0017-naming-pass.md diff --git a/docs/explanations/decisions/0017-naming-pass.md b/docs/explanations/decisions/0017-naming-pass.md new file mode 100644 index 000000000..f0b8989e4 --- /dev/null +++ b/docs/explanations/decisions/0017-naming-pass.md @@ -0,0 +1,105 @@ +# 17. Naming Pass: precision, Limits Alignment, Array1D/Table Hints + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +FastCS and ophyd-async independently arrived at similar concepts with +different names, which is exactly the "false friend" risk #388 opens with: +a developer moving between the two projects can be misled into assuming a +name means the same thing, or reach for a name that does not exist. + +Concretely, `_Numeric` (`src/fastcs/datatypes/_numeric.py`) and `Float` +(`src/fastcs/datatypes/float.py`) use `prec`/`min`/`max`/`min_alarm`/ +`max_alarm`; ophyd-async and the wider bluesky event-model use +`precision` and a `Limits` structure (`Limits(low, high)` per category, e.g. +control/display/alarm/warning) rather than five flat fields. FastCS's +`Waveform(array_dtype, shape)` and `Table` datatypes have no hint-level +spelling analogous to ophyd-async's `Array1D[np.int32]` (a `numpy.ndarray` +subscripted for shape) and `Table` (pydantic-based) hint syntax — under +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md), bare +hints are now load-bearing (they are what `ControllerFiller` scans), so +having ophyd-async-compatible hint spellings for array/table attributes +becomes more valuable than it was when hints were validation-only. + +Since this is a pre-1.0 breaking-change window (per #388's framing — +"while breaking pre-1.0"), this is the point to make these renames, not +after 1.0 when they become a deprecation cycle. + +## Decision + +1. **`prec` → `precision`.** Rename across `_Numeric`/`Float`/wherever + `prec` appears (transports, docs, snippets). No behaviour change. +2. **Limits alignment.** Align `min`/`max`/`min_alarm`/`max_alarm` naming + with event-model `Limits` naming. This ADR records the *intent* + (converge with bluesky event-model naming so alarm/control/display limits + read the same way in FastCS and ophyd-async docs); the exact target + shape (keep four flat fields renamed, or restructure into a `Limits`-like + object) is an open question for the prototype, since it interacts with + how `DataType.validate` currently accesses these fields directly as + dataclass attributes. +3. **`Array1D`/`Table` hint spellings.** Adopt `Array1D[np.int32]` and + `Table` as the FastCS *hint* spellings a `ControllerFiller`-scanned class + body uses, mapping internally to the existing `Waveform`/table `DataType` + runtime objects (constructed the same way as today via + `AttrRW(Waveform(np.int32, shape=(4,)), io=...)` in procedural code) — + the hint is sugar for `ControllerFiller`'s type-hint scan, not a + replacement for the runtime `DataType` classes, matching decision 7 of + #388 (`DataType` classes stay as the procedural/runtime value; hints are + what `ControllerFiller` reads). + +This is explicitly the smallest naming-pass scope agreed in #388 for 1.0. A +`Prec`/`Units`/`Shape` `Annotated` extras vocabulary (letting a hint carry +precision/units/shape without a full `DataType` instance) is called out in +#388 as a **post-1.0** option enabled by, but not required by, the +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md) extras +mechanism — not part of this ADR. + +## Consequences + +- Every driver using `Float(prec=...)`, `.min`/`.max`/`.min_alarm`/ + `.max_alarm` needs a mechanical rename. This is a wide, shallow diff + across all downstream repos (`fastcs-eiger`, `fastcs-catio`, + `fastcs-secop`, `fastcs-PandABlocks` all use `Float`/numeric limits + somewhere) but not a structural one, unless the Limits restructuring + (open question 2) turns out to be more than a rename. +- Transports serving `precision`/limits metadata (EPICS record fields, + Tango attribute properties, REST/GraphQL schema) need their field-name + mapping updated to read from the renamed dataclass fields. +- `Array1D`/`Table` hint spellings only affect declarative (hinted) + attribute declarations; procedural construction with `Waveform(...)`/ + `Table(...)` DataType instances is unchanged. + +## Open questions + +1. Does the Limits alignment keep four flat fields (just renamed to match + event-model terms) or restructure into an actual `Limits`-like nested + object? The latter is a bigger, more disruptive change to + `DataType.validate` and every downstream driver constructing `Float(...)` + with keyword limits. +2. Which event-model `Limits` categories does FastCS need — + control/display/alarm/warning all four, or a subset? EPICS records only + naturally distinguish alarm vs. display/control limits; does the mapping + from four FastCS fields to N event-model categories lose or need to + invent information for some transports? +3. Is `precision` an `int` (decimal places, as `prec` is today) or does + aligning with event-model conventions change its meaning/type too? +4. For `Array1D`/`Table` hints: is `Array1D[np.int32]` a real usable type at + both class-definition time (for `ControllerFiller` to scan) and at + type-checking time (for pyright), or a `TypeAlias`/`Annotated` wrapper + around `Waveform`? What does the two-way mapping (hint → `ControllerFiller` + constructs a `Waveform`; introspection-provisioned `Waveform` → does the + hint still validate it, per decision in + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) + open question 1) look like precisely? +5. Should this rename land in the same PR as + [ADR 14](0014-attribute-io-rw-rework.md) (since both touch `DataType`- + adjacent code and every downstream driver already has to touch these + files), or stay a separate, later PR per §8 work-plan ordering (item 5, + after items 1-4)? From cd10e12d815bfdcd1c71d43ea0c77877350f0e9f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:22:52 +0000 Subject: [PATCH 06/16] docs: ADR 18 - @attr_r/@attr_rw decorator sugar --- .../decisions/0018-attr-decorator-sugar.md | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 docs/explanations/decisions/0018-attr-decorator-sugar.md diff --git a/docs/explanations/decisions/0018-attr-decorator-sugar.md b/docs/explanations/decisions/0018-attr-decorator-sugar.md new file mode 100644 index 000000000..0a79f8f25 --- /dev/null +++ b/docs/explanations/decisions/0018-attr-decorator-sugar.md @@ -0,0 +1,131 @@ +# 18. Attr-from-Method Decorator Sugar (@attr_r / @attr_rw) + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +#388 §7.5 makes the case that FastCS must also sell as a way to write Tango +Device Servers, competing directly with PyTango on the trivial case, not +just on the advanced multi-transport pitch. PyTango's hello-world is one +decorated getter: + +```python +@attribute +def current(self) -> float: + return 2.5 +``` + +Under the [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) +harsh declarative/procedural split (bare hints only in the class body; all +IO wiring procedural), the equivalent trivial case regresses to a method +plus a `CallbackReadIO` adapter plus explicit `__init__` wiring — strictly +more ceremony than PyTango for the simple case that most new users hit +first. This is exactly the kind of "false friend" gap #388 warns about: a +PyTango user evaluating FastCS should not find the *simple* case harder than +what they're moving away from. + +FastCS already has precedent for binding class-body decorated methods to +per-instance callables without any deepcopy hazard: `@command`/`@scan` +(`src/fastcs/methods/command.py`, `scan.py`) use `UnboundCommand`/ +`UnboundScan`, which wrap an unbound function and `.bind(controller)` a +fresh `Command`/`Scan` object per instance at `_bind_attrs` time. Because +these are fresh objects constructed per-instance (not deepcopied +prototypes), they carry none of the aliasing hazard that class-scope +`Attribute` *instances* had — which is why [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) +removes the latter but keeps `@command`/`@scan`. + +## Decision + +Add `@attr_r`/`@attr_rw` (and, for symmetry, whatever `@attr_w`-only case +makes sense) as pure sugar over `AttrR`/`AttrW`/`AttrRW` plus a generated +callback-based `io=`, built on the same `Unbound*`-style bind machinery as +`@command`/`@scan` — fresh objects per instance, no prototype/deepcopy +hazard, consistent with keeping this a class-body citizen under +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md). + +```python +class PowerSupply(Controller): + @attr_rw(units="V", update_period=0.5) # dtype inferred from -> float + async def voltage(self) -> float: + return await self._conn.query("V?") + + @voltage.send + async def voltage(self, value: float) -> None: + await self._conn.send(f"V={value}") +``` + +- The datatype is inferred from the return type annotation of the getter + (`-> float` → `Float()`), matching how `DataType` mapping already works + elsewhere (`numpy_to_fastcs_datatype`), rather than requiring a `dtype=` + keyword the way PyTango does — decision 14 of #388 explicitly calls this + out as *better* than PyTango's `dtype=` kwarg since it's one real + annotation, checked statically. +- `@attr_r`/`@attr_rw` decorator keyword arguments (`units`, `update_period`, + etc.) map onto the equivalent `DataType`/`ReadIO`/`WriteIO` constructor + arguments from [ADR 14](0014-attribute-io-rw-rework.md) — this is sugar + over that mechanism, not a parallel one. +- `@attr_rw`'s `.send` decorator mirrors the `@voltage.send` pattern shown + above (property-style, matching `@property`/`@x.setter`), giving the + read+write pair a single logical name (`voltage`) with two decorated + methods. +- This degrades gracefully into the full `io=` object form for protocol + families with more complex needs, and into + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s + filler for introspection-driven attributes — `@attr_rw` is explicitly the + *simple* case, not a replacement for either. +- Refines the class-body rule stated in + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) to: + *class body = declarations + decorated behaviour; instance scope = + construction with data* — already true today via `@command`/`@scan`, now + extended to attributes. +- Docs gain a "FastCS for PyTango users" page pairing this decorator with + the equivalent PyTango snippet, landing alongside this PR per #388 §8 + item 5b. + +## Consequences + +- New driver code for the common "one attribute, one device call" case gets + noticeably shorter — closing the gap #388 §7.5 identifies against PyTango. +- The generated `io=` object needs a name/shape (an internal + `CallbackReadIO`/`CallbackWriteIO`-alike, per + [ADR 14](0014-attribute-io-rw-rework.md)'s open question 5) — this ADR's + sugar and that ADR's escape hatch should likely share the same underlying + callback-IO implementation rather than duplicating it. +- Adds a third way to declare an attribute (bare hint + filler; explicit + `AttrRW(..., io=...)`; `@attr_rw` sugar) — the docs need to be clear about + when to reach for which, so this doesn't become three equally-weighted + options with no guidance, undermining the "harsh split" clarity + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) is + trying to establish. +- Per #388 §8 item 5b, this sits on PR 1 (the `AttributeIO` rework) — + small and independent of the `ControllerFiller` work — so it can land + early and not block on [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). + +## Open questions + +1. Exact decorator names — `@attr_r`/`@attr_rw` as #388 proposes, or + something more explicit (`@readable_attribute`?) — and whether a + write-only `@attr_w` variant is worth adding for symmetry given `AttrW` + without a paired getter is a rarer shape in practice. +2. How are `min`/`max`/`precision` (post [ADR 17](0017-naming-pass.md)) + and other `DataType`-level metadata passed through the decorator's + keyword arguments — do they get their own decorator kwargs, or does the + decorator only take IO-shaped kwargs (`update_period`) and require + dropping to explicit `AttrRW(...)` construction for richer datatype + metadata? +3. Does `@attr_rw` support the `Array1D`/`Table` hint spellings from + [ADR 17](0017-naming-pass.md), or is decorator sugar scoped to scalar + datatypes only for 1.0? +4. Should `ControllerFiller` treat `@attr_rw`-decorated methods specially + (they don't need filling — they're already fully constructed at bind + time), or are they simply invisible to the filler the same way + `@command`/`@scan` are today? +5. Does the getter's docstring become the attribute's `description`, + mirroring how `Method._docstring` already captures `getdoc(fn)` for + `@command`/`@scan`? From 5e8705139bd1683caac092630b86f9680d0f9099 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:23:42 +0000 Subject: [PATCH 07/16] docs: ADR 19 - embedded ophyd-async connector --- .../0019-embedded-ophyd-async-connector.md | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 docs/explanations/decisions/0019-embedded-ophyd-async-connector.md diff --git a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md new file mode 100644 index 000000000..9614421df --- /dev/null +++ b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md @@ -0,0 +1,168 @@ +# 19. Embedded ophyd-async Connector + +Date: 2026-07-20 + +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) + +## Status + +Proposed + +## Context + +ophyd-async already bridges to FastCS over the network: +`ophyd_async.fastcs.core.fastcs_connector(uri)` is a `PviDeviceConnector` +talking PVA+PVI. #388 proposes an **in-process** embedding as well — running +a FastCS `Controller` directly inside a bluesky/ophyd-async process, with no +network hop, for cases like running a `TemperatureController` straight from +a bluesky plan. + +Researching ophyd-async's `DeviceFiller` +(`ophyd_async/core/_device_filler.py`) as the direct structural reference +for [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s +`ControllerFiller` surfaced the exact shape this connector needs to take: + +- `DeviceConnector.create_children_from_annotations` builds a `DeviceFiller` + once (memoised via `hasattr(self, "filler")`), then either fills + immediately or defers to `connect_real`. +- `connect_real` is where PVI and Tango connectors both actually introspect + and fill children — there is no ophyd-async precedent for "fill + everything at construction time" in a connect-time-introspecting + connector; embedding should follow the same connect-time pattern rather + than trying to fill eagerly. +- `SignalBackend`'s methods (`get_value`, `get_setpoint`, `set_callback`, + `put`, `get_datakey`) are the exact surface a `FastCSSignalBackend` needs + to implement in terms of FastCS's `AttrR.get`/`AttrW.put`/setpoint cache/ + native timestamps (from [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md)). +- `CommandBackend.execute`/`.signature` is the equivalent surface for typed + commands (from [ADR 15](0015-typed-commands.md)). + +This connector is explicitly the motivating consumer for +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md), +[ADR 15](0015-typed-commands.md), and +[ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) — it is +what forces those three to define a genuinely stable, documented surface +rather than an implicit one, since it lives in a different package +(ophyd-async) and cannot reach into FastCS internals the way FastCS's own +transports currently can. + +## Decision + +Per decision 6 of #388: no shared package. `FastCSDeviceConnector` lives +entirely on the ophyd-async side, behind an `ophyd-async[fastcs-embed]` +extra, importing only the stable FastCS surface formalised by +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md) +(`ControllerFiller`) and [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) +(`ControllerAPI` tree, `ControllerRunner`, and the attribute/command runtime +methods). Convergence is by convention (the two projects agreeing on shape), +not by shared code. + +```python +from ophyd_async.fastcs import embedded_fastcs_connector + +class TempStage(Device): + ramp_rate: SignalRW[float] + power: SignalR[float] + cancel_all: TriggerableCommand + ramps: DeviceVector[TempRamp] + +stage = TempStage(connector=embedded_fastcs_connector(TemperatureController(settings))) +await stage.connect() # runs controller lifecycle in-process +``` + +Mechanics, directly mirroring `PviDeviceConnector`/`TangoDeviceConnector`: + +- `create_children_from_annotations`: builds a `DeviceFiller` with + `FastCSSignalBackend`/`FastCSCommandBackend` factories, `filled=False` — + same lazy pattern as the network connectors. +- `connect_real` (top level): starts the `ControllerRunner` — `initialise()`, + `post_initialise()`, `create_api_and_tasks()`, `Controller.connect()`, + initial coroutines, scan tasks scheduled on the *running* (bluesky) event + loop — then walks the `ControllerAPI` tree filling children via the + `DeviceFiller`, `check_filled()`, `set_name()`. Idempotent across + reconnects, per [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md)'s + `ControllerRunner` requirement. +- `connect_mock` never touches the controller, so mock-mode ophyd-async + usage stays free (no FastCS controller/connection is instantiated at all). +- Lifecycle (decision 8 of #388): the connector owns the runner; shutdown + via an `atexit` hook plus an explicit `await connector.shutdown()`, which + cancels scan tasks and calls `Controller.disconnect()`. Upstream + `Device.disconnect()` is a follow-up (item 8 in #388 §8), not blocking. +- Embedded + transports simultaneously (decision 9 of #388, e.g. a CA GUI + running next to a bluesky plan) is explicitly out of scope for the first + cut, but the `ControllerRunner` is designed so a transport list can be + attached later without redesigning it. + +Backend mappings (from #388 §5, grounded against the researched +`DeviceFiller`/`SignalBackend` surface): + +| ophyd-async | FastCS | +|---|---| +| `SignalBackend.get_value` | `AttrR.get()` | +| `SignalBackend.set_callback` | `AttrR.add_on_update_callback(cb, always=True)`; stamped per [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | +| `SignalBackend.put` | `AttrW.put(value)` | +| `SignalBackend.get_setpoint` | `AttrW` cached setpoint, [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | +| `SignalBackend.get_datakey` | `Attribute.datatype` → `SignalMetadata` (units, precision, limits, choices) + `make_datakey` | +| `CommandBackend.execute`/`.signature` | `Command.__call__` / captured `Signature`, [ADR 15](0015-typed-commands.md) | +| `SignalBackend.source` | e.g. `fastcs://.` | +| child `Device` / `DeviceVector` | sub-`Controller` / `ControllerVector` | +| (not exposed) | `@scan` methods — server-side only, not surfaced to ophyd-async | + +Datatype mapping: `Int`/`Float`/`Bool`/`String` → `int`/`float`/`bool`/`str`; +`Waveform(array_dtype, shape)`/`Array1D` hint (per +[ADR 17](0017-naming-pass.md)) → ophyd-async `Array1D[dtype]`; `Enum(cls)` → +the enum class itself. Two mismatches flagged as **prototype risk** in #388 +and carried into this ADR unresolved: + +- ophyd-async constrains enums to `EnumTypes` (`StrictEnum`/`SubsetEnum`/ + `SupersetEnum`); FastCS accepts any `enum.Enum`. +- fastcs `Table` vs. ophyd-async `Table` (pydantic-based) — mapping is + best-effort, mismatches should be flagged early rather than silently + coerced. + +## Consequences + +- The stable FastCS interface promised in + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)/ + [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) gets + its first external, cross-repo consumer — any accidental internal + dependency this connector picks up is a signal that surface isn't + actually stable yet. +- This is entirely an ophyd-async-side deliverable (§8 items 6-8); + fastcs-core work is a dependency, not part of this repo's PRs. +- Per #388 coordination note: items 1-4 of the §8 work plan land in FastCS + before item 6 is mergeable; prototyping item 6 against a FastCS branch to + validate the stable interface *before* freezing it is the recommended + order, i.e. this connector should be prototyped against the `refactor` + branch here as the other ADRs' implementations land, not written blind + against a spec. +- `fastcs-demo`'s temperature controller simulation + (`fastcs.demo.simulation`) is the existing sim device ophyd-async tests + against — no new simulated device is needed for the first cut. + +## Open questions + +1. Enum conversion: does the connector require FastCS `Enum` datatypes used + with embedding to be `StrictEnum`/`SubsetEnum`/`SupersetEnum` subclasses + (pushing a constraint back onto FastCS driver authors who want embedding + support), or does it do runtime conversion/wrapping, and what happens to + values that don't fit ophyd-async's stricter model? +2. `Table` mapping: is a real bidirectional pydantic-model ↔ fastcs-`Table` + converter in scope for the first cut, or is `Table` explicitly + unsupported/best-effort-only initially, with a hard error on mismatch + rather than silent coercion? +3. Where does `@scan`-derived state that isn't exposed as a `Signal` go — + is it simply invisible to ophyd-async (server-side only, as the mapping + table states), or does some `@scan` output need a path to surface as a + `SignalR` (e.g. `fastcs-eiger`'s `update_voltages` `@scan` feeding + per-ramp `AttrR`s — those `AttrR`s are visible, but would a *pure* + `@scan`-only value ever need exposing)? +4. How does `embedded_fastcs_connector` handle a `Controller` that raises + during `initialise()` (e.g. a device that's unreachable at embed time) + — does `connect_real` propagate the exception directly to + `Device.connect()`, retry, or something else? +5. Should the embedded connector's shutdown (`atexit` + explicit + `await connector.shutdown()`) also be triggered by ophyd-async's own + `Device.disconnect()` once that upstream work lands (#388 §8 item 8), and + does that imply `ControllerRunner.stop()` needs to be safely callable + from a synchronous `atexit` context as well as an async one? From 56b03fa0ca0ce9222ca95c3134b1e5c6a6ef1127 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:26:29 +0000 Subject: [PATCH 08/16] examples: scaffold living-review-artifact package for #388 --- examples/README.md | 65 ++++++++++++++++++++++++++++++++++++++++++++ examples/__init__.py | 4 +++ 2 files changed, 69 insertions(+) create mode 100644 examples/README.md create mode 100644 examples/__init__.py diff --git a/examples/README.md b/examples/README.md new file mode 100644 index 000000000..d769ffeef --- /dev/null +++ b/examples/README.md @@ -0,0 +1,65 @@ +# examples/ + +This package is the **living review artifact** for the FastCS / ophyd-async +API-convergence refactor tracked in +[issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) and its +[ADRs](../docs/explanations/decisions/) (0013-0019). + +It exists so that every framework PR in the refactor has something concrete +to run against, alongside the unit test suite: three example controllers, +each written in one of the three styles the refactor is converging FastCS +towards. As each framework PR lands (`AttributeIO` rework, `ControllerFiller`, +typed commands, the setpoint cache/timestamps/`ControllerRunner`, the naming +pass, `@attr_rw` sugar), the example(s) it affects are updated in the *same* +PR, so `examples/` always reflects current `main` and stays green under +`uv run --locked tox`. It is not a tutorial or documentation snippet source — +those live under `docs/`. + +## The three styles + +1. **IORef temperature controller** (`examples/example_1_ioref/`, tracked by + the `DRAFT: Example 1` sub-issue of #388). A deliberately-messy adaptation + of `src/fastcs/demo/controllers.py`, written against **whatever the + current API is** at the time it's updated. It starts on the *pre-refactor* + `AttributeIORef`/class-scope-instance API and is gradually cleaned up as + each framework PR lands — e.g. once + [ADR 14](../docs/explanations/decisions/0014-attribute-io-rw-rework.md)'s + `AttributeIO` rework merges, this example's `io_ref=`/`ios=[...]` wiring + is updated to `io=` in that same PR. This is intentional: it is the + baseline that proves each framework PR doesn't break a real (if simple) + driver, and it visibly tracks the migration path a downstream repo like + `fastcs-eiger` or `fastcs-catio` would need to follow. + +2. **Introspectable Eiger-style controller** (`examples/example_2_introspectable/`, + tracked by the `DRAFT: Example 2` sub-issue of #388). Mirrors a REST API + the way `fastcs-eiger`, `fastcs-secop`, and `fastcs-PandABlocks` do today: + attributes are built from data queried at `initialise()` time, not + declared as class-scope instances. This is the example that exercises + [ADR 13](../docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md)'s + `ControllerFiller` and bare-hint declarations once that PR lands. + +3. **PyTango-style `@attr_rw` device** (`examples/example_3_decorator/`, + tracked by the `DRAFT: Example 3` sub-issue of #388, blocked on the + `@attr_rw` decorator-sugar issue). A trivial getter/setter device in the + style of + [ADR 18](../docs/explanations/decisions/0018-attr-decorator-sugar.md), + demonstrating the simple case FastCS needs to match PyTango on. + +## Status + +**Scaffold only.** This PR (the ADR seed) adds this package, its structure, +and this README — it does not implement the framework changes or the three +example controllers themselves. Each example is implemented by its own +tracked sub-issue of #388, against the framework API as it exists once that +issue's dependencies have landed. See the `Blocked by:` lines on each +`DRAFT:` sub-issue for the landing order. + +## Keeping it green + +Every framework PR that touches `src/fastcs/` and affects one of these three +styles must update the corresponding example(s) in the same PR, and +`uv run --locked tox` must pass. This is the acceptance criterion listed on +every sub-issue of #388 for exactly this reason: it keeps the examples +honest as a description of "how do I actually write a FastCS driver today," +rather than letting them drift out of sync with the framework the way +standalone documentation snippets can. diff --git a/examples/__init__.py b/examples/__init__.py new file mode 100644 index 000000000..7cb4edb3d --- /dev/null +++ b/examples/__init__.py @@ -0,0 +1,4 @@ +"""Living review artifacts for the #388 API-convergence refactor. + +See ``examples/README.md`` for what this package is and how it's used. +""" From ce9ccc05367b3df08d45f241e646a51c8b4e7ae6 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 21 Jul 2026 10:09:34 +0000 Subject: [PATCH 09/16] docs: fold #402 review resolutions into ADRs 0013-0019 Incorporate coretl's inline review decisions: drop DataType (python types + *Meta), unify callback IO into the @attr factory, the __init__ filler rule + no-hints/Optional/external-add, nested Limits with inheritance, get_setpoint + ControllerRunner(start/stop) owning reconnect, severity enum, args/returns typed separately (kwargs -> spike #403), enum/Table handling, and dropping the Device.disconnect proposal for connect(force_reconnect=True) + atexit. Remaining deferred items (@shihab-dls, @Tom-Willemsen) kept as explicit open questions. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- ...-procedural-split-and-controller-filler.md | 92 +++++++++---------- .../decisions/0014-attribute-io-rw-rework.md | 65 +++++++------ .../decisions/0015-typed-commands.md | 62 ++++++++----- ...-cache-timestamps-and-controller-runner.md | 54 +++++------ .../decisions/0017-naming-pass.md | 47 ++++------ .../decisions/0018-attr-decorator-sugar.md | 64 ++++++------- .../0019-embedded-ophyd-async-connector.md | 67 +++++++------- 7 files changed, 226 insertions(+), 225 deletions(-) diff --git a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md index 9e2578865..847de3a58 100644 --- a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md +++ b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md @@ -83,35 +83,44 @@ bare type hints only; instance scope = procedural construction.** Concretely: citizens, since none of them require per-instance deepcopy — they bind a method to `self` at construction time instead. -Example, before and after: +Two patterns follow, and the class body distinguishes them: -```python -# Before: class-scope instance, deepcopy'd per-instance -class TemperatureRampController(Controller): - start = AttrRW(Int(), io_ref=TemperatureControllerAttributeIORef(name="S")) +**Procedural, no hint** — the value is fully constructed in `__init__`, so it +needs no class-body declaration at all (the temperature controller): -# After: bare hint, filled procedurally +```python class TemperatureRampController(Controller): - start: AttrRW[int] - def __init__(self, index: int, conn: IPConnection) -> None: super().__init__() suffix = f"{index:02d}" self.start = AttrRW(Int(), io=TempIO(conn, "S", suffix)) ``` -Introspecting controllers (`fastcs-eiger`, `fastcs-secop`, -`fastcs-PandABlocks`, `fastcs-catio`'s dynamic path) keep working exactly as -today's `initialise()` + `add_attribute` pattern, but the fully-dynamic case -where the *set* of attributes is not known until a network round-trip -completes (`fastcs-PandABlocks`, `fastcs-secop`) needs `ControllerFiller` to -support filling children that were never hinted at all — mirroring -`DeviceFiller.fill_child_signal`'s "no annotation existed, introspection -added an undeclared attribute" path. This is a harder requirement than most -of ophyd-async's own connectors exercise (PVI and Tango both fill *some* -undeclared children, but FastCS's dynamic drivers may have **zero** static -hints and still need to build a full attribute tree from nothing) and is -called out below as an open question. +**Declarative hint + filler** — the value is *promised* by a hint and +provisioned by introspection at connect time (the Eiger-like case): + +```python +class OdinDetector(Controller): + frames: AttrRW[int] # must exist by the end of initialise() + + async def initialise(self) -> None: + for name, meta in await self._query_parameter_tree(): + self.filler.fill_attribute(name, ...) +``` + +**The rule** (identical to ophyd-async's rule for `Signal`s): *at the end of +`__init__`, any Attribute referenced in code — and therefore carrying a type +hint — must exist.* Only `__init__` is serial; `initialise()` may then run in +parallel across controllers. + +Introspecting controllers keep working as today's `initialise()` + +`add_attribute` pattern. The fully-dynamic case — where the *set* of +attributes is not known until a network round-trip completes +(`fastcs-PandABlocks`, `fastcs-secop`) — needs `ControllerFiller` to fill +children that were never hinted at all. This is **no harder than ophyd-async +already supports**: `Device(connector=PviConnector(prefix))` fills a whole +`Signal` tree from introspection with no hints required, and `ControllerFiller` +mirrors that `DeviceFiller` path directly. ## Consequences @@ -130,29 +139,20 @@ called out below as an open question. it interacts with the stable `ControllerAPI` surface consumed by the embedded ophyd-async connector. -## Open questions - -1. Does `ControllerFiller` need to support "no hints exist at all — build the - entire attribute tree from introspected data" (the `fastcs-PandABlocks` - and `fastcs-secop` case), or is some minimal static shape (even just a - marker on the `Controller` subclass) always required? `DeviceFiller` has - no precedent for the fully-hint-free case. -2. `fastcs-catio`'s dynamic path builds whole controller *classes* at runtime - via `type(...)` from YAML definitions, before any instance (and hence any - `ControllerFiller`) exists. Is that pattern still supported, unsupported, - or does it need to move to instance-level dynamic attribute construction - under the new model? -3. `fastcs-eiger`'s `OdinController.initialise()` constructs new attributes - that reference sibling sub-controllers' attributes, assuming those - sub-controllers already exist. Does `ControllerFiller` impose an - ordering/dependency mechanism between sibling children, or is this left - as an `initialise()` implementation detail (call `super().initialise()` - first)? -4. Should `check_filled` be able to distinguish "this hinted child is - optional" (ophyd-async's `Optional[X]` convention), or does FastCS treat - every hint as required for 1.0? -5. Exact `ControllerFiller` method names/signatures are left to the - prototype — should they mirror `DeviceFiller`'s names 1:1 - (`fill_child_signal` → `fill_child_attribute`?) for discoverability by - developers who know both libraries, or diverge where FastCS's vocabulary - (`Attribute` vs `Signal`) differs? +## Resolved in review (#402) + +1. **`ControllerFiller` must support "no hints at all"** — build the whole + attribute tree from introspected data (`fastcs-PandABlocks`, `fastcs-secop`). +2. **`fastcs-catio`'s runtime `type(...)` class-building is *not* supported.** + A bare `Controller` instead allows attributes to be added onto it from the + outside — which is exactly what the fillers do — so catio moves to + instance-level dynamic attribute construction. +3. **No sibling-ordering mechanism.** The rule "any hint-referenced Attribute + must exist by the end of `__init__`" makes `initialise()` parallelisable; + sibling dependencies are an `initialise()` implementation detail (call + `super().initialise()` first). +4. **`Optional[X]` hints are supported** — `check_filled` treats an optional + hint as not-required. +5. **Follow `DeviceFiller`'s structure, not its names.** Architectural + similarity matters; method names match only where FastCS's vocabulary + (`Attribute` vs `Signal`) makes them fit. diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 7d731a366..fc9e2ed0f 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -73,7 +73,9 @@ class ReadWriteIO(ReadIO[DType_T], WriteIO[DType_T], ABC): ... (Working names per #388; exact naming — `ReadIO`/`WriteIO`/`ReadWriteIO` vs. `AttrRIO`/`AttrWIO`/`AttrRWIO` — is an open question below and in -[ADR 17](0017-naming-pass.md).) +[ADR 17](0017-naming-pass.md).) Concrete IO classes are **dataclasses**, so +per-attribute fields (register name, command string, `update_period`) are +declared without a boilerplate `__init__`. - `AttrR(dt, io: ReadIO[DType_T] | None)`, `AttrW(dt, io: WriteIO[DType_T] | None)`, `AttrRW(dt, io: ReadWriteIO[DType_T] | None)`. Passing a @@ -97,10 +99,16 @@ class ReadWriteIO(ReadIO[DType_T], WriteIO[DType_T], ABC): ... setpoint→readback via `_internal_update`, and the sync-setpoint machinery is unaffected. This remains the analogue of ophyd-async's `soft_signal_rw`. -- A concrete `CallbackReadIO`/`CallbackWriteIO` pair ships in core as an - escape hatch for one-off attributes, mirroring `soft_command` — e.g. - `CallbackReadIO(update=cb, update_period=0.2)` — without requiring a full - subclass. +- The one-off "no subclass needed" case is served by the unified `attr` + factory (see [ADR 18](0018-attr-decorator-sugar.md)) — `self.x = + attr(getter=cb)` / `@attr` — **not** by separate `CallbackReadIO`/ + `CallbackWriteIO` classes. The same `attr` covers the read-only and + read/write callback cases, so the two adapters are not shipped. + +> Datatype spelling: the `Float()` etc. in the examples below predate +> [ADR 17](0017-naming-pass.md); read them as the ADR-17 python-type + `*Meta` +> form (`AttrRW(float, precision=3, io=...)`), since the `DataType` family is +> removed. Migration is mechanical for the common case (an old `AttributeIO` subclass absorbs its `AttributeIORef`'s fields into its own `__init__` and is @@ -159,28 +167,25 @@ a public method on `AttrW`/`ReadWriteIO` — exact shape is an open question. get a shorter driver-local name) still applies to the new `ReadIO`/ `WriteIO`/`ReadWriteIO` names. -## Open questions - -1. Final class names: `ReadIO`/`WriteIO`/`ReadWriteIO` vs. `AttrRIO`/ - `AttrWIO`/`AttrRWIO` (mirroring the `Attr` family) vs. something else - entirely — see [ADR 17](0017-naming-pass.md). -2. What is the public replacement for `fastcs-secop`'s - `_call_sync_setpoint_callbacks` workaround? Does `WriteIO.send` get an - optional `sync_setpoint` callback argument, or does `AttrW.put` grow a - public method IO authors can call from `send`? -3. Should there be a runtime check (e.g. at `post_initialise`) that - catches "read-only IO passed to a write-capable `Attr`" for cases the - static type checker cannot see (e.g. an `Any`-typed IO built - dynamically, as in `fastcs-secop`'s and `fastcs-PandABlocks`'s - introspection-driven construction)? Both of those drivers build - attributes and their IO from runtime data where static checking cannot - help. -4. `fastcs-PandABlocks`'s `UnitsIO.send` mutates a sibling attribute's - datatype and `fastcs-catio` recovers per-attribute metadata via - `attribute.io_ref` from *outside* the attribute's own `send`/`update` - (`panda_controller.py:_coerce_value_to_panda_type`). With `io_ref` - removed, what is the sanctioned way to recover an attribute's IO-specific - metadata (e.g. `attr.io` becoming a public, typed property)? -5. Do we ship `CallbackReadIO`/`CallbackWriteIO` in `fastcs` core for 1.0, or - leave the "no subclass needed" one-off case entirely to driver authors - using `io=None` plus manual `set_update_callback`? +## Resolved in review (#402) + +- **Runtime check: yes.** Alongside the static type error, a runtime check + (e.g. at `post_initialise`) catches a read-only IO on a write-capable `Attr` + for the dynamically-built `Any`-typed case (`fastcs-secop`, + `fastcs-PandABlocks`). +- **`attr.io` becomes a public, typed property** — the sanctioned way to + recover an attribute's IO-specific metadata from *outside* its `send`/ + `update` (replaces `fastcs-catio`'s `attribute.io_ref` access). +- **No `CallbackReadIO`/`CallbackWriteIO` in core.** The one-off callback case + folds into the unified `attr` factory ([ADR 18](0018-attr-decorator-sugar.md)); + the same decorator/factory covers the read-only and read/write cases. + +## Open questions (awaiting input) + +1. Final class names: `ReadIO`/`WriteIO`/`ReadWriteIO` vs. `AttrRIO`/`AttrWIO`/ + `AttrRWIO` vs. something else — see [ADR 17](0017-naming-pass.md). + *(awaiting @shihab-dls)* +2. Public replacement for `fastcs-secop`'s `_call_sync_setpoint_callbacks`: + an optional `sync_setpoint` argument on `WriteIO.send`, or a public method + on `AttrW` an IO author can call from `send`? *(awaiting @shihab-dls / + @Tom-Willemsen)* diff --git a/docs/explanations/decisions/0015-typed-commands.md b/docs/explanations/decisions/0015-typed-commands.md index a3d6c9b37..9df453dec 100644 --- a/docs/explanations/decisions/0015-typed-commands.md +++ b/docs/explanations/decisions/0015-typed-commands.md @@ -60,6 +60,19 @@ class Ramp(Controller): stop: Command[[], None] # void/void: served everywhere ``` +**Argument and return typing are independent** (not all-or-nothing): + +- *Args*: `[]` (none) · `[DT1, DT2, …]` (positional, known types — validated) · + `Any` (must be a command, signature introspected at runtime). +- *Returns*: `None` · `DT` (a single typed value) · `Any` (introspected). + +Following the [ADR 17](0017-naming-pass.md) `DataType` drop, command +arguments/returns use plain python types + `*Meta` exactly as attributes do +(no metadata ⇒ "use the python type"), and the serialisation machinery is +**shared** with `Attribute`, not duplicated. **Keyword-argument** commands +need a `TYPE_CHECKING` stub trick and are prototyped separately in the spike +[#403](https://github.com/DiamondLightSource/fastcs/issues/403), not here. + ## Consequences - `Command.__call__` gains real `*args`/`**kwargs` forwarding instead of a @@ -76,27 +89,28 @@ class Ramp(Controller): signatures is not achievable for introspection-driven drivers, only for statically-declared ones. This mirrors the same "hint vs. no-hint" tension as [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). -- Command args/return values need datatype validation analogous to - `Attribute`'s `DataType.validate` — whether they reuse the `DataType` - family directly or a separate mechanism is an open question. - -## Open questions - -1. Do command arguments/return values validate through the same `DataType` - family attributes use, or is a separate (lighter-weight, since there's no - "current value" to cache) validation path introduced? -2. For `fastcs-secop`-style dynamically-typed commands, what's the - recommended pattern — `Command[Any, Any]` with manual validation inside - the handler, or a documented way to construct a `Command[P, T]` with `P`/ - `T` determined at runtime (which conflicts with normal generic typing)? -3. Exactly what should the EPICS skip-with-warning message say, and where — - at controller construction, at `post_initialise`, or lazily the first - time a typed command is looked up by the transport? -4. Should typed commands support partial typing (e.g. typed arguments but - void return, or vice versa), or is it all-or-nothing relative to - `Command[[], None]`? -5. Does the REST/GraphQL/Tango serialisation of complex argument/return - types (numpy arrays, `Enum`, `Table`) reuse existing `DataType` - serialisation code from attributes, and if so does that argue for - sharing more machinery between `Attribute` and `Command` than they do - today? +- Command args/return values validate through the **same** python-type + + `*Meta` mechanism as attributes (the `DataType` family is removed, ADR 17) — + one shared validation/serialisation path, no command-specific duplicate. + +## Resolved in review (#402) + +- **Validation shares the attribute path.** With `DataType` dropped (ADR 17), + command args/returns use python types + `*Meta` like attributes; no separate + mechanism, and complex-type serialisation (arrays, `Enum`, `Table`) is shared + with `Attribute`. +- **Args and returns are typed independently** — Args `[]` / `[DT…]` / `Any`; + Returns `None` / `DT` / `Any` (see Decision). Not all-or-nothing. +- **EPICS skip-with-warning fires at IOC startup** — post controller + construction, when the fully populated controllers are handed to the + transports to serve. +- **Keyword-arg commands → spike [#403](https://github.com/DiamondLightSource/fastcs/issues/403)** + (interactive/Opus; needs a `TYPE_CHECKING` stub). Out of scope for core + typed-command work. + +## Open questions (awaiting input) + +1. Are there real `fastcs-secop` devices where you know something is a + `Command` but not its signature until runtime? Determines whether + `Command[Any, Any]` + runtime introspection suffices, or the kw-arg trick + (#403) is truly needed. *(awaiting @Tom-Willemsen)* diff --git a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md index 3c20a85bd..5148edf4c 100644 --- a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md +++ b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md @@ -41,16 +41,18 @@ to using — no reaching into `BaseController` internals. ## Decision **Setpoint cache:** `AttrW` gains an internally-tracked last-applied -setpoint, exposed via a public getter (name TBD — see open questions), -updated whenever `put` is called, independent of whether the underlying -`send` succeeds. This is available to all transports (not just the embedded +setpoint, exposed via a public `AttrW.get_setpoint()` method (mirroring +ophyd-async's `SignalBackend.get_setpoint()`), updated whenever `put` is +called, independent of whether the underlying `send` succeeds. This is available to all transports (not just the embedded connector) as a "what did we last ask for" query distinct from `AttrR.get()` ("what did we last read back"). **Native timestamps (+ severity):** `AttrR.update` accepts an optional timestamp (and, where meaningful, severity) alongside the value, defaulting -to current time if not supplied by the caller. This is FastCS-native, not -EPICS-specific — Tango event pushes and other IO can supply a device-side +to current time if not supplied by the caller. The timestamp/severity pair +**follows bluesky's `Reading` shape but shares no code** with it, and severity +is a **FastCS enum using the same strings as EPICS** alarm severities. This is +FastCS-native, not EPICS-specific — Tango event pushes and other IO can supply a device-side timestamp through the same path a `ReadIO.update` call already uses. The embedded connector stamps receive-time only as an interim measure until this lands, per decision 10 of #388 — this is 1.0 scope, not a follow-up. @@ -65,9 +67,15 @@ transport-serving and interactive-shell logic that stays in `FastCS`/ - Running `initialise()`/`post_initialise()`/`create_api_and_tasks()` once. - Running `connect()` and the initial coroutines. - Starting/stopping the periodic scan tasks. -- Being **idempotent** — safe to call start again after a stop, since the - embedded connector's `connect_real` may run more than once across - reconnects (see [ADR 19](0019-embedded-ophyd-async-connector.md)). +- The **whole lifecycle including reconnect** — calling `Controller.reconnect()` + on scan-task failure; reconnect is owned by the runner, not left + controller-specific. + +The runner is a class with `start()`/`stop()` (ophyd-async calls `start`/`stop`; +an `async with` context manager is added only if it also suits the `FastCS()` +case). **Idempotency is the caller's responsibility**, not the runner's — the +embedded connector's `connect_real` may run more than once across reconnects +(see [ADR 19](0019-embedded-ophyd-async-connector.md)). This, together with `ControllerAPI` and the attribute/command runtime methods (`AttrR.get`/`add_on_update_callback`, `AttrW.put` + cached @@ -89,24 +97,12 @@ the documented stable surface referenced by decision 13 of #388. narrow surface instead of `BaseController` internals — see [ADR 19](0019-embedded-ophyd-async-connector.md). -## Open questions - -1. Setpoint cache accessor name and shape — `AttrW.setpoint` property, - `AttrW.get_setpoint()` method (mirroring `SignalBackend.get_setpoint()`), - or folded into `AttrW.put`'s return value? -2. Timestamp/severity type — reuse a existing convention (e.g. - `ophyd_async`/bluesky's `Reading`/event-model shape) or define a - FastCS-native pair? Decision 12 of #388 already aligns numeric limits - naming with event-model `Limits` — should timestamps/severity follow the - same alignment for consistency? -3. Severity: what are the FastCS-native severity levels, and do they map - 1:1 to EPICS alarm severities, or is EPICS's severity model transport- - specific with FastCS defining its own smaller/different vocabulary? -4. Exact `ControllerRunner` API shape — a class with `start()`/`stop()`, or - `async` context-manager semantics (`async with runner:`)? The embedded - connector needs idempotent start across reconnects; does the chosen shape - make idempotency the caller's responsibility or the runner's? -5. Does `ControllerRunner` own reconnect logic (calling `Controller.reconnect()` - on scan-task failure, as `Controller._create_periodic_scan_coro` does - today), or does that stay controller-specific and out of the runner's - documented surface? +## Resolved in review (#402) + +- **Setpoint accessor:** a `AttrW.get_setpoint()` method (mirrors + `SignalBackend.get_setpoint()`). +- **Timestamp/severity:** follow bluesky's `Reading` shape but **share no + code**; severity is a **FastCS enum using the same strings as EPICS**. +- **`ControllerRunner`:** a class with `start()`/`stop()` (context manager only + if it also suits `FastCS()`); **idempotency is the caller's responsibility**. +- **The runner owns the whole lifecycle, including reconnect.** diff --git a/docs/explanations/decisions/0017-naming-pass.md b/docs/explanations/decisions/0017-naming-pass.md index f0b8989e4..3ac954894 100644 --- a/docs/explanations/decisions/0017-naming-pass.md +++ b/docs/explanations/decisions/0017-naming-pass.md @@ -34,6 +34,14 @@ after 1.0 when they become a deprecation cycle. ## Decision +> **Review update (#402): `DataType` is dropped** (see +> [ADR 15](0015-typed-commands.md)). The renames below now live on python +> types + `*Meta` typed dicts, not on `DataType` classes, and this pass folds +> into the `AttributeIO` rework ([ADR 14](0014-attribute-io-rw-rework.md) / +> issue #392) rather than a separate late PR. `Array1D`/`Table` become *both* +> the hint and the runtime structure passed around as the datatype — there is +> no separate `Waveform`/`DataType` object to map to. + 1. **`prec` → `precision`.** Rename across `_Numeric`/`Float`/wherever `prec` appears (transports, docs, snippets). No behaviour change. 2. **Limits alignment.** Align `min`/`max`/`min_alarm`/`max_alarm` naming @@ -76,30 +84,15 @@ mechanism — not part of this ADR. attribute declarations; procedural construction with `Waveform(...)`/ `Table(...)` DataType instances is unchanged. -## Open questions - -1. Does the Limits alignment keep four flat fields (just renamed to match - event-model terms) or restructure into an actual `Limits`-like nested - object? The latter is a bigger, more disruptive change to - `DataType.validate` and every downstream driver constructing `Float(...)` - with keyword limits. -2. Which event-model `Limits` categories does FastCS need — - control/display/alarm/warning all four, or a subset? EPICS records only - naturally distinguish alarm vs. display/control limits; does the mapping - from four FastCS fields to N event-model categories lose or need to - invent information for some transports? -3. Is `precision` an `int` (decimal places, as `prec` is today) or does - aligning with event-model conventions change its meaning/type too? -4. For `Array1D`/`Table` hints: is `Array1D[np.int32]` a real usable type at - both class-definition time (for `ControllerFiller` to scan) and at - type-checking time (for pyright), or a `TypeAlias`/`Annotated` wrapper - around `Waveform`? What does the two-way mapping (hint → `ControllerFiller` - constructs a `Waveform`; introspection-provisioned `Waveform` → does the - hint still validate it, per decision in - [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) - open question 1) look like precisely? -5. Should this rename land in the same PR as - [ADR 14](0014-attribute-io-rw-rework.md) (since both touch `DataType`- - adjacent code and every downstream driver already has to touch these - files), or stay a separate, later PR per §8 work-plan ordering (item 5, - after items 1-4)? +## Resolved in review (#402) + +1. **Limits are nested**, not four flat fields. +2. **All four categories (control/display/alarm/warning), all optional**, with + inheritance: supply none ⇒ all unbounded; Display but not Control ⇒ Control + inherits Display (for writeable); Alarm but not Warning ⇒ Warning inherits + Alarm; both ⇒ assert Warning ⊆ Alarm; otherwise unspecified ⇒ unbounded. +3. **`precision` stays an `int`** (decimal places). +4. **`Array1D` is both the hint and the runtime structure** — with `DataType` + dropped it falls out in the wash; there is no `Waveform` object to map to. +5. **Where it lands is the implementer's choice** — folds naturally into the + `AttributeIO`/DataType-drop PR (#392). diff --git a/docs/explanations/decisions/0018-attr-decorator-sugar.md b/docs/explanations/decisions/0018-attr-decorator-sugar.md index 0a79f8f25..f5ca998b0 100644 --- a/docs/explanations/decisions/0018-attr-decorator-sugar.md +++ b/docs/explanations/decisions/0018-attr-decorator-sugar.md @@ -42,8 +42,15 @@ removes the latter but keeps `@command`/`@scan`. ## Decision -Add `@attr_r`/`@attr_rw` (and, for symmetry, whatever `@attr_w`-only case -makes sense) as pure sugar over `AttrR`/`AttrW`/`AttrRW` plus a generated +> **Review update (#402): the decorator is `@attr`, mirroring `@property`.** +> `@attr` on the getter, `@voltage.setter` on the writer (not `@attr_r`/ +> `@attr_rw`/`.send`). It unifies with the callback IO from +> [ADR 14](0014-attribute-io-rw-rework.md): `self.x = attr(getter=…, setter=…)` +> is the `__init__` spelling of the same thing. `AttrW`-only (write with no +> paired getter) is rare, so it is written longhand rather than given its own +> decorator. + +Add `@attr` as pure sugar over `AttrR`/`AttrRW` plus a generated callback-based `io=`, built on the same `Unbound*`-style bind machinery as `@command`/`@scan` — fresh objects per instance, no prototype/deepcopy hazard, consistent with keeping this a class-body citizen under @@ -51,11 +58,11 @@ hazard, consistent with keeping this a class-body citizen under ```python class PowerSupply(Controller): - @attr_rw(units="V", update_period=0.5) # dtype inferred from -> float + @attr(units="V", update_period=0.5) # dtype inferred from -> float async def voltage(self) -> float: return await self._conn.query("V?") - @voltage.send + @voltage.setter async def voltage(self, value: float) -> None: await self._conn.send(f"V={value}") ``` @@ -70,10 +77,9 @@ class PowerSupply(Controller): etc.) map onto the equivalent `DataType`/`ReadIO`/`WriteIO` constructor arguments from [ADR 14](0014-attribute-io-rw-rework.md) — this is sugar over that mechanism, not a parallel one. -- `@attr_rw`'s `.send` decorator mirrors the `@voltage.send` pattern shown - above (property-style, matching `@property`/`@x.setter`), giving the +- `@attr`'s `.setter` decorator mirrors `@property`/`@x.setter`, giving the read+write pair a single logical name (`voltage`) with two decorated - methods. + methods. (`.send` is not used.) - This degrades gracefully into the full `io=` object form for protocol families with more complex needs, and into [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s @@ -92,11 +98,10 @@ class PowerSupply(Controller): - New driver code for the common "one attribute, one device call" case gets noticeably shorter — closing the gap #388 §7.5 identifies against PyTango. -- The generated `io=` object needs a name/shape (an internal - `CallbackReadIO`/`CallbackWriteIO`-alike, per - [ADR 14](0014-attribute-io-rw-rework.md)'s open question 5) — this ADR's - sugar and that ADR's escape hatch should likely share the same underlying - callback-IO implementation rather than duplicating it. +- The generated callback `io=` **is** the unified callback mechanism from + [ADR 14](0014-attribute-io-rw-rework.md) (which no longer ships separate + `CallbackReadIO`/`CallbackWriteIO`): `@attr` and `attr(getter=…)` are two + spellings over one implementation. - Adds a third way to declare an attribute (bare hint + filler; explicit `AttrRW(..., io=...)`; `@attr_rw` sugar) — the docs need to be clear about when to reach for which, so this doesn't become three equally-weighted @@ -107,25 +112,16 @@ class PowerSupply(Controller): small and independent of the `ControllerFiller` work — so it can land early and not block on [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). -## Open questions - -1. Exact decorator names — `@attr_r`/`@attr_rw` as #388 proposes, or - something more explicit (`@readable_attribute`?) — and whether a - write-only `@attr_w` variant is worth adding for symmetry given `AttrW` - without a paired getter is a rarer shape in practice. -2. How are `min`/`max`/`precision` (post [ADR 17](0017-naming-pass.md)) - and other `DataType`-level metadata passed through the decorator's - keyword arguments — do they get their own decorator kwargs, or does the - decorator only take IO-shaped kwargs (`update_period`) and require - dropping to explicit `AttrRW(...)` construction for richer datatype - metadata? -3. Does `@attr_rw` support the `Array1D`/`Table` hint spellings from - [ADR 17](0017-naming-pass.md), or is decorator sugar scoped to scalar - datatypes only for 1.0? -4. Should `ControllerFiller` treat `@attr_rw`-decorated methods specially - (they don't need filling — they're already fully constructed at bind - time), or are they simply invisible to the filler the same way - `@command`/`@scan` are today? -5. Does the getter's docstring become the attribute's `description`, - mirroring how `Method._docstring` already captures `getdoc(fn)` for - `@command`/`@scan`? +## Resolved in review (#402) + +1. **Decorator is `@attr` + `@x.setter`** (property-mirroring), not + `@attr_r`/`@attr_rw`/`.send`. No dedicated write-only decorator — `AttrW` + alone is rare, written longhand. +2. **Datatype/limits metadata passes via decorator kwargs** (`precision`, + `units`, limits — the ADR 17 `*Meta` fields). +3. **Supports the ADR 17 `Array1D`/`Table` hints.** +4. **`@attr`-decorated attrs are treated specially by the filler** — already + defined, so not shadowed; a clash between an introspected name and a + decorated name raises. +5. **Yes — the getter's docstring becomes the attribute's `description`** + (as `@command`/`@scan` already do). diff --git a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md index 9614421df..4b44af193 100644 --- a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md +++ b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md @@ -86,8 +86,10 @@ Mechanics, directly mirroring `PviDeviceConnector`/`TangoDeviceConnector`: usage stays free (no FastCS controller/connection is instantiated at all). - Lifecycle (decision 8 of #388): the connector owns the runner; shutdown via an `atexit` hook plus an explicit `await connector.shutdown()`, which - cancels scan tasks and calls `Controller.disconnect()`. Upstream - `Device.disconnect()` is a follow-up (item 8 in #388 §8), not blocking. + cancels scan tasks and calls `Controller.disconnect()`. **The + `Device.disconnect()` proposal is dropped** — reconnect is + `Device.connect(force_reconnect=True)`, and the only disconnect we want is + `atexit` (review #402). - Embedded + transports simultaneously (decision 9 of #388, e.g. a CA GUI running next to a bluesky plan) is explicitly out of scope for the first cut, but the `ControllerRunner` is designed so a transport list can be @@ -111,14 +113,15 @@ Backend mappings (from #388 §5, grounded against the researched Datatype mapping: `Int`/`Float`/`Bool`/`String` → `int`/`float`/`bool`/`str`; `Waveform(array_dtype, shape)`/`Array1D` hint (per [ADR 17](0017-naming-pass.md)) → ophyd-async `Array1D[dtype]`; `Enum(cls)` → -the enum class itself. Two mismatches flagged as **prototype risk** in #388 -and carried into this ADR unresolved: +the enum class itself. Resolved in review (#402): -- ophyd-async constrains enums to `EnumTypes` (`StrictEnum`/`SubsetEnum`/ - `SupersetEnum`); FastCS accepts any `enum.Enum`. -- fastcs `Table` vs. ophyd-async `Table` (pydantic-based) — mapping is - best-effort, mismatches should be flagged early rather than silently - coerced. +- **Enums:** un-hinted enum classes introspect at runtime and drop to a string + datatype retaining the choices as metadata; hint-typed enums require the + author to duplicate as a `StrictEnum`/`SubsetEnum`/`SupersetEnum` (as they + would for remote FastCS) for now — revisit once there are use cases. +- **`Table`:** a real bidirectional converter **is in scope for the first + cut**, used as the opportunity to bring the FastCS and ophyd-async `Table` + implementations closer together. ## Consequences @@ -140,29 +143,23 @@ and carried into this ADR unresolved: (`fastcs.demo.simulation`) is the existing sim device ophyd-async tests against — no new simulated device is needed for the first cut. -## Open questions - -1. Enum conversion: does the connector require FastCS `Enum` datatypes used - with embedding to be `StrictEnum`/`SubsetEnum`/`SupersetEnum` subclasses - (pushing a constraint back onto FastCS driver authors who want embedding - support), or does it do runtime conversion/wrapping, and what happens to - values that don't fit ophyd-async's stricter model? -2. `Table` mapping: is a real bidirectional pydantic-model ↔ fastcs-`Table` - converter in scope for the first cut, or is `Table` explicitly - unsupported/best-effort-only initially, with a hard error on mismatch - rather than silent coercion? -3. Where does `@scan`-derived state that isn't exposed as a `Signal` go — - is it simply invisible to ophyd-async (server-side only, as the mapping - table states), or does some `@scan` output need a path to surface as a - `SignalR` (e.g. `fastcs-eiger`'s `update_voltages` `@scan` feeding - per-ramp `AttrR`s — those `AttrR`s are visible, but would a *pure* - `@scan`-only value ever need exposing)? -4. How does `embedded_fastcs_connector` handle a `Controller` that raises - during `initialise()` (e.g. a device that's unreachable at embed time) - — does `connect_real` propagate the exception directly to - `Device.connect()`, retry, or something else? -5. Should the embedded connector's shutdown (`atexit` + explicit - `await connector.shutdown()`) also be triggered by ophyd-async's own - `Device.disconnect()` once that upstream work lands (#388 §8 item 8), and - does that imply `ControllerRunner.stop()` needs to be safely callable - from a synchronous `atexit` context as well as an async one? +## Resolved in review (#402) + +- **Enums:** un-hinted → runtime-introspect, drop to string keeping choices as + metadata; hinted → require `StrictEnum`/`SubsetEnum`/`SupersetEnum` + duplication for now, revisit with use cases. +- **`Table`:** bidirectional converter in scope for the first cut; use it to + converge the two `Table` implementations. +- **Errors:** FastCS gains a `ConnectionFailedError` (raised when the device + doesn't respond); the connector converts it to `NotConnectedError` and keeps + retrying to connect in the background. All other errors surface unconverted. +- **Disconnect dropped:** reconnect is `Device.connect(force_reconnect=True)`; + the only disconnect is `atexit`. No `Device.disconnect()` proposal (so #388 + §8 item 8 / issue #401 is rewritten accordingly). + +## Open questions (awaiting input) + +1. Where does `@scan`-derived state that isn't exposed as a `Signal` go? All + attribute data already lives in `Attr` instances mapped to `Signal`s, so it + may be that `@scan` only drives updates and nothing extra needs surfacing — + needs confirming. *(awaiting @shihab-dls)* From 8979bd00e2383c11014e365998f57c7a2bda9e19 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 21 Jul 2026 10:35:26 +0000 Subject: [PATCH 10/16] docs: spell out the *Meta metadata model in ADRs (DataType replacement) Define the *Meta TypedDicts + Unpack overloads for the procedural spelling, the superset Meta for generic extras (SCPIParam), attribute-stored resolved meta, and the filler's runtime validation of annotated metadata against the datatype. Clarify in ADR 13 that hinted attributes exist after __init__ (filler creates them unfilled), not after initialise(). Module home for the new names deferred to #406 (top-level API namespace: flat vs nested). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- ...-procedural-split-and-controller-filler.md | 25 +++++++++--- .../decisions/0014-attribute-io-rw-rework.md | 40 +++++++++++++++++-- .../decisions/0017-naming-pass.md | 5 +++ .../decisions/0018-attr-decorator-sugar.md | 5 ++- .../0019-embedded-ophyd-async-connector.md | 2 +- 5 files changed, 64 insertions(+), 13 deletions(-) diff --git a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md index 847de3a58..2de4e7d63 100644 --- a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md +++ b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md @@ -77,6 +77,12 @@ bare type hints only; instance scope = procedural construction.** Concretely: own extras vocabulary the same way ophyd-async's `PvSuffix`/`TangoPolling` do. Core FastCS defines **no** extras vocabulary for 1.0 (decision 3 of #388). +- When a child is filled from an `Annotated[Attr[T], extras]` hint, the filler + **runtime-validates** the metadata the extras carries (`FloatMeta`, or a + protocol object's `.meta` such as `SCPIParam(...).meta`) against the datatype + `T` — e.g. `precision` supplied for a `str` raises. This is the runtime + counterpart to the static `Unpack[FloatMeta]` check on the procedural `Attr*` + constructors (see [ADR 14](0014-attribute-io-rw-rework.md)). - The refined rule from decision 14 of #388: *class body = declarations + decorated behaviour; instance scope = construction with data.* This keeps `@command`/`@scan`, and the new `@attr_r`/`@attr_rw` sugar, as class-body @@ -96,22 +102,29 @@ class TemperatureRampController(Controller): self.start = AttrRW(Int(), io=TempIO(conn, "S", suffix)) ``` -**Declarative hint + filler** — the value is *promised* by a hint and -provisioned by introspection at connect time (the Eiger-like case): +**Declarative hint + filler** — the value is *promised* by a hint; the +`ControllerFiller` (run from `Controller.__init__`) creates it as an +**unfilled** `Attribute` so it **exists as soon as `__init__` returns**, and +`initialise()` later *fills* it (provisions `io` + metadata) by introspection: ```python class OdinDetector(Controller): - frames: AttrRW[int] # must exist by the end of initialise() + frames: AttrRW[int] # created UNFILLED by the filler in __init__; + # self.frames EXISTS after __init__, before initialise() async def initialise(self) -> None: + # introspection FILLS the already-created hinted attrs (io + metadata), + # and may add wholly-undeclared dynamic attrs (which carry no hint) for name, meta in await self._query_parameter_tree(): - self.filler.fill_attribute(name, ...) + self.filler.fill_attribute(name, ...) # validates meta vs datatype + self.filler.check_filled() ``` **The rule** (identical to ophyd-async's rule for `Signal`s): *at the end of `__init__`, any Attribute referenced in code — and therefore carrying a type -hint — must exist.* Only `__init__` is serial; `initialise()` may then run in -parallel across controllers. +hint — must exist* (the filler guarantees this for hinted children by creating +them unfilled during `__init__`). Only `__init__` is serial; `initialise()` +may then run in parallel across controllers. Introspecting controllers keep working as today's `initialise()` + `add_attribute` pattern. The fully-dynamic case — where the *set* of diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index fc9e2ed0f..8c762fcda 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -105,10 +105,42 @@ declared without a boilerplate `__init__`. `CallbackWriteIO` classes. The same `attr` covers the read-only and read/write callback cases, so the two adapters are not shipped. -> Datatype spelling: the `Float()` etc. in the examples below predate -> [ADR 17](0017-naming-pass.md); read them as the ADR-17 python-type + `*Meta` -> form (`AttrRW(float, precision=3, io=...)`), since the `DataType` family is -> removed. +### Datatype metadata: the `*Meta` TypedDicts + +`DataType` classes are gone (ADR 15/17). The metadata they carried (precision, +units, nested limits, …) moves to a per-datatype `TypedDict` — `FloatMeta`, +`IntMeta`, `StrMeta`, `BoolMeta`, `EnumMeta`, `Array1DMeta`, `TableMeta` — and +**the resolved metadata is stored on the `Attribute` itself** (`attr.meta`), +not on a separate datatype object. Every transport/connector that read +`attr.datatype.precision`/`.units`/`.limits`/`.choices` now reads `attr.meta` +(enum `choices` come from the python type; `EnumMeta` is display-only). + +Two spellings, two validation layers: + +- **Procedural (statically checked):** the `Attr*` constructors are overloaded + per datatype so the right `*Meta` is unpacked into `**kwargs`: + + ```python + # conceptually, one overload per datatype: + def AttrRW(dtype: type[float], *, io=..., **kwargs: Unpack[FloatMeta]) -> AttrRW[float]: ... + + self.temperature = AttrRW(float, precision=3, units="deg", io=TempIO(...)) + # AttrRW(str, precision=3) is a static type error + ``` + +- **Declarative (runtime-checked by the filler):** + `Annotated[AttrRW[float], FloatMeta(precision=3)]` (rare) or + `Annotated[AttrRW[float], SCPIParam("P", precision=3)]` (common). Neither + ties the metadata to the `AttrRW[...]` type param statically, so + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s + `ControllerFiller` validates it against the datatype at fill time. A generic + extras object takes the **superset** `Meta` TypedDict — + `SCPIParam(param: str, **kwargs: Unpack[Meta])` (`Meta` being the union of + `FloatMeta`/`StrMeta`/… fields, all optional) — stores a `.meta`, and the + filler passes that `.meta` into the constructed `AttrRW`. + +The `*Meta` module location is deferred to the public-API-namespace decision +(#406); land it provisionally until then. Migration is mechanical for the common case (an old `AttributeIO` subclass absorbs its `AttributeIORef`'s fields into its own `__init__` and is diff --git a/docs/explanations/decisions/0017-naming-pass.md b/docs/explanations/decisions/0017-naming-pass.md index 3ac954894..a781e002e 100644 --- a/docs/explanations/decisions/0017-naming-pass.md +++ b/docs/explanations/decisions/0017-naming-pass.md @@ -41,6 +41,11 @@ after 1.0 when they become a deprecation cycle. > issue #392) rather than a separate late PR. `Array1D`/`Table` become *both* > the hint and the runtime structure passed around as the datatype — there is > no separate `Waveform`/`DataType` object to map to. +> +> The concrete `*Meta` mechanism (per-datatype `TypedDict`s, the superset +> `Meta` for extras, `attr.meta` storage on the attribute, `Unpack` overloads) +> is specified in [ADR 14](0014-attribute-io-rw-rework.md); the module home for +> these public names is decided in #406. 1. **`prec` → `precision`.** Rename across `_Numeric`/`Float`/wherever `prec` appears (transports, docs, snippets). No behaviour change. diff --git a/docs/explanations/decisions/0018-attr-decorator-sugar.md b/docs/explanations/decisions/0018-attr-decorator-sugar.md index f5ca998b0..b4b7a6d07 100644 --- a/docs/explanations/decisions/0018-attr-decorator-sugar.md +++ b/docs/explanations/decisions/0018-attr-decorator-sugar.md @@ -117,8 +117,9 @@ class PowerSupply(Controller): 1. **Decorator is `@attr` + `@x.setter`** (property-mirroring), not `@attr_r`/`@attr_rw`/`.send`. No dedicated write-only decorator — `AttrW` alone is rare, written longhand. -2. **Datatype/limits metadata passes via decorator kwargs** (`precision`, - `units`, limits — the ADR 17 `*Meta` fields). +2. **Datatype/limits metadata passes via decorator kwargs**, typed with + `Unpack[…Meta]` (`precision`, `units`, limits — the ADR 14/17 `*Meta` + fields), validated against the getter's return type. 3. **Supports the ADR 17 `Array1D`/`Table` hints.** 4. **`@attr`-decorated attrs are treated specially by the filler** — already defined, so not shadowed; a clash between an introspected name and a diff --git a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md index 4b44af193..6bdd3ca87 100644 --- a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md +++ b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md @@ -104,7 +104,7 @@ Backend mappings (from #388 §5, grounded against the researched | `SignalBackend.set_callback` | `AttrR.add_on_update_callback(cb, always=True)`; stamped per [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | | `SignalBackend.put` | `AttrW.put(value)` | | `SignalBackend.get_setpoint` | `AttrW` cached setpoint, [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | -| `SignalBackend.get_datakey` | `Attribute.datatype` → `SignalMetadata` (units, precision, limits, choices) + `make_datakey` | +| `SignalBackend.get_datakey` | `attr.meta` (units, precision, limits) + python-type/enum choices → `SignalMetadata` + `make_datakey` | | `CommandBackend.execute`/`.signature` | `Command.__call__` / captured `Signature`, [ADR 15](0015-typed-commands.md) | | `SignalBackend.source` | e.g. `fastcs://.` | | child `Device` / `DeviceVector` | sub-`Controller` / `ControllerVector` | From 85df0f595bff5d40197104f6186b913c1f51e10b Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 21 Jul 2026 14:15:41 +0000 Subject: [PATCH 11/16] docs: ADR 15 - drop Command[Any, Any], no partial typing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @Tom-Willemsen on PR #402 (r3621453680): SECoP devices are discovered entirely from an over-the-wire `describe`, so you never statically know something is a command without knowing its signature. There is no "known command, unknown args" middle case — P/T are either completely known (static `Command[P, T]`) or the whole structure is unknown (built at runtime). Remove the `Any` args/returns option and the `Command[Any, Any]` consequence; resolve the open question awaiting @Tom-Willemsen. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- .../decisions/0015-typed-commands.md | 50 ++++++++++++------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/docs/explanations/decisions/0015-typed-commands.md b/docs/explanations/decisions/0015-typed-commands.md index 9df453dec..f4d488da7 100644 --- a/docs/explanations/decisions/0015-typed-commands.md +++ b/docs/explanations/decisions/0015-typed-commands.md @@ -62,9 +62,20 @@ class Ramp(Controller): **Argument and return typing are independent** (not all-or-nothing): -- *Args*: `[]` (none) · `[DT1, DT2, …]` (positional, known types — validated) · - `Any` (must be a command, signature introspected at runtime). -- *Returns*: `None` · `DT` (a single typed value) · `Any` (introspected). +- *Args*: `[]` (none) · `[DT1, DT2, …]` (positional, known types — validated). +- *Returns*: `None` · `DT` (a single typed value). + +There is **no partial `Command[Any, Any]`** — a statically-declared `Command` +always has its parameter and return types fully known. The alternative is not +a half-known command but a fully-dynamic controller: a driver that knows +*nothing* statically (`fastcs-secop`, discovering everything from an +over-the-wire `describe`) does not annotate a `Command` at all — it builds the +whole structure, attributes and commands alike, at runtime. So `P`/`T` are +either **completely known** (static declaration) or the **whole structure is +unknown** (runtime construction); there is no in-between case where you know +something is a command but not its signature. This is the same "hint vs. +no-hint" split as [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) +for attributes — with no partial hint. Following the [ADR 17](0017-naming-pass.md) `DataType` drop, command arguments/returns use plain python types + `*Meta` exactly as attributes do @@ -83,12 +94,14 @@ need a `TYPE_CHECKING` stub trick and are prototyped separately in the spike - EPICS transports (`transports/epics/ca`, `transports/epics/pva`) need a capability check at controller-API-build time, producing a startup-time warning log rather than a runtime failure per typed-command call. -- `fastcs-secop`'s dynamically-typed command args/results likely still need - `Command[Any, Any]` or a per-instance generated type, since SECoP's - `datainfo` is only known at connect time — full static typing of command - signatures is not achievable for introspection-driven drivers, only for - statically-declared ones. This mirrors the same "hint vs. no-hint" tension - as [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). +- `fastcs-secop` (and any introspection-driven driver) does **not** get a + `Command[Any, Any]`. Since SECoP's `describe` reveals the entire structure + only at connect time — there is no static declaration of *anything*, let + alone a command of unknown signature — such drivers build their commands + programmatically at runtime, each carrying a concrete signature derived from + the wire `datainfo`. Static `Command[P, T]` is for statically-declared + controllers; fully-dynamic drivers construct commands (or keep the existing + `SecopCommandController` explode-to-PVs workaround) at runtime instead. - Command args/return values validate through the **same** python-type + `*Meta` mechanism as attributes (the `DataType` family is removed, ADR 17) — one shared validation/serialisation path, no command-specific duplicate. @@ -99,18 +112,19 @@ need a `TYPE_CHECKING` stub trick and are prototyped separately in the spike command args/returns use python types + `*Meta` like attributes; no separate mechanism, and complex-type serialisation (arrays, `Enum`, `Table`) is shared with `Attribute`. -- **Args and returns are typed independently** — Args `[]` / `[DT…]` / `Any`; - Returns `None` / `DT` / `Any` (see Decision). Not all-or-nothing. +- **Args and returns are typed independently** — Args `[]` / `[DT…]`; + Returns `None` / `DT` (see Decision). Not all-or-nothing, but each is fully + known — there is no `Any` middle case. +- **No partial `Command[Any, Any]`.** @Tom-Willemsen confirmed on + [#402](https://github.com/DiamondLightSource/fastcs/pull/402#discussion_r3621453680) + that SECoP devices are discovered entirely from an over-the-wire `describe`: + you never statically know something is a command but not its signature — you + either know the full `Command[P, T]` or you know nothing at all and build the + whole controller at runtime. `P`/`T` are therefore completely known or the + whole structure is unknown, with no in-between. - **EPICS skip-with-warning fires at IOC startup** — post controller construction, when the fully populated controllers are handed to the transports to serve. - **Keyword-arg commands → spike [#403](https://github.com/DiamondLightSource/fastcs/issues/403)** (interactive/Opus; needs a `TYPE_CHECKING` stub). Out of scope for core typed-command work. - -## Open questions (awaiting input) - -1. Are there real `fastcs-secop` devices where you know something is a - `Command` but not its signature until runtime? Determines whether - `Command[Any, Any]` + runtime introspection suffices, or the kw-arg trick - (#403) is truly needed. *(awaiting @Tom-Willemsen)* From 8a4e4a8bfe3dfa269f64ed609093bfb70363bbe4 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 22 Jul 2026 10:32:27 +0000 Subject: [PATCH 12/16] docs: consolidate examples into fastcs.demo; SCPIParam one-spec design Fold the top-level examples/ scaffold into the fastcs.demo package (mirrors ophyd-async; examples install with fastcs[demo] and become the single source of the tutorial code). Replace the stale 3-style scaffold README with the final 5-example hello-world -> complicated-device ladder (two backends: temperature sim for steps 1-4, cut-down Eiger REST sim for step 5), keyed to issues #398/#404/#390/#405/#391. ADR 0014: spell out the "one spec object per declaratively-filled attribute" design - SCPIParam carries the binding token AND all metadata via Unpack[Meta], is the exclusive spec source (filler does not merge a separate *Meta extra), and pays for its ergonomics with runtime validation. Record why it is SCPIParam not SCPIMeta (a binding extra you instantiate, sibling of PvSuffix/TangoPolling, not a Meta TypedDict) and that it lives in the demo/ protocol layer, not core (decision 3). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- .../decisions/0014-attribute-io-rw-rework.md | 27 ++++++++ examples/README.md | 65 ------------------- examples/__init__.py | 4 -- src/fastcs/demo/README.md | 61 +++++++++++++++++ 4 files changed, 88 insertions(+), 69 deletions(-) delete mode 100644 examples/README.md delete mode 100644 examples/__init__.py create mode 100644 src/fastcs/demo/README.md diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 8c762fcda..3bcb981cd 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -139,6 +139,33 @@ Two spellings, two validation layers: `FloatMeta`/`StrMeta`/… fields, all optional) — stores a `.meta`, and the filler passes that `.meta` into the constructed `AttrRW`. +**One spec object per declaratively-filled attribute.** A protocol extra like +`SCPIParam` is the *single place* an attribute's whole specification is +written: both the protocol binding (the command token, `"P"`) and all its +generic metadata (`description`, `precision`, `units`, limits…) via +`**Unpack[Meta]`. The filler treats that extra as the **exclusive** spec +source for its attribute — it does **not** also merge a separate +`FloatMeta`/`Meta` extra sitting on the same `Annotated[...]` hint, so there +is no precedence question to resolve. The trade this accepts is deliberate: +routing metadata through the superset `Meta` (not a per-datatype +`Unpack[FloatMeta]`) means correctness is the filler's **runtime** job, not a +static check — a separate `Annotated` extra cannot tie its `**Meta` to the +`AttrRW[...]` datatype param, and making the extra generic +(`SCPIParam[float](...)`) only forces the user to restate a type already in +the hint. So the declarative path pays for its ergonomics with runtime +validation; the filler's error must name the attribute and field (e.g. +"`precision` is not valid for `str` attribute `device_id`"). + +Naming: the extra is `SCPIParam` (a binding object you *instantiate* as an +`Annotated` extra), **not** `SCPIMeta` — the `*Meta` suffix is reserved for +the metadata TypedDicts you `Unpack` (`FloatMeta`, `Meta`), a different kind +of Python object. `SCPIParam` is a sibling of ophyd-async's +`PvSuffix`/`TangoPolling`, not of `SignalMetadata`. It is **not** part of core +FastCS (decision 3: core defines no extras vocabulary for 1.0) — it lives in a +protocol layer; the demo package ships an example `SCPIController` + +`SCPIParam` to show how a third party builds one on the filler's +`(child, extras)` mechanism. + The `*Meta` module location is deferred to the public-API-namespace decision (#406); land it provisionally until then. diff --git a/examples/README.md b/examples/README.md deleted file mode 100644 index d769ffeef..000000000 --- a/examples/README.md +++ /dev/null @@ -1,65 +0,0 @@ -# examples/ - -This package is the **living review artifact** for the FastCS / ophyd-async -API-convergence refactor tracked in -[issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) and its -[ADRs](../docs/explanations/decisions/) (0013-0019). - -It exists so that every framework PR in the refactor has something concrete -to run against, alongside the unit test suite: three example controllers, -each written in one of the three styles the refactor is converging FastCS -towards. As each framework PR lands (`AttributeIO` rework, `ControllerFiller`, -typed commands, the setpoint cache/timestamps/`ControllerRunner`, the naming -pass, `@attr_rw` sugar), the example(s) it affects are updated in the *same* -PR, so `examples/` always reflects current `main` and stays green under -`uv run --locked tox`. It is not a tutorial or documentation snippet source — -those live under `docs/`. - -## The three styles - -1. **IORef temperature controller** (`examples/example_1_ioref/`, tracked by - the `DRAFT: Example 1` sub-issue of #388). A deliberately-messy adaptation - of `src/fastcs/demo/controllers.py`, written against **whatever the - current API is** at the time it's updated. It starts on the *pre-refactor* - `AttributeIORef`/class-scope-instance API and is gradually cleaned up as - each framework PR lands — e.g. once - [ADR 14](../docs/explanations/decisions/0014-attribute-io-rw-rework.md)'s - `AttributeIO` rework merges, this example's `io_ref=`/`ios=[...]` wiring - is updated to `io=` in that same PR. This is intentional: it is the - baseline that proves each framework PR doesn't break a real (if simple) - driver, and it visibly tracks the migration path a downstream repo like - `fastcs-eiger` or `fastcs-catio` would need to follow. - -2. **Introspectable Eiger-style controller** (`examples/example_2_introspectable/`, - tracked by the `DRAFT: Example 2` sub-issue of #388). Mirrors a REST API - the way `fastcs-eiger`, `fastcs-secop`, and `fastcs-PandABlocks` do today: - attributes are built from data queried at `initialise()` time, not - declared as class-scope instances. This is the example that exercises - [ADR 13](../docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md)'s - `ControllerFiller` and bare-hint declarations once that PR lands. - -3. **PyTango-style `@attr_rw` device** (`examples/example_3_decorator/`, - tracked by the `DRAFT: Example 3` sub-issue of #388, blocked on the - `@attr_rw` decorator-sugar issue). A trivial getter/setter device in the - style of - [ADR 18](../docs/explanations/decisions/0018-attr-decorator-sugar.md), - demonstrating the simple case FastCS needs to match PyTango on. - -## Status - -**Scaffold only.** This PR (the ADR seed) adds this package, its structure, -and this README — it does not implement the framework changes or the three -example controllers themselves. Each example is implemented by its own -tracked sub-issue of #388, against the framework API as it exists once that -issue's dependencies have landed. See the `Blocked by:` lines on each -`DRAFT:` sub-issue for the landing order. - -## Keeping it green - -Every framework PR that touches `src/fastcs/` and affects one of these three -styles must update the corresponding example(s) in the same PR, and -`uv run --locked tox` must pass. This is the acceptance criterion listed on -every sub-issue of #388 for exactly this reason: it keeps the examples -honest as a description of "how do I actually write a FastCS driver today," -rather than letting them drift out of sync with the framework the way -standalone documentation snippets can. diff --git a/examples/__init__.py b/examples/__init__.py deleted file mode 100644 index 7cb4edb3d..000000000 --- a/examples/__init__.py +++ /dev/null @@ -1,4 +0,0 @@ -"""Living review artifacts for the #388 API-convergence refactor. - -See ``examples/README.md`` for what this package is and how it's used. -""" diff --git a/src/fastcs/demo/README.md b/src/fastcs/demo/README.md new file mode 100644 index 000000000..9651ca596 --- /dev/null +++ b/src/fastcs/demo/README.md @@ -0,0 +1,61 @@ +# `fastcs.demo` + +The demo package ships FastCS's **living example controllers** for the +ophyd-async / FastCS API-convergence refactor +([issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), ADRs +0013–0019). Consolidated here (rather than a top-level `examples/` package) to +mirror ophyd-async, so the examples install with `fastcs[demo]` and can be run, +imported, and — crucially — used as the **single source of the tutorial code**. + +These modules are the canonical source the tutorials `literalinclude` from +(one tutorial per example, see the docs `tutorials/`). They are kept green +under `uv run --locked tox`, so the tutorials cannot drift from the framework: +every framework PR that changes an API updates the example(s) it affects in the +*same* PR. This replaces the old "hand-authored `docs/snippets/` that drift" +approach — the examples are the docs. + +## The five examples — a hello-world → complicated-device ladder + +Two hardware backends: a temperature-controller sim (steps 1–4) and a +cut-down Eiger REST sim (step 5). Step 1 is pure-soft (no backend). Each rung +introduces exactly one new concept. + +| # | Module | Concept | Backend | Issue | +|---|--------|---------|---------|-------| +| 1 | `hello_world.py` | pure-soft `@attr`/`@attr_rw` decorator over in-memory values | none (soft) | [#398](https://github.com/DiamondLightSource/fastcs/issues/398) | +| 2 | `temperature_attr.py` | callback getter/setter via the `attr` factory in `__init__` | temperature sim | [#404](https://github.com/DiamondLightSource/fastcs/issues/404) | +| 3 | `controllers.py` | reusable per-attribute `io=` `ReadWriteIO` objects, sub-controllers/vectors, `@scan`/`@command` | temperature sim | [#390](https://github.com/DiamondLightSource/fastcs/issues/390) | +| 4 | `temperature_scpi.py` (+ `scpi.py`) | declarative annotated attributes; `ControllerFiller` builds each `io` from **static** `SCPIParam` extras metadata | temperature sim | [#405](https://github.com/DiamondLightSource/fastcs/issues/405) | +| 5 | `eiger.py` (+ `simulation/eiger.py`) | introspectable device: bare hints filled from a **runtime** REST parameter tree | Eiger REST sim | [#391](https://github.com/DiamondLightSource/fastcs/issues/391) | + +Notes: + +- **Steps 2 vs 3** are the same device wired two ways — inline per-attribute + getter/setter, then the same IO factored into a reusable `io=` object — a + natural refactoring story on one backend. +- **Step 4 is deliberately *not* introspectable.** A SCPI device does not + describe itself, which is exactly why you hand-annotate: the metadata lives + in your Python (`SCPIParam("P", precision=3, …)`), not on the wire. Do **not** + invent SCPI introspection — that would erase the contrast with step 5. The + example `SCPIController`/`SCPIParam` vocabulary lives *here in the demo*, not + in core FastCS (decision 3: core ships no extras vocabulary for 1.0); it + demonstrates how a protocol layer builds on the filler's `(child, extras)` + mechanism. +- **Step 5 uses a separate Eiger REST backend on purpose.** Introspection earns + its complexity only when a device's parameters aren't knowable at author time + (a detector, not a fixed-command temp controller). The backend switch *is* + the lesson — "small & known → declare; large & self-describing → introspect" + — and the REST sim also exercises an HTTP client backend the temp examples + never touch, matching real downstream drivers (`fastcs-eiger`, `fastcs-secop`, + PandABlocks). + +## Baselines vs framework PRs + +Steps 2, 3, 5 have current-API baselines that can be written **now** +(deliberately messy against the pre-refactor API) and are cleaned up as each +framework PR lands. Steps 1 and 4 need framework work first (`attr` factory +#397; `ControllerFiller` #394). See each issue's `Blocked by:` line. + +`literalinclude` region markers are added to each module as part of writing its +tutorial (the umbrella docs pass, +[#408](https://github.com/DiamondLightSource/fastcs/issues/408)), not up front. From 281819580d4582ff093ad5fbd0e162440012ff01 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 22 Jul 2026 15:54:31 +0000 Subject: [PATCH 13/16] docs: getter/setter IO model; @attr decorator-only; 4-tutorial ladder Fold in the 2026-07-22 review decisions: ADR 0014: io= objects (ReadIO/WriteIO/ReadWriteIO) superseded by getter/setter callables on AttrR/AttrW/AttrRW. getter() -> T | Update[T]; setter -> None | T | Update[T] (value return = accepted/clamped readback, the sanctioned secop setpoint echo). Update[T] = value/timestamp/severity. Datatype optional when getter/setter given (inferred, unwrapping Update[T]). update_period: ONCE (default) / float / None (on-demand); no getter = @scan-fed soft. Access mode from which params exist; ReadIO trio dropped. Both prior open questions closed. ADR 0018: @attr is decorator-only (@attr / @attr(precision=3) + @x.setter); no @attr_r/@attr_rw, no free-function attr() factory; procedural is AttrR/AttrRW. Title + stale refs swept; 0013 refs swept too. demo README: five modules, four tutorials (the reusable-io= rung is gone); controllers.py repurposed to the composition/@scan/@command example folded into the declarative tutorial. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- ...-procedural-split-and-controller-filler.md | 4 +- .../decisions/0014-attribute-io-rw-rework.md | 60 +++++++++++-- .../decisions/0018-attr-decorator-sugar.md | 21 +++-- src/fastcs/demo/README.md | 87 +++++++++++-------- 4 files changed, 119 insertions(+), 53 deletions(-) diff --git a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md index 2de4e7d63..599cfe624 100644 --- a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md +++ b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md @@ -58,7 +58,7 @@ bare type hints only; instance scope = procedural construction.** Concretely: io=...)` may no longer be assigned directly in a class body. - Remove the deepcopy half of `_bind_attrs`. Method binding for `@command`/ `@scan` (the `UnboundCommand`/`UnboundScan` machinery) is unaffected and - stays, since it does not require deepcopy — see decision 14 (`@attr_rw` + stays, since it does not require deepcopy — see decision 14 (`@attr` decorator sugar). - Remove `HintedAttribute` and `_validate_type_hints`/`_validate_hinted_*` as a *separate* validation-only pass. Their job — "this hinted child must @@ -85,7 +85,7 @@ bare type hints only; instance scope = procedural construction.** Concretely: constructors (see [ADR 14](0014-attribute-io-rw-rework.md)). - The refined rule from decision 14 of #388: *class body = declarations + decorated behaviour; instance scope = construction with data.* This keeps - `@command`/`@scan`, and the new `@attr_r`/`@attr_rw` sugar, as class-body + `@command`/`@scan`, and the new `@attr`/`@x.setter` sugar, as class-body citizens, since none of them require per-instance deepcopy — they bind a method to `self` at construction time instead. diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 3bcb981cd..4077c536c 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -51,6 +51,52 @@ The dispatch-by-type registry has real costs in our downstream drivers: ## Decision +> **Review update (#402, 2026-07-22): `io=` objects replaced by `getter`/`setter` callables.** +> The `ReadIO`/`WriteIO`/`ReadWriteIO` hierarchy and the `io=` argument described +> below are **superseded.** Per-attribute IO is supplied as plain callables on the +> constructors, which *is* the procedural spelling of the `@attr` decorator +> ([ADR 18](0018-attr-decorator-sugar.md)): +> +> - `AttrR(getter=g)`, `AttrW(setter=s)`, `AttrRW(getter=g, setter=s)`. Access mode +> is enforced by **which parameters exist** (an `AttrR` has no `setter`), so the +> three-class IO hierarchy and its abstract-method enforcement are dropped +> entirely — and `getter=` on a read-only attr is honest where `io=` was a false +> friend. +> - **The getter returns the value; the framework applies it** — `getter() -> T | +> Update[T]` — instead of the old imperative `io.update(attr)`. Imperative / +> multi-attribute periodic logic stays with `@scan`, which is *why* per-attribute +> IO shrinks to "one value in / out". The **setter** returns `None | T | +> Update[T]`: `None` = fire-and-forget (readback catches up on the next poll / the +> setpoint cache); a returned value is the device's *accepted* value (clamp/echo) +> and updates the readback + `AttrW` setpoint cache immediately — the sanctioned +> replacement for `fastcs-secop`'s private `_call_sync_setpoint_callbacks`. +> - `Update[T]` = `value: T`, `timestamp: float | None` (epoch seconds; `None` ⇒ +> framework stamps receive-time), `severity: Severity = OK` (the decision-10b +> severity enum); used for both the getter return and a value-returning setter — +> this is how device-native timestamps/severity reach `attr.update()`. +> - **Datatype is optional when a getter/setter is given** — inferred from the +> getter's return annotation (or the setter's param), unwrapping `Update[T]` to +> `T`, so `AttrR(getter=g)` yields `AttrR[float]` with no restated type (parity +> with `@attr`). Only the bare python type is optional; `precision`/`units`/… stay +> explicit kwargs, and the per-datatype `Unpack[*Meta]` static check keys off the +> inferred return type. Not inferable (`-> Any`, unannotated lambda) ⇒ the +> positional datatype is required (fail-fast at construction). +> - `update_period` is a read-side kwarg: `ONCE` = read once at connect (the default +> when a getter is given); a float = poll at that rate; `None` = **on-demand only** +> (read when a client asks, never auto-polled). **No getter** = soft, value pushed +> via `attr.update()` from a `@scan`/callback. +> - Soft is now simply the *absence* of getter/setter (`AttrRW(float)` self-wires +> setpoint→readback as before); the `io=None` sentinel is gone. +> - The declarative/filler path lowers to the **same** getter/setter (a +> `SCPIController`'s filler builds the callables from `SCPIParam`); getter/setter +> are where the old `_connect_attribute_ios` wiring now lives, so transports and +> the embedded connector are unaffected. +> +> `attr` is a **decorator only** (`@attr` / `@attr(precision=3)` + `@my_attr.setter`); +> there is no free-function `attr()` factory — the procedural spelling is `AttrR`/ +> `AttrRW` directly. The `io=` prose below is kept for the `AttributeIORef`→callable +> migration context; read `getter=`/`setter=` for the final shape. + Replace `AttributeIO`/`AttributeIORef` with three focused, per-attribute IO base classes with abstract `update`/`send` methods, passed as a single `io=` constructor argument: @@ -241,10 +287,10 @@ a public method on `AttrW`/`ReadWriteIO` — exact shape is an open question. ## Open questions (awaiting input) -1. Final class names: `ReadIO`/`WriteIO`/`ReadWriteIO` vs. `AttrRIO`/`AttrWIO`/ - `AttrRWIO` vs. something else — see [ADR 17](0017-naming-pass.md). - *(awaiting @shihab-dls)* -2. Public replacement for `fastcs-secop`'s `_call_sync_setpoint_callbacks`: - an optional `sync_setpoint` argument on `WriteIO.send`, or a public method - on `AttrW` an IO author can call from `send`? *(awaiting @shihab-dls / - @Tom-Willemsen)* +Both original open questions are closed by the 2026-07-22 getter/setter model: + +1. ~~Final IO class names (`ReadIO`/`WriteIO`/`ReadWriteIO` vs …)~~ — **moot**: the + IO class hierarchy is gone; IO is plain `getter`/`setter` callables. +2. ~~Public replacement for `fastcs-secop`'s `_call_sync_setpoint_callbacks`~~ — + **resolved**: a `setter` returning `T | Update[T]` *is* the sanctioned setpoint + echo (updates readback + `AttrW` setpoint cache). diff --git a/docs/explanations/decisions/0018-attr-decorator-sugar.md b/docs/explanations/decisions/0018-attr-decorator-sugar.md index b4b7a6d07..87a1924cc 100644 --- a/docs/explanations/decisions/0018-attr-decorator-sugar.md +++ b/docs/explanations/decisions/0018-attr-decorator-sugar.md @@ -1,4 +1,4 @@ -# 18. Attr-from-Method Decorator Sugar (@attr_r / @attr_rw) +# 18. Attr-from-Method Decorator Sugar (`@attr` + `@x.setter`) Date: 2026-07-20 @@ -73,17 +73,20 @@ class PowerSupply(Controller): keyword the way PyTango does — decision 14 of #388 explicitly calls this out as *better* than PyTango's `dtype=` kwarg since it's one real annotation, checked statically. -- `@attr_r`/`@attr_rw` decorator keyword arguments (`units`, `update_period`, - etc.) map onto the equivalent `DataType`/`ReadIO`/`WriteIO` constructor - arguments from [ADR 14](0014-attribute-io-rw-rework.md) — this is sugar - over that mechanism, not a parallel one. +- `@attr` comes in two forms: bare `@attr` and parameterised `@attr(precision=3, + units="V", update_period=0.5)`; the keyword arguments map onto the same `*Meta` + fields and the `getter`/`setter` + `update_period` constructor arguments of + `AttrR`/`AttrRW` ([ADR 14](0014-attribute-io-rw-rework.md)) — sugar over that + mechanism, not a parallel one. There is **no** free-function `attr()` factory: + the procedural spelling is `AttrR(getter=…)` / `AttrRW(getter=…, setter=…)` + directly. - `@attr`'s `.setter` decorator mirrors `@property`/`@x.setter`, giving the read+write pair a single logical name (`voltage`) with two decorated methods. (`.send` is not used.) -- This degrades gracefully into the full `io=` object form for protocol - families with more complex needs, and into +- This degrades gracefully into the procedural `AttrR`/`AttrRW(getter=…, + setter=…)` form for protocol families with more complex needs, and into [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s - filler for introspection-driven attributes — `@attr_rw` is explicitly the + filler for introspection-driven attributes — `@attr` is explicitly the *simple* case, not a replacement for either. - Refines the class-body rule stated in [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) to: @@ -103,7 +106,7 @@ class PowerSupply(Controller): `CallbackReadIO`/`CallbackWriteIO`): `@attr` and `attr(getter=…)` are two spellings over one implementation. - Adds a third way to declare an attribute (bare hint + filler; explicit - `AttrRW(..., io=...)`; `@attr_rw` sugar) — the docs need to be clear about + `AttrRW(getter=…, setter=…)`; `@attr` sugar) — the docs need to be clear about when to reach for which, so this doesn't become three equally-weighted options with no guidance, undermining the "harsh split" clarity [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) is diff --git a/src/fastcs/demo/README.md b/src/fastcs/demo/README.md index 9651ca596..110f518a7 100644 --- a/src/fastcs/demo/README.md +++ b/src/fastcs/demo/README.md @@ -8,40 +8,56 @@ mirror ophyd-async, so the examples install with `fastcs[demo]` and can be run, imported, and — crucially — used as the **single source of the tutorial code**. These modules are the canonical source the tutorials `literalinclude` from -(one tutorial per example, see the docs `tutorials/`). They are kept green -under `uv run --locked tox`, so the tutorials cannot drift from the framework: -every framework PR that changes an API updates the example(s) it affects in the -*same* PR. This replaces the old "hand-authored `docs/snippets/` that drift" -approach — the examples are the docs. - -## The five examples — a hello-world → complicated-device ladder - -Two hardware backends: a temperature-controller sim (steps 1–4) and a -cut-down Eiger REST sim (step 5). Step 1 is pure-soft (no backend). Each rung -introduces exactly one new concept. - -| # | Module | Concept | Backend | Issue | -|---|--------|---------|---------|-------| -| 1 | `hello_world.py` | pure-soft `@attr`/`@attr_rw` decorator over in-memory values | none (soft) | [#398](https://github.com/DiamondLightSource/fastcs/issues/398) | -| 2 | `temperature_attr.py` | callback getter/setter via the `attr` factory in `__init__` | temperature sim | [#404](https://github.com/DiamondLightSource/fastcs/issues/404) | -| 3 | `controllers.py` | reusable per-attribute `io=` `ReadWriteIO` objects, sub-controllers/vectors, `@scan`/`@command` | temperature sim | [#390](https://github.com/DiamondLightSource/fastcs/issues/390) | -| 4 | `temperature_scpi.py` (+ `scpi.py`) | declarative annotated attributes; `ControllerFiller` builds each `io` from **static** `SCPIParam` extras metadata | temperature sim | [#405](https://github.com/DiamondLightSource/fastcs/issues/405) | -| 5 | `eiger.py` (+ `simulation/eiger.py`) | introspectable device: bare hints filled from a **runtime** REST parameter tree | Eiger REST sim | [#391](https://github.com/DiamondLightSource/fastcs/issues/391) | +(see the docs `tutorials/`). They are kept green under `uv run --locked tox`, +so the tutorials cannot drift from the framework: every framework PR that +changes an API updates the example(s) it affects in the *same* PR. This +replaces the old "hand-authored `docs/snippets/` that drift" approach — the +examples are the docs. + +## The example modules — a hello-world → complicated-device ladder + +Two hardware backends: a temperature-controller sim and a cut-down Eiger REST +sim. The hello-world is pure-soft (no backend). IO is supplied as plain +`getter`/`setter` callables on `AttrR`/`AttrW`/`AttrRW` (or the `@attr` +decorator) — there is no `io=` object and no `DataType`. + +| Module | Concept | Backend | Issue | +|--------|---------|---------|-------| +| `hello_world.py` | pure-soft `@attr` decorator over in-memory values | none (soft) | [#398](https://github.com/DiamondLightSource/fastcs/issues/398) | +| `temperature_attr.py` | `getter`/`setter` callables in `__init__` (`AttrRW(getter=…, setter=…)`) | temperature sim | [#404](https://github.com/DiamondLightSource/fastcs/issues/404) | +| `controllers.py` | composition & methods: sub-controllers / `ControllerVector`, `@scan`, `@command` (getter/setter IO) | temperature sim | [#390](https://github.com/DiamondLightSource/fastcs/issues/390) | +| `temperature_scpi.py` (+ `scpi.py`) | declarative annotated attributes; `ControllerFiller` builds each getter/setter from **static** `SCPIParam` extras metadata | temperature sim | [#405](https://github.com/DiamondLightSource/fastcs/issues/405) | +| `eiger.py` (+ `simulation/eiger.py`) | introspectable device: bare hints filled from a **runtime** REST parameter tree | Eiger REST sim | [#391](https://github.com/DiamondLightSource/fastcs/issues/391) | + +## The four tutorials + +Five modules, **four** tutorials (the old "reusable `io=` object" rung is gone — +`io=` objects were replaced by getter/setter callables, so there is nothing to +factor into): + +1. **hello world** — `hello_world.py` (soft `@attr`). +2. **getter/setter** — `temperature_attr.py`; closes with *"when the shared + pattern is worth naming, reach for the declarative style →"*. +3. **declarative** — `temperature_scpi.py` (annotated `SCPIParam` + filler), + and this is where **composition + `@scan` + `@command`** are shown, walking + the full multi-ramp temperature controller (`controllers.py`, #390). +4. **introspectable** — `eiger.py`. Notes: -- **Steps 2 vs 3** are the same device wired two ways — inline per-attribute - getter/setter, then the same IO factored into a reusable `io=` object — a - natural refactoring story on one backend. -- **Step 4 is deliberately *not* introspectable.** A SCPI device does not - describe itself, which is exactly why you hand-annotate: the metadata lives - in your Python (`SCPIParam("P", precision=3, …)`), not on the wire. Do **not** - invent SCPI introspection — that would erase the contrast with step 5. The - example `SCPIController`/`SCPIParam` vocabulary lives *here in the demo*, not - in core FastCS (decision 3: core ships no extras vocabulary for 1.0); it - demonstrates how a protocol layer builds on the filler's `(child, extras)` - mechanism. -- **Step 5 uses a separate Eiger REST backend on purpose.** Introspection earns +- **The declarative style is the DRY answer for a real protocol family**, not + a reusable IO object. Recommend it when the shared wire pattern is worth + naming (a protocol you'll reuse); for a handful of bespoke attributes, + getter/setter in `__init__` is lighter and fine. +- **`temperature_scpi.py` is deliberately *not* introspectable.** A SCPI device + does not describe itself, which is exactly why you hand-annotate: the metadata + lives in your Python (`SCPIParam("P", precision=3, …)`), not on the wire. Do + **not** invent SCPI introspection — that would erase the contrast with the + Eiger example. The `SCPIController`/`SCPIParam` vocabulary lives *here in the + demo*, not in core FastCS (decision 3: core ships no extras vocabulary for + 1.0); it demonstrates how a protocol layer builds on the filler's + `(child, extras)` mechanism. +- **`eiger.py` uses a separate REST backend on purpose.** Introspection earns its complexity only when a device's parameters aren't knowable at author time (a detector, not a fixed-command temp controller). The backend switch *is* the lesson — "small & known → declare; large & self-describing → introspect" @@ -51,10 +67,11 @@ Notes: ## Baselines vs framework PRs -Steps 2, 3, 5 have current-API baselines that can be written **now** -(deliberately messy against the pre-refactor API) and are cleaned up as each -framework PR lands. Steps 1 and 4 need framework work first (`attr` factory -#397; `ControllerFiller` #394). See each issue's `Blocked by:` line. +`temperature_attr.py`, `controllers.py`, and `eiger.py` have current-API +baselines that can be written **now** (deliberately messy against the +pre-refactor API) and are cleaned up as each framework PR lands. `hello_world.py` +and `temperature_scpi.py` need framework work first (`@attr` #397; +`ControllerFiller` #394). See each issue's `Blocked by:` line. `literalinclude` region markers are added to each module as part of writing its tutorial (the umbrella docs pass, From 3da025fa56a2a0594ca7697eb5ca10e9920c580c Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Wed, 22 Jul 2026 16:18:08 +0000 Subject: [PATCH 14/16] docs: ADR 14 - runtime surface (.readback/.setpoint, poll()/poll_period, set()) Rename/split the get()/update(value)/put(value) trio so access mode and device-IO are legible from the member set: - .value -> .readback + .setpoint (read-only properties; mirror bluesky/ ophyd Location(setpoint, readback); presence tracks access mode). - no-arg update() -> poll() (returns the value); update_period -> poll_period (schedule only). Deletes set_update_callback/bind_update_callback. - update(value) is now a pure cache push (no IO, no None sentinel). - put() -> set() (bluesky verb); caches .setpoint then runs setter; setter's T|Update[T] return feeds .readback. sync_setpoint kwarg gone. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- .../decisions/0014-attribute-io-rw-rework.md | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 4077c536c..551238154 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -97,6 +97,43 @@ The dispatch-by-type registry has real costs in our downstream drivers: > `AttrRW` directly. The `io=` prose below is kept for the `AttributeIORef`→callable > migration context; read `getter=`/`setter=` for the final shape. +### Runtime surface (review update, 2026-07-22) + +The `get()` / `update(value)` / `put(value)` method trio is renamed and split so +that both **access mode** and **whether a call touches the device** are legible +from the member set: + +| Member | Kind | AttrR | AttrW | AttrRW | Device IO? | +|---|---|---|---|---|---| +| `.readback` | property (sync) | ✓ | — | ✓ | no (cached) | +| `.setpoint` | property (sync) | — | ✓ | ✓ | no (cached) | +| `poll()` | async method | ✓ | — | ✓ | **yes** (getter) | +| `update(value)` | async method | ✓ | — | ✓ | no (cache push) | +| `set(value)` | async method | — | ✓ | ✓ | **yes** (setter) | + +- **`.readback` / `.setpoint` replace `.value`.** Two explicitly-named cached + properties instead of one whose meaning shifted per class. Each class exposes + only the ones it has (AttrR has no `.setpoint`, AttrW no `.readback`), so + access mode reads off the surface — and the pair mirrors bluesky / ophyd-async's + `Location(setpoint, readback)` exactly, so `AttrRW` maps 1:1 onto `locate()` + and the embedded connector's `get_value`/`get_setpoint`. Both are **read-only** + properties: writes are async (validate + `await` callbacks) and so cannot be + property setters. +- **`poll()` replaces the no-arg `update()`; `update_period` → `poll_period`.** + `poll()` does a live getter read, caches it, and **returns** the value (so an + on-demand read is `await attr.poll()`, mirroring ophyd's live `get_value()`); + `poll_period` (`ONCE` / float / `None`) is only the *schedule* the framework + calls it on. This deletes the `set_update_callback` / `bind_update_callback` + plumbing — the getter lives on the attr and `poll()` calls it. +- **`update(value)` is now purely a cache push** — a `value` or `Update[T]` from a + `@scan`/subscription — with no device IO and no `None` sentinel. +- **`set(value)` replaces `put()`** (the bluesky/ophyd verb): it caches + `.setpoint` immediately (decision 10a), then runs the setter; the setter's + `T | Update[T]` return feeds `.readback` via `update()`. The old + `sync_setpoint=` kwarg and `_call_sync_setpoint_callbacks` are gone. + +So `poll()`/`set()` touch the device; `.readback`/`.setpoint`/`update()` do not. + Replace `AttributeIO`/`AttributeIORef` with three focused, per-attribute IO base classes with abstract `update`/`send` methods, passed as a single `io=` constructor argument: From 54522d7581b526679c105f2bae8e2be2e4052101 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Thu, 23 Jul 2026 11:42:15 +0000 Subject: [PATCH 15/16] docs: fold @shihab-dls #402 replies into ADRs 0014 & 0019 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the two review threads Shihab answered on 2026-07-22: - ADR 0019 (@scan): confirmed @scan is a purely-internal periodic coroutine bound to no Attr and surfacing no Signal; @command, by contrast, creates an AttrW and IS exposed. Closes the last open question on 0019; folded into the mapping table + Resolved-in-review. - ADR 0014 (setpoint echo): set() caching .setpoint is an attribute-cache guarantee only. Records Shihab's CA-vs-PVA divergence — PVA posts the setpoint immediately (may later alarm), CA posts only after the update callback completes, so a long-running setter delays the CA-visible setpoint. CA/PVA ordering realignment noted as a transport follow-up, not gating this rework. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- .../decisions/0014-attribute-io-rw-rework.md | 18 ++++++++++++++++- .../0019-embedded-ophyd-async-connector.md | 20 +++++++++++-------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 551238154..92dfb4071 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -130,7 +130,10 @@ from the member set: - **`set(value)` replaces `put()`** (the bluesky/ophyd verb): it caches `.setpoint` immediately (decision 10a), then runs the setter; the setter's `T | Update[T]` return feeds `.readback` via `update()`. The old - `sync_setpoint=` kwarg and `_call_sync_setpoint_callbacks` are gone. + `sync_setpoint=` kwarg and `_call_sync_setpoint_callbacks` are gone. (Caching + `.setpoint` first is an *attribute-cache* guarantee; *when a remote client sees + it* is transport-dependent and differs between CA and PVA — see *Resolved in + review* below.) So `poll()`/`set()` touch the device; `.readback`/`.setpoint`/`update()` do not. @@ -321,6 +324,19 @@ a public method on `AttrW`/`ReadWriteIO` — exact shape is an open question. - **No `CallbackReadIO`/`CallbackWriteIO` in core.** The one-off callback case folds into the unified `attr` factory ([ADR 18](0018-attr-decorator-sugar.md)); the same decorator/factory covers the read-only and read/write cases. +- **Setpoint echo is an attribute-cache guarantee, not a transport one + (@Tom-Willemsen / @shihab-dls, #402):** `set()` caching `.setpoint` before it + runs the setter fixes the *framework*-level report that a setpoint PV didn't + reflect the just-written value, and is the sanctioned secop echo. Whether a + *remote client* sees that value immediately is transport-dependent, and the two + transports differ. **PVA** posts the setpoint as soon as it is written, then the + record may later go into alarm if the setter rejects it. **CA** posts the PV + update only *after* the update callback — where alarms are set — completes, so a + long-running setter delays the CA-visible setpoint until the send returns. This + means the `set()` semantics above are **not** a cross-transport "instantly + visible" guarantee. Realigning CA to PVA's post-before-send ordering (so GUIs + get immediate feedback on CA too) is a **transport-layer** follow-up, tracked + separately from this attribute-IO rework and not gating it. ## Open questions (awaiting input) diff --git a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md index 6bdd3ca87..92ba31c3f 100644 --- a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md +++ b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md @@ -108,7 +108,7 @@ Backend mappings (from #388 §5, grounded against the researched | `CommandBackend.execute`/`.signature` | `Command.__call__` / captured `Signature`, [ADR 15](0015-typed-commands.md) | | `SignalBackend.source` | e.g. `fastcs://.` | | child `Device` / `DeviceVector` | sub-`Controller` / `ControllerVector` | -| (not exposed) | `@scan` methods — server-side only, not surfaced to ophyd-async | +| (not exposed) | `@scan` methods — purely-internal periodic coroutines, bound to no `Attr`, not surfaced to ophyd-async (@shihab-dls, #402) | Datatype mapping: `Int`/`Float`/`Bool`/`String` → `int`/`float`/`bool`/`str`; `Waveform(array_dtype, shape)`/`Array1D` hint (per @@ -156,10 +156,14 @@ the enum class itself. Resolved in review (#402): - **Disconnect dropped:** reconnect is `Device.connect(force_reconnect=True)`; the only disconnect is `atexit`. No `Device.disconnect()` proposal (so #388 §8 item 8 / issue #401 is rewritten accordingly). - -## Open questions (awaiting input) - -1. Where does `@scan`-derived state that isn't exposed as a `Signal` go? All - attribute data already lives in `Attr` instances mapped to `Signal`s, so it - may be that `@scan` only drives updates and nothing extra needs surfacing — - needs confirming. *(awaiting @shihab-dls)* +- **`@scan` surfaces nothing; `@command` does (@shihab-dls, #402):** confirmed — + a `@scan`-decorated method is a **purely internal** coroutine run periodically; + it is *not* bound to an `Attr` and produces **no** `Signal`. All exposed state + already lives in `Attr` instances: a getter-based `AttrR` schedules its getter + as a scan-style task *and* is bound to the Attr (→ `Signal`), and a soft `AttrR` + fed by `@scan` via `update()` is likewise the exposed Signal — so `@scan` needs + nothing extra surfaced. A `@command` method **is** different: it creates an + `AttrW` and **is** exposed (the `CommandBackend` row above). Both `@scan` and + getter/update coroutines are collected onto the running loop in + `create_api_and_tasks()`; the connector schedules `@scan` coroutines as internal + tasks but never maps them to Signals. From ef0c3c6677b2e612b71f5387d0698abf08d3fa28 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Thu, 23 Jul 2026 14:39:06 +0000 Subject: [PATCH 16/16] docs: rewrite ADRs 0013-0019 with resolved-question tangle removed Fold the #402 review updates, "Resolved in review" statement dumps, and "Open questions" sections into one clean shape per ADR (Context / Decision / Consequences / Questions resolved in review). Make all seven consistent with the deleted io= and DataType: code examples are now all getter/setter + *Meta, and the runtime surface (.readback/.setpoint, poll()/poll_period, set()) is used uniformly across 0014/0016/0018/0019 instead of the old get()/put()/update_period/Waveform names. Preserves the @shihab-dls #402 replies (CA-vs-PVA setpoint visibility in 0014; @scan-vs-@command exposure in 0019), rewoven into the clean structure. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX --- ...-procedural-split-and-controller-filler.md | 93 +++-- .../decisions/0014-attribute-io-rw-rework.md | 369 ++++++++---------- .../decisions/0015-typed-commands.md | 46 +-- ...-cache-timestamps-and-controller-runner.md | 90 +++-- .../decisions/0017-naming-pass.md | 122 +++--- .../decisions/0018-attr-decorator-sugar.md | 151 +++---- .../0019-embedded-ophyd-async-connector.md | 90 +++-- 7 files changed, 492 insertions(+), 469 deletions(-) diff --git a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md index 599cfe624..1b802a5be 100644 --- a/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md +++ b/docs/explanations/decisions/0013-declarative-procedural-split-and-controller-filler.md @@ -52,60 +52,69 @@ introspecting drivers use. ## Decision Adopt a single declarative mechanism, matching ophyd-async: **class body = -bare type hints only; instance scope = procedural construction.** Concretely: +declarations + decorated behaviour; instance scope = construction with data.** +Concretely: -- Remove class-scope `Attribute` **instances** entirely. `AttrRW(Float(), - io=...)` may no longer be assigned directly in a class body. +- Remove class-scope `Attribute` **instances** entirely. `AttrRW(getter=..., + setter=...)` may no longer be assigned directly in a class body. - Remove the deepcopy half of `_bind_attrs`. Method binding for `@command`/ - `@scan` (the `UnboundCommand`/`UnboundScan` machinery) is unaffected and - stays, since it does not require deepcopy — see decision 14 (`@attr` - decorator sugar). + `@scan` — and the new `@attr`/`@x.setter` sugar + ([ADR 18](0018-attr-decorator-sugar.md)) — is unaffected and stays, since it + does not require deepcopy: it binds a method to `self` at construction time + via the `UnboundCommand`/`UnboundScan` machinery rather than deepcopying a + prototype. - Remove `HintedAttribute` and `_validate_type_hints`/`_validate_hinted_*` as a *separate* validation-only pass. Their job — "this hinted child must exist with the right type after initialisation" — is subsumed into the new `ControllerFiller`. - Introduce `ControllerFiller`, a direct structural port of ophyd-async's `DeviceFiller`. It scans class-body type hints (`AttrR/W/RW[T]`, - `Command[P, T]` — see [ADR 15](0015-typed-commands.md), nested `Controller` - / `ControllerVector[T]`), creates children **unfilled**, and tracks - filled/unfilled state per child. `check_filled(source)` raises, listing by - name, anything a `Controller`'s `initialise()` promised via a hint but did - not provision. + `Command[P, T]` — see [ADR 15](0015-typed-commands.md) — and nested + `Controller` / `ControllerVector[T]`), creates children **unfilled**, and + tracks filled/unfilled state per child. `check_filled(source)` raises, + listing by name, anything a `Controller`'s `initialise()` promised via a hint + but did not provision. - `ControllerFiller` yields `(child, extras)` for each created child — the `extras` being anything else found in an `Annotated[...]` hint — so that protocol libraries (a future SCPI package, for example) can define their own extras vocabulary the same way ophyd-async's `PvSuffix`/`TangoPolling` do. Core FastCS defines **no** extras vocabulary for 1.0 (decision 3 of #388). -- When a child is filled from an `Annotated[Attr[T], extras]` hint, the filler - **runtime-validates** the metadata the extras carries (`FloatMeta`, or a +- When a child is filled from an `Annotated[AttrRW[T], extras]` hint, the filler + **runtime-validates** the metadata the extras carries (a `FloatMeta`, or a protocol object's `.meta` such as `SCPIParam(...).meta`) against the datatype `T` — e.g. `precision` supplied for a `str` raises. This is the runtime counterpart to the static `Unpack[FloatMeta]` check on the procedural `Attr*` constructors (see [ADR 14](0014-attribute-io-rw-rework.md)). -- The refined rule from decision 14 of #388: *class body = declarations + - decorated behaviour; instance scope = construction with data.* This keeps - `@command`/`@scan`, and the new `@attr`/`@x.setter` sugar, as class-body - citizens, since none of them require per-instance deepcopy — they bind a - method to `self` at construction time instead. -Two patterns follow, and the class body distinguishes them: +Two patterns follow, and the class body distinguishes them. **Procedural, no hint** — the value is fully constructed in `__init__`, so it -needs no class-body declaration at all (the temperature controller): +needs no class-body declaration at all. Per-attribute IO is a `getter`/`setter` +pair of callables ([ADR 14](0014-attribute-io-rw-rework.md)); the datatype is +inferred from the getter's return annotation: ```python class TemperatureRampController(Controller): def __init__(self, index: int, conn: IPConnection) -> None: super().__init__() suffix = f"{index:02d}" - self.start = AttrRW(Int(), io=TempIO(conn, "S", suffix)) + + async def get_start() -> int: + return int(await conn.send_query(f"S{suffix}?\r\n")) + + async def set_start(value: int) -> None: + await conn.send_command(f"S{suffix}={value}\r\n") + + # datatype int is inferred from get_start's return annotation + self.start = AttrRW(getter=get_start, setter=set_start, poll_period=0.2) ``` **Declarative hint + filler** — the value is *promised* by a hint; the `ControllerFiller` (run from `Controller.__init__`) creates it as an **unfilled** `Attribute` so it **exists as soon as `__init__` returns**, and -`initialise()` later *fills* it (provisions `io` + metadata) by introspection: +`initialise()` later *fills* it (provisions the getter/setter + metadata) by +introspection: ```python class OdinDetector(Controller): @@ -113,10 +122,10 @@ class OdinDetector(Controller): # self.frames EXISTS after __init__, before initialise() async def initialise(self) -> None: - # introspection FILLS the already-created hinted attrs (io + metadata), - # and may add wholly-undeclared dynamic attrs (which carry no hint) - for name, meta in await self._query_parameter_tree(): - self.filler.fill_attribute(name, ...) # validates meta vs datatype + # introspection FILLS the already-created hinted attrs (getter/setter + + # metadata), and may add wholly-undeclared dynamic attrs (no hint) + for name, spec in await self._query_parameter_tree(): + self.filler.fill_attribute(name, spec) # validates meta vs datatype self.filler.check_filled() ``` @@ -150,22 +159,24 @@ mirrors that `DeviceFiller` path directly. - `ControllerFiller` becomes a new stable, documented surface — see [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) for how it interacts with the stable `ControllerAPI` surface consumed by the - embedded ophyd-async connector. + embedded ophyd-async connector ([ADR 19](0019-embedded-ophyd-async-connector.md)). -## Resolved in review (#402) +## Questions resolved in review (#402) -1. **`ControllerFiller` must support "no hints at all"** — build the whole - attribute tree from introspected data (`fastcs-PandABlocks`, `fastcs-secop`). -2. **`fastcs-catio`'s runtime `type(...)` class-building is *not* supported.** - A bare `Controller` instead allows attributes to be added onto it from the +1. **Must `ControllerFiller` support "no hints at all"?** Yes — it must build + the whole attribute tree from introspected data with nothing declared in the + class body (`fastcs-PandABlocks`, `fastcs-secop`), exactly as + `DeviceFiller` does for a `PviConnector`. +2. **Is `fastcs-catio`'s runtime `type(...)` class-building supported?** No. A + bare `Controller` instead allows attributes to be added onto it from the outside — which is exactly what the fillers do — so catio moves to instance-level dynamic attribute construction. -3. **No sibling-ordering mechanism.** The rule "any hint-referenced Attribute - must exist by the end of `__init__`" makes `initialise()` parallelisable; - sibling dependencies are an `initialise()` implementation detail (call - `super().initialise()` first). -4. **`Optional[X]` hints are supported** — `check_filled` treats an optional - hint as not-required. -5. **Follow `DeviceFiller`'s structure, not its names.** Architectural - similarity matters; method names match only where FastCS's vocabulary - (`Attribute` vs `Signal`) makes them fit. +3. **Is there a sibling-ordering mechanism?** No. The rule "any hint-referenced + Attribute must exist by the end of `__init__`" makes `initialise()` + parallelisable; sibling dependencies are an `initialise()` implementation + detail (call `super().initialise()` first). +4. **Are `Optional[X]` hints supported?** Yes — `check_filled` treats an + optional hint as not-required. +5. **Do we follow `DeviceFiller`'s names?** Follow its *structure*, not its + names. Architectural similarity matters; method names match only where + FastCS's vocabulary (`Attribute` vs `Signal`) makes them fit. diff --git a/docs/explanations/decisions/0014-attribute-io-rw-rework.md b/docs/explanations/decisions/0014-attribute-io-rw-rework.md index 92dfb4071..0b939687d 100644 --- a/docs/explanations/decisions/0014-attribute-io-rw-rework.md +++ b/docs/explanations/decisions/0014-attribute-io-rw-rework.md @@ -1,9 +1,10 @@ -# 14. AttributeIO R/W/RW Rework and Removal of AttributeIORef +# 14. Per-Attribute IO as getter/setter Callables Date: 2026-07-20 **Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), -[ADR 9](0009-handler-to-attribute-io-pattern.md), [ADR 12](0012-attribute-io-naming-convention.md) +[ADR 9](0009-handler-to-attribute-io-pattern.md), [ADR 12](0012-attribute-io-naming-convention.md), +[ADR 18](0018-attr-decorator-sugar.md) ## Status @@ -38,8 +39,8 @@ The dispatch-by-type registry has real costs in our downstream drivers: - `fastcs-catio` registers **three** separate `AttributeIO`/`AttributeIORef` pairs on one `Controller` (`ios=[poll_io, symbol_io, coe_io]`) purely so each attribute's `io_ref` type can select the right one at - `_connect_attribute_ios` time — indirection that a direct `io=` argument - removes outright. + `_connect_attribute_ios` time — indirection that a direct per-attribute + callable removes outright. - `fastcs-secop` needs a private escape hatch, `attr._call_sync_setpoint_callbacks`, to push setpoint echoes from its `send()` implementation, because the current `AttributeIO.send` signature @@ -51,57 +52,79 @@ The dispatch-by-type registry has real costs in our downstream drivers: ## Decision -> **Review update (#402, 2026-07-22): `io=` objects replaced by `getter`/`setter` callables.** -> The `ReadIO`/`WriteIO`/`ReadWriteIO` hierarchy and the `io=` argument described -> below are **superseded.** Per-attribute IO is supplied as plain callables on the -> constructors, which *is* the procedural spelling of the `@attr` decorator -> ([ADR 18](0018-attr-decorator-sugar.md)): -> -> - `AttrR(getter=g)`, `AttrW(setter=s)`, `AttrRW(getter=g, setter=s)`. Access mode -> is enforced by **which parameters exist** (an `AttrR` has no `setter`), so the -> three-class IO hierarchy and its abstract-method enforcement are dropped -> entirely — and `getter=` on a read-only attr is honest where `io=` was a false -> friend. -> - **The getter returns the value; the framework applies it** — `getter() -> T | -> Update[T]` — instead of the old imperative `io.update(attr)`. Imperative / -> multi-attribute periodic logic stays with `@scan`, which is *why* per-attribute -> IO shrinks to "one value in / out". The **setter** returns `None | T | -> Update[T]`: `None` = fire-and-forget (readback catches up on the next poll / the -> setpoint cache); a returned value is the device's *accepted* value (clamp/echo) -> and updates the readback + `AttrW` setpoint cache immediately — the sanctioned -> replacement for `fastcs-secop`'s private `_call_sync_setpoint_callbacks`. -> - `Update[T]` = `value: T`, `timestamp: float | None` (epoch seconds; `None` ⇒ -> framework stamps receive-time), `severity: Severity = OK` (the decision-10b -> severity enum); used for both the getter return and a value-returning setter — -> this is how device-native timestamps/severity reach `attr.update()`. -> - **Datatype is optional when a getter/setter is given** — inferred from the -> getter's return annotation (or the setter's param), unwrapping `Update[T]` to -> `T`, so `AttrR(getter=g)` yields `AttrR[float]` with no restated type (parity -> with `@attr`). Only the bare python type is optional; `precision`/`units`/… stay -> explicit kwargs, and the per-datatype `Unpack[*Meta]` static check keys off the -> inferred return type. Not inferable (`-> Any`, unannotated lambda) ⇒ the -> positional datatype is required (fail-fast at construction). -> - `update_period` is a read-side kwarg: `ONCE` = read once at connect (the default -> when a getter is given); a float = poll at that rate; `None` = **on-demand only** -> (read when a client asks, never auto-polled). **No getter** = soft, value pushed -> via `attr.update()` from a `@scan`/callback. -> - Soft is now simply the *absence* of getter/setter (`AttrRW(float)` self-wires -> setpoint→readback as before); the `io=None` sentinel is gone. -> - The declarative/filler path lowers to the **same** getter/setter (a -> `SCPIController`'s filler builds the callables from `SCPIParam`); getter/setter -> are where the old `_connect_attribute_ios` wiring now lives, so transports and -> the embedded connector are unaffected. -> -> `attr` is a **decorator only** (`@attr` / `@attr(precision=3)` + `@my_attr.setter`); -> there is no free-function `attr()` factory — the procedural spelling is `AttrR`/ -> `AttrRW` directly. The `io=` prose below is kept for the `AttributeIORef`→callable -> migration context; read `getter=`/`setter=` for the final shape. - -### Runtime surface (review update, 2026-07-22) - -The `get()` / `update(value)` / `put(value)` method trio is renamed and split so -that both **access mode** and **whether a call touches the device** are legible -from the member set: +Delete `AttributeIO` and `AttributeIORef` and their whole dispatch machinery. +Per-attribute IO is supplied as plain **`getter`/`setter` callables** on the +`Attr*` constructors — which *is* the procedural spelling of the `@attr` +decorator ([ADR 18](0018-attr-decorator-sugar.md)): + +- `AttrR(getter=g)`, `AttrW(setter=s)`, `AttrRW(getter=g, setter=s)`. Access + mode is enforced by **which parameters exist** (an `AttrR` has no `setter`), + so there is no IO class hierarchy and no abstract-method enforcement to + carry — and `getter=` on a read-only attr is honest where `io=` was a false + friend. +- **The getter returns the value; the framework applies it** — + `getter() -> T | Update[T]` — instead of the old imperative `io.update(attr)`. + Imperative / multi-attribute periodic logic stays with `@scan`, which is + *why* per-attribute IO shrinks to "one value in / out". +- The **setter** returns `None | T | Update[T]`: `None` = fire-and-forget + (readback catches up on the next poll / the setpoint cache); a returned value + is the device's *accepted* value (a clamp or echo) and updates the readback + + the `AttrW` setpoint cache immediately — the sanctioned replacement for + `fastcs-secop`'s private `_call_sync_setpoint_callbacks`. +- `Update[T]` carries `value: T`, `timestamp: float | None` (epoch seconds; + `None` ⇒ framework stamps receive-time), and `severity: Severity = OK` (the + decision-10b severity enum, see + [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md)). It is + used for both the getter return and a value-returning setter — this is how + device-native timestamps/severity reach `attr.update()`. +- **Datatype is optional when a getter/setter is given** — inferred from the + getter's return annotation (or the setter's parameter), unwrapping `Update[T]` + to `T`, so `AttrR(getter=g)` yields `AttrR[float]` with no restated type + (parity with `@attr`). Only the bare python type is optional; + `precision`/`units`/… stay explicit kwargs, and the per-datatype + `Unpack[*Meta]` static check keys off the inferred return type. Not inferable + (`-> Any`, an unannotated lambda) ⇒ the positional datatype is required + (fail-fast at construction). +- `poll_period` is a read-side kwarg: `ONCE` = read once at connect (the + default when a getter is given); a float = poll at that rate; `None` = + **on-demand only** (read when a client asks, never auto-polled). **No + getter** = soft, value pushed via `attr.update()` from a `@scan`/callback. +- Soft is now simply the *absence* of a getter/setter (`AttrRW(float)` + self-wires setpoint→readback as before, the analogue of ophyd-async's + `soft_signal_rw`); the old `io=None` sentinel is gone. +- The declarative/filler path lowers to the **same** getter/setter (a + `SCPIController`'s filler builds the callables from a `SCPIParam`). getter and + setter are where the old `_connect_attribute_ios` wiring now lives, so + transports and the embedded connector are unaffected. + +`attr` is a **decorator only** (`@attr` / `@attr(precision=3)` + +`@voltage.setter`, [ADR 18](0018-attr-decorator-sugar.md)); there is no +free-function `attr()` factory — the procedural spelling is `AttrR`/`AttrRW` +directly. + +```python +class TemperatureRampController(Controller): + def __init__(self, index: int, conn: IPConnection) -> None: + super().__init__() + name = f"R{index:02d}" + + async def get_ramp_rate() -> float: + return float(await conn.send_query(f"{name}?\r\n")) + + async def set_ramp_rate(value: float) -> None: + await conn.send_command(f"{name}={value}\r\n") + + # datatype float inferred from get_ramp_rate's return annotation + self.ramp_rate = AttrRW( + getter=get_ramp_rate, setter=set_ramp_rate, units="deg", poll_period=0.2 + ) +``` + +### Runtime surface + +The old `get()` / `update(value)` / `put(value)` method trio is renamed and +split so that both **access mode** and **whether a call touches the device** +are legible from the member set: | Member | Kind | AttrR | AttrW | AttrRW | Device IO? | |---|---|---|---|---|---| @@ -113,91 +136,41 @@ from the member set: - **`.readback` / `.setpoint` replace `.value`.** Two explicitly-named cached properties instead of one whose meaning shifted per class. Each class exposes - only the ones it has (AttrR has no `.setpoint`, AttrW no `.readback`), so - access mode reads off the surface — and the pair mirrors bluesky / ophyd-async's - `Location(setpoint, readback)` exactly, so `AttrRW` maps 1:1 onto `locate()` - and the embedded connector's `get_value`/`get_setpoint`. Both are **read-only** - properties: writes are async (validate + `await` callbacks) and so cannot be - property setters. + only the ones it has (`AttrR` has no `.setpoint`, `AttrW` no `.readback`), so + access mode reads off the surface — and the pair mirrors bluesky / + ophyd-async's `Location(setpoint, readback)` exactly, so `AttrRW` maps 1:1 + onto `locate()` and the embedded connector's `get_value`/`get_setpoint`. Both + are **read-only** properties: writes are async (validate + `await` callbacks) + and so cannot be property setters. - **`poll()` replaces the no-arg `update()`; `update_period` → `poll_period`.** `poll()` does a live getter read, caches it, and **returns** the value (so an on-demand read is `await attr.poll()`, mirroring ophyd's live `get_value()`); `poll_period` (`ONCE` / float / `None`) is only the *schedule* the framework calls it on. This deletes the `set_update_callback` / `bind_update_callback` plumbing — the getter lives on the attr and `poll()` calls it. -- **`update(value)` is now purely a cache push** — a `value` or `Update[T]` from a - `@scan`/subscription — with no device IO and no `None` sentinel. +- **`update(value)` is now purely a cache push** — a `value` or `Update[T]` + from a `@scan`/subscription — with no device IO and no `None` sentinel. - **`set(value)` replaces `put()`** (the bluesky/ophyd verb): it caches `.setpoint` immediately (decision 10a), then runs the setter; the setter's `T | Update[T]` return feeds `.readback` via `update()`. The old - `sync_setpoint=` kwarg and `_call_sync_setpoint_callbacks` are gone. (Caching - `.setpoint` first is an *attribute-cache* guarantee; *when a remote client sees - it* is transport-dependent and differs between CA and PVA — see *Resolved in - review* below.) - -So `poll()`/`set()` touch the device; `.readback`/`.setpoint`/`update()` do not. + `sync_setpoint=` kwarg and `_call_sync_setpoint_callbacks` are gone. Caching + `.setpoint` first is an *attribute-cache* guarantee only; *when a remote + client sees it* is transport-dependent and differs between CA and PVA — see + the Questions resolved below. -Replace `AttributeIO`/`AttributeIORef` with three focused, per-attribute IO -base classes with abstract `update`/`send` methods, passed as a single `io=` -constructor argument: - -```python -class ReadIO(Generic[DType_T], ABC): - def __init__(self, update_period: float | None = None): ... - - @abstractmethod - async def update(self, attr: AttrR[DType_T]) -> None: ... - - -class WriteIO(Generic[DType_T], ABC): - @abstractmethod - async def send(self, attr: AttrW[DType_T], value: DType_T) -> None: ... - - -class ReadWriteIO(ReadIO[DType_T], WriteIO[DType_T], ABC): ... -``` - -(Working names per #388; exact naming — `ReadIO`/`WriteIO`/`ReadWriteIO` vs. -`AttrRIO`/`AttrWIO`/`AttrRWIO` — is an open question below and in -[ADR 17](0017-naming-pass.md).) Concrete IO classes are **dataclasses**, so -per-attribute fields (register name, command string, `update_period`) are -declared without a boilerplate `__init__`. - -- `AttrR(dt, io: ReadIO[DType_T] | None)`, `AttrW(dt, io: WriteIO[DType_T] | - None)`, `AttrRW(dt, io: ReadWriteIO[DType_T] | None)`. Passing a - read-only IO to an `AttrRW` is a **static** type error, not a runtime - `_validate_io` check — the abstract methods force a subclass to implement - the right surface for the `Attr` flavour it is attached to. -- `update_period` moves onto `ReadIO` — it describes the IO's polling - behaviour, not a property of the attribute. `Controller.create_api_and_tasks` - schedules from `attr.io.update_period` instead of pattern-matching on - `AttributeIORef` (`control_system.py`/`controller.py`'s - `case AttrR(_io_ref=AttributeIORef(update_period=update_period))` becomes a - direct attribute access). -- **Delete:** `AttributeIORef`, the `ios=` constructor kwarg on - `BaseController`/`Controller`/`ControllerVector`, `_validate_io`, - `_connect_attribute_ios`, `_attribute_ref_io_map`, `__init_subclass__`'s - generic-arg sniffing in `AttributeIO`, and the second TypeVar — - `Attribute[DType_T, AttributeIORefT]` collapses to `Attribute[DType_T]`, - making `AttrRW[float]` structurally isomorphic to ophyd-async's - `SignalRW[float]`. -- `io=None` keeps today's soft-attribute behaviour: `AttrRW` self-wires - setpoint→readback via `_internal_update`, and the sync-setpoint machinery - is unaffected. This remains the analogue of ophyd-async's - `soft_signal_rw`. -- The one-off "no subclass needed" case is served by the unified `attr` - factory (see [ADR 18](0018-attr-decorator-sugar.md)) — `self.x = - attr(getter=cb)` / `@attr` — **not** by separate `CallbackReadIO`/ - `CallbackWriteIO` classes. The same `attr` covers the read-only and - read/write callback cases, so the two adapters are not shipped. +So `poll()`/`set()` touch the device; `.readback`/`.setpoint`/`update()` do +not. `Attribute` also loses its second generic parameter — +`Attribute[DType_T, AttributeIORefT]` collapses to `Attribute[DType_T]`, making +`AttrRW[float]` structurally isomorphic to ophyd-async's `SignalRW[float]`. ### Datatype metadata: the `*Meta` TypedDicts -`DataType` classes are gone (ADR 15/17). The metadata they carried (precision, -units, nested limits, …) moves to a per-datatype `TypedDict` — `FloatMeta`, -`IntMeta`, `StrMeta`, `BoolMeta`, `EnumMeta`, `Array1DMeta`, `TableMeta` — and -**the resolved metadata is stored on the `Attribute` itself** (`attr.meta`), -not on a separate datatype object. Every transport/connector that read +`DataType` classes are gone ([ADR 15](0015-typed-commands.md) / +[ADR 17](0017-naming-pass.md)). The metadata they carried (precision, units, +nested limits, …) moves to a per-datatype `TypedDict` — `FloatMeta`, `IntMeta`, +`StrMeta`, `BoolMeta`, `EnumMeta`, `Array1DMeta`, `TableMeta` — and **the +resolved metadata is stored on the `Attribute` itself** (`attr.meta`), not on a +separate datatype object. Every transport/connector that read `attr.datatype.precision`/`.units`/`.limits`/`.choices` now reads `attr.meta` (enum `choices` come from the python type; `EnumMeta` is display-only). @@ -208,12 +181,16 @@ Two spellings, two validation layers: ```python # conceptually, one overload per datatype: - def AttrRW(dtype: type[float], *, io=..., **kwargs: Unpack[FloatMeta]) -> AttrRW[float]: ... + def AttrRW(dtype: type[float], *, getter=..., setter=..., + **kwargs: Unpack[FloatMeta]) -> AttrRW[float]: ... - self.temperature = AttrRW(float, precision=3, units="deg", io=TempIO(...)) + self.temperature = AttrRW(float, precision=3, units="deg", setter=apply_temp) # AttrRW(str, precision=3) is a static type error ``` + (The `dtype` positional is only needed when it cannot be inferred from a + getter/setter annotation, as above.) + - **Declarative (runtime-checked by the filler):** `Annotated[AttrRW[float], FloatMeta(precision=3)]` (rare) or `Annotated[AttrRW[float], SCPIParam("P", precision=3)]` (common). Neither @@ -250,14 +227,14 @@ of Python object. `SCPIParam` is a sibling of ophyd-async's FastCS (decision 3: core defines no extras vocabulary for 1.0) — it lives in a protocol layer; the demo package ships an example `SCPIController` + `SCPIParam` to show how a third party builds one on the filler's -`(child, extras)` mechanism. +`(child, extras)` mechanism. The `*Meta` module location is deferred to the +public-API-namespace decision (#406); land it provisionally until then. -The `*Meta` module location is deferred to the public-API-namespace decision -(#406); land it provisionally until then. +### Migration -Migration is mechanical for the common case (an old `AttributeIO` subclass -absorbs its `AttributeIORef`'s fields into its own `__init__` and is -constructed once per attribute instead of once per controller): +Migration collapses an `AttributeIO`/`AttributeIORef` pair into two callables: +the old `update`/`send` method bodies become the `getter`/`setter`, constructed +once per attribute instead of once per controller. ```python # Before (ADR 9 shape) @@ -269,81 +246,75 @@ class TempIO(AttributeIO[float, TempIORef]): resp = await self._conn.send_query(f"{attr.io_ref.name}?\r\n") await attr.update(float(resp)) + async def send(self, attr: AttrW[float, TempIORef], value: float) -> None: + await self._conn.send_command(f"{attr.io_ref.name}={value}\r\n") + ramp_rate = AttrRW(Float(), io_ref=TempIORef(name="R")) # ... elsewhere: Controller(ios=[TempIO(conn)]) # After -class TempIO(ReadWriteIO[float]): - def __init__(self, conn: IPConnection, name: str, update_period=0.2): - super().__init__(update_period=update_period) - self._conn, self._name = conn, name +def temp_io(conn: IPConnection, name: str): + async def getter() -> float: + return float(await conn.send_query(f"{name}?\r\n")) - async def update(self, attr: AttrR[float]) -> None: - resp = await self._conn.send_query(f"{self._name}?\r\n") - await attr.update(float(resp)) + async def setter(value: float) -> None: + await conn.send_command(f"{name}={value}\r\n") - async def send(self, attr: AttrW[float], value: float) -> None: - await self._conn.send_command(f"{self._name}={value}\r\n") + return getter, setter -self.ramp_rate = AttrRW(Float(), io=TempIO(conn, "R")) +get_ramp, set_ramp = temp_io(conn, "R") +self.ramp_rate = AttrRW(getter=get_ramp, setter=set_ramp, poll_period=0.2) ``` -`fastcs-catio`'s three-IO-per-controller pattern becomes three IO -*instances*, one per relevant attribute, with no registry needed at all. -`fastcs-secop`'s private `_call_sync_setpoint_callbacks` call is replaced by -a public method on `AttrW`/`ReadWriteIO` — exact shape is an open question. +`fastcs-catio`'s three-IO-per-controller pattern becomes per-attribute +callables with no registry needed at all. `fastcs-secop`'s private +`_call_sync_setpoint_callbacks` call is replaced by a value-returning setter. ## Consequences -- Every driver that declared `AttributeIORef` subclasses must migrate them - into `AttributeIO.__init__` fields — see the affected §9 files in the - sub-issues of #388 (`attributes/`, `controllers/base_controller.py`, - `controllers/controller.py`) and the corresponding downstream repo issues. +- Every driver that declared `AttributeIO`/`AttributeIORef` subclasses migrates + their `update`/`send` bodies into `getter`/`setter` callables — see the + affected §9 files in the sub-issues of #388 (`attributes/`, + `controllers/base_controller.py`, `controllers/controller.py`) and the + corresponding downstream repo issues. The migration is mechanical. - `Attribute` loses its second generic parameter, simplifying every type hint in downstream code (`AttrR[float, MyRef]` → `AttrR[float]`). -- Access-mode compatibility between an `Attr` and its `io=` argument is - caught by the type checker instead of at runtime in `_validate_io` — - earlier feedback for driver authors, at the cost of losing the runtime - "no AttributeIO registered for this ref type" error message; a - misconfigured `io=None` on an attribute that needed IO now simply behaves - as a soft attribute rather than raising loudly. Whether this needs a - runtime check as well (e.g. in `post_initialise`) is an open question. -- [ADR 12](0012-attribute-io-naming-convention.md)'s guidance (subclass to - get a shorter driver-local name) still applies to the new `ReadIO`/ - `WriteIO`/`ReadWriteIO` names. - -## Resolved in review (#402) - -- **Runtime check: yes.** Alongside the static type error, a runtime check - (e.g. at `post_initialise`) catches a read-only IO on a write-capable `Attr` - for the dynamically-built `Any`-typed case (`fastcs-secop`, - `fastcs-PandABlocks`). -- **`attr.io` becomes a public, typed property** — the sanctioned way to - recover an attribute's IO-specific metadata from *outside* its `send`/ - `update` (replaces `fastcs-catio`'s `attribute.io_ref` access). -- **No `CallbackReadIO`/`CallbackWriteIO` in core.** The one-off callback case - folds into the unified `attr` factory ([ADR 18](0018-attr-decorator-sugar.md)); - the same decorator/factory covers the read-only and read/write cases. -- **Setpoint echo is an attribute-cache guarantee, not a transport one - (@Tom-Willemsen / @shihab-dls, #402):** `set()` caching `.setpoint` before it - runs the setter fixes the *framework*-level report that a setpoint PV didn't - reflect the just-written value, and is the sanctioned secop echo. Whether a - *remote client* sees that value immediately is transport-dependent, and the two - transports differ. **PVA** posts the setpoint as soon as it is written, then the - record may later go into alarm if the setter rejects it. **CA** posts the PV - update only *after* the update callback — where alarms are set — completes, so a - long-running setter delays the CA-visible setpoint until the send returns. This - means the `set()` semantics above are **not** a cross-transport "instantly - visible" guarantee. Realigning CA to PVA's post-before-send ordering (so GUIs - get immediate feedback on CA too) is a **transport-layer** follow-up, tracked - separately from this attribute-IO rework and not gating it. - -## Open questions (awaiting input) - -Both original open questions are closed by the 2026-07-22 getter/setter model: - -1. ~~Final IO class names (`ReadIO`/`WriteIO`/`ReadWriteIO` vs …)~~ — **moot**: the - IO class hierarchy is gone; IO is plain `getter`/`setter` callables. -2. ~~Public replacement for `fastcs-secop`'s `_call_sync_setpoint_callbacks`~~ — - **resolved**: a `setter` returning `T | Update[T]` *is* the sanctioned setpoint - echo (updates readback + `AttrW` setpoint cache). +- Access-mode compatibility is enforced by the parameter set — an `AttrR` has + no `setter`, so there is no `_validate_io` runtime check and no way to attach + a read-only IO to a write-capable attr statically. For the dynamically-built + `Any`-typed case (`fastcs-secop`, `fastcs-PandABlocks`) a runtime check at + `post_initialise` still catches a missing setter on a write-capable attr. +- The IO no longer has a place to hang per-attribute metadata that + `fastcs-catio` used to read off `attribute.io_ref`; `attr.meta` and the + attribute's own attributes replace that access. + +## Questions resolved in review (#402) + +1. **What replaces the `io=` object and the `ReadIO`/`WriteIO`/`ReadWriteIO` + hierarchy?** Plain `getter`/`setter` callables on the constructors. The IO + class hierarchy and its abstract-method enforcement are dropped entirely; + access mode is enforced by which parameters exist. +2. **Do we still need a runtime access-mode check?** Yes, in addition to the + static shape: a runtime check (e.g. at `post_initialise`) catches a missing + setter on a write-capable `Attr` for the dynamically-built `Any`-typed case. +3. **What is the public replacement for `fastcs-secop`'s + `_call_sync_setpoint_callbacks`?** A `setter` returning `T | Update[T]` *is* + the sanctioned setpoint echo — the returned value updates the readback and + the `AttrW` setpoint cache. +4. **Are there `CallbackReadIO`/`CallbackWriteIO` classes in core?** No. The + one-off callback case folds into `@attr` / `AttrR(getter=…)` + ([ADR 18](0018-attr-decorator-sugar.md)); the same spelling covers the + read-only and read/write cases. +5. **How is per-attribute IO metadata recovered from outside `getter`/`setter`?** + Through `attr.meta` and the attribute's own public members, replacing + `fastcs-catio`'s `attribute.io_ref` access. +6. **Is the setpoint echo a cross-transport "instantly visible" guarantee?** + (@Tom-Willemsen / @shihab-dls.) No — caching `.setpoint` before running the + setter is an *attribute-cache* guarantee (and the sanctioned secop echo); + whether a *remote client* sees it immediately is transport-dependent. **PVA** + posts the setpoint as soon as it is written, then the record may later go + into alarm if the setter rejects it. **CA** posts the PV update only *after* + the update callback (where alarms are set) completes, so a long-running + setter delays the CA-visible setpoint until the send returns. Realigning CA + to PVA's post-before-send ordering is a **transport-layer** follow-up, + tracked separately and not gating this rework. diff --git a/docs/explanations/decisions/0015-typed-commands.md b/docs/explanations/decisions/0015-typed-commands.md index f4d488da7..e59f9017d 100644 --- a/docs/explanations/decisions/0015-typed-commands.md +++ b/docs/explanations/decisions/0015-typed-commands.md @@ -106,25 +106,27 @@ need a `TYPE_CHECKING` stub trick and are prototyped separately in the spike `*Meta` mechanism as attributes (the `DataType` family is removed, ADR 17) — one shared validation/serialisation path, no command-specific duplicate. -## Resolved in review (#402) - -- **Validation shares the attribute path.** With `DataType` dropped (ADR 17), - command args/returns use python types + `*Meta` like attributes; no separate - mechanism, and complex-type serialisation (arrays, `Enum`, `Table`) is shared - with `Attribute`. -- **Args and returns are typed independently** — Args `[]` / `[DT…]`; - Returns `None` / `DT` (see Decision). Not all-or-nothing, but each is fully - known — there is no `Any` middle case. -- **No partial `Command[Any, Any]`.** @Tom-Willemsen confirmed on - [#402](https://github.com/DiamondLightSource/fastcs/pull/402#discussion_r3621453680) - that SECoP devices are discovered entirely from an over-the-wire `describe`: - you never statically know something is a command but not its signature — you - either know the full `Command[P, T]` or you know nothing at all and build the - whole controller at runtime. `P`/`T` are therefore completely known or the - whole structure is unknown, with no in-between. -- **EPICS skip-with-warning fires at IOC startup** — post controller - construction, when the fully populated controllers are handed to the - transports to serve. -- **Keyword-arg commands → spike [#403](https://github.com/DiamondLightSource/fastcs/issues/403)** - (interactive/Opus; needs a `TYPE_CHECKING` stub). Out of scope for core - typed-command work. +## Questions resolved in review (#402) + +1. **Do commands need their own type/serialisation mechanism?** No — they share + the attribute path. With `DataType` dropped ([ADR 17](0017-naming-pass.md)), + command args/returns use python types + `*Meta` like attributes, and + complex-type serialisation (arrays, `Enum`, `Table`) is shared with + `Attribute`. +2. **Are args and returns typed all-or-nothing?** No — independently: args `[]` + / `[DT…]`; returns `None` / `DT` (see Decision). Each is fully known — there + is no `Any` middle case. +3. **Is there a partial `Command[Any, Any]`?** No. @Tom-Willemsen confirmed on + [#402](https://github.com/DiamondLightSource/fastcs/pull/402#discussion_r3621453680) + that SECoP devices are discovered entirely from an over-the-wire `describe`: + you never statically know something is a command but not its signature — you + either know the full `Command[P, T]` or you know nothing at all and build the + whole controller at runtime. `P`/`T` are therefore completely known or the + whole structure is unknown, with no in-between. +4. **When does the EPICS skip-with-warning fire?** At IOC startup — post + controller construction, when the fully populated controllers are handed to + the transports to serve. +5. **What about keyword-argument commands?** Deferred to spike + [#403](https://github.com/DiamondLightSource/fastcs/issues/403) + (interactive/Opus; needs a `TYPE_CHECKING` stub). Out of scope for core + typed-command work. diff --git a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md index 5148edf4c..4f67b0528 100644 --- a/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md +++ b/docs/explanations/decisions/0016-setpoint-cache-timestamps-and-controller-runner.md @@ -2,7 +2,8 @@ Date: 2026-07-20 -**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), +[ADR 14](0014-attribute-io-rw-rework.md) ## Status @@ -14,12 +15,12 @@ Three related gaps block a clean embedded ophyd-async connector (see [ADR 19](0019-embedded-ophyd-async-connector.md)) and are useful to all transports independently of embedding: -1. **No cached setpoint.** `AttrW.put` (`src/fastcs/attributes/attr_w.py`) - applies a setpoint via `_on_put_callback` but does not retain it anywhere +1. **No cached setpoint.** The old `AttrW.put` (`src/fastcs/attributes/attr_w.py`) + applied a setpoint via `_on_put_callback` but did not retain it anywhere queryable. ophyd-async's `SignalBackend.get_setpoint()` — needed for `locate()` — has no FastCS equivalent to read from. -2. **No FastCS-native timestamps.** `AttrR.update` (`attr_r.py`) stamps - nothing; individual transports each do their own thing (EPICS records +2. **No FastCS-native timestamps.** The old `AttrR.update` (`attr_r.py`) stamped + nothing; individual transports each did their own thing (EPICS records get a timestamp from the record subsystem, Tango pushes are unstamped). An embedded connector currently has no choice but to stamp receive-time only, which is a real information loss versus what the underlying device @@ -40,24 +41,29 @@ to using — no reaching into `BaseController` internals. ## Decision -**Setpoint cache:** `AttrW` gains an internally-tracked last-applied -setpoint, exposed via a public `AttrW.get_setpoint()` method (mirroring -ophyd-async's `SignalBackend.get_setpoint()`), updated whenever `put` is -called, independent of whether the underlying `send` succeeds. This is available to all transports (not just the embedded -connector) as a "what did we last ask for" query distinct from `AttrR.get()` -("what did we last read back"). - -**Native timestamps (+ severity):** `AttrR.update` accepts an optional -timestamp (and, where meaningful, severity) alongside the value, defaulting -to current time if not supplied by the caller. The timestamp/severity pair -**follows bluesky's `Reading` shape but shares no code** with it, and severity -is a **FastCS enum using the same strings as EPICS** alarm severities. This is -FastCS-native, not EPICS-specific — Tango event pushes and other IO can supply a device-side -timestamp through the same path a `ReadIO.update` call already uses. The -embedded connector stamps receive-time only as an interim measure until this -lands, per decision 10 of #388 — this is 1.0 scope, not a follow-up. - -**ControllerRunner:** Extract the controller lifecycle currently inlined in +**Setpoint cache.** `AttrW`/`AttrRW` retain the last-applied setpoint, exposed +via the sync `.setpoint` property from [ADR 14](0014-attribute-io-rw-rework.md)'s +runtime surface (the FastCS analogue of ophyd-async's +`SignalBackend.get_setpoint()`). `set(value)` caches it immediately — before +the setter runs and independent of whether the setter succeeds — so `.setpoint` +is a "what did we last ask for" query, distinct from `.readback` ("what did we +last read back"). This is available to all transports, not just the embedded +connector. + +**Native timestamps (+ severity).** A value entering an `AttrR`/`AttrRW` may +carry a timestamp and severity by arriving as an `Update[T]` +([ADR 14](0014-attribute-io-rw-rework.md)) — from a getter's return, a +value-returning setter, or a `@scan`/subscription `update()` push — defaulting +to framework receive-time when the timestamp is `None`. The timestamp/severity +pair **follows bluesky's `Reading` shape but shares no code** with it, and +severity is a **FastCS enum using the same strings as EPICS** alarm severities. +This is FastCS-native, not EPICS-specific — Tango event pushes and other IO can +supply a device-side timestamp through the same `Update[T]` path a getter +already uses. The embedded connector stamps receive-time only as an interim +measure until this lands, per decision 10 of #388 — this is 1.0 scope, not a +follow-up. + +**ControllerRunner.** Extract the controller lifecycle currently inlined in `FastCS.serve` into a standalone `ControllerRunner` (or equivalent `Controller.serve()`/`Controller.stop()` API), independent of the transport-serving and interactive-shell logic that stays in `FastCS`/ @@ -77,19 +83,20 @@ case). **Idempotency is the caller's responsibility**, not the runner's — the embedded connector's `connect_real` may run more than once across reconnects (see [ADR 19](0019-embedded-ophyd-async-connector.md)). -This, together with `ControllerAPI` and the attribute/command runtime -methods (`AttrR.get`/`add_on_update_callback`, `AttrW.put` + cached -setpoint, `Attribute.datatype`/`access_mode`/`description`/`group`), becomes -the documented stable surface referenced by decision 13 of #388. +This, together with `ControllerAPI` and the attribute/command runtime surface +from [ADR 14](0014-attribute-io-rw-rework.md) (`.readback`/`poll()` + +update-callback registration, `set()` + the `.setpoint` cache, +`attr.meta`/`access_mode`/`description`/`group`), becomes the documented stable +surface referenced by decision 13 of #388. ## Consequences - `FastCS.serve` shrinks to transport orchestration; the controller lifecycle it currently inlines becomes independently testable and reusable without instantiating a `FastCS` object or any `Transport`. -- Every `ReadIO.update` implementation *may* supply a timestamp/severity, - but existing IO that does not is unaffected — defaults to current time, - severity unset. +- Every getter/setter *may* return an `Update[T]` to supply a + timestamp/severity, but a bare value is unaffected — it defaults to + framework receive-time, severity unset. - Transports gain access to a real setpoint distinct from the readback value; whether EPICS/Tango/REST/GraphQL surface this as new fields is transport-specific follow-up work, not part of this ADR. @@ -97,12 +104,17 @@ the documented stable surface referenced by decision 13 of #388. narrow surface instead of `BaseController` internals — see [ADR 19](0019-embedded-ophyd-async-connector.md). -## Resolved in review (#402) - -- **Setpoint accessor:** a `AttrW.get_setpoint()` method (mirrors - `SignalBackend.get_setpoint()`). -- **Timestamp/severity:** follow bluesky's `Reading` shape but **share no - code**; severity is a **FastCS enum using the same strings as EPICS**. -- **`ControllerRunner`:** a class with `start()`/`stop()` (context manager only - if it also suits `FastCS()`); **idempotency is the caller's responsibility**. -- **The runner owns the whole lifecycle, including reconnect.** +## Questions resolved in review (#402) + +1. **How is the cached setpoint exposed?** Via the `.setpoint` property from + [ADR 14](0014-attribute-io-rw-rework.md)'s runtime surface (the FastCS + analogue of `SignalBackend.get_setpoint()`), cached by `set()` before the + setter runs. +2. **What shape do timestamp/severity take?** They follow bluesky's `Reading` + shape but **share no code**; severity is a **FastCS enum using the same + strings as EPICS**, carried on `Update[T]`. +3. **What is the runner's shape?** A class with `start()`/`stop()` (context + manager only if it also suits `FastCS()`); **idempotency is the caller's + responsibility**. +4. **Who owns reconnect?** The runner owns the whole lifecycle, including + reconnect. diff --git a/docs/explanations/decisions/0017-naming-pass.md b/docs/explanations/decisions/0017-naming-pass.md index a781e002e..85af66a07 100644 --- a/docs/explanations/decisions/0017-naming-pass.md +++ b/docs/explanations/decisions/0017-naming-pass.md @@ -2,7 +2,8 @@ Date: 2026-07-20 -**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), +[ADR 14](0014-attribute-io-rw-rework.md), [ADR 15](0015-typed-commands.md) ## Status @@ -28,76 +29,81 @@ hints are now load-bearing (they are what `ControllerFiller` scans), so having ophyd-async-compatible hint spellings for array/table attributes becomes more valuable than it was when hints were validation-only. +This pass now lives on python types + `*Meta` typed dicts, not on `DataType` +classes: the `DataType` family is dropped ([ADR 14](0014-attribute-io-rw-rework.md), +[ADR 15](0015-typed-commands.md)), so these renames fold into the per-attribute +IO rework (issue #392) rather than landing as a separate late PR. The concrete +`*Meta` mechanism (per-datatype `TypedDict`s, the superset `Meta` for extras, +`attr.meta` storage, the `Unpack` overloads) is specified in +[ADR 14](0014-attribute-io-rw-rework.md); the module home for these public +names is decided in #406. + Since this is a pre-1.0 breaking-change window (per #388's framing — "while breaking pre-1.0"), this is the point to make these renames, not after 1.0 when they become a deprecation cycle. ## Decision -> **Review update (#402): `DataType` is dropped** (see -> [ADR 15](0015-typed-commands.md)). The renames below now live on python -> types + `*Meta` typed dicts, not on `DataType` classes, and this pass folds -> into the `AttributeIO` rework ([ADR 14](0014-attribute-io-rw-rework.md) / -> issue #392) rather than a separate late PR. `Array1D`/`Table` become *both* -> the hint and the runtime structure passed around as the datatype — there is -> no separate `Waveform`/`DataType` object to map to. -> -> The concrete `*Meta` mechanism (per-datatype `TypedDict`s, the superset -> `Meta` for extras, `attr.meta` storage on the attribute, `Unpack` overloads) -> is specified in [ADR 14](0014-attribute-io-rw-rework.md); the module home for -> these public names is decided in #406. - -1. **`prec` → `precision`.** Rename across `_Numeric`/`Float`/wherever - `prec` appears (transports, docs, snippets). No behaviour change. -2. **Limits alignment.** Align `min`/`max`/`min_alarm`/`max_alarm` naming - with event-model `Limits` naming. This ADR records the *intent* - (converge with bluesky event-model naming so alarm/control/display limits - read the same way in FastCS and ophyd-async docs); the exact target - shape (keep four flat fields renamed, or restructure into a `Limits`-like - object) is an open question for the prototype, since it interacts with - how `DataType.validate` currently accesses these fields directly as - dataclass attributes. -3. **`Array1D`/`Table` hint spellings.** Adopt `Array1D[np.int32]` and - `Table` as the FastCS *hint* spellings a `ControllerFiller`-scanned class - body uses, mapping internally to the existing `Waveform`/table `DataType` - runtime objects (constructed the same way as today via - `AttrRW(Waveform(np.int32, shape=(4,)), io=...)` in procedural code) — - the hint is sugar for `ControllerFiller`'s type-hint scan, not a - replacement for the runtime `DataType` classes, matching decision 7 of - #388 (`DataType` classes stay as the procedural/runtime value; hints are - what `ControllerFiller` reads). +1. **`prec` → `precision`.** Rename across the numeric metadata (`FloatMeta`, + transports, docs, snippets) wherever `prec` appears. `precision` stays an + `int` (decimal places). No behaviour change. + +2. **Limits alignment — nested, not flat.** Replace the flat + `min`/`max`/`min_alarm`/`max_alarm` fields with a nested `Limits` structure + aligned to the bluesky event-model, so alarm/control/display limits read the + same way in FastCS and ophyd-async docs. **All four categories** — control, + display, alarm, warning — are present and **all optional**, with inheritance: + + - supply none ⇒ all unbounded; + - Display but not Control ⇒ Control inherits Display (for a writeable attr); + - Alarm but not Warning ⇒ Warning inherits Alarm; + - both Alarm and Warning ⇒ assert Warning ⊆ Alarm; + - otherwise unspecified ⇒ unbounded. + +3. **`Array1D`/`Table` hint spellings, which are also the runtime structure.** + Adopt `Array1D[np.int32]` and `Table` as the FastCS *hint* spellings a + `ControllerFiller`-scanned class body uses. With `DataType` dropped, these + are **both** the hint and the runtime structure passed around as the + datatype — there is no separate `Waveform`/table `DataType` object to map to. + Procedural construction passes the same types plus `*Meta` (e.g. + `AttrRW(Array1D[np.int32], shape=(4,), getter=...)`), and shape/array + metadata rides on `Array1DMeta` exactly as `precision`/`units` ride on + `FloatMeta`. This is explicitly the smallest naming-pass scope agreed in #388 for 1.0. A `Prec`/`Units`/`Shape` `Annotated` extras vocabulary (letting a hint carry -precision/units/shape without a full `DataType` instance) is called out in -#388 as a **post-1.0** option enabled by, but not required by, the +precision/units/shape without a spec object) is called out in #388 as a +**post-1.0** option enabled by, but not required by, the [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) extras mechanism — not part of this ADR. ## Consequences - Every driver using `Float(prec=...)`, `.min`/`.max`/`.min_alarm`/ - `.max_alarm` needs a mechanical rename. This is a wide, shallow diff - across all downstream repos (`fastcs-eiger`, `fastcs-catio`, - `fastcs-secop`, `fastcs-PandABlocks` all use `Float`/numeric limits - somewhere) but not a structural one, unless the Limits restructuring - (open question 2) turns out to be more than a rename. + `.max_alarm` needs a rename to `precision` and the nested `Limits` + structure. This is a wide diff across all downstream repos (`fastcs-eiger`, + `fastcs-catio`, `fastcs-secop`, `fastcs-PandABlocks` all use numeric + limits somewhere); the flat→nested Limits change is structural, not purely a + rename. - Transports serving `precision`/limits metadata (EPICS record fields, - Tango attribute properties, REST/GraphQL schema) need their field-name - mapping updated to read from the renamed dataclass fields. -- `Array1D`/`Table` hint spellings only affect declarative (hinted) - attribute declarations; procedural construction with `Waveform(...)`/ - `Table(...)` DataType instances is unchanged. - -## Resolved in review (#402) - -1. **Limits are nested**, not four flat fields. -2. **All four categories (control/display/alarm/warning), all optional**, with - inheritance: supply none ⇒ all unbounded; Display but not Control ⇒ Control - inherits Display (for writeable); Alarm but not Warning ⇒ Warning inherits - Alarm; both ⇒ assert Warning ⊆ Alarm; otherwise unspecified ⇒ unbounded. -3. **`precision` stays an `int`** (decimal places). -4. **`Array1D` is both the hint and the runtime structure** — with `DataType` - dropped it falls out in the wash; there is no `Waveform` object to map to. -5. **Where it lands is the implementer's choice** — folds naturally into the - `AttributeIO`/DataType-drop PR (#392). + Tango attribute properties, REST/GraphQL schema) read these from `attr.meta` + ([ADR 14](0014-attribute-io-rw-rework.md)) and need their field-name mapping + updated to the renamed / nested fields. +- `Array1D`/`Table` become the single array/table representation for both + hinted and procedural attributes, so there is no hint-vs-runtime mapping + layer to keep in sync. + +## Questions resolved in review (#402) + +1. **Flat or nested limits?** Nested — a `Limits` structure, not four flat + fields. +2. **Which limit categories, and how do they combine?** All four + (control/display/alarm/warning), all optional, with the inheritance rules in + Decision point 2 (Control inherits Display, Warning inherits Alarm, assert + Warning ⊆ Alarm, otherwise unbounded). +3. **Is `precision` an int or a float?** An `int` (decimal places). +4. **Do `Array1D`/`Table` map onto a separate runtime `DataType`?** No — with + `DataType` dropped they *are* both the hint and the runtime structure; there + is no `Waveform` object to map to. +5. **Where does this land?** It folds naturally into the per-attribute IO / + `DataType`-drop PR (#392) — the implementer's choice, not a separate late PR. diff --git a/docs/explanations/decisions/0018-attr-decorator-sugar.md b/docs/explanations/decisions/0018-attr-decorator-sugar.md index 87a1924cc..923364a25 100644 --- a/docs/explanations/decisions/0018-attr-decorator-sugar.md +++ b/docs/explanations/decisions/0018-attr-decorator-sugar.md @@ -2,7 +2,9 @@ Date: 2026-07-20 -**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md), +[ADR 14](0014-attribute-io-rw-rework.md) ## Status @@ -23,43 +25,40 @@ def current(self) -> float: Under the [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) harsh declarative/procedural split (bare hints only in the class body; all -IO wiring procedural), the equivalent trivial case regresses to a method -plus a `CallbackReadIO` adapter plus explicit `__init__` wiring — strictly -more ceremony than PyTango for the simple case that most new users hit -first. This is exactly the kind of "false friend" gap #388 warns about: a -PyTango user evaluating FastCS should not find the *simple* case harder than -what they're moving away from. +IO wiring procedural), the equivalent trivial case would regress to a method +plus explicit `AttrR(getter=...)` wiring in `__init__` — strictly more +ceremony than PyTango for the simple case that most new users hit first. This +is exactly the kind of "false friend" gap #388 warns about: a PyTango user +evaluating FastCS should not find the *simple* case harder than what they're +moving away from. FastCS already has precedent for binding class-body decorated methods to per-instance callables without any deepcopy hazard: `@command`/`@scan` (`src/fastcs/methods/command.py`, `scan.py`) use `UnboundCommand`/ `UnboundScan`, which wrap an unbound function and `.bind(controller)` a -fresh `Command`/`Scan` object per instance at `_bind_attrs` time. Because +fresh `Command`/`Scan` object per instance at construction time. Because these are fresh objects constructed per-instance (not deepcopied prototypes), they carry none of the aliasing hazard that class-scope -`Attribute` *instances* had — which is why [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) +`Attribute` *instances* had — which is why +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md) removes the latter but keeps `@command`/`@scan`. ## Decision -> **Review update (#402): the decorator is `@attr`, mirroring `@property`.** -> `@attr` on the getter, `@voltage.setter` on the writer (not `@attr_r`/ -> `@attr_rw`/`.send`). It unifies with the callback IO from -> [ADR 14](0014-attribute-io-rw-rework.md): `self.x = attr(getter=…, setter=…)` -> is the `__init__` spelling of the same thing. `AttrW`-only (write with no -> paired getter) is rare, so it is written longhand rather than given its own -> decorator. - -Add `@attr` as pure sugar over `AttrR`/`AttrRW` plus a generated -callback-based `io=`, built on the same `Unbound*`-style bind machinery as -`@command`/`@scan` — fresh objects per instance, no prototype/deepcopy -hazard, consistent with keeping this a class-body citizen under -[ADR 13](0013-declarative-procedural-split-and-controller-filler.md). +Add `@attr` as pure sugar over `AttrR`/`AttrRW` plus generated getter/setter +callables ([ADR 14](0014-attribute-io-rw-rework.md)), built on the same +`Unbound*`-style bind machinery as `@command`/`@scan` — fresh objects per +instance, no prototype/deepcopy hazard, consistent with keeping this a +class-body citizen under +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md). The +decorator mirrors `@property`: `@attr` on the getter, `@voltage.setter` on the +writer. ```python class PowerSupply(Controller): - @attr(units="V", update_period=0.5) # dtype inferred from -> float + @attr(units="V", poll_period=0.5) # datatype inferred from -> float async def voltage(self) -> float: + """Output voltage.""" return await self._conn.query("V?") @voltage.setter @@ -68,64 +67,76 @@ class PowerSupply(Controller): ``` - The datatype is inferred from the return type annotation of the getter - (`-> float` → `Float()`), matching how `DataType` mapping already works - elsewhere (`numpy_to_fastcs_datatype`), rather than requiring a `dtype=` - keyword the way PyTango does — decision 14 of #388 explicitly calls this - out as *better* than PyTango's `dtype=` kwarg since it's one real - annotation, checked statically. -- `@attr` comes in two forms: bare `@attr` and parameterised `@attr(precision=3, - units="V", update_period=0.5)`; the keyword arguments map onto the same `*Meta` - fields and the `getter`/`setter` + `update_period` constructor arguments of - `AttrR`/`AttrRW` ([ADR 14](0014-attribute-io-rw-rework.md)) — sugar over that - mechanism, not a parallel one. There is **no** free-function `attr()` factory: - the procedural spelling is `AttrR(getter=…)` / `AttrRW(getter=…, setter=…)` - directly. + (`-> float` → a `float` attribute), matching how the datatype is inferred + from a getter's annotation on the procedural + `AttrR(getter=…)`/`AttrRW(getter=…, setter=…)` form + ([ADR 14](0014-attribute-io-rw-rework.md)), rather than requiring a `dtype=` + keyword the way PyTango does — decision 14 of #388 explicitly calls this out + as *better* than PyTango's `dtype=` kwarg since it's one real annotation, + checked statically. +- `@attr` comes in two forms: bare `@attr` and parameterised + `@attr(precision=3, units="V", poll_period=0.5)`; the keyword arguments map + onto the same `*Meta` fields (typed with `Unpack[…Meta]`, validated against + the getter's return type) and the `poll_period` read-side kwarg of + `AttrR`/`AttrRW` — sugar over that mechanism, not a parallel one. - `@attr`'s `.setter` decorator mirrors `@property`/`@x.setter`, giving the - read+write pair a single logical name (`voltage`) with two decorated - methods. (`.send` is not used.) -- This degrades gracefully into the procedural `AttrR`/`AttrRW(getter=…, - setter=…)` form for protocol families with more complex needs, and into - [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s - filler for introspection-driven attributes — `@attr` is explicitly the - *simple* case, not a replacement for either. + read+write pair a single logical name (`voltage`) with two decorated methods. + There is **no dedicated write-only decorator** — a paired-getter-less `AttrW` + is rare, so it is written longhand as `AttrW(setter=…)`. +- The getter's docstring becomes the attribute's `description`, as + `@command`/`@scan` already do. +- `@attr` supports the [ADR 17](0017-naming-pass.md) `Array1D`/`Table` hint + spellings as the getter's return annotation. +- There is **no** free-function `attr()` factory: the procedural spelling is + `AttrR(getter=…)` / `AttrRW(getter=…, setter=…)` directly. `@attr` degrades + gracefully into that procedural form for protocol families with more complex + needs, and into + [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s filler + for introspection-driven attributes — `@attr` is explicitly the *simple* + case, not a replacement for either. - Refines the class-body rule stated in [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) to: *class body = declarations + decorated behaviour; instance scope = construction with data* — already true today via `@command`/`@scan`, now extended to attributes. -- Docs gain a "FastCS for PyTango users" page pairing this decorator with - the equivalent PyTango snippet, landing alongside this PR per #388 §8 - item 5b. +- Docs gain a "FastCS for PyTango users" page pairing this decorator with the + equivalent PyTango snippet, landing alongside this PR per #388 §8 item 5b. + +Interaction with the filler: an `@attr`-decorated attribute is already defined, +so [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s +`ControllerFiller` treats it as filled and does not shadow it; a clash between +an introspected name and a decorated name raises. ## Consequences - New driver code for the common "one attribute, one device call" case gets noticeably shorter — closing the gap #388 §7.5 identifies against PyTango. -- The generated callback `io=` **is** the unified callback mechanism from - [ADR 14](0014-attribute-io-rw-rework.md) (which no longer ships separate - `CallbackReadIO`/`CallbackWriteIO`): `@attr` and `attr(getter=…)` are two - spellings over one implementation. -- Adds a third way to declare an attribute (bare hint + filler; explicit - `AttrRW(getter=…, setter=…)`; `@attr` sugar) — the docs need to be clear about - when to reach for which, so this doesn't become three equally-weighted +- `@attr` and the procedural `AttrR(getter=…)` / `AttrRW(getter=…, setter=…)` + form are two spellings over one implementation — the generated getter/setter + from [ADR 14](0014-attribute-io-rw-rework.md), with no separate callback-IO + classes. +- There are three ways to declare an attribute (bare hint + filler; explicit + `AttrRW(getter=…, setter=…)`; `@attr` sugar) — the docs need to be clear + about when to reach for which, so this doesn't become three equally-weighted options with no guidance, undermining the "harsh split" clarity [ADR 13](0013-declarative-procedural-split-and-controller-filler.md) is trying to establish. -- Per #388 §8 item 5b, this sits on PR 1 (the `AttributeIO` rework) — - small and independent of the `ControllerFiller` work — so it can land - early and not block on [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). - -## Resolved in review (#402) - -1. **Decorator is `@attr` + `@x.setter`** (property-mirroring), not - `@attr_r`/`@attr_rw`/`.send`. No dedicated write-only decorator — `AttrW` - alone is rare, written longhand. -2. **Datatype/limits metadata passes via decorator kwargs**, typed with - `Unpack[…Meta]` (`precision`, `units`, limits — the ADR 14/17 `*Meta` - fields), validated against the getter's return type. -3. **Supports the ADR 17 `Array1D`/`Table` hints.** -4. **`@attr`-decorated attrs are treated specially by the filler** — already - defined, so not shadowed; a clash between an introspected name and a - decorated name raises. -5. **Yes — the getter's docstring becomes the attribute's `description`** - (as `@command`/`@scan` already do). +- Per #388 §8 item 5b, this sits on PR 1 (the per-attribute IO rework) — small + and independent of the `ControllerFiller` work — so it can land early and not + block on [ADR 13](0013-declarative-procedural-split-and-controller-filler.md). + +## Questions resolved in review (#402) + +1. **What is the decorator spelling?** `@attr` + `@x.setter` + (property-mirroring), not `@attr_r`/`@attr_rw`/`.send`. No dedicated + write-only decorator — `AttrW` alone is rare, written longhand. +2. **How is datatype/limits metadata passed?** Via decorator kwargs, typed with + `Unpack[…Meta]` (`precision`, `units`, limits — the + [ADR 14](0014-attribute-io-rw-rework.md)/[ADR 17](0017-naming-pass.md) + `*Meta` fields), validated against the getter's return type. +3. **Does it support the `Array1D`/`Table` hints?** Yes, as the getter's return + annotation. +4. **How does the filler treat a decorated attr?** As already defined — not + shadowed; a clash between an introspected name and a decorated name raises. +5. **Does the getter's docstring become the `description`?** Yes, as + `@command`/`@scan` already do. diff --git a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md index 92ba31c3f..b61c43704 100644 --- a/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md +++ b/docs/explanations/decisions/0019-embedded-ophyd-async-connector.md @@ -2,7 +2,10 @@ Date: 2026-07-20 -**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388) +**Related:** [Issue #388](https://github.com/DiamondLightSource/fastcs/issues/388), +[ADR 13](0013-declarative-procedural-split-and-controller-filler.md), +[ADR 14](0014-attribute-io-rw-rework.md), [ADR 15](0015-typed-commands.md), +[ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) ## Status @@ -32,8 +35,9 @@ for [ADR 13](0013-declarative-procedural-split-and-controller-filler.md)'s than trying to fill eagerly. - `SignalBackend`'s methods (`get_value`, `get_setpoint`, `set_callback`, `put`, `get_datakey`) are the exact surface a `FastCSSignalBackend` needs - to implement in terms of FastCS's `AttrR.get`/`AttrW.put`/setpoint cache/ - native timestamps (from [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md)). + to implement in terms of FastCS's `.readback`/`.set()`/setpoint cache/native + timestamps (from [ADR 14](0014-attribute-io-rw-rework.md) and + [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md)). - `CommandBackend.execute`/`.signature` is the equivalent surface for typed commands (from [ADR 15](0015-typed-commands.md)). @@ -86,10 +90,12 @@ Mechanics, directly mirroring `PviDeviceConnector`/`TangoDeviceConnector`: usage stays free (no FastCS controller/connection is instantiated at all). - Lifecycle (decision 8 of #388): the connector owns the runner; shutdown via an `atexit` hook plus an explicit `await connector.shutdown()`, which - cancels scan tasks and calls `Controller.disconnect()`. **The - `Device.disconnect()` proposal is dropped** — reconnect is - `Device.connect(force_reconnect=True)`, and the only disconnect we want is - `atexit` (review #402). + cancels scan tasks and calls `Controller.disconnect()`. Reconnect is + `Device.connect(force_reconnect=True)`; there is no `Device.disconnect()` + proposal, and the only disconnect we want is `atexit`. +- Errors: FastCS gains a `ConnectionFailedError` (raised when the device + doesn't respond); the connector converts it to `NotConnectedError` and keeps + retrying to connect in the background. All other errors surface unconverted. - Embedded + transports simultaneously (decision 9 of #388, e.g. a CA GUI running next to a bluesky plan) is explicitly out of scope for the first cut, but the `ControllerRunner` is designed so a transport list can be @@ -100,20 +106,20 @@ Backend mappings (from #388 §5, grounded against the researched | ophyd-async | FastCS | |---|---| -| `SignalBackend.get_value` | `AttrR.get()` | -| `SignalBackend.set_callback` | `AttrR.add_on_update_callback(cb, always=True)`; stamped per [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | -| `SignalBackend.put` | `AttrW.put(value)` | -| `SignalBackend.get_setpoint` | `AttrW` cached setpoint, [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | +| `SignalBackend.get_value` | `AttrR.readback` | +| `SignalBackend.set_callback` | update-callback registration (`always=True`); stamped per [ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | +| `SignalBackend.put` | `AttrW.set(value)` | +| `SignalBackend.get_setpoint` | `AttrW.setpoint` cache, [ADR 14](0014-attribute-io-rw-rework.md)/[ADR 16](0016-setpoint-cache-timestamps-and-controller-runner.md) | | `SignalBackend.get_datakey` | `attr.meta` (units, precision, limits) + python-type/enum choices → `SignalMetadata` + `make_datakey` | | `CommandBackend.execute`/`.signature` | `Command.__call__` / captured `Signature`, [ADR 15](0015-typed-commands.md) | | `SignalBackend.source` | e.g. `fastcs://.` | | child `Device` / `DeviceVector` | sub-`Controller` / `ControllerVector` | | (not exposed) | `@scan` methods — purely-internal periodic coroutines, bound to no `Attr`, not surfaced to ophyd-async (@shihab-dls, #402) | -Datatype mapping: `Int`/`Float`/`Bool`/`String` → `int`/`float`/`bool`/`str`; -`Waveform(array_dtype, shape)`/`Array1D` hint (per -[ADR 17](0017-naming-pass.md)) → ophyd-async `Array1D[dtype]`; `Enum(cls)` → -the enum class itself. Resolved in review (#402): +Datatype mapping: `int`/`float`/`bool`/`str` map straight across; the +`Array1D[dtype]` hint/runtime type (per [ADR 17](0017-naming-pass.md)) → +ophyd-async `Array1D[dtype]`; an enum class → the enum class itself. Two cases +needed a decision: - **Enums:** un-hinted enum classes introspect at runtime and drop to a string datatype retaining the choices as metadata; hint-typed enums require the @@ -142,28 +148,32 @@ the enum class itself. Resolved in review (#402): - `fastcs-demo`'s temperature controller simulation (`fastcs.demo.simulation`) is the existing sim device ophyd-async tests against — no new simulated device is needed for the first cut. - -## Resolved in review (#402) - -- **Enums:** un-hinted → runtime-introspect, drop to string keeping choices as - metadata; hinted → require `StrictEnum`/`SubsetEnum`/`SupersetEnum` - duplication for now, revisit with use cases. -- **`Table`:** bidirectional converter in scope for the first cut; use it to - converge the two `Table` implementations. -- **Errors:** FastCS gains a `ConnectionFailedError` (raised when the device - doesn't respond); the connector converts it to `NotConnectedError` and keeps - retrying to connect in the background. All other errors surface unconverted. -- **Disconnect dropped:** reconnect is `Device.connect(force_reconnect=True)`; - the only disconnect is `atexit`. No `Device.disconnect()` proposal (so #388 - §8 item 8 / issue #401 is rewritten accordingly). -- **`@scan` surfaces nothing; `@command` does (@shihab-dls, #402):** confirmed — - a `@scan`-decorated method is a **purely internal** coroutine run periodically; - it is *not* bound to an `Attr` and produces **no** `Signal`. All exposed state - already lives in `Attr` instances: a getter-based `AttrR` schedules its getter - as a scan-style task *and* is bound to the Attr (→ `Signal`), and a soft `AttrR` - fed by `@scan` via `update()` is likewise the exposed Signal — so `@scan` needs - nothing extra surfaced. A `@command` method **is** different: it creates an - `AttrW` and **is** exposed (the `CommandBackend` row above). Both `@scan` and - getter/update coroutines are collected onto the running loop in - `create_api_and_tasks()`; the connector schedules `@scan` coroutines as internal - tasks but never maps them to Signals. +- Dropping `Device.disconnect()` means #388 §8 item 8 / issue #401 is + rewritten accordingly (reconnect via `force_reconnect=True`, disconnect via + `atexit` only). + +## Questions resolved in review (#402) + +1. **How are enums mapped?** Un-hinted → runtime-introspect, drop to string + keeping choices as metadata; hinted → require + `StrictEnum`/`SubsetEnum`/`SupersetEnum` duplication for now, revisit with + use cases. +2. **Is `Table` supported in the first cut?** Yes — a bidirectional converter, + used to converge the two `Table` implementations. +3. **How are connection errors handled?** FastCS gains a + `ConnectionFailedError`; the connector converts it to `NotConnectedError` and + keeps retrying to connect in the background. All other errors surface + unconverted. +4. **Is there a `Device.disconnect()`?** No — reconnect is + `Device.connect(force_reconnect=True)`; the only disconnect is `atexit`. +5. **Where does `@scan`-derived state that isn't a `Signal` go?** (@shihab-dls.) + Nowhere extra is needed. A `@scan`-decorated method is a **purely internal** + coroutine run periodically; it is *not* bound to an `Attr` and produces **no** + `Signal`. All exposed state already lives in `Attr` instances: a getter-based + `AttrR` schedules its getter as a scan-style task *and* is bound to the Attr + (→ `Signal`), and a soft `AttrR` fed by `@scan` via `update()` is likewise + the exposed Signal — so `@scan` surfaces nothing extra. A `@command` method + **is** different: it creates an `AttrW` and **is** exposed (the + `CommandBackend` row above). Both `@scan` and getter/update coroutines are + collected onto the running loop in `create_api_and_tasks()`; the connector + schedules `@scan` coroutines as internal tasks but never maps them to Signals.