controllers: ControllerRunner, plus native timestamps and severity on attributes - #420
controllers: ControllerRunner, plus native timestamps and severity on attributes#420coretl wants to merge 2 commits into
Conversation
- `ControllerRunner` owns the controller lifecycle - initialise, connect, the initial and periodic tasks, reconnect, disconnect - with no transport or interactive-shell concerns. `FastCS.serve` becomes a caller of it. Starting is in two halves so a transport can be wired to the APIs before the first values are read; `start()` alone does both, for an embedder that does not need them in between. - The runner also owns reconnect. A scan task that raises marks its controller disconnected and pauses; until now nothing ever called `reconnect()`, so it stayed paused unless the driver wired its own recovery. - `Controller.connected` exposes the connection state that was only readable through the private `_connected`. - A value entering an attribute may carry when it was obtained and how wrong it is, via `Update(timestamp=..., severity=...)`; a bare value is stamped on arrival and reported as no alarm. `Severity` is a FastCS enum using the same strings as EPICS. `AttrR.timestamp` and `AttrR.severity` read them back. - Documents the stable interface an embedder is restricted to. The `AttrW` setpoint cache the issue also lists was already delivered by #412, as the `.setpoint` property ADR 0016 settled on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zpFjvbhUisfVUq3NH8M1G
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #420 +/- ##
============================================
+ Coverage 91.25% 91.37% +0.12%
============================================
Files 72 74 +2
Lines 2892 3002 +110
============================================
+ Hits 2639 2743 +104
- Misses 253 259 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The `controllers` property has no caller, and `Task.cancel` does not raise - the guards `FastCS._stop_scan_tasks` wrapped it in never fired, so moving them across only moved unreachable code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zpFjvbhUisfVUq3NH8M1G
Closes #395
Three related pieces from ADR 0016, all of which the embedded ophyd-async connector (#399) needs and none of which are embedding-specific.
ControllerRunnerThe controller lifecycle was inlined in
FastCS.serve, so there was no way to run controllers without also pulling in transport-serving and the interactive shell. It moves tofastcs.controllers.ControllerRunner, andFastCS.servebecomes a caller of it.Starting is in two halves because anything serving the controllers has to register its callbacks before the first values are read — the PVA transport already carries a comment explaining exactly this — so
setup()builds the APIs andstart()does the rest.start()runssetup()first if you have not, so an embedder that does not need the APIs in between just callsstart()/stop(), which is the shape the ADR asks for. Idempotency is the caller's responsibility, as agreed.The runner also owns reconnect, which closes a live gap. A scan task whose callback raises sets
_connected = Falseand pauses rather than dying — but nothing in FastCS ever calledController.reconnect(), so unless a driver wired its own recovery the controller stayed paused for good. The runner now watches for it and reconnects, so every controller recovers the same way.Controller.connectedexposes the state that was previously only readable through the private_connected.Native timestamps and severity
A value entering an attribute may now say when it was obtained and how wrong it is, through the
Updatethat getters and setters could already return:AttrR.timestampandAttrR.severityread them back. A bare value is stamped with the time it arrived and reported asSeverity.NO_ALARM, so nothing changes for a driver that does not care.Severityis a FastCS enum that happens to use the same strings as EPICS alarm severities; the value/timestamp/severity trio follows the shape of bluesky'sReadingand shares no code with it, per the ADR.Setpoint cache
Already delivered by #412, as the
.setpointproperty — cached byset()before the setter runs and regardless of whether it succeeds, which is exactly what ADR 0016's question 1 settled on. Nothing to do here beyond documenting it as part of the stable surface.Stable interface
New
docs/explanations/stable-interface.mdwrites down the narrow surface an embedder is restricted to (decision 13): the runner,ControllerAPI, and the attribute/command runtime methods — and that nothing should reach intoBaseController.Instructions to reviewer on how to test:
uv run pytest tests/test_controller_runner.py tests/test_attributes.py -vpython -m fastcs.demo run src/fastcs/demo/fastcs.yaml) against the sim, kill the sim so the scan tasks fail, restart it, and confirm the controller reconnects on its own rather than staying paused.Checks for reviewer
Controller.connectedis a new read-only property, so a controller that assignedself.connected = ...for its own bookkeeping now gets anAttributeError. Two test controllers in this repo did exactly that and are renamed; downstream drivers may too. The failure is loud rather than silent, and pre-1.0 is the window for it, but say if you would rather it wereis_connected.RECONNECT_PERIOD). A driver that previously relied on a controller staying paused after a failure will now see it retried. This is what the ADR asks for — "the runner owns the whole lifecycle including reconnect" — but it is a behaviour change, not just a move.NO_ALARM, not "unset". The ADR's consequences say "severity unset" for a bare value; modelling that as a fourth state (None) would push anOptionalinto every transport that reads it, and EPICS' own zero value already means the same thing. Say if you want the tri-state.Notes
(value)signature; a transport that wants the timestamp or severity reads them off the attribute. Widening the callback would have touched every transport for no caller that needs it yet.FastCS._scan_tasks/_initial_corosare gone — they live on the runner now.tests/test_control_system.pyreached into them and is updated.FastCS._stop_scan_tasksare not carried across:Task.cancelreturns whether the task was cancellable rather than raising, so theexcept (CancelledError, RuntimeError)andexcept Exception -> raise RuntimeErrorarms were unreachable.*Metatyped dicts #418 (Remove the DataType family — python types +*Metatyped dicts #413) insrc/fastcs/attributes/attr_r.py: that PR changes howupdate()validates, this one adds the stamping either side of it. Both are independent branches offrefactor, so whichever merges second needs a small conflict resolution inAttrR.update.uv run --locked tox -e pre-commit,type-checking, both green in full. For thetestsenv, this sandbox can't rundocs(needs outbound network) or the PVA/p4p-backed tests (RuntimeError: Address family not supported by protocol), the same known limitation noted on demo: use ControllerVector for temperature ramp sub-controllers #409/demo: cut-down Eiger REST sim + introspectable controller example #410/demo: convert temperature controller to getter/setter style #411/attributes: getter/setter IO rework, remove AttributeIORef/AttributeIO #412. Excluding those,pytest src tests --ignore=tests/benchmarkingpasses 349/359, with only the same 10 pre-existing p4p/socket-family failures, which I confirmed are identical onrefactoritself. Real CI coversdocsand PVA.