controllers: ControllerFiller, and one declarative mechanism for attributes

claude.md

@@ -53,6 +53,51 @@ Conventions for this repository. Follow these when writing or reviewing code and
   Rule of thumb: a helper's parameters should describe *what it's being asked to
   validate/produce*, never *whether/how the caller wants it validated*.
 
+- The same applies to a parameter that only names *where the data came from* so the
+  helper can put it in its message (`source`, `origin`, `context`). The call-site
+  already knows that; if it wants it in the error, it should catch the helper's plain
+  exception and say so itself. Prefer a generic exception from the helper to a
+  parameter that exists only to decorate one.
+
+  ```python
+  # Bad: `source` is threaded in purely to phrase the message
+  def check_filled(self, source: str | None = None) -> None:
+      ...
+      raise RuntimeError(f"did not provision from {source}: {missing}")
+
+  # Good: the helper says what it knows
+  def check_filled(self) -> None:
+      ...
+      raise RuntimeError(f"did not provision: {missing}")
+  ```
+
+## Typing
+
+- Do **not** annotate a parameter `Any` when a narrower type says what the function
+  actually accepts. `Any` in a signature is a promise the function cannot keep: it
+  turns an author-time error into a runtime one, or into no error at all.
+
+  ```python
+  # Bad: any object at all type checks
+  def fill_attribute(self, name: str, datatype: Any = None) -> Attribute: ...
+
+  # Good: only the types an attribute can hold
+  def fill_attribute(self, name: str, datatype: type[DType_T] | None = None) -> Attribute: ...
+  ```
+
+- This holds especially for callables that will be attached to something already
+  typed. IO handed to an existing `Attribute` must be typed with the datatype
+  TypeVar (`Getter[DType_T]`, `Setter[DType_T]`), not `Getter[Any]` - otherwise a
+  setter taking a `datetime.datetime` type checks against a `float` attribute and only
+  fails, if at all, at runtime. Type it so the author is warned where they wrote it.
+
+- A `# type: ignore` or `# pyright: ignore` must be **required**. Before adding one,
+  remove it and re-run the type checker: if nothing is reported, it does not belong.
+  Before reaching for one at all, look for a signature that makes it unnecessary -
+  `**meta: Unpack[Meta]` rather than `**meta: Any` plus an ignore at the call that
+  passes them on. Every remaining ignore should be specific (`[reportCallIssue]`, not
+  bare) and should have a reason a reader can check.
+
 ## Tests: `pytest.raises`
 
 - The `with pytest.raises(...):` block should contain the **minimal code that raises
@@ -115,3 +160,84 @@ Conventions for this repository. Follow these when writing or reviewing code and
 
   This keeps tests readable as documentation: every line is evidence for the claim in
   the test's name, not incidental noise carried over from copy-pasting another test.
+
+## Tests: one behaviour per test
+
+- A test checks **one** thing, and its name says which. Do not put two `pytest.raises`
+  blocks, or a failing case and a happy path, in one test: they are separate claims
+  about the code, and when the first fails the rest never run.
+- Setup shared by the split tests belongs in a fixture, a module-level class, or a
+  couple of repeated lines - repeating three lines of setup is cheaper than a test
+  that verifies four unrelated things.
+
+  ```python
+  # Bad: three claims, one name, and the last two only run if the first passes
+  def test_hint_validation():
+      controller = HintedController()
+
+      with pytest.raises(RuntimeError, match="never added"):
+          controller.check_filled()
+
+      with pytest.raises(RuntimeError, match="expected 'AttrR'"):
+          controller.add_attribute("state", AttrW(int))
+
+      controller.add_attribute("state", AttrR(int))
+      controller.check_filled()
+
+  # Good: one claim each, each named for what it checks
+  def test_a_hint_without_a_datatype_is_promised(): ...
+  def test_adding_a_promised_attribute_with_the_wrong_access_mode_raises(): ...
+  def test_adding_a_promised_attribute_satisfies_the_declaration(): ...
+  ```
+
+## Tests: parametrize instead of near-identical tests
+
+- If two or more tests differ only in the values they use - a different hint, a
+  different datatype, a different expected message - write one
+  `@pytest.mark.parametrize`d test rather than copies of one body.
+- Keep them separate when the *shape* of the test differs, not just its data: a case
+  that needs different setup, different assertions, or a different name to make sense
+  is a different test.
+
+## Unused names
+
+- Declare a name only where something reads it. An unused local, argument or
+  `@pytest.mark.parametrize` column is a reader's question with no answer: they have to
+  scan the whole body to find out that nothing uses it.
+- This bites hardest when a parametrized test is split off another one. The columns are
+  copied across whole, and a column the new body never reads survives in every case:
+
+  ```python
+  # Bad: `name` is in every case and read by nothing
+  @pytest.mark.parametrize(
+      "parent_type, name, expected",
+      [
+          (ChildHintedParent, "child", "child .declared Child, never added."),
+          (VectorHintedParent, "children", "children .declared ControllerVector"),
+      ],
+  )
+  def test_a_controller_hint_is_not_created(
+      parent_type: type[Controller], name: str, expected: str
+  ):
+      controller = parent_type()
+
+      with pytest.raises(RuntimeError, match=expected):
+          controller.check_filled()
+
+  # Good
+  @pytest.mark.parametrize(
+      "parent_type, expected",
+      [
+          (ChildHintedParent, "child .declared Child, never added."),
+          (VectorHintedParent, "children .declared ControllerVector"),
+      ],
+  )
+  def test_a_controller_hint_is_not_created(
+      parent_type: type[Controller], expected: str
+  ):
+      ...
+  ```
+
+- The exception is a name a caller outside your control decides: a signature that
+  implements an interface, or an unpacking that has to consume every element. Prefix
+  those with an underscore rather than deleting them.

docs/explanations/controllers.md

@@ -20,11 +20,16 @@ lifecycle, if required.
 
 | Method | Purpose |
 |---|---|
-| `initialise` | Dynamically add attributes on startup, before the API is built |
+| `initialise` | Fill declared attributes, and add dynamic ones, before the API is built |
 | `connect` | Open connection to device |
 | `reconnect` | Re-open connection after scan error |
 | `disconnect` | Release device resources before shutdown |
 
+Attributes are constructed in `__init__`, or declared as class-body type hints
+and created for you - see [](declaring-attributes.md) for which to use when.
+An `Attribute` assigned in the class body is rejected: one object would be
+shared by every instance of the controller.
+
 ### Scan task behaviour
 
 When used as the root controller, FastCS collects all `@scan` methods and readable
@@ -41,8 +46,12 @@ from fastcs.methods import scan
 
 
 class TemperatureController(Controller):
-    temperature = AttrR(float, units="degC")
-    setpoint = AttrRW(float, units="degC")
+    def __init__(self, host, port):
+        super().__init__()
+        self._host, self._port = host, port
+
+        self.temperature = AttrR(float, units="degC")
+        self.setpoint = AttrRW(float, units="degC")
 
     async def connect(self):
         self._client = await DeviceClient.connect(self._host, self._port)
@@ -72,7 +81,9 @@ controller also has connection logic, the parent must invoke it explicitly:
 
 ```python
 class ChannelController(Controller):
-    value = AttrR(float)
+    def __init__(self):
+        super().__init__()
+        self.value = AttrR(float)
 
     async def connect(self):
         ...
@@ -107,7 +118,9 @@ from fastcs.controllers import Controller, ControllerVector
 
 
 class ChannelController(Controller):
-    value = AttrR(float)
+    def __init__(self):
+        super().__init__()
+        self.value = AttrR(float)
 
 
 class RootController(Controller):

docs/explanations/declaring-attributes.md

@@ -0,0 +1,121 @@
+# Declaring Attributes: Class Body vs `__init__`
+
+A `Controller`'s class body holds **declarations**; its `__init__` holds
+**construction with data**. Which of the two an attribute belongs in follows
+from one question: is everything about the attribute known when you write the
+class?
+
+## Construct it in `__init__` when you know everything
+
+If you can write the attribute down in full — its datatype, its metadata, and
+the IO that reads and writes it — construct it in `__init__` and assign it to
+`self`:
+
+```python
+class PowerSupplyController(Controller):
+    def __init__(self, protocol: PowerSupplyProtocol) -> None:
+        super().__init__()
+
+        self.voltage = AttrRW(
+            float,
+            getter=Polled(protocol.get_voltage, period=0.5),
+            setter=protocol.set_voltage,
+            units="V",
+            precision=3,
+        )
+```
+
+This is most drivers, most of the time. Because the attribute is built per
+instance, it can close over per-instance state — which is what lets one
+channel's index be baked into its own getter rather than dispatched on at IO
+time.
+
+An `Attribute` **may not** be assigned in the class body. One built there would
+be a single object shared by every instance of the controller, so two devices
+of the same model would write into each other; FastCS raises at construction
+rather than let that happen, and names the attribute.
+
+## Declare it as a hint when the data arrives later
+
+Some drivers cannot write the attribute down in full, because what it needs
+comes from the device — a detector that reports its own parameter tree — or
+from a protocol library that turns one line of metadata into a getter and a
+setter. Those declare a **type hint**, and `ControllerFiller` creates the
+attribute from it:
+
+```python
+class OdinDetector(Controller):
+    frames: AttrRW[int]
+
+    async def initialise(self) -> None:
+        for name, spec in await self._query_parameter_tree():
+            self.filler.fill_attribute(
+                name, getter=spec.getter, setter=spec.setter, **spec.meta
+            )
+
+        self.filler.check_filled()
+```
+
+The hint is not a promise to build something later. `self.frames` **exists as
+soon as `__init__` returns** — as an `AttrRW[int]` with no IO yet — so the rest
+of `__init__` can reference it, hand it to a sibling, or subscribe to it. That
+rule is what makes `initialise` safe to run in parallel across controllers:
+only `__init__` is serial, and by the time it ends every attribute anything
+refers to is there.
+
+`fill_attribute` provisions the attribute **in place**, so a reference taken
+during `__init__` is the same object that ends up serving the device. It
+validates as it goes: the metadata against the datatype the hint declared
+(`precision` on a `str` raises, naming the field and the attribute), and, when
+you pass `datatype=`, what the device reported against what you declared.
+
+### A hint that cannot name its datatype
+
+Occasionally the datatype itself is only knowable over the wire — an enum whose
+members the device reports. Write the hint without a subscript:
+
+```python
+class EigerDetector(Controller):
+    state: AttrR          # enum built from the device's `allowed_values`
+```
+
+FastCS cannot create that one, so it is a **promise** instead: introspection
+must add it with `add_attribute`, and `check_filled` fails if nothing did. The
+access mode is still checked — adding an `AttrW` where an `AttrR` was promised
+raises.
+
+### Extras: metadata a protocol layer defines
+
+An `Annotated` hint carries anything else you put in it, and the filler hands
+it back untouched:
+
+```python
+class Instrument(SCPIController):
+    power: Annotated[AttrRW[float], SCPIParam("P", precision=3, units="W")]
+```
+
+Core FastCS defines **no** extras vocabulary. `SCPIParam` above belongs to
+whatever protocol package you build on top: it reads the extras off each
+declaration, builds the getter and setter its protocol implies, and fills the
+attribute. This is how a protocol library gets a declarative spelling of its
+own without FastCS knowing anything about it.
+
+## Checking what was promised
+
+`check_filled()` raises if anything the class body declared is missing, listing
+it by name. FastCS calls it across the whole controller tree after
+`initialise`, so a driver that forgets cannot serve a half-built API; call it
+yourself at the end of your own `initialise` to fail before anything else runs.
+An `| None` hint is not required.
+
+## Summary
+
+| You know | Write |
+|---|---|
+| Everything about the attribute | `self.x = AttrRW(...)` in `__init__` |
+| Its type, but not its IO or metadata | `x: AttrRW[int]` and fill it in `initialise` |
+| Its access mode only | `x: AttrR` and `add_attribute` it in `initialise` |
+| Nothing until the device answers | No declaration; `add_attribute` in `initialise` |
+
+See [ADR 0013](decisions/0013-declarative-procedural-split-and-controller-filler.md)
+for why there is one declarative mechanism rather than two.

docs/how-to/arrange-epics-screens.md

@@ -20,12 +20,15 @@ from fastcs.methods import command
 
 
 class PowerSupplyController(Controller):
-    voltage = AttrRW(float, group="Output")
-    current = AttrRW(float, group="Output")
-    power = AttrR(float, group="Output")
+    def __init__(self) -> None:
+        super().__init__()
+
+        self.voltage = AttrRW(float, group="Output")
+        self.current = AttrRW(float, group="Output")
+        self.power = AttrR(float, group="Output")
 
-    temperature = AttrR(float, group="Status")
-    fault_code = AttrR(int, group="Status")
+        self.temperature = AttrR(float, group="Status")
+        self.fault_code = AttrR(int, group="Status")
 
     @command(group="Actions")
     async def reset_faults(self) -> None:
@@ -54,24 +57,27 @@ from fastcs.methods import command
 
 
 class ChannelController(Controller):
-    voltage = AttrRW(float, group="Output")
-    current = AttrRW(float, group="Output")
-    temperature = AttrR(float, group="Status")
+    def __init__(self) -> None:
+        super().__init__()
+
+        self.voltage = AttrRW(float, group="Output")
+        self.current = AttrRW(float, group="Output")
+        self.temperature = AttrR(float, group="Status")
 
     @command(group="Actions")
     async def enable(self) -> None:
         ...
 
 
 class MultiChannelPSU(Controller):
-    total_power = AttrR(float)
-
     @command()
     async def disable_all(self) -> None:
         ...
 
     def __init__(self, num_channels: int) -> None:
         super().__init__()
+
+        self.total_power = AttrR(float)
         for i in range(1, num_channels + 1):
             self.add_sub_controller(f"Ch{i:02d}", ChannelController())
 ```