feat(runtime): run/stop state control with a hardware mode switch - #162
Conversation
Lets a vendor plugin that owns a physical RUN/STOP switch hold the PLC in stop, so hardware is authoritative over the editor. The transition flow is unchanged: request_plc_start mirrors the existing request_plc_stop, and both are thin wrappers over the same plc_begin_transition() the socket START/STOP handlers already drive. The only genuinely new state is one atomic holding the switch position. A plugin reports the position with set_switch_position() and then asks for the matching transition. Store first, then request: on a falling edge that closes the window where a start could slip in, and on a rising edge it stops the request being refused by the guard still reading a stale STOP. Sample the switch from a thread started in init() and torn down in cleanup(), not start_loop()/stop_loop() -- the runtime calls stop_loop when the PLC stops, which is exactly when the position matters most. Every start path consults the same guard: the socket START (now answering START:ERROR_SWITCH_STOP), the boot auto-start, and request_plc_start itself. Refused, never queued, so the editor can explain why. transition_worker gains a converge-on-switch re-check, because plc_begin_transition() returns false while another transition is in flight and a stop request can therefore be dropped -- which would leave the PLC running with the switch in STOP and nobody retrying. Re-checking after the gate clears makes every interleaving converge on STOPPED. This also fixes the same pre-existing drop for a plugin reporting a hardware fault during a start. Also fixes a pre-existing ABI bug in the Python ctypes mirror: it omitted request_plc_stop while the C struct had it, so every field after it was shifted one pointer and base_tick_ns was reading the request_plc_stop function pointer. Offsets and sizeof now match the C struct exactly (verified in a container: 552 bytes, all offsets equal). Verified on a Docker runtime with a test VPP plugin: boot gated with no start-then-stop bounce, START refused while STOP, and both switch edges driving transitions with no API call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Appending `request_plc_start`, `set_switch_position` and `get_plc_state` to
`plugin_runtime_args_t` kept BINARY compatibility, which is what the comment there
claims — and that is all it kept. A VPP plugin ships as SOURCE and is compiled on
the device against whatever runtime is installed, so a plugin referring to those
members fails to build on an older runtime:
synergy_mode.c:92: error: 'plugin_runtime_args_t' has no member named
'set_switch_position'
That is a confusing failure a long way from its cause, hit on a real SLM-RP4.
`PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH` lets a plugin support both vintages with a
feature test, the source-level equivalent of the weak symbols the baremetal HAL
uses for the same purpose. Older runtimes simply do not define it, which is exactly
what makes the test work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ature macro" This reverts commit fb4cec0. Compatibility between a VPP and the runtime it is compiled on belongs in version metadata — "this package requires runtime 4.1.9+" — not in per-feature macros a plugin has to test. Version gating is being added across the components anyway (`minEditorVersion` already exists in the manifest schema; the runtime half does not yet), and once it lands nothing would reference this macro. Better removed now than left as the kind of dead code that outlives the reason for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The program .so defined STB_GNU_UNIQUE symbols (strucpp's template statics, the debug type_ops table, statics in inline functions), and glibc will not unmap a DSO that defines them. dlclose() reported success, the runtime logged "PLC program unloaded successfully", and the mapping stayed resident with all its statics intact -- so the next start, which dlopens the same path, got the previous run's initialized image handed back by name match. Stop therefore stopped the scan threads but never reset the program: a CTU that had counted to 100 came back still at 100, and the program appeared to run while producing nothing. Both the editor's run/stop button and the SLM-RP4's mode switch hit it, because both go through plc_set_state(). Compiling the program with -fno-gnu-unique drops the attribute. Nothing else in the process defines these symbols (plc_main exports only the ext_strucpp_* hook pointers), so there is no cross-DSO identity to preserve. Verified on an SLM-RP4 through the normal upload path: the rebuilt .so has zero UNIQUE symbols, leaves /proc/<pid>/maps on every stop, and re-maps on every start. The previously built .so is still mapped in the same process and never comes back -- it was pinned for the process's lifetime.
Rapid run/stop flips could wedge the runtime permanently: STATUS stuck at TRANSITIONING, every other command answering COMMAND:BUSY (which surfaced in the editor as "Could not verify MD5: Unexpected response format"), the editor showing stopped, and the program still scanning. Reproduced on hardware: 25 start/stop pairs 150 ms apart wedged at round 17. Root cause was two sources of truth. plc_state was written eagerly by whoever requested a change, and an is_transitioning atomic tracked overlap alongside it. plc_cycle_thread then re-asserted RUNNING unconditionally after being spawned, so a stop landing in the window before that thread was first scheduled wrote STOPPED, joined the thread, and had RUNNING put back underneath it. Every loop in that thread exits only on plc_get_state() != RUNNING, so it never exited, the join never returned, and the atomic was never cleared. The state is now the interlock. plc_claim_transition() publishes TRANSITIONING_TO_RUN or TRANSITIONING_TO_STOP under the state lock and refuses any request while one is in flight -- you cannot change state in the middle of changing state -- and plc_publish_final_state() ends it. Nothing writes a target state up front, so there is nothing left to resurrect, and there is no second flag to keep in step. RUNNING now means running: it is published immediately before the dispatcher releases the first scan, not when a start is requested. Both premature writes are gone, as is the mid-transition PLC_STATE_INIT write that made STATUS flicker to INIT and momentarily hid the fact that a transition was in flight. Audited every consumer that tests for RUNNING; none needed changing, and the watchdog's "reset the baseline when not running" behaviour makes a longer start safer rather than riskier. Dropped requests no longer lose the switch's intent. plc_switch records only that the switch MOVED, and the transition-completion path compares where it came to rest against the state actually reached, correcting a mismatch. Several flips during one transition collapse to the final position. Movement-gated on purpose: an editor stop with the switch untouched records nothing, so Stop still works with the switch in RUN. This is runtime-side, so every platform's VPP gets it without changes. The completion path waits to observe the landing before reconciling. A start returns as soon as the PLC thread is spawned and lands seconds later, so reconciling immediately discarded the movement while the state was still TRANSITIONING -- verified on hardware, then fixed. Watchdog backstop: TRANSITIONING persisting past a bound forces ERROR. With the state as the interlock there is no flag anyone could clear, so an unpublished transition would otherwise be an unrecoverable wedge. Verified on an SLM-RP4: transitions land in 0.6-0.8 s (~4 s for a boot start, almost all plugin bring-up); repeated start/stop pairs never stick; and a boot with the switch in STOP shows a plugin stop request dropped mid-start, the start landing RUNNING, then "Mode switch came to rest in STOP but the PLC landed on RUNNING — correcting" and convergence to STOPPED.
A plugin asking for a stop because someone moved the mode switch is normal operation, but it was logged at ERROR — so every ordinary flip to STOP painted the journal red, alongside the real errors. Now INFO, matching the start request it mirrors. Also corrects two comments that no longer describe the code. The dropped-request recovery is the switch-movement reconciliation, not the old converge-on-switch re-check. And plc_main's auto-start gate does NOT see a switch position reported by a VPP plugin, because that plugin initialises inside the start transition the gate is deciding about: a device powered up with the switch in STOP starts and is then corrected by reconciliation, rather than being held stopped up front. Worth saying plainly, since the old comment claimed the opposite.
Every finding here is one shape: the state said a transition was over while the work was still running. plc_state has 15 write sites across 3 threads and nothing distinguishes "I own this transition" from "I am writing a variable", so a landing published by a non-owner either erased a state someone else had landed or reported work that never happened. These are targeted fixes to the paths that do it; the single-owner refactor is tracked separately. Critical - plc_cycle_thread no longer publishes RUNNING unconditionally. New plc_publish_running_if_claimed() lands it only while the state is still TRANSITIONING_TO_RUN, doing the check and the write in one critical section; on refusal the thread reaps its workers and exits instead of entering the dispatcher loop. Paired with the guard below, since either one alone leaves the hang in place. - unload_plc_program publishes TRANSITIONING_TO_STOP for any non-ERROR state, not just RUNNING. On SIGTERM during a start the state is TRANSITIONING_TO_RUN, so the old guard published nothing, the cycle thread then published RUNNING underneath the teardown, and pthread_join(plc_thread) never returned -- systemd waited out its timeout and SIGKILLed a runtime that was still scanning. - plc_begin_transition completes the transition on the calling thread when the worker cannot be spawned, instead of publishing STOPPED. The claim has already published TRANSITIONING_TO_STOP, which is what makes the scan loops exit, so the old path ended the scan but skipped unload_plc_program entirely: journal_cleanup, plugin_driver_stop, plugin_manager_destroy and the dlclose were all skipped, plc_program stayed non-NULL, plc_thread was never joined, and STATUS reported a stop that tore nothing down. The next start then re-entered plugin_driver_init on live plugin state and re-ran a program whose statics were never reinitialised. Blocking the caller on an allocation or thread-exhaustion path is the cheaper cost. - plc_main stops the program before destroying the plugin driver. The teardown calls plugin_driver_stop(plugin_driver), so destroying the driver first left that reading freed memory on every shutdown with a program loaded. High - The stuck-transition bound is derived from the landing bound rather than being an independent number: PLC_TRANSITION_STUCK_TIMEOUT_MS is PLC_TRANSITION_LANDING_TIMEOUT_MS + 30 s, shared through plc_state_manager.h. The watchdog's 60 s used to fire 30 s BEFORE the runtime's own 90 s wait had given up, forcing ERROR while the transition worker was still executing -- which releases the interlock and lets a START be accepted from ERROR on top of a live load or unload. The window is now unreachable in normal operation; the comment on plc_force_error_state records that forcing ERROR still does not abort the work, which needs the transition owner to be able to abort itself. - Mode-switch reconciliation re-arms on refusal. plc_switch_take_movement() consumes the record before plc_begin_transition() is known to have started, and the return value was dropped -- so a request accepted in the gap between the landing and the correction lost the switch's intent entirely, with no retry and no log. New plc_switch_note_movement() hands the record back so the next landing reconciles. - SWITCH is answered during a transition. It is a plain atomic load of plc_switch with no coupling to plc_state, but COMMAND:BUSY made parse_switch_position return None, so the webserver dropped switchPosition from every status response for the whole duration of a start or stop and GET /switch answered "unknown" -- losing the field exactly while the editor polls through a transition it just requested. Medium - Worker SCHED_FIFO priority is clamped to 98, reserving 99 for the dispatcher. The dispatcher runs at 99 to be strictly above every worker, but the clamp allowed a task to TIE it, and SCHED_FIFO does not time-slice between equal priorities: an unbounded loop in IEC code at 99 keeps the dispatcher off that CPU entirely. - plc_set_state's header doc states its precondition (a claim must already have published a TRANSITIONING state) and points callers at plc_begin_transition. It still read like a setter, which is now a footgun: a direct call tears the program down with no claim and then publishes a final state nobody claimed. - Drop the dead `extern PLCState plc_state` in unix_socket.c; the definition is static, so nothing could ever have linked to it. Extract format_switch_response() and reap_task_threads() rather than duplicating either body. Not addressed here: a device that boots with the mode switch in STOP still scans for the duration of the start plus the corrective stop. Aborting a start safely needs one component owning both directions -- a veto inside plc_cycle_thread would leave the same partial teardown described above. The behaviour is documented at plc_main.c:171. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while building an end-to-end test for the PR #162 findings: the runtime could not survive the very path those fixes are about. Four defects, all pre-existing on development, each verified by a test that fails without the fix. - SIGTERM had no handler. RuntimeManager stops the runtime with process.terminate() and systemd's default KillSignal is also SIGTERM, so the default disposition applied and every supervisor-initiated stop killed the process outright: the program was never unloaded, plugins never stopped, the journal never flushed, the program .so never dlclose'd. Installed alongside SIGINT; handle_sigint is now handle_shutdown_signal, since it serves both. - Py_FinalizeEx() ran without the GIL, segfaulting the process on every graceful shutdown of a runtime whose PLC had never started -- which includes safe mode, the crash-recovery path. plugin_driver_destroy releases the state PyGILState_Ensure() gave it and then restores main_tstate, but main_tstate was only ever assigned by plugin_driver_start(); plc_main called PyEval_SaveThread() directly and discarded the result. So a runtime that had run once exited cleanly and one that had not crashed in PyImport_GetModule. New plugin_driver_release_gil() keeps that bookkeeping next to the code that consumes it, and the destroy path no longer drops the GIL it needs. plugin_driver_start() no longer overwrites main_tstate either: it runs on the cycle thread, so a shutdown after a start was restoring the state of a thread that no longer existed. - The command socket accepted commands before the plugin driver was built. A START arriving in that window ran load_plc_program -> plugin_driver_init on the transition worker while the main thread was still inside plugin_driver_load_config/init, and rebuilding a slot dlcloses the .so -- so a plugin sleeping in its own init() returned into an unmapped page. Observed as a SIGSEGV in the main thread at an address inside the just-unloaded plugin. Transition arbitration cannot help: it is one transition racing driver setup, not two transitions. setup_unix_socket() now runs after the driver is ready. - plc_state_manager_cleanup() tore down on top of a transition in flight. During plugin bring-up load_plc_program has not assigned plc_thread yet, so the join ran on a handle that was never set; a moment later the cycle thread would have RUNNING published underneath the teardown. It now waits for the transition to land first, bounded by PLC_TRANSITION_LANDING_TIMEOUT_MS, which turns shutdown-during-a-start into an ordinary stop. RuntimeManager waits RUNTIME_SHUTDOWN_TIMEOUT_S (15 s) for the graceful stop before SIGKILL, since 5 s would preempt the cleanup SIGTERM now performs -- a boot start with plugin bring-up is ~4 s on an SLM-RP4. The kill remains the backstop for a teardown that hangs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Boots a real plc_main with a real compiled program and drives it over the command socket, judging it only by what an outside observer can see: socket replies, the journal, and /proc/<pid>/maps. The bugs in this area are not wrong return values but a state claiming a transition finished while the work is still running, so the mapping is what tells you whether a reported stop unloaded anything -- STATUS says STOPPED either way. Scores 24 checks. Against the branch before the review fixes, 9 fail; after, none do. Notably it catches the phantom stop directly: STOP:ERROR, the .so still mapped, no unload in the journal, with the injected spawn failure confirmed. - fakevpp_plugin.c stands in for a board's VPP package. Configurable sleeps in init()/stop_loop() widen a transition enough to aim a signal or a competing request at it (bring-up is ~150 ms in a container, and a stop tens of ms), and a file-driven mode switch drives set_switch_position + request_plc_start/stop exactly as the plugin contract prescribes. - failinject.c fails exactly one pthread_create, one-shot and self-disarming, so the "transition worker could not be spawned" path is reachable without inducing real thread exhaustion. - logserver.py accepts the log socket. Without it, --print-logs writes nothing to stdout either and every journal assertion silently passes. Two traps are documented in the README because both cost real time: keeping the Python plugins in plugins.conf is what initialises the interpreter, and without that the whole Py_FinalizeEx class is untestable while every test still passes; and a plugin must not start a thread in init(), because update_config dlcloses the .so on each start and the thread returns into an unmapped page. Still argued from code rather than observed, and listed as such: the re-arm branch of switch reconciliation (needs a refusal that cannot be forced without a hook), the watchdog forcing ERROR on a stuck transition (needs a 2-minute transition), and terminating a runaway IEC task (needs the abort ladder that does not exist). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings addressed —
|
| Finding | Fix |
|---|---|
| 2 + 3 | plc_publish_running_if_claimed() checks TRANSITIONING_TO_RUN and writes RUNNING in one critical section; on refusal the cycle thread reaps its workers and exits. Paired with the unload_plc_program guard restored to != PLC_STATE_ERROR. Both are needed — either alone leaves the hang. |
| 1 | PLC_TRANSITION_STUCK_TIMEOUT_MS = PLC_TRANSITION_LANDING_TIMEOUT_MS + 30s, one definition, two consumers. |
| 4 | Spawn-failure paths complete the transition inline instead of publishing a landing nothing reached. |
| 5 | plc_switch_note_movement() re-arms the record when a correction is refused. |
| 6 | SWITCH joins PING/STATUS in the mid-transition allowlist. |
| 8 | plc_set_state()'s doc states its precondition and points at plc_begin_transition(). |
Plus: the dead extern PLCState plc_state in unix_socket.c (the definition is static, so nothing could ever have linked to it), and worker SCHED_FIFO priority clamped to 98 so the dispatcher's 99 is actually above every worker rather than tying one.
d7f30ad — graceful shutdown, four defects, all pre-existing on development
Found while building a test for finding 2: the runtime could not survive the path those fixes are about.
- SIGTERM had no handler.
RuntimeManagerstops the runtime withprocess.terminate(), and systemd defaults to SIGTERM, so every supervisor-initiated stop killed the process outright — program never unloaded, plugins never stopped, journal never flushed,.soneverdlclosed. Py_FinalizeEx()ran without the GIL, segfaulting on every graceful shutdown of a runtime whose PLC had never started — including safe mode, i.e. the crash-recovery path.main_tstatewas only ever assigned byplugin_driver_start();plc_maincalledPyEval_SaveThread()and discarded the result. So a runtime that had run once exited cleanly and one that had not crashed inPyImport_GetModule.- The command socket accepted commands before the plugin driver was built. A START in that window ran
plugin_driver_initon the transition worker while the main thread was still inplugin_driver_load_config, and rebuilding a slotdlcloses the.so— a plugin sleeping in its owninit()returned into an unmapped page. Seen as a SIGSEGV in the main thread inside the just-unloaded plugin. plc_state_manager_cleanup()tore down on top of an in-flight transition, joining aplc_threadthat bring-up had not assigned yet. It now waits for the landing first.
RuntimeManager waits RUNTIME_SHUTDOWN_TIMEOUT_S = 15 before SIGKILL, since 5 s would preempt the cleanup SIGTERM now performs (~4 s boot start on an SLM-RP4).
c4d88c5 — tests/lifecycle
Boots a real plc_main with a real compiled program, drives it over the command socket, and judges it by what an outside observer can see — socket replies, the journal, and /proc/<pid>/maps. That last one matters: STATUS answers STOPPED whether or not the stop unloaded anything.
24 checks. 9 fail against the branch before these fixes; 0 after. Two fixtures make the unreachable reachable: a fake VPP plugin with configurable sleeps in init()/stop_loop() (a container brings up in ~150 ms and stops in tens of ms — nothing to aim at) and a file-driven mode switch, plus an LD_PRELOAD shim that fails exactly one pthread_create.
The phantom stop is now observed rather than argued — before the fix: STOP:ERROR, .so still mapped, no unload in the journal, with the injection confirmed to have fired.
Still argued from code, and listed as such in the README rather than left implicit: the re-arm branch of reconciliation (needs a refusal that cannot be forced without a hook), the watchdog forcing ERROR (needs a transition outliving a 2-minute bound), and terminating a runaway IEC task (needs an abort ladder that does not exist yet).
Not in this PR
- Finding 7, the boot-with-switch-in-STOP window — a veto inside
plc_cycle_threadwould leave the same partial teardown as finding 4. Needs one component owning both directions. - The single-owner refactor, which is what removes this class rather than these instances.
plc_set_state()inplc_state_manager.cppwas the single entry point; the orchestration left inb54a592to make the socket non-blocking. A standing lifecycle executor there satisfies both, and takes the remaining four protocol functions out of the public header.
Verification
Linux build clean, zero warnings. pytest: 53 passed, with the same 3 pre-existing modbus_master mock failures present on development. Not yet re-run on hardware — the SLM-RP4 should confirm the shutdown timings, since every measurement above is from a container.
Run/stop state control for Runtime v4, plus two bugs found on hardware while testing it.
1. A hardware mode switch can gate run/stop (
a0289b2)plc_switchstores a position that any platform's VPP plugin pushes throughplc_set_switch_position(). The runtime refuses a start while it reads STOP — hardware is authoritative no matter who asks, including a buggy plugin. Defaults to RUN, so a runtime with no switch-aware plugin behaves exactly as before. Plugins gainedrequest_plc_start,request_plc_stop,set_switch_positionandget_plc_state; the last maps the internal state onto the 0/1/2 values baremetal reports via FC 0x49, so vendor code can share one status-LED mapping across target types.2. Stop now actually reinitialises the program (
3d160f5)A CTU that had counted to 100 came back still at 100 after stop→start, and the program appeared to run while producing nothing.
dlclose()was called and logged "PLC program unloaded successfully", but glibc will not unmap a DSO that definesSTB_GNU_UNIQUEsymbols — and strucpp's headers emit them for C++17inlinenamespace-scope variables (strucpp::GLOBALVAR,GLOBALBOOL,debug::type_ops,__CURRENT_TIME_NS). The mapping stayed resident with every static intact, and becausefind_libplc_filehandsdlopenthe same path, glibc matched the still-loaded object by name and handed it straight back.Fixed by compiling the program with
-fno-gnu-unique. Verified safe: only the program.sodefines those symbols (every plugin DSO andplc_mainitself: zero), and the runtime deliberately crosses the DSO boundary throughext_strucpp_*pointers rather than symbol identity. The flag leaves them asWEAKat identical addresses, so one-instance-per-DSO is still enforced by the linker.Measured on an SLM-RP4: with the flag the
.soleaves/proc/<pid>/mapson every stop and re-maps on every start; without it, one build's image stays pinned for the life of the process.3. The state itself is now the transition interlock (
b93d27b)Rapid run/stop flips could wedge the runtime permanently: STATUS stuck at TRANSITIONING, every other command answering
COMMAND:BUSY(surfacing in the editor as "Could not verify MD5: Unexpected response format"), the editor showing stopped, and the program still scanning. Reproduced: 25 start/stop pairs 150 ms apart wedged at round 17.Two sources of truth caused it.
plc_statewas written eagerly by whoever requested a change, with anis_transitioningatomic tracking overlap alongside it, andplc_cycle_threadthen re-asserted RUNNING unconditionally once scheduled. A stop landing in the window before that thread first ran wrote STOPPED, joined the thread, and had RUNNING put back underneath it — after which no loop in that thread would ever exit, the join never returned, and the atomic was never cleared.plc_claim_transition()publishesTRANSITIONING_TO_RUN/TRANSITIONING_TO_STOPunder the state lock and refuses any request while one is in flight;plc_publish_final_state()ends it. The atomic is gone.PLC_STATE_INITwrite that made STATUS flicker to INIT and briefly hid that a transition was in flight.plc_switchrecords only that the switch moved, and the completion path compares where it came to rest against the state reached, correcting a mismatch. Movement-gated, so an editor Stop with the switch untouched is never reversed. Runtime-side, so no VPP needs changing.The enum values are appended, never inserted — the first five are wire-visible through FC 0x46 and the plugin mapping.
4. Cleanup (
147b131)A plugin requesting a stop because someone moved the switch was logged at ERROR, so every ordinary flip painted the journal red. Now INFO. Two comments corrected, including
plc_main's claim that the auto-start gate sees a VPP-reported switch position — it does not, because that plugin initialises inside the start transition the gate is deciding about.Validation on hardware (SLM-RP4)
150 switch edges from rapid manual flipping produced:
150 edges collapsing to 19 transitions is the dropping working as intended. Reconciliation fired in both directions, including
came to rest in RUN but the PLC landed on STOPPED — correcting, i.e. a dropped start recovered. Transitions land in 0.6–0.8 s (~4 s for a boot start, nearly all plugin bring-up).Notes for review
fb4cec0and its revertda408b9are noise in the history (a plugin-ABI feature macro, added then removed in favour of component-level version control). Left as-is rather than rewriting a pushed branch.developmentis merged in (ce35c40); a post-merge rebuild on the device is running as of opening this PR.🤖 Generated with Claude Code