P1AM: scale the power-supply thermocouple, fault on missing feedback, one units contract - #4059
P1AM: scale the power-supply thermocouple, fault on missing feedback, one units contract#4059dieterolson wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 099dabe02b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| temp_full_scale_c: float = Field( | ||
| default=hardware.THERMOCOUPLE_FULL_SCALE_C, | ||
| gt=0.0, | ||
| description=( | ||
| "deg C at 100 % of temp_tag. The firmware publishes thermocouples " | ||
| "as percent of full scale, so this is what converts the tag to the " | ||
| "degC domain temp_alarm_max_c is expressed in. Defaults to the " | ||
| "firmware contract value; must match it." |
There was a problem hiding this comment.
Reject configurations where temp_full_scale_c differs from hardware.THERMOCOUPLE_FULL_SCALE_C rather than accepting any positive value. The admin PUT /api/power_supply/config route exposes this model, while SignalBroker.cpp always encodes temperatures against the fixed firmware constant; for example, accepting 700 makes an actual 1200 °C reading decode as 600 °C and can prevent the default HH temperature trip from firing. The field description says the values must match, but the model validator never enforces that API-boundary invariant.
AGENTS.md reference: AGENTS.md:L129-L133
Useful? React with 👍 / 👎.
| def _rebuild_filters(self) -> None: | ||
| """Re-create both deglitch filters from the controller's live config.""" | ||
| config = self.controller.config | ||
| self._k_filter = self._build_filter(config.type_k) | ||
| self._r_filter = self._build_filter(config.type_r) | ||
| self._control_sensor_holding = False |
There was a problem hiding this comment.
Avoid replacing both filters with empty instances whenever configuration is updated while the controller is running. If low-side burnout is selected and the active thermocouple opens immediately before or during such an update, the next zero becomes the new filter's trusted bootstrap sample; subsequent zeros have no jump from last_good and are accepted indefinitely, so with the backup channel unavailable the controller can continue calling for heat instead of reaching the hold timeout and tripping. Rebuild only affected filters while safely preserving/seeding their history, or force the controller out of RUNNING before clearing it.
Useful? React with 👍 / 👎.
| else: | ||
| # Rejected: report what is still in effect, not the request | ||
| # (issue #4017). | ||
| return float(self._setpoint_w) if self._setpoint_w is not None else 0.0 | ||
| return achievable_w |
There was a problem hiding this comment.
Handle IDLE and TRIPPED as the rejected cases instead of placing the effective-setpoint return in the final else. Those two states are already consumed by the preceding elif branches, so with valid voltage they fall through to return achievable_w; for example, an idle controller with 20 V feedback reports a 200 W request as achievable even though _setpoint_w remains unset. This leaves the power-mode API reporting rejected requests as applied despite the new contract.
Useful? React with 👍 / 👎.
| applied = float(self.controller.set_current_setpoint(value_a)) | ||
| self._record_setpoint( | ||
| PowerSupplyLastSetpoint(mode=PowerSupplyMode.CURRENT, value_a=value_a) | ||
| ) | ||
| # Persist only what actually took effect. Recording the raw request | ||
| # meant a setpoint the controller rejected was written to durable | ||
| # storage and pre-filled into the HMI after the next restart, so the | ||
| # operator was shown a value the plant never ran at (issue #4017). | ||
| if applied == float(value_a): | ||
| self._record_setpoint( | ||
| PowerSupplyLastSetpoint(mode=PowerSupplyMode.CURRENT, value_a=applied) |
There was a problem hiding this comment.
Determine acceptance from controller state/result semantics rather than requiring applied == value_a. A valid armed request outside the configured bounds is intentionally accepted and clamped—for example, 250 A becomes 200 A—but this equality check skips _record_setpoint, leaving the HMI with no value or a stale previous value instead of the applied 200 A. Conversely, a rejected request equal to the prior setpoint can satisfy the equality and still be persisted, so the comparison does not reliably distinguish acceptance.
Useful? React with 👍 / 👎.
…back Four defects in the power-supply and temperature sensor paths, each with a failing test written first. #4003 -- the firmware publishes thermocouples as PERCENT of full scale, and _inputs_from_tags scaled current and voltage but passed the temperature tag through as though it were already degC. HH_TEMP compares that against a degC threshold (default 1200), and broker tags are bounded [0, 100], so the power supply's only over-temperature interlock could not fire under ANY physically possible reading. The HMI showed "86 degC" on a load at 1200 degC, so the automatic protection and the human backstop were reading the same wrong number. #3998 -- full scale was declared in three places (firmware, temperature_models, thermocouple_filter) with a comment asking them to agree. It is now one definition in hardware.py, the module that already owns the firmware contract, with percent_to_celsius/celsius_to_percent as the only conversion. A new test parses the firmware source and fails if the two halves drift, so the contract is a gate rather than a comment. #4016 -- any absent tag became 0.0 via dict.get, and non-finite readings were coerced to 0.0. Unlike the temperature controller there was no sensor-fault state, so a renamed tag or a pulled monitor lead silently zeroed both trip inputs while the output stayed energized, and reported a confident, cold-looking supply. Missing or unusable feedback now raises SensorFeedbackError and latches a new SENSOR_FAULT trip; a genuine zero is still a reading. #4017 -- set_current_setpoint/set_power_setpoint returned the clamped request on every path including the IDLE and TRIPPED branches that discard it, and the service persisted the raw request regardless. An operator commanding 40 A into a latched controller got 200 {"applied_a": 40.0} and went troubleshooting the load instead of the trip; the bogus value was also pre-filled after the next restart. Both setters now return what is in effect, and persistence is gated on the command actually taking. #4035 -- both deglitch filters were constructed bare, pinning them to the 1400 C default whatever the channel was configured for. On a shorter-range channel the high-side burnout rail sat above any reachable reading, so an open thermocouple was accepted as genuine -- the exact condition the filter exists to catch. They are now built per channel with max_step_c scaled to the span, and rebuilt when the config changes so a rescale cannot strand filter state and trip TC_FAULT mid-run. Three existing tests encoded the defects and are updated to the corrected contract: the units pass-through assertion (#4003), IDLE echoing the request back (#4017), and "no tags uses zero feedback" (#4016). Also gitignores a root-level dcs_scada.db -- the suite creates the historian relative to the working directory, so a run from the repo root drops a stray artifact that is easy to commit by accident. 894 passed, 6 skipped in the backend suite. Refs #4003 #3998 #4016 #4017 #4035
099dabe to
8b495b2
Compare
Performance Benchmark ResultsNo benchmark results available. |
|
Superseded by #4448 (the P1AM SCADA safety consolidation), which merged to Containment was verified before closing, with all three stages of Closing to reduce CI load rather than because the work was unwanted. |
Pull request was closed
1 participant
Closes #4003. Closes #3998. Closes #4016. Closes #4017. Closes #4035.
Four defects in the power-supply and temperature sensor paths that share a root cause: the code treated a raw broker tag as though it were already in engineering units, and treated a missing tag as though it were a real reading of zero. Each fix has a failing test written first.
#4003 — the over-temperature interlock could not fire
The firmware publishes thermocouples as percent of full scale.
_inputs_from_tagsscaled current and voltage correctly and passed the temperature tag straight through as if it were degrees C:HH_TEMPcompares that against a degC threshold (default 1200), and broker tags are bounded[0, 100]. So the trip was unreachable by any physically possible reading — at the 1400 °C top of range the tag reads 100.0.A load at 1200 °C showed as "86" on the HMI, so the automatic protection and the human backstop were reading the same wrong number.
#3998 — full scale was declared three times
The constant lived in the firmware, in
temperature_models, and again inthermocouple_filter, held together by a comment asking them to agree. It is now one definition inhardware.py— the module that already owns the firmware register map — withpercent_to_celsius/celsius_to_percentas the only conversion.The new
tests/test_units_contract.pyparses the firmware source and fails if the two halves drift, so the contract is a gate rather than a comment. It searches the whole firmware directory rather than one file, so it keeps working now that the constant has moved fromSignalBroker.cppintoSignalBroker.h(#4044).#4016 — missing feedback was fabricated as a real zero
tags.get(name, 0.0)plus a_safe_finitecoercion meant an absent tag, a renamed route, or a pulled monitor lead silently zeroed both trip inputs while the output stayed energized — andstatus()reported a confident0.0 W/0.0 °C, which reads as a cold, idle supply.Unusable feedback now raises
SensorFeedbackErrorand latches a newSENSOR_FAULTtrip, driving the output safe. A genuine zero is still a reading — that distinction is what the old code could not express, and it is covered by its own test.#4017 — rejected setpoints were reported as applied
Both setters returned the clamped request on every path, including the IDLE and TRIPPED branches that discard it, and the service persisted the raw request regardless of whether it took.
An operator commanding 40 A into a latched controller got
200 {"applied_a": 40.0}and went off troubleshooting the load instead of noticing the trip. The bogus value was also written to durable storage and pre-filled after the next restart.Both setters now return the setpoint in effect, and persistence is gated on the command actually taking.
#4035 — deglitch filters ignored their channel's range
Both filters were constructed bare, pinning them to the 1400 °C default whatever the channel was configured for. On a shorter-range channel the high-side burnout rail sat above any reachable reading, so an open thermocouple was accepted as a genuine measurement — the exact condition the filter exists to catch.
They are now built per channel, with
max_step_cscaled to the span so "non-physical single-scan step" means the same fraction of range everywhere, and rebuilt on config change so a rescale cannot strand filter state and tripTC_FAULTmid-run.Three existing tests encoded the defects
These are updated to the corrected contract rather than the behaviour being preserved to keep them passing:
test_inputs_from_tags_scales_percent_to_engineeringassertedtemp_c == 800.0for a tag of800.0under the comment "Temp passes through unchanged" — it pinned Power supply over-temperature trip compares percent-of-full-scale against a degC threshold, so HH_TEMP can never fire #4003 as correct.test_setpoint_ignored_in_idle_stateasserted the setter echoes25.0back while the controller discards it (Setpoints rejected by the power-supply state machine are reported to the operator as applied, and persisted #4017).test_poll_with_no_tags_uses_zero_feedbackasserted the fabricated zeros were correct (Power-supply feedback fabricates 0.0 for missing or non-finite tags, silently disabling both trips #4016).Verification
894 passed, 6 skipped in the backend suite.
ruff check,ruff format,mypy,banditandpip-auditall clean through the pre-push gate.Housekeeping
Gitignores a root-level
dcs_scada.db. The suite creates the historian relative to the working directory, so a run started from the repo root drops a stray artifact that is easy to commit by accident — three separate work streams hit this today.Relationship to other PRs
SignalBroker.h; this PR's contract test reads it from either location, so the two can land in any order.🤖 Generated with Claude Code