Read a two-state signal by its index, not the facility's word for it - #718
Merged
Conversation
Three times now a facility spelled its enum labels differently and CORA
read nothing: the hutch permit on 2026-08-09 (int('ON') raised), the
beam permits on 2026-08-18 (identical), and now BLEPS. Each was fixed by
widening the label set, which cannot converge. ZNAM/ONAM, Tango's
enum_labels and NTEnum's choices are free text one engineer chose at one
facility, constrained by nothing, so no label set is ever complete.
Measured on arcturus 2026-08-23 against the running 2bmBLEPS IOC
(iocBoot/ioc2bmBLEPS/bleps.substitutions, records from bleps_bi.db):
trips and warnings declare ZNAM="NO_FAULT" / ONAM="TRIP", faults and the
comms flag declare ZNAM="" (the empty string) / ONAM="Present". caget
returns those literal words. So every BLEPS channel resolved to None,
the comms flag included, which reads as a dark feed and suppresses the
whole observer: the supply half of #717 could not have recorded anything
at 2-BM. This answers BLEPS-4, which questions.md carried as
Blocks-go-live.
The index behind those labels is not free text. Every adapter already
held it at the moment it picked a label and then discarded it, so
Measurement gains an `ordinal` carried BESIDE the label, and binary_code
prefers it with the conventional labels kept as the fallback. Same
division quality (domain meaning) already makes with quality_detail
(substrate breadcrumb). The label stays load-bearing in its own right:
scan settings like ScanType are recorded as words, and that is the
meaningful fact there.
This generalizes where a wider dictionary could not. All four adapters
hand over an index; caproto already published one, so the fleet was
inconsistent and only binary_code's dual int/str handling hid it. MAX IV
and ESRF are Tango floors on the same growth ladder, and their labels
are theirs to choose.
Tango DevState is carved out. It has an ordinal, but it indexes Tango's
global device-state vocabulary (ON = 0, OFF = 1), so publishing it would
resolve an ON device to false. The carve-out keys on attr.type, the same
field _kind_for reads, not on a property of the value: a proxy check
would miss a DevState delivered as a bare int and would silently
withhold a DevEnum that happened to expose .name.
Also closes a fail-open the hoist would otherwise have spread: int(0.4)
truncates to 0, and 0 is a confirmed state to every caller, meaning
"open" on the polarity-inverted shutters. The beam lookup guarded that
at its own call site; the guard belongs in the shared decoder now that
five consumers share it, and it stays there as belt-and-braces.
Every guard is mutation-verified. Two of the new tests were decorative
when first written and are recorded as such in their docstrings: the
DevState test passed because its fake was not int-convertible rather
than because the carve-out fired, and the out-of-range test used labels
the fallback also rejects. Both now fail when their guard is deleted.
The soft IOC gains two records carrying BLEPS's real label pairs so the
proof runs over real DBR_ENUM rather than a shape no bi record produces,
which is how the same defect shipped twice before.
What this gives up is written into binary_signal's docstring rather than
left implicit: an unconventional label used to make a MIS-POINTED record
fail closed by accident, and it no longer does. That protection was a
coincidence, firing only when the wrong record had unusual labels,
missing every wrong record with conventional ones, and rejecting correct
records with unusual ones. The out-of-range check catches only a wrong
record resting outside 0/1. State-count hardening was considered and
declined here: it would couple the ordinal to label resolution, and its
independence from the label cache is what makes it correct on a cold
first read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The names this repo carried for 2-BM's BLEPS channels do not exist on the beamline. operations.md stated that the EPICS PV is the PLC tag with dots swapped for underscores, and every PV built from that rule was wrong: PLC `Flow2.Below_Set_Point_Trip` is `FLOW2_TRIP`, not `FLOW2_BELOW_SET_POINT_TRIP`. The rule holds for some tags (`GV1.Faulted` really is `GV1_FAULTED`) and not others, which is what made it look right. The config example in `Settings.bleps_supply_channels` is the part that mattered: it is the block a deployment copies, and every PV in it was fictional, so BLEPS_SUPPLY_CHANNELS built from it would have bound nothing and the observer would have sat silent with a valid-looking config. Corrected to the real names, with the derivation warning next to them. Read off the running 2bmBLEPS IOC on arcturus 2026-08-23 (iocBoot/ioc2bmBLEPS/dbl-all.txt, and bleps.substitutions for the tag-to-PV pairing). The sharpest example is one channel disagreeing with itself: the trip is `FLOW2_TRIP` and its own warning is `FLOW_2_UNDER_WRN`, with an underscore before the digit. No rule produces both. Nothing here is derivable; it has to be read. The PLC tag names elsewhere on the page were already right and are left alone. What changed is the claim that you can get a PV name from one. Also resolves BLEPS-4, which questions.md carried as Blocks-go-live. Settled by reading the IOC rather than by asking staff: the flags are `bi` records labelled NO_FAULT / TRIP, and the fault and comms flags carry an EMPTY ZNAM with ONAM `Present`. That page now says so, and says that CORA reads the index rather than any of those words, so nothing further is needed from the beamline. The empty ZNAM is noted as worth mentioning to staff, not as a fix CORA is waiting on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The diff-coverage gate on #718 failed on exactly the three lines an adversarial review had already called unreachable, which is a pleasing convergence: the review reasoned it from call order, CI measured it. `_enum_ordinal` in the CA and PVA adapters wrapped its cast in `try / except (TypeError, ValueError)`. That branch cannot execute. `_to_reading` calls `_unpack_value` first, and its Categorical arm casts the same object, so anything that will not cast has already raised out of `read` before the ordinal is asked for. The guard was not defensive depth, it was an untestable line asserting a behavior the module does not have, and the docstring I wrote defending it was wrong on its own terms. Both are now unguarded and typed `-> int`, failing the same way as the cast one line earlier. Tango is genuinely different and keeps its guard. Its `_unpack_value` returns `value.name` early for any named value and never reaches its own cast, so a `DevEnum` carrying a named-but-not-numeric value arrives at `_enum_ordinal` uncast and the guard is the first thing to see it. That path now has a test rather than an assertion: the reading keeps a usable label and simply carries no ordinal, so a two-state consumer falls back to the label. Mutation-verified, deleting the guard turns that one test red and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 23, 2026
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
1 participant
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.
Summary
CORA reads interlock trips, hutch permits and beam shutters as two-state signals. Enum-valued ones arrive carrying both a numeric index and a facility-authored text label, and CORA kept only the label, matching it against a hardcoded set (
ON/OFF,TRUE/FALSE,YES/NO). That set can never be complete, because nothing constrains what those words can be.Three incidents have now had the same cause: the hutch permit (2026-08-09), the beam permits (2026-08-18), and BLEPS. Each was fixed by widening the dictionary.
Measurement.ordinal, the index carried beside the label rather than instead of it. Same divisionqualityalready makes withquality_detail. All four adapters populate it; every one already held the index at the moment it picked a label, then discarded it.binary_codeprefers the ordinal, keeps the conventional labels as the fallback for genuine string records. The keyword is required, so pyright enumerates the call sites rather than a reviewer.DevStateis carved out: it has an ordinal, but it indexes Tango's global state vocabulary (ON = 0), so publishing it would resolve an ON device to false. Keyed onattr.type, not on a property of the value.int(0.4)truncates to0, which means open on the polarity-inverted shutters. One consumer guarded this locally; the shared decoder now does.Measured on arcturus 2026-08-23: the
2bmBLEPSIOC labels tripsNO_FAULT/TRIPand its fault and comms flags""/Present. The empty one is the healthy state of the flag saying whether BLEPS is talking at all. Resolves BLEPS-4, whichquestions.mdcarried asBlocks-go-live.Why the index rather than a wider dictionary
The index generalizes where a dictionary cannot. Every substrate CORA supports hands one over; caproto already published it, so the fleet was inconsistent and only
binary_codeaccepting both int and str hid it. MAX IV and ESRF are Tango floors on the same growth ladder and their labels are theirs to choose.Known limitation, deliberately accepted
An unconventional label used to make a mis-pointed record fail closed by accident, and it no longer does. That protection was a coincidence: it fired only when the wrong record had unusual labels, missed every wrong record with conventional ones, and rejected correct records with unusual ones. Written into
binary_signal's docstring, including why state-count hardening was considered and declined.Test plan
birecord produces (how the same defect shipped twice before)subscriberather thanreadtest_absent_timestamp_mapping.pyFollow-up found while verifying, NOT in this PR
A tripped BLEPS flag arrives with
MAJORseverity (the IOC setsOSV=MAJORso it annunciates), which CORA maps toUncertain, and the BLEPS reader discards anything butGood. So a trip is still dropped even with this merged. Same bug the hutch permit already fixed by loosening its floor toBad-only. Next slice, before BLEPS is enabled at 2-BM.🤖 Generated with Claude Code