Conversation
|
Important Review skippedDraft detected. 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 @@
## main #402 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 72 72
Lines 2892 2892
=======================================
Hits 2639 2639
Misses 253 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coretl
commented
Jul 21, 2026
This was referenced Jul 21, 2026
Incorporate coretl's inline review decisions: drop DataType (python types + *Meta), unify callback IO into the @attr factory, the __init__ filler rule + no-hints/Optional/external-add, nested Limits with inheritance, get_setpoint + ControllerRunner(start/stop) owning reconnect, severity enum, args/returns typed separately (kwargs -> spike #403), enum/Table handling, and dropping the Device.disconnect proposal for connect(force_reconnect=True) + atexit. Remaining deferred items (@shihab-dls, @Tom-Willemsen) kept as explicit open questions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Define the *Meta TypedDicts + Unpack overloads for the procedural spelling, the superset Meta for generic extras (SCPIParam), attribute-stored resolved meta, and the filler's runtime validation of annotated metadata against the datatype. Clarify in ADR 13 that hinted attributes exist after __init__ (filler creates them unfilled), not after initialise(). Module home for the new names deferred to #406 (top-level API namespace: flat vs nested). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Per @Tom-Willemsen on PR #402 (r3621453680): SECoP devices are discovered entirely from an over-the-wire `describe`, so you never statically know something is a command without knowing its signature. There is no "known command, unknown args" middle case — P/T are either completely known (static `Command[P, T]`) or the whole structure is unknown (built at runtime). Remove the `Any` args/returns option and the `Command[Any, Any]` consequence; resolve the open question awaiting @Tom-Willemsen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Fold the top-level examples/ scaffold into the fastcs.demo package (mirrors ophyd-async; examples install with fastcs[demo] and become the single source of the tutorial code). Replace the stale 3-style scaffold README with the final 5-example hello-world -> complicated-device ladder (two backends: temperature sim for steps 1-4, cut-down Eiger REST sim for step 5), keyed to issues #398/#404/#390/#405/#391. ADR 0014: spell out the "one spec object per declaratively-filled attribute" design - SCPIParam carries the binding token AND all metadata via Unpack[Meta], is the exclusive spec source (filler does not merge a separate *Meta extra), and pays for its ergonomics with runtime validation. Record why it is SCPIParam not SCPIMeta (a binding extra you instantiate, sibling of PvSuffix/TangoPolling, not a Meta TypedDict) and that it lives in the demo/ protocol layer, not core (decision 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Fold in the 2026-07-22 review decisions: ADR 0014: io= objects (ReadIO/WriteIO/ReadWriteIO) superseded by getter/setter callables on AttrR/AttrW/AttrRW. getter() -> T | Update[T]; setter -> None | T | Update[T] (value return = accepted/clamped readback, the sanctioned secop setpoint echo). Update[T] = value/timestamp/severity. Datatype optional when getter/setter given (inferred, unwrapping Update[T]). update_period: ONCE (default) / float / None (on-demand); no getter = @scan-fed soft. Access mode from which params exist; ReadIO trio dropped. Both prior open questions closed. ADR 0018: @attr is decorator-only (@attr / @attr(precision=3) + @x.setter); no @attr_r/@attr_rw, no free-function attr() factory; procedural is AttrR/AttrRW. Title + stale refs swept; 0013 refs swept too. demo README: five modules, four tutorials (the reusable-io= rung is gone); controllers.py repurposed to the composition/@scan/@command example folded into the declarative tutorial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
…od, set()) Rename/split the get()/update(value)/put(value) trio so access mode and device-IO are legible from the member set: - .value -> .readback + .setpoint (read-only properties; mirror bluesky/ ophyd Location(setpoint, readback); presence tracks access mode). - no-arg update() -> poll() (returns the value); update_period -> poll_period (schedule only). Deletes set_update_callback/bind_update_callback. - update(value) is now a pure cache push (no IO, no None sentinel). - put() -> set() (bluesky verb); caches .setpoint then runs setter; setter's T|Update[T] return feeds .readback. sync_setpoint kwarg gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Evolve the demo temperature controller composition example onto the documented ControllerVector pattern instead of a manual list + add_sub_controller loop, and add unit tests exercising cancel_all and the voltage-distributing scan against a mocked IPConnection. Closes #390
Add a FastAPI fake REST sim (demo/simulation/eiger.py) shaped like a detector parameter tree (subsystems of named parameters, a keys listing endpoint, per-parameter GET/PUT), and an EigerDetector controller (demo/eiger.py) that type-hints half its attributes (checked via the current HintedAttribute mechanism) and fills the rest by introspecting the sim's keys endpoints in initialise(). Baseline uses the current API (AttrR/AttrRW + io_ref/AttributeIO); migrates to ControllerFiller when #394 lands. Closes #391
sphinx-build --fail-on-warning was erroring on autodoc cross-references to httpx.AsyncBaseTransport and fastapi.applications.FastAPI, which have no intersphinx mapping - same class of issue already worked around for p4p types.
Resolves the two review threads Shihab answered on 2026-07-22: - ADR 0019 (@scan): confirmed @scan is a purely-internal periodic coroutine bound to no Attr and surfacing no Signal; @command, by contrast, creates an AttrW and IS exposed. Closes the last open question on 0019; folded into the mapping table + Resolved-in-review. - ADR 0014 (setpoint echo): set() caching .setpoint is an attribute-cache guarantee only. Records Shihab's CA-vs-PVA divergence — PVA posts the setpoint immediately (may later alarm), CA posts only after the update callback completes, so a long-running setter delays the CA-visible setpoint. CA/PVA ordering realignment noted as a transport follow-up, not gating this rework. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Address review comment: pyright already infers ControllerVector[TemperatureRampController] from the dict literal, so the explicit annotation was redundant.
…l read-only params - eiger.py: add soft `idle: AttrR[bool]` derived from the introspected `state` param (state == "idle"), kept in sync via an on-update callback - shows why we declare `state` as a checked attribute (to build code on top of it). - eiger.py: give read-only params a poll `update_period` in `initialise()`; rw params still read once (ONCE). - simulation/eiger.py: add a lifespan background task that sweeps `temperature` between two values so the front end shows something updating (real server only; the in-process ASGI transport used in tests stays deterministic). - tests: cover idle-from-state, read-only poll vs rw read-once, and oscillation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fold the #402 review updates, "Resolved in review" statement dumps, and "Open questions" sections into one clean shape per ADR (Context / Decision / Consequences / Questions resolved in review). Make all seven consistent with the deleted io= and DataType: code examples are now all getter/setter + *Meta, and the runtime surface (.readback/.setpoint, poll()/poll_period, set()) is used uniformly across 0014/0016/0018/0019 instead of the old get()/put()/update_period/Waveform names. Preserves the @shihab-dls #402 replies (CA-vs-PVA setpoint visibility in 0014; @scan-vs-@command exposure in 0019), rewoven into the clean structure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
…ests - simulation/eiger.py: replace the sine sweep with a simple flip between two known temperatures every 0.5s (predictable); expose the parameter tree via `app.state.sim` as a test backdoor for read-only params with no PUT route. - tests: drop the sim-only tests; drive everything through the controller attributes. Idle test now pokes `state` via the sim backdoor and polls the attribute (rather than calling AttrR.update directly). Oscillation test builds a controller under the app lifespan and observes temperature via subscribe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Check the behaviour cancel_all is responsible for (disabling every ramp via its `enabled` attribute) rather than the wire-format strings the attribute IO layer happens to emit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cast - Sim: EigerParameter gains allowed_values, reported by GET only for discrete params (as the real detector does). state now advertises [idle, ready, acquire]. - Controller: a param reporting allowed_values is introspected as an Enum over an enum class built from those values. The members are only knowable over the wire, so state's hint drops to a bare AttrR - the exact-dtype hint check has no author-time class to match against. - EigerAttributeIO.update no longer casts to the dtype; attr.update validates, which is the one place a bad device value should be coerced or complained about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
demo: use ControllerVector for temperature ramp sub-controllers
demo: cut-down Eiger REST sim + introspectable controller example
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is not to be merged, but is there so we can make drive-by comments on lines of code as we see them be accepted in PRs into the refactor branch. When we are happy we will flip this PR to non-draft and this will become what is merged.