controllers: ControllerFiller, and one declarative mechanism for attributes

[coretl]

Closes #394

ADR 0013's declarative/procedural split. Class body = declarations + decorated behaviour; instance scope = construction with data. FastCS had two declarative mechanisms — class-scope Attribute instances, deepcopied per instance, and bare hints that were validated but never created. This leaves one.

class OdinDetector(Controller):
    frames: AttrRW[int]        # exists as soon as __init__ returns

    async def initialise(self) -> None:
        for name, spec in await self._query_parameter_tree():
            self.filler.fill_attribute(name, getter=spec.getter, **spec.meta)

        self.filler.check_filled("the Odin parameter tree")

Scope

• New ControllerFiller (src/fastcs/controllers/filler.py), on every controller as controller.filler. It reads the class hints during __init__, in two phases either side of _bind_attrs so that a hint and an @attr of the same name are one declaration rather than a clash.
• A hint that names its datatype is created unfilled, so self.frames exists before __init__ returns and the rest of __init__ can reference it — ADR 0013's rule, and what makes initialise parallelisable.
• fill_attribute(name, getter=, setter=, datatype=, **meta) provisions in place, so a reference taken during __init__ is the object that ends up serving the device. It validates the metadata against the declared datatype (precision on a str raises, naming field and attribute), rejects IO the access mode has no half for, and — when you pass datatype= — checks what the device reported against what was declared. AttrR.set_getter/AttrW.set_setter are the new fill points, and refuse to overwrite IO an attribute already has.
• check_filled(source) reports the promised-but-missing, listing them by name and naming where the data should have come from. BaseController.check_filled walks the tree and is what post_initialise now calls.
• Extras: an Annotated hint's extras are carried untouched and yielded as (child, extras) by iterating the filler — the mechanism a protocol package (Example 4 — SCPI device: annotated attributes + per-attribute filler data #405's SCPIParam) builds its own declarative vocabulary on. Core FastCS defines none, per decision 3.
• Deleted: HintedAttribute, _validate_type_hints, _validate_hinted_member/_method/_attribute/_controller, and the deepcopy branch of _bind_attrs. @attr/@command/@scan binding is untouched.
• Optional[X] hints are not required by check_filled; trailing-underscore names (description_: AttrR[str]) declare the attribute without the underscore, ophyd-async's convention.
• A class-body Attribute instance now raises at construction, naming the attribute and both alternatives, rather than being silently deepcopied.
• Migrated: fastcs.demo.eiger to fill_attribute + check_filled, docs/snippets/static03–06, the prose examples in explanations/controllers.md, how-to/arrange-epics-screens.md, table-waveform-data.md, typed-commands.md, update-attributes-from-device.md, wait-methods.md, and every test controller in the repo. New docs/explanations/declaring-attributes.md on which spelling to use when.

Instructions to reviewer on how to test:

1. uv run pytest tests/test_controller_filler.py tests/test_controllers.py -v
2. uv run pytest tests/demo/test_eiger.py -v — the introspecting example, filling two declared parameters out of a discovered tree.

Checks for reviewer

• Would the PR title make sense to a user on a set of release notes
• A hint that cannot name its datatype is a promise, not a created child. ADR 0013 says every hint-referenced attribute must exist by the end of __init__, but state: AttrR on the Eiger has no author-time datatype at all — the enum's members come off the wire — so there is nothing to construct. Rather than drop that case or invent a placeholder datatype, an unsubscripted hint keeps exactly the old HintedAttribute behaviour: not created, access-mode checked when introspection adds it, and check_filled fails if nothing did. Say if you would rather unsubscripted hints were simply an error, which would mean changing how the Eiger example declares state.
• Sub-controller, ControllerVector and Command hints are scanned and promised, not created. ophyd-async's filler constructs child Devices because Device() takes no required arguments; a Controller subclass generally does, so guessing a constructor is not available to us. An empty ControllerVector could be created — say if you would like that one, it is a couple of lines and would let self.ramps[i] = ... work from initialise without the parent building the vector.
• check_filled checks existence, not IO. An attribute created from a hint and never filled is legitimate — a @scan on the parent may be what drives it, which is exactly what docs/how-to/update-attributes-from-device.md recommends — so requiring a getter would reject a documented pattern. The issue's wording ("reports promised-but-missing") is what is implemented. The cost is that a driver which forgets to fill a hinted attribute gets an attribute stuck at its default rather than an error.
• Every test controller in the repo moved, ~140 attributes. Where the class body said x = AttrR(int) with nothing else, it is now the hint x: AttrR[int], which produces the identical unfilled attribute; where it carried metadata, IO or an initial value, it moved into __init__. Worth a skim for anywhere the change of construction order matters — hinted attributes are created after __init__-assigned ones within a controller.
• root_attribute is deliberately still a class-body Attribute. It is declared on BaseController itself and is what a parent shows for this controller rather than an attribute of it, so it is neither the filler's to create nor covered by ADR 0013. Both _bind_attrs and the filler skip the name. Say if it should move too — it would need a different mechanism, since it must not appear in its own controller's attributes.

⚠️ Investigate: fastcs-catio

Answered by ADR 0013's own review (question 2): no, the filler does not support building Controller classes at runtime with type(...), and catio moves to instance-level dynamic attributes instead. Nothing in this PR needs to change for that — adding attributes onto a bare Controller from the outside is exactly what a filler does, and there is a test for it (test_attributes_can_be_added_to_a_bare_controller_from_outside). No ADR update needed; the decision was already recorded.

Notes

• post_initialise keeps its name and its place in the lifecycle; only what it calls changed. controllers: connections own health, reconnect and the retry budget #424 renames these hooks (initialise → build, post_initialise → setup) — that PR and this one both touch runner.py's call site and base_controller.py, so whichever merges second needs a small conflict resolution there.
• add_command flattens the error it catches: a Method hint mismatch surfaces as Cannot add command method <Command object ...> rather than the "does not match defined type" message underneath. That is pre-existing (the except (ValueError, RuntimeError) in add_command/add_scan re-raises with only its own message) and I have not changed it, but the test now asserts the outer message, so it is worth knowing it is there.
• Verified locally with uv run --locked tox -e pre-commit,type-checking, both green in full. As 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/attributes: replace the DataType family with python types and *Meta typed dicts #418/methods: typed commands — positional arguments and a return value #419/controllers: ControllerRunner, plus native timestamps and severity on attributes #420/attributes: @attr decorator sugar over the getter/setter constructors #423, this sandbox cannot run the PVA/p4p-backed tests (RuntimeError: Address family not supported by protocol). Excluding those, pytest src tests --ignore=tests/benchmarking passes 466, with only the same 10 pre-existing p4p/socket-family failures. I also built the docs offline with the version-switcher fetch stubbed out: it succeeds, and the only warnings are the intersphinx misses that come of having no network — including catching and fixing one ambiguous check_filled cross-reference that would have failed the real --fail-on-warning docs job.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PP7FfLHyvQYkzKs3nm8ARv

---

Generated by Claude Code

[coderabbitai]

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: Team

Run ID: 26379d80-c864-4a4c-87fe-a4c5c7d0eb0a

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

❌ Patch coverage is 97.20670% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.72%. Comparing base (e73453b) to head (d576551).
⚠️ Report is 6 commits behind head on refactor.

Files with missing lines	Patch %	Lines
src/fastcs/controllers/filler.py	98.36%	2 Missing ⚠️
src/fastcs/attributes/attr_r.py	91.66%	1 Missing ⚠️
src/fastcs/attributes/attr_w.py	75.00%	1 Missing ⚠️
src/fastcs/controllers/base_controller.py	97.43%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           refactor     #426      +/-   ##
============================================
+ Coverage     91.25%   92.72%   +1.47%     
============================================
  Files            72       70       -2     
  Lines          2892     3380     +488     
============================================
+ Hits           2639     3134     +495     
+ Misses          253      246       -7     

☔ 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.

[coretl]

Green on d5765512 — lint, docs, dist, tests on 3.11/3.12/3.13, and both codecov checks.

CI found a real bug in the first push, worth knowing about because it changes one of the rules in the diff. _bind_attrs had been letting a class-body Attribute through when its name also carried an annotation (attr_1: AttrRW = AttrRW(...)), on the reasoning that the hint was the declaration and the assignment the old spelling of the same thing. But a bare AttrRW hint names no datatype, so the filler treated it as a promise and created nothing: the attribute vanished with no error at all — exactly the silent failure the loud rejection exists to prevent. The PVA PVI test caught it as four parameters missing from the served structure. Every class-body Attribute now raises, annotated or not.

That also surfaced the controllers in test_p4p.py, which are declared inside their test functions and so were missed by the first migration pass. Bare ones are hints now, the ones carrying metadata moved into __init__, and SomeController.attr_1 — declared twice, int then float — keeps the float one it actually had.

Two things this sandbox cannot check, both now covered by real CI: the p4p tests (no PVA-capable socket family here), and that pre-commit run --all-files skips files git does not yet track, which is how the first push went out unformatted.

— overnight agent

---

Generated by Claude Code