Skip to content

demo: SCPI example — annotated attributes with per-attribute filler data - #427

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

demo: SCPI example — annotated attributes with per-attribute filler data#427
coretl wants to merge 2 commits into
refactorfrom
refactor-issue-405

Conversation

@coretl

@coretl coretl commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Closes #405

Tutorial example 4, and the declarative rung: a device that does not describe itself, whose attributes are hand-annotated in the class body and provisioned from that static metadata by ControllerFiller. A SCPI device is the honest case for this — it has no parameter tree to walk, which is why you annotate — and keeping it non-introspectable preserves the contrast with the Eiger example (#391).

class TemperatureController(SCPIController):
    ramp_rate: Annotated[
        AttrRW[float], SCPIParam("R", precision=3, units="K/s", description="Rate of change")
    ]
    power: Annotated[AttrR[float], SCPIParam("P", precision=3, units="W")]

Runs on the existing temperature-controller backend — its text protocol is already SCPI-shaped (R? to read, R=1.5 to write), so no new sim is invented and, crucially, no introspection is.

Scope

  • src/fastcs/demo/scpi.pySCPIParam and SCPIController.
    • SCPIParam(param, **kwargs: Unpack[Meta]) is the single place an attribute's whole spec is written: the command token plus all generic metadata. It stores a .meta and is the exclusive spec source for its attribute — the controller does not also merge a separate FloatMeta extra on the same hint, so there is one place to look for what an attribute is.
    • SCPIController.initialise reads its own declarations, builds each attribute's getter and setter from the token, and passes .meta to fill_attribute, which runtime-validates it against the datatype the hint declared.
    • Access mode comes from the hint: an AttrR[...] gets no setter, an AttrRW[...] gets both. A suffix addresses one channel, so a per-ramp sub controller is the same class with 01 appended to every token — no dispatching on which ramp at IO time.
  • src/fastcs/demo/temperature_scpi.py — the full multi-ramp temperature controller declared this way. It carries the composition and methods rungs too (the old "io= pattern" tutorial being gone): a ControllerVector of ramp sub controllers, the voltages @scan, and the cancel-all @command.
  • Tests — the wire format, the per-ramp suffix, the enum datatype coming from the hint, and the negative case the issue asks for.

Where this lives, and why

Both modules are in the demo (protocol) layer, not core FastCS, per ADR 0014 decision 3: core ships no extras vocabulary for 1.0. Core defines the mechanism — the extras an Annotated hint carries through to ControllerFiller — and this is the worked example of a third party building on it. Named SCPIParam rather than SCPIMeta: it is a binding extra you instantiate, a sibling of ophyd-async's PvSuffix, and the *Meta suffix is reserved for the Unpack-able typed dicts.

Instructions to reviewer on how to test:

  1. uv run pytest tests/demo/test_temperature_scpi.py -v
  2. Compare src/fastcs/demo/temperature_scpi.py against temperature_attr.py side by side — same device, same attributes, procedural vs declarative.

Checks for reviewer

  • Would the PR title make sense to a user on a set of release notes
  • Two attributes deliberately carry no token. voltages and each ramp's voltage are updated by the parent's @scan, which reads every ramp's voltage in one query rather than one per ramp, so the driver fills them itself (fill_attribute("voltage", precision=3, units="V")). That is also what demonstrates SCPIController filling what it owns and leaving the rest to the driver — but say if you would rather every attribute went through SCPIParam for uniformity, even the ones with no command of their own.
  • poll_period is one class attribute, not per-SCPIParam. The issue specifies SCPIParam(param, **kwargs: Unpack[Meta]), and a poll period is not metadata — it is not something a client is served — so it has no place in Meta. A controller whose attributes want different periods would need a period argument on SCPIParam outside the Meta unpack; say if that is wanted and it is a small change.
  • A hint that names no datatype is an error here, not a promise. state: Annotated[AttrR, SCPIParam("ST")] has nothing to parse the device's text answer into, and a SCPI device cannot be asked — so the error says the hint must name its datatype. Unsubscripted hints without a SCPIParam keep core's behaviour (a promise, reported by check_filled).
  • The datatype doubles as the parser for the device's answer (float("1.5"), OnOffEnum("1")). Every datatype FastCS serves happens to support that, but nothing in type[DType] says so, so there is one cast with a comment. The alternative is a per-datatype parse table in the demo, which seemed more machinery than the example is worth.
  • Tests mock the connection rather than driving the tickit sim, matching tests/demo/test_temperature_attr.py. The negative case is precision on a str attribute, asserted to name both the field and the attribute.

Notes

  • Built on the old lifecycle (initialise/connect/reconnect), which is what is on refactor today. controllers: connections own health, reconnect and the retry budget #424 renames those to build/setup and moves connection ownership onto Connection; whichever merges second will want initialisebuild and the three connection hooks dropped from TemperatureController here. Kept independent rather than stacked, per one issue = one branch = one PR.
  • Not wired into demo/fastcs.yaml: registering a second class there would turn the entry into a discriminated union and change the demo's schema, which is not this issue's scope.
  • Verified locally with uv run --locked tox -e pre-commit,type-checking, both green in full. As on the rest of this series, this sandbox cannot run docs (needs outbound network) or the PVA/p4p-backed tests (RuntimeError: Address family not supported by protocol). Excluding those, pytest src tests --ignore=tests/benchmarking passes 507, with only the same 10 pre-existing p4p/socket-family failures. I also built the docs offline with the version-switcher fetch stubbed — which caught four ambiguous module cross-references in the new docstrings that would have failed the real --fail-on-warning docs job, now written as explicit :mod: roles.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BymsiALDVCGv1nJWK8DyAV


Generated by Claude Code

Example 4 of the tutorial sequence, and the declarative rung: a device that
does NOT describe itself, whose attributes are hand-annotated in the class
body and provisioned from that static metadata by a `ControllerFiller`.

`SCPIParam` is one spec object per attribute - the command token plus all
generic metadata in the same place, because they describe the same thing:

    ramp_rate: Annotated[
        AttrRW[float], SCPIParam("R", precision=3, units="K/s")
    ]

Both new modules live in the demo (protocol) layer, not core FastCS: ADR 0014
decision 3 says core ships no extras vocabulary for 1.0. Core defines the
mechanism - the extras an `Annotated` hint carries through to the filler -
and this is the worked example of a third party building on it. `SCPIParam`
is a sibling of ophyd-async's `PvSuffix`, not a FastCS type; hence the name,
rather than `SCPIMeta`, since the `*Meta` suffix is reserved for the
`Unpack`-able typed dicts.

- `demo/scpi.py`: `SCPIParam` and `SCPIController`. The controller reads its
  own declarations, builds each attribute's getter and setter from the token,
  and passes the param's `.meta` to `fill_attribute`, which runtime-validates
  it against the datatype the hint declared. The access mode comes from the
  hint: an `AttrR[...]` gets no setter. A `suffix` addresses one channel of a
  device, so a per-ramp sub controller is the same class with `01` appended
  to every token.
- `demo/temperature_scpi.py`: the full multi-ramp temperature controller
  declared this way, running on the existing temperature sim - its text
  protocol is already SCPI-shaped, so no new sim is invented and, crucially,
  no introspection is. It carries the composition and methods rungs too: a
  `ControllerVector` of ramp sub controllers, the voltages `@scan`, and the
  cancel-all `@command`.

Two attributes deliberately carry no token: `voltages` and each ramp's
`voltage` are updated by the parent's scan in one query rather than one per
ramp, so the driver fills them itself. That is what shows `SCPIController`
filling what it owns and leaving the rest alone.

Tests cover the wire format, the per-ramp suffix, the enum datatype coming
from the hint, and the negative case the issue asks for: `precision` on a
`str` attribute raises, naming the attribute and the field.

Closes #405

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BymsiALDVCGv1nJWK8DyAV
@coderabbitai

coderabbitai Bot commented Sep 9, 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: Advanced

Run ID: 2e13a778-d059-44d7-ba15-fc7b0e711b6b

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 Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.63%. Comparing base (e73453b) to head (0141c3d).
⚠️ Report is 8 commits behind head on refactor.

Additional details and impacted files
@@             Coverage Diff              @@
##           refactor     #427      +/-   ##
============================================
+ Coverage     91.25%   92.63%   +1.38%     
============================================
  Files            72       70       -2     
  Lines          2892     3449     +557     
============================================
+ Hits           2639     3195     +556     
- Misses          253      254       +1     

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

Both failures were in what I could not see locally rather than in the code.

`pre-commit run --all-files` only sees files git knows about, so the three
new modules were invisible to it until they were staged - ruff-format then
wanted `TemperatureRampController.__init__`'s super() call on one line.
Staged first this time, and verified.

The docs job builds with a network, so `default_role = "any"` resolves
against the real intersphinx inventories; two single-backtick spans in the
new docstrings - ``__init__`` and ``voltage`` - are prose rather than
cross-references, and are now literals. Offline they were hidden among the
intersphinx misses that having no network produces.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BymsiALDVCGv1nJWK8DyAV
@shihab-dls
shihab-dls self-requested a review September 9, 2026 17:04

@shihab-dls shihab-dls left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example has demonstrated the need for some kind of AttrBackend and AttrFactory. Follow this attached spec to implement these requirements, in order to update the SCPI example.
attribute_factory.md

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.

3 participants