Read the detector-done poll by index, and give it a quality floor - #724
Merged
Merged
Conversation
`Acquire_RBV` decided "the acquisition is over" with `value in (0, "Done")`. A real areaDetector `Acquire_RBV` is a `bi` record, so it arrives as `kind="Categorical"` carrying whatever ZNAM/ONAM the IOC declares, with the index it resolved from on `Measurement.ordinal`. "Done" is ADCore's default label, not a guarantee: a relabelled record, or a build whose defaults differ, produces a value the check cannot recognise, and the poll loop then waits forever on a detector that already finished. This is the fourth instance of a defect family already fixed three times this month (the hutch permit, the BLEPS interlock flags, the beam-availability gate), and the comparison also read `reading.value` with no quality floor at all, so a `Bad` reading whose stale value happened to decode to Done would have recorded a capture as finished when the value meant nothing. `_acquisition_finished` reads the ordinal via the same `binary_code` decoder the other three fixes use, and floors on `believable`, not `actionable`. That choice matters here specifically: a detector carrying a standing `Uncertain` alarm for a reason unrelated to whether it finished (a temperature warning, a nearly-full file-writer disk) must still be readable as Done, or every acquisition on it would hang instead of finish. An alarm on `Acquire_RBV` says nothing about whether the acquisition is over; only `Bad` may withhold a conclusion. The two duplicated poll loops (`_run_collect_cycle`, `continuous`) are now one shared `_await_acquire_done`, which also closes the reason the bug existed in two copies to begin with. `stream`'s `NumCaptured_RBV` comparison had the same missing floor, worse in one way: a non-numeric reading raises `TypeError`, which is not one of the Conductor's closed `_CONTROL_ERRORS`, so it would have escaped the Conductor uncaught rather than being recorded as a step failure. None of this bounds the poll loop's wait, which stays the documented v1 choice (Procedure abort is the caller-side timeout). The fix stops an ordinary label mismatch or a stray alarm from making that wait silent and indistinguishable from a slow detector; it does not change when the wait ends. Not reachable at 2-BM (`CONTROL_WRITES_ENABLED=false`), so this is a correctness fix for the first deployment that drives a detector, caught by review rather than by a facility. `quality.py` gains a "per QUESTION, not per component" section: the Conductor itself now uses both floors (`actionable` for check steps, `believable` for this poll), so the module's earlier framing that one BC means one floor was no longer accurate. Both softIOC-backed test tiers previously seeded `Acquire_RBV` as a bare Scalar 0, a shape the substrate cannot produce, which is why neither could see this bug. The integration fixture is now a real `bi` (ZNAM=Done); the unit fixtures across all four action-body test files now build the Categorical + ordinal shape. Five mutations (drop the ordinal path, drop the believable guard, tighten to actionable, drop the once-per-episode log flag, drop stream's guard) were each run and each caught by exactly the test built for it.
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
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
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.
Why
Acquire_RBVdecided "the acquisition is over" withvalue in (0, "Done"). A real areaDetectorAcquire_RBVis abirecord, so it arrives askind="Categorical"carrying whateverZNAM/ONAMthe IOC declares, with the index it resolved from onMeasurement.ordinal."Done"is ADCore's default label, not a guarantee: a relabelled record, or a build whose defaults differ, produces a value the check cannot recognise, and the poll loop waits forever on a detector that already finished.This is the fourth instance of a defect family already fixed three times this month:
SecureM)The comparison also read
reading.valuewith no quality floor at all. ABadreading whose stale value happened to decode to Done would have recorded a capture as finished when the value meant nothing — a fail-open into the recorded evidence.Not reachable at 2-BM (
CONTROL_WRITES_ENABLED=false), so this is a correctness fix for the first deployment that drives a detector, caught by review rather than by a facility.What
_acquisition_finishedreads the ordinal via the samebinary_codedecoder the other three fixes use, and floors onbelievable, notactionable. That choice is the interesting part: a detector carrying a standingUncertainalarm for a reason unrelated to whether it finished (a temperature warning, a nearly-full file-writer disk) must still be readable as Done, or every acquisition on it would hang. An alarm onAcquire_RBVsays nothing about whether the acquisition is over; onlyBadmay withhold a conclusion.The two duplicated poll loops (
_run_collect_cycle,continuous) are now one shared_await_acquire_done— they had carried identical copies of the loop, which is how the same bug needed fixing in two places.stream'sNumCaptured_RBVcomparison had the same missing floor, worse in one way: a non-numeric reading raisesTypeError, which is not one of the Conductor's closed_CONTROL_ERRORS, so it would escape the Conductor uncaught rather than being recorded as a step failure.Scope, confirmed before starting: decode + quality only. The poll loop's wait stays unbounded, which is a documented v1 choice (
_POLL_INTERVAL_S: "relies on caller-side cancellation (Procedure abort) for hard timeout"), consistent with the cancellation model locked in the scan-primitives design (Conductor halts, records, operator/saga decides). This fix stops an ordinary label mismatch or a stray alarm from making that wait silent and indistinguishable from a slow detector; it does not change when the wait ends. An unreadable reading now logs once per continuous stretch (not once per 50ms tick) and recovers with an info line.cora/shared/quality.pygains a "per QUESTION, not per component" section: the Conductor itself now uses both floors (actionablefor check steps,believablefor this poll), so the earlier framing that one BC means one floor was no longer accurate.Test fixtures were part of the bug
Both softIOC-backed test tiers seeded
Acquire_RBVas a bareScalar0, a shape the substrate cannot produce — the same blind spot that hid the previous three occurrences. Fixed:tests/integration/_softioc.py:cam1:Acquire_RBVis now a realbi(ZNAM="Done"/ONAM="Acquiring"), not alongin.Categorical+ordinalshape (_acquire_rbv_reading()intest_collect_action_body.py, mirrored intocontinuous/discrete).Verification
Full suite: architecture 34,121 passed, unit 14,333 passed. Pyright, ruff, tach clean.
Five mutations run, five caught, each isolated with a small
asyncio.wait_for-bounded standalone script (pytest-timeout's thread method doesn't actually cancel a genuinely-hungwhile True: await asyncio.sleep()loop, so mutations that hang the poll were verified outside pytest to keep the loop bounded):believableguardactionablestream's guard🤖 Generated with Claude Code