fix(p1am-firmware): recover the comms watchdog and bumpless setpoints stranded off main (#3999, #4002)

[dieterolson]

Recovers the safe half of a firmware commit that was squash-merged onto a feature branch instead of main, leaving four safety defects live.

Closes #3999 — no Modbus comms watchdog, so the heater relay and analog outputs held their last command forever after host or link loss.
Closes #4002 — PID integral not reset when the setpoint is zeroed, so the AO held full output for tens of seconds after an E-stop.

Does NOT close #4001 — fixing it needs new trip semantics and a real percent-scaled limit set in defaults.py. The change that claimed to fix it is excluded here; see below.
Does NOT close #4032 — nothing in the original commit actually fixes it. SignalBroker::SetTag still clamps to [0.0, 100.0]; the only thing touching the issue is an IsLimitEffective docstring asserting a host-side rejection that is implemented nowhere.

Why this PR exists

PR #4044 was squash-merged onto origin/scada/firmware-harness-ci, not main. Its commit aaff7a76e is on no other ref, and that branch then stalled. #3999, #4001, #4002 and #4032 were all closed completed against it on 2026-08-02.

Verified by content, not ancestry: src/p1am_control_system/firmware/CommsWatchdog.cpp and .h do not exist on main, and the branch carries 10 changed files / 333 insertions under firmware/ that never shipped.

What ships

Change	Issue
CommsWatchdog.cpp/.h — dead-man timer on the SCADA link	#3999
firmware.ino watchdog wiring: re-arm on client accept and on a change to holding register 560, expiry drives AOs to 0, opens the heater relay, asserts Inhibit	#3999
PIDController::Hold/Release/IsHeld + integral reset on setpoint change	#4002
Measured-dt scan integration, bounded to [1 ms, 1 s]	#4009 (partial)
Harness repair + p1am-firmware.yml CI gate (from 50f753579)	—

The watchdog uses two independent re-arm signals because each misses what the other catches: a live Modbus TCP client covers host power loss, a killed backend and a pulled cable (all drop the socket), while the heartbeat register additionally catches a wedged backend holding an idle socket open.

Register 560 is the firmware half of a contract with the backend's HOST_HEARTBEAT_REGISTER. #4448 already ships the host half (write_heartbeat plus a per-scan stroke), so without this PR the backend strokes a heartbeat register no firmware reads.

Two hardening changes on top of 50f753579:

• The arduino-cli installer is pinned to v1.5.1 instead of tracking master. It is fetched and piped straight into a shell on a self-hosted runner, so a moving branch means the fleet executes whatever lands on it.
• Both jobs are now gated against fork pull requests. firmware-compile pipes that installer into a shell; firmware-unit-tests compiles and executes contributor-authored code via make test. Both run on d-sorg-fleet — the owner's own machines — with default_workflow_permissions=WRITE and can_approve_pull_request_reviews=true, while the repo is public with forking enabled and fork-PR approval set to first_time_contributors_new_to_github (required only of accounts brand new to GitHub). Only one other workflow in the repo gates on forks; a uniformly exposed repo is a reason not to add to the exposure, not a licence to match it.

What is deliberately excluded, and why

Everything below is from the same commit and is not in this PR:

• All of SafetyInterlock.cpp/.h. The trip-tier change makes lolo = 0.0 "effective" (IsLimitEffective accepts the closed [0, 100] interval, and its own test asserts IsLimitEffective(0.0f) is true) and trips on val <= lolo. With the shipped defaults.py (lolo=0.0 on all 32 tags), every routed tag sitting at exactly 0.0 — TAG_10/11, the analog outputs, plus TAG_4/5 — trips on the first scan. The trip action then re-writes 0.0 to the routed output tags, so the condition stays true and ClearTrip() can never stick: a permanent, unclearable latch, which is the Default interlock config permanently trips the PLC, and there is no path to reset the trip #4001 symptom the commit claims to fix. Separately hihi = 100.0 sits exactly on SetTag's clamp ceiling, so the over-temperature trip that previously fired at high = 95.0 (≈1330 °C) becomes unreachable. The IsTagRouted guard narrows evaluation from 32 tags to the 8 routed ones but does not rescue it.
• SignalBroker::IsTagRouted() — used only by Evaluate, so it belongs with the part that is not shipping.
• The coil-1 → ClearTrip() pulse in firmware.ino. Inert without the tier change (Evaluate re-trips on the next scan) and it is interlock behaviour, so it should land with the reviewed fix. Note main currently has no coil-1 wiring at all, so a latched trip is recoverable only by power cycle — that is part of Default interlock config permanently trips the PLC, and there is no path to reset the trip #4001, not something this PR changes.
• Four tests: TestInterlockTripsOnHihiLoloNotHighLow, TestInterlockIgnoresUnroutedTags, TestInterlockTripIsRecoverable, TestInterlockLimitDomain. The last asserts IsLimitEffective(0.0f) is true — it encodes the defect as correct.

Everything that does ship only ever reduces output — WriteHeaterRelay(false), WriteAnalogOutput(0), WriteInhibit(true), shed integral — so it cannot make an already-broken interlock worse.

Reviewer notes

Two things a firmware reviewer should look at deliberately. Neither blocks this PR.

1. The comms-lost block writes into the tags the interlock trips on. On expiry it does broker.SetTag(tag_id, 0.0f) for each routed output tag, then zeroes the hardware. That 0.0 is exactly what main's Evaluate treats as below low = 5.0, so it will trip the interlock on the following scan. On main's already-unconditionally-latched interlock this is moot, and the direction is de-energize either way — but it is the one place where this PR's behaviour touches interlock semantics. Clearing the stale command is deliberate: without it, a restored link would let Evaluate re-drive the AOs from the last host value.
2. The #4002 reset is scoped to the zeroed case, deliberately. The recovered commit reset the integral on next != setpoint_, i.e. any change. Since SyncModbusToDCS (firmware.ino:108) calls SetSetpoint on every scan whenever the host register differs, that cleared the integrator every scan throughout a host-driven ramp — or on 1-LSB float jitter through the register round-trip — leaving the loop running P+D only with a steady-state offset it could never close, and no indication integral action had been disabled. Narrowed to next == 0.0f && setpoint_ != 0.0f, which is the condition issue 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 actually specifies. If reset-on-change is ever wanted for bumpless transfer on large steps, it needs a deadband, not an equality test.

Verification

The host harness compiles all six sources and all 9 tests pass with assertions active, including the pre-existing TestSafetyInterlock against main's unchanged interlock — which is what demonstrates the separation is real:

TestSignalBroker / TestPIDController / TestSafetyInterlock / TestStorageManager
TestSoftFailRuntimeContracts / TestPidResetsIntegralOnSetpointZeroed
TestPidKeepsIntegralAcrossNonZeroSetpointChange
TestPidDoesNotIntegrateWhileTripped / TestCommsWatchdog          -> all 9 PASSED (exit 0)

Built locally with MSVC 14.50 because this box has no POSIX toolchain; firmware-unit-tests re-runs the same sources under g++ with -Wall -Wextra. firmware-compile is the first real arduino-cli compile of firmware.ino in CI, so treat it as the gate on the sketch itself.

TestPidKeepsIntegralAcrossNonZeroSetpointChange was verified to be a real regression test, not a tautology: with the broad next != setpoint_ condition restored it fails at the clamp assertion, and it passes with the narrowed condition. Committed and pushed with hooks enabled (no --no-verify). Based on current main (73fd3d49); expects a re-sync after #4448 lands.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42f08aea42

ℹ️ 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".

[chatgpt-codex-connector]

Keep the timeout above the slowest healthy poll interval

With the backend in its default lightweight mode, settings.py sets the poll interval to 2.0 seconds and main.py:366-430 sleeps for that interval only after completing each scan, so successive Modbus request bursts are necessarily more than 2 seconds apart. This commit contains no backend writer for register 560, leaving those requests as the only recurring activity; consequently this exact 2000 ms timeout expires during normal operation, zeros the output tags, and can make the existing low-limit interlock latch on the following firmware scan. Use a timeout with scheduling margin above every supported healthy cadence or ship the heartbeat writer atomically with this change.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Gate host unit tests against fork pull requests

In the inspected p1am-firmware.yml, this job runs on the self-hosted d-sorg-fleet for every matching pull_request, unlike the compile job's same-repository guard at lines 90-92. A fork can modify the in-scope Makefile or C++ test sources and have make test compile and execute that attacker-controlled code on a persistent fleet machine; apply the same fork condition before assigning this job to the self-hosted runner.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve integral across ordinary setpoint updates

For any nonzero setpoint step or host-driven ramp, this condition clears the accumulated integral and derivative history, so the next output drops the previous I contribution instead of transferring bumplessly; repeated setpoint changes prevent an integrating loop from eliminating steady-state error. The safety requirement in SPEC.md:71-75 is specifically to shed the integral when the setpoint is zeroed, so restrict the reset to that transition or rebalance the state to preserve the current output for ordinary updates.

Useful? React with 👍 / 👎.