Repair orphaned P1AM firmware test harness and gate it in CI - #4043
Repair orphaned P1AM firmware test harness and gate it in CI#4043dieterolson wants to merge 7 commits into
Conversation
The P1AM firmware owns the safety interlock, the four PID loops, and the
analog/relay outputs -- it is what keeps the plant safe when the Raspberry Pi
host is gone. Nothing in CI compiled it and nothing ran its tests.
A host-side harness already existed (tests/p1am_control_system/firmware/): a
Makefile that builds the real firmware sources against a fake HardwareInterface,
with assertions on scaling, anti-windup, trip latching and NaN soft-fail. It
sits inside a directory pytest collects but holds no .py files, so it was never
executed -- and it had rotted to the point of not compiling:
- MockHardware did not implement WriteHeaterRelay, so it was abstract.
- The StorageManager round-trip still used the pre-lolo/hihi 4-argument
Save/Load signature.
- TestSignalBroker asserted 350 C -> 35.0%, an expectation left over from a
1000 C full scale; the firmware has since moved to 1400 C.
Repairs, and what they buy:
- MockHardware implements the full interface. It also records the highest
value each analog output was ever commanded to, so a safety test can assert
an output was *never* energized rather than merely reading zero now.
- The StorageManager test round-trips all four interlock tiers and checks an
untouched tag keeps its saved value, so a future struct change cannot
silently drop lolo/hihi the way the last one did.
- kThermocoupleFullScaleC moves from a function-local literal in
SignalBroker.cpp to a public constant in SignalBroker.h. The test derives
its expected percentage from it instead of hardcoding one, which is what
let that assertion go stale. This also gives the backend cross-check in
#3998 a single named definition to read.
- test_dcs.cpp #errors under NDEBUG. Every check is an assert(), so a build
that compiled them away would exit 0 having tested nothing.
CI gains two gates: firmware-unit-tests (plain g++, seconds, the TDD vehicle
for firmware work) and firmware-compile (arduino-cli against the real board
package, catching library and signature breakage the host harness cannot see).
Toolchain versions are written to the job summary so a binary can be traced to
the toolchain that built it.
Refs #3995
![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: 50f7535794
ℹ️ 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".
| curl -fsSL https://raw.githubusercontent.com/arduino/arduino-cli/master/install.sh \ | ||
| | BINDIR="$HOME/.local/bin" sh |
There was a problem hiding this comment.
When the firmware compile job runs, it downloads install.sh from the mutable master branch and pipes it directly into a shell on a self-hosted fleet runner. An upstream compromise or unexpected branch change can therefore execute arbitrary commands under the runner account; install a versioned Arduino CLI artifact and verify its checksum before execution.
Useful? React with 👍 / 👎.
| const float kExpectedPct = | ||
| kTempC * (100.0f / SignalBroker::kThermocoupleFullScaleC); |
There was a problem hiding this comment.
Deriving kExpectedPct from the same firmware constant used by ReadHardwareInputs makes this assertion pass for any full-scale value, even if someone changes the firmware constant without updating the backend's independent 1400 °C default in backend/temperature_models.py:63-66. In that scenario every reported temperature is scaled incorrectly while this new firmware gate remains green, so assert the contractual 1400 °C value or an independently calculated 25% result instead.
Useful? React with 👍 / 👎.
…ints (#4044) Four safety defects in the P1AM firmware, each with a failing host test written first (the harness repaired in the parent PR is what makes that possible). #3999 -- no dead-man timer on the SCADA link. The heater relay command is a coil read and the analog outputs are driven from broker tags; both held their last value forever once the host died. The host cannot cover this case because the host is the thing that died. New CommsWatchdog drives all AOs to 0%, opens the heater relay, asserts Inhibit and holds the PID loops after 2 s of silence. Two independent activity signals, because each misses a case the other catches: a live Modbus TCP connection (covers host power loss, killed backend, pulled cable) and a host heartbeat register at 560 (covers a wedged backend holding an idle socket open). Deliberately Arduino-free so the rollover behaviour is testable -- millis() wraps every ~49.7 days and a naive comparison would disarm the watchdog for another 49 days at the wrap. #4001 -- the trip fired on the low/high band, which is the SCADA layer's severity-1 *warning* tier, and evaluated all 32 tags including unrouted ones sitting at 0.0. The stock config writes low=5.0 to every tag, so deploying it latched the plant off; and since ClearTrip() had no callers and coil 1 was never read, the latch was unrecoverable short of a power cycle -- after which the flash-saved config tripped it again on the first scan. Evaluate() now trips on hihi/lolo and skips unrouted tags (SignalBroker::IsTagRouted -- the broker owns routing, so the predicate lives there). Coil 1 clears the latch and is written back to 0 as a pulse acknowledgement. #4002 -- SetSetpoint did not touch the integrator, and Compute() ran even while tripped. Zeroing the setpoints is the only part of the host E-stop that reaches the plant, so a wound-up integral held the AO at 100% of its 4-20 mA span for tens of seconds after the operator commanded a stop. SetSetpoint now clears integral/derivative history on a change (bumpless transfer, which is also the right behaviour for ordinary retargeting), and Hold()/Release() freeze the loops while tripped or blind so recovery starts clean. #4032 -- broker tags are clamped to [0,100], so any limit above 100 was unreachable and its trip silently dead. An operator entering 900 for 900 degC on a percent-scaled tag disabled the interlock with no indication. SafetyInterlock::IsLimitEffective distinguishes a deliberate +/-99999 "never trip" sentinel from an unreachable entry; Evaluate() skips both rather than comparing against a threshold that cannot be crossed, and the host can use the same predicate to reject the configuration at the API boundary. Also folds in the firmware half of #4009: the scan integrated against a hardcoded 0.1 s while the real period runs well past that (~300 register reads, SPI thermocouple reads, and a blocking flash write on config deploy). It now uses the measured interval, bounded to [1 ms, 1 s]. The pre-existing TestSafetyInterlock encoded the old warning-band-trips behaviour and is updated to the corrected contract. All 12 host suites pass under g++ 11.4. The sketch itself is covered by the arduino-cli gate. Refs #3999 #4001 #4002 #4032 #4009
Performance Benchmark ResultsNo benchmark results available. |
Performance Benchmark ResultsNo benchmark results available. |
|
Deliberately NOT included in the consolidated P1AM safety batch (#4448). Left open — please do not merge as-is. The other eleven PRs in that batch merged. This one is held back because the second commit ( The first commit ( The blocker, verified against the working tree rather than inferred
lolo_limit=0.0, low_limit=5.0, high_limit=95.0, hihi_limit=100.0and This commit adds: if (IsLimitEffective(hihi_limits_[i]) && val >= hihi_limits_[i]) {
if (IsLimitEffective(lolo_limits_[i]) && val <= lolo_limits_[i]) {with assert(SafetyInterlock::IsLimitEffective(0.0f));
assert(SafetyInterlock::IsLimitEffective(100.0f));Two consequences, in opposite directions:
Root cause: the closed comparisons are degenerate at the clamp boundaries ( Other findings, in rough priority
Suggested path
Note that #3995 (firmware never compiled or tested in CI) is therefore not carried by #4448 and stays open — it is this PR's issue. |
|
Closing — excluded from the P1AM SCADA consolidation (#4448) on safety grounds, not superseded. Against the shipped
That is precisely the symptom described in #4001, which this PR claims to fix — so as written it Commit 1 is good and should land on its own — the orphaned firmware test harness genuinely needs Why closed rather than left open: auto-merge (SQUASH) was armed on this PR, and in this repo Context: part of the 2026-08-13 consolidation drive. The other 11 P1AM PRs are consolidated in #4448. |
1 participant
Closes #3995.
The P1AM firmware owns the safety interlock, the four PID loops, and the analog/relay outputs — it is what keeps the plant safe when the Raspberry Pi host is gone. Nothing in CI compiled it and nothing ran its tests.
What was already there, and why it stopped working
A host-side harness existed at
tests/p1am_control_system/firmware/: a Makefile that builds the real firmware sources against a fakeHardwareInterface, with assertions covering scaling, anti-windup, trip latching and NaN soft-fail. It sits inside a directory CI does collect (ci-standard.yml:701addstests/p1am_control_system/) but holds no.pyfiles, so pytest walked straight past it.Never being run, it rotted until it no longer compiled:
MockHardwaredid not implementWriteHeaterRelay, so the class was abstract and everyMockHardware hw;was a hard error.StorageManagerround-trip still used the pre-lolo/hihi 4-argumentSave/Load; the real signature took 6 onceInterlockConfigDatagrew to four limits (kMagicbumped 0xDC51 → 0xDC52).TestSignalBrokerasserted 350 °C → 35.0 %, an expectation left over from a 1000 °C full scale. The firmware moved to 1400 °C, making the correct answer 25.0 %.That last one is the interesting failure: the suite was not merely unbuilt, it encoded a stale physical constant. This is the class of drift the harness exists to catch.
Changes
Harness repair
MockHardwareimplements the full interface. It additionally records the highest value each analog output was ever commanded to, so a safety test can assert an output was never energized rather than merely reading zero at the end — the distinction that matters for the watchdog and E-stop work in Firmware has no Modbus comms watchdog: heater and analog outputs stay energized forever after host or link loss #3999/Firmware PID integral is not reset when the setpoint is zeroed, so the AO holds full output for tens of seconds after E-stop #4002.StorageManagertest round-trips all four interlock tiers and asserts an untouched tag keeps its saved value, so a future struct change cannot silently drop lolo/hihi the way the last one did.test_dcs.cppnow#errors underNDEBUG. Every check in the suite is anassert(); a build that compiled them away would exit 0 having tested nothing.DRY fix that makes the stale assertion impossible to repeat
kThermocoupleFullScaleCmoves from a function-local literal inSignalBroker.cppto a publicstatic constexprinSignalBroker.h. The test derives its expected percentage from that constant instead of hardcoding one — hardcoding is exactly what let the assertion go stale. This also gives the backend cross-check in Thermocouple full-scale is a firmware compile-time constant but an operator-editable backend field, with no cross-check #3998 a single named definition to read against.CI
.github/workflows/p1am-firmware.yml, path-filtered to the firmware and its tests, pinned to[d-sorg-fleet, Linux](the pool is heterogeneous and the Makefile needs a POSIX toolchain):firmware-unit-tests— plaing++, no board toolchain, runs in seconds. This is the TDD vehicle for the rest of the firmware cluster.firmware-compile—arduino-cliagainst the realP1AM-100:samdpackage, catching library and signature breakage the host harness cannot see (it never includesP1AM.horEthernet.h).Housekeeping
.gitignorefor*.o,test_dcs, andconfig.bin(StorageManager persists to it when exercised on the host), so a localmake testcannot dirty the tree.Verification
Built and run under WSL Ubuntu-22.04, g++ 11.4:
No Python or frontend code is touched.
Follow-on
This PR deliberately does not fix any firmware behaviour — it only makes the firmware testable and gated. The defects the harness now protects against are next, each with a failing test written first:
ClearTripreachabilityOne gap this PR exposes and does not close:
SafetyInterlock::Evaluatereads onlyhigh_limits_/low_limits_, and the harness only ever exercised those — nothing tests the HIHI/LOLO tier because nothing evaluates it. That is #4001's job.🤖 Generated with Claude Code