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
Add temperature_attr.py: a small temperature controller with per-attribute IO (a fresh AttributeIO/AttributeIORef pair per attribute) wired directly in __init__ rather than shared class-body declarations, foreshadowing the AttrRW(getter=, setter=) constructor params landing in #392. Baseline against the current callback-IO API. Closes #404
Address CodeRabbit review comment: close() closed the socket but left _connected True, so a subsequent status check would still report connected.
…ape) Rewrites the getter/setter baseline to the intended shape: one generic TemperatureIO drives every attribute, and each TemperatureIORef carries the command-building callables (read_cmd/write_cmd) sourced from a single TemperatureProtocol class - mirroring fastcs-thorlabs-mff's MFFAttributeIO/MFFAttributeIORef/ThorlabsAPTProtocol. This is the honest precursor to #392's AttrRW(getter=, setter=): read_cmd/ write_cmd ARE the getter/setter, promoted onto the constructor when the IO/ref wrapper is deleted, while TemperatureProtocol survives unchanged. Replaces the previous per-attribute AttributeIO subclasses (RampRateIO/PowerIO), which hardcoded commands and foreshadowed nothing. Response parsing (float()) is inline in TemperatureIO.update rather than a response_handler callable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
…ttr tests The project type-checks with pyright (standard mode), which does not flag assigning an AsyncMock over a bound method here, and `method-assign` is a mypy error code pyright never emits. pyright src tests is clean without them.
Rather than adding a second temperature module, retarget #404 onto the existing `fastcs.demo.controllers` so there is one temperature demo. `TemperatureProtocol`/`TemperatureRampProtocol` carry one method per wire command, `TemperatureIORef` carries the `read_cmd`/`write_cmd` callables, and a single generic `TemperatureIO` just invokes them - the same shape as `fastcs-thorlabs-mff`, and the honest precursor to `AttrRW(getter=…, setter=…)` in #392. Attributes move from the class body into `__init__`, which is what lets each ramp bake its index into its own protocol instance instead of the IO dispatching on a `name` string plus suffix. Composition, `@scan` and `@command` are unchanged, so this module now covers both the getter/setter rung and the composition rung; the README ladder collapses accordingly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Match the naming of the other demo modules (hello_world.py, temperature_scpi.py, eiger.py), which are named for the device and the style they demonstrate rather than for the framework concept. Updates the importers: `fastcs.demo.__main__`, the test module, the README ladder and the docs nitpick-ignore entry. The launch `type:` in fastcs.yaml is derived from the top-level package, not the submodule, so `fastcs.TemperatureController` and the checked-in schema.json are unaffected (verified by regenerating the schema). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
demo: convert temperature controller to getter/setter style
#412) * attributes: getter/setter IO rework, remove AttributeIORef/AttributeIO Per-attribute IO moves from a shared, ref-dispatched AttributeIO/ AttributeIORef pair onto plain getter/setter callables passed straight to AttrR/AttrW/AttrRW. Datatype is now optional on the constructors when it can be inferred from the getter/setter annotation. Runtime surface rename: get() -> .readback / .setpoint properties, no-arg update() -> poll() (does the getter read + caches + returns), update(value) stays as a pure cache-push (now also accepting Update[T]), put() -> set() (caches .setpoint, runs the setter, a non-None return updates .readback - the replacement for the old sync_setpoint-callback mechanism). Scheduling in Controller.create_api_and_tasks now polls getter-bearing attrs directly instead of going through an IO update-callback indirection. Removed: AttributeIO, AttributeIORef, ios=, _connect_attribute_ios, _validate_io, the second Attribute/AttrR/AttrW/AttrRW TypeVar. Migrates the demo composition example and all docs snippets that used the old io_ref= wiring. Deliberately out of scope for this PR (left for a follow-up): the DataType family / *Meta TypedDict replacement and the associated precision/Limits naming pass - the issue's own sizing note allows splitting the getter/setter half from the DataType-removal half. Closes #392 * docs: rewrite AttributeIO tutorial/how-to content for getter/setter API The docs build failed CI (fail-on-warning) because docs/tutorials/static-drivers.md's literalinclude emphasize-lines directives pointed at line numbers that no longer existed after the snippet rewrite. Fixing that surfaced the deeper issue: several tutorial and how-to pages narrated the removed AttributeIO/AttributeIORef pattern in prose, with code examples that no longer import. - Rewrite docs/tutorials/static-drivers.md and dynamic-drivers.md prose + literalinclude line references to match the getter/setter snippets. - Give docs/snippets/static15.py's TemperatureProtocol a Tracer base and thread `topic` through send_query, so the tutorial's per-attribute tracing walkthrough (enable_tracing on one attribute, see only its queries) still holds - a plain logger.trace call wouldn't respect per-attribute enable_tracing() at all. - Rewrite docs/how-to/update-attributes-from-device.md's four patterns (poll via getter, event-driven updates from a set, batched scan updates, scan-as-cache) for getter/setter. - Fix remaining AttributeIO/.get()/.put()/update_period mentions in docs/explanations/{transports,controllers,what-is-fastcs,datatypes}.md and docs/how-to/{table-waveform-data,wait-methods}.md. * attributes: address review - Polled, callback symmetry, setpoint mirroring Addresses the six review threads on #412. - Merge `getter` and `poll_period` into one argument via `Polled`: `AttrR(getter=Polled(protocol.get_temperature, period=0.1))`. A bare getter still means ONCE; `Polled(getter, period=None)` is on-demand only. - Symmetric callbacks: `add_on_update_callback` -> `add_readback_callback`, and a new `AttrW.add_setpoint_callback` alongside it. - `sync_setpoint` is gone. `Update` is now `readback`/`timestamp`/`setpoint`, where a `setpoint` of None leaves the cached setpoint alone. A bare value returned from a setter means both. - An AttrRW starts with no known setpoint; the first readback establishes it, which removes the need for transports to seed one. - Transports mirror the attribute's setpoint via `add_setpoint_callback` instead of tracking their own, so every transport agrees on it and CA no longer lags PVA. Recorded in ADR 0020; the one-shot seeding blocks in the CA and PVA transports are deleted. - `Attribute.__init__` is now strict about the datatype and the subclasses use cooperative `super().__init__()`: AttrR infers from the getter, AttrW from the setter, and AttrRW just passes both down the MRO, so the duplicated inference in AttrRW goes away. Also migrates the demo controllers that landed on refactor since this branch was cut (temperature_attr.py, eiger.py) off AttributeIO/AttributeIORef. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: drop unsupported "what most parameters want" claim about ONCE The ONCE default is settled in ADR 0014, but the ADR gives no rationale and this claim was not derived from anything - the repo's own examples lean the other way (49 Polled vs 0 bare getters across docs/snippets, 7 vs 0 in the temperature demo; only eiger.py's rw config branch uses a bare getter). Replace it with the criterion eiger.py actually applies: ONCE for values that change only when you change them, Polled for values the device changes itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * attributes: add NotPolled, keep bare getter as read-once-at-connect Replaces the `Polled(getter, period=None)` spelling for "never scheduled" with an explicit `NotPolled(getter)`, so all three schedules read as what they do: AttrR(Float(), getter=self._get_config) # once, at connect AttrR(Float(), getter=Polled(self._get_reading, period=0.2)) # every 0.2s AttrR(String(), getter=NotPolled(self._get_label)) # never; poll() only AttrR(Float()) # soft, no getter `period` is keyword-only, so a period always says what it is. Both wrappers take an optional getter and bind one when called, which lets the same objects serve the declarative spelling in #397, where the getter arrives by decoration rather than as an argument: `@attr(Polled(0.5), units="V")`. A bare getter stays read-once-at-connect rather than becoming unpolled. A bare `@attr` has to resolve to some schedule (ADR 18), so a constructor that refused to default while the decorator defaulted would reintroduce the asymmetry these wrappers exist to remove - and of the two candidate defaults, once-at-connect is the one that fails safe. Unpolled-by-default leaves an AttrRW at the datatype default, which under ADR 20 never establishes a setpoint either, so every transport would show 0/""/False until someone wrote to it. Amends ADR 0014 (schedule travels with the getter; records the three options considered) and ADR 0018 (`@attr` takes a schedule positionally instead of a `poll_period=` kwarg the constructor no longer has, with a table pairing the two spellings). Fixes the stale `poll_period=` examples in ADR 0013. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: rewrite ADR 0014 to describe the design as built The refactor-branch ADRs are unreleased, so 0014 is rewritten in place rather than accumulating amendments. Every decision and justification is kept; only stale text describing intermediate designs is dropped. - The schedule-travels-with-the-getter amendment is folded into the Decision as its own section, with the table pairing the procedural and declarative spellings and the three candidate defaults with the reason bare-means-once was chosen. - New section documenting Update as built (readback/timestamp/setpoint), why setpoint is there, and that severity belongs to ADR 16 rather than being described here as if it already existed. - Runtime surface table gains update_setpoint() and the two symmetric callback registrars, with a pointer to ADR 20 for why transports must not track their own setpoint. - Question 6 (is the setpoint echo visible across transports?) is answered rather than deferred: the CA-lags-PVA follow-up it left open is closed by ADR 20. Added question 7 for the poll_period merge. - Migration section now covers what happens to a ref's update_period, and Consequences names the one non-mechanical migration step: a driver relying on the old update_period=None default gains a connect-time read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * demo: protocol methods do their own IO, so they are the getters and setters The protocol class only built command strings, so an adapter (TemperatureLink) had to bind them to a connection before an attribute could call them - which put a layer between the protocol and the attribute and undersold the point of getter/setter. Make the protocol what a manufacturer would actually ship: one async method per command, doing its own IO and returning an annotated type. Those methods are then handed straight over: self.ramp_rate = AttrRW( getter=Polled(protocol.get_ramp_rate, period=0.2), setter=protocol.set_ramp_rate, ) TemperatureLink is deleted. Because the methods annotate their types, the datatype is now inferred for every attribute except target/actual, which state Float(prec=3) to carry display precision an annotation cannot - which shows both halves of the inference rule in one file. The enum infers its members from get_enabled's `-> OnOffEnum` return type. TemperatureRampProtocol becomes a subclass carrying a per-index suffix rather than a separate class, since the query/command plumbing is now shared. The wire format is unchanged - all existing tests pass untouched. Typing get_voltages caught a latent bug the untyped json.loads had hidden: it fed a list to a Waveform attribute rather than an ndarray. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: await the cancelled server task in the CA initial-value test The test requested cancellation but never awaited it, so the server's sockets and event loop were still live when the forked child exited. At interpreter shutdown that emits ResourceWarnings, which `filterwarnings = "error"` turns into a failure - reported against whichever test the collection lands on. It surfaced on 3.12 only, and not locally, so this is a fix for CI rather than something reproducible here; the redundant `except Exception: raise` is dropped while touching the block. Unrelated to the rest of this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert "test: await the cancelled server task in the CA initial-value test" This reverts commit 8992ab2. The change was speculative and did not fix the 3.12 failure - the leaked loop and sockets come from somewhere else, so the commit message's claim was wrong and the change is unrelated churn in this PR. The awaiting-a-cancelled-task point still stands on its own merits and is worth doing separately, alongside finding the actual leak. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * TEMP: enable tracemalloc in the tests env to locate the leaked event loop Diagnostic only - to be reverted before merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert "TEMP: enable tracemalloc in the tests env to locate the leaked event loop" This reverts commit f5f80c7. * test: stop out-of-band warnings failing whichever test is running PytestUnraisableExceptionWarning and PytestUnhandledThreadExceptionWarning are raised for events that happen outside any test - an exception during garbage collection, or in a non-main thread - and pytest attributes them to whichever test is running at the time. With `filterwarnings = "error"` that fails an unrelated test. This suite leaves objects alive in its subprocess and multiprocessing fixtures (run_ioc_as_subprocess's forkserver and Queues, the tickit Popen in test_docs_snippets), so a ResourceWarning is emitted whenever they are collected. Which test it lands on varied by Python version and by run: 3.12 was failing on tests/transports/epics/ca/test_initial_value.py, which neither touches those fixtures nor fails in isolation. Confirmed by running CI with PYTHONTRACEMALLOC=25, which adds the allocation traceback to each warning: they point at test_docs_snippets.py's Popen and conftest.py's run_ioc_as_subprocess/p4p_subprocess/softioc_subprocess. With tracemalloc's extra overhead all three Python versions failed, confirming the leak is universal and only masked by timing. Both warnings are downgraded to "report but do not fail" rather than silenced, so real leaks stay visible in the output; everything else still errors. The underlying fixture leaks are worth fixing separately - this only stops them failing unrelated tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * attributes: address review - document `always`, tighten test assertions Review follow-ups from @shihab-dls: - `AttrR.add_readback_callback`: document the `always` parameter. Its effect was only inferrable by reading `update()`, which decides whether to call a callback by comparing the new value with the cached one. - `AttrRW.set`: drop the "sanctioned replacement for the old private setpoint-echo mechanism" sentence. That is ADR material (0014/0020), not something a caller of `set()` needs; the docstring now just says what a returned value means. - `tests/test_attributes.py`: match on the exception message, not just the type. Applied to the two `pytest.raises` calls raised in review and to the four others in the same file, so the file is consistent - happy to narrow it back to the two if that is too wide. - `tests/example_p4p_ioc.py`: give the manual PVA test IOC some IO again. It lost all of it when `AttributeIO` went, so nothing in it exercised the replacement. `ChildController.clamped` is a getter/setter pair over an in-memory value whose setter clamps to 0..100 and returns what it accepted, which exercises both halves of ADR 0020 by hand: the getter seeds the setpoint at connect, and the clamped return drives readback and setpoint together. `test_ioc`'s PVI assertion is updated for the new PV. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * pva: build PVs during connect() so the seeded setpoint is served An AttrRW seeds its setpoint from its first readback (ADR 0020), and that readback arrives from the initial poll, which FastCS.serve() runs before it gathers the transports' serve() coroutines. P4PIOC built its PVs inside run(), i.e. inside serve(), so the setpoint callback did not exist yet when the seed happened: attribute.setpoint held the seeded value but a pvget on the setpoint PV returned the datatype default. EpicsCAIOC already builds its records in __init__ (during connect()) and was unaffected. Build the providers in P4PIOC.__init__ instead, leaving run() to serve them. parse_attributes had no awaits, so it becomes a plain function. Also addresses two review points on the tests: hoist the repeated expected message in test_datatype_required_when_not_inferable into a variable, and assert test_set_setter_exception_is_caught_and_logged actually logs the setter's exception rather than only implying it. Refs #392 * tests: add synced setpoint check in CA and PVA system tests --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Shihab Suliman <shihab.suliman@diamond.ac.uk>
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.