Skip to content

controllers: ControllerRunner, plus native timestamps and severity on attributes - #420

Open
coretl wants to merge 2 commits into
refactorfrom
refactor-issue-395
Open

controllers: ControllerRunner, plus native timestamps and severity on attributes#420
coretl wants to merge 2 commits into
refactorfrom
refactor-issue-395

Conversation

@coretl

@coretl coretl commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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.

ControllerRunner

The 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 to fastcs.controllers.ControllerRunner, and FastCS.serve becomes a caller of it.

runner = ControllerRunner(controller)
apis = await runner.setup()   # initialise, and build the ControllerAPIs
await runner.start()          # connect, run initial tasks, start scanning
await runner.stop()

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 and start() does the rest. start() runs setup() first if you have not, so an embedder that does not need the APIs in between just calls start()/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 = False and pauses rather than dying — but nothing in FastCS ever called Controller.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.connected exposes 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 Update that getters and setters could already return:

async def get_temperature() -> Update[float]:
    value, device_time = await protocol.read_with_timestamp()
    return Update(readback=value, timestamp=device_time, severity=Severity.NO_ALARM)

AttrR.timestamp and AttrR.severity read them back. A bare value is stamped with the time it arrived and reported as Severity.NO_ALARM, so nothing changes for a driver that does not care. Severity is a FastCS enum that happens to use the same strings as EPICS alarm severities; the value/timestamp/severity trio follows the shape of bluesky's Reading and shares no code with it, per the ADR.

Setpoint cache

Already delivered by #412, as the .setpoint property — cached by set() 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.md writes 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 into BaseController.

Instructions to reviewer on how to test:

  1. uv run pytest tests/test_controller_runner.py tests/test_attributes.py -v
  2. Run the demo (python -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

  • Would the PR title make sense to a user on a set of release notes
  • Controller.connected is a new read-only property, so a controller that assigned self.connected = ... for its own bookkeeping now gets an AttributeError. 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 were is_connected.
  • Reconnect is now automatic, on a 1 s poll (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.
  • Severity defaults to NO_ALARM, not "unset". The ADR's consequences say "severity unset" for a bare value; modelling that as a fourth state (None) would push an Optional into every transport that reads it, and EPICS' own zero value already means the same thing. Say if you want the tri-state.

Notes

  • Transports are untouched: whether EPICS/Tango/REST/GraphQL surface the new timestamp and severity is called out in the ADR as transport-specific follow-up, not part of this issue. The PVA transport still derives its alarm severity from the numeric limits.
  • Readback callbacks keep their (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.
  • The timestamp and severity are applied only after the value validates, so a rejected update leaves the cached value and the time it was obtained agreeing with each other.
  • FastCS._scan_tasks/_initial_coros are gone — they live on the runner now. tests/test_control_system.py reached into them and is updated.
  • The task-cancelling guards from FastCS._stop_scan_tasks are not carried across: Task.cancel returns whether the task was cancellable rather than raising, so the except (CancelledError, RuntimeError) and except Exception -> raise RuntimeError arms were unreachable.
  • Overlaps attributes: replace the DataType family with python types and *Meta typed dicts #418 (Remove the DataType family — python types + *Meta typed dicts #413) in src/fastcs/attributes/attr_r.py: that PR changes how update() validates, this one adds the stamping either side of it. Both are independent branches off refactor, so whichever merges second needs a small conflict resolution in AttrR.update.
  • Verified locally with uv run --locked tox -e pre-commit,type-checking, both green in full. For the tests env, this sandbox can't run docs (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/benchmarking passes 349/359, with only the same 10 pre-existing p4p/socket-family failures, which I confirmed are identical on refactor itself. Real CI covers docs and PVA.

- `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
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5f797b6b-d45e-410c-91e4-f51528eb32ac

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.37%. Comparing base (e73453b) to head (0cb710d).
⚠️ Report is 2 commits behind head on refactor.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants