From a0289b202e7ec96b707e8ee5d3e3f92028323bd4 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 28 Jul 2026 21:28:31 -0400 Subject: [PATCH 1/9] feat(runtime): gate run/stop on a VPP-reported mode switch 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) --- core/src/CMakeLists.txt | 1 + core/src/drivers/plugin_driver.c | 57 ++++++++++++ core/src/drivers/plugin_types.h | 90 +++++++++++++++++++ .../python/shared/plugin_runtime_args.py | 16 ++++ core/src/plc_app/plc_main.c | 10 +++ core/src/plc_app/plc_switch.c | 47 ++++++++++ core/src/plc_app/plc_switch.h | 64 +++++++++++++ core/src/plc_app/unix_socket.c | 42 ++++++++- webserver/app.py | 35 ++++++++ webserver/runtimemanager.py | 16 ++++ 10 files changed, 377 insertions(+), 1 deletion(-) create mode 100644 core/src/plc_app/plc_switch.c create mode 100644 core/src/plc_app/plc_switch.h diff --git a/core/src/CMakeLists.txt b/core/src/CMakeLists.txt index 64527aad..7c1cb028 100644 --- a/core/src/CMakeLists.txt +++ b/core/src/CMakeLists.txt @@ -44,6 +44,7 @@ add_executable(plc_main ${CMAKE_SOURCE_DIR}/core/src/plc_app/debug_write_journal.cpp ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_io_cycle.cpp ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_state_manager.cpp + ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_switch.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/plcapp_manager.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/scan_cycle_manager.c ${CMAKE_SOURCE_DIR}/core/src/drivers/plugin_driver.c diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 2aab583c..06138319 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -17,6 +17,7 @@ #include "../plc_app/image_tables.h" #include "../plc_app/journal_buffer.h" #include "../plc_app/plc_state_manager.h" +#include "../plc_app/plc_switch.h" #include "../plc_app/unix_socket.h" #include "../plc_app/utils/log.h" #include "../plc_app/utils/utils.h" @@ -169,10 +170,59 @@ static void plugin_request_plc_stop(const char *reason) { // Either the PLC is already stopping/stopped or another stop // is already in flight — either way, nothing to do. + // + // A stop dropped because another transition was in flight is picked up + // by the converge-on-switch re-check at the end of transition_worker + // (unix_socket.c), so a switch-driven stop can't be lost here. log_warn("[PLUGIN] stop request collapsed (already transitioning or not running)"); } } +// Mirror of plugin_request_plc_stop, routed through the same transition path +// the socket START command uses. Gated on the mode switch: hardware is +// authoritative no matter who asks, so a plugin cannot start a PLC whose switch +// reads STOP (which also keeps a buggy plugin from defeating the interlock). +static void plugin_request_plc_start(const char *reason) +{ + if (!plc_switch_allows_run()) + { + log_warn("[PLUGIN] start request refused — mode switch is in STOP (%s)", + reason ? reason : "(no reason given)"); + return; + } + + log_info("[PLUGIN] start requested: %s", reason ? reason : "(no reason given)"); + if (!plc_begin_transition(PLC_STATE_RUNNING)) + { + log_warn("[PLUGIN] start request collapsed (already transitioning or already running)"); + } +} + +// Thin adapter so plugin_types.h can declare the position as a plain int and +// stay free of the plc_app include tree. +static void plugin_set_switch_position(int position) +{ + plc_set_switch_position(position == PLC_SWITCH_STOP ? PLC_SWITCH_STOP : PLC_SWITCH_RUN); +} + +// Map the runtime's PLCState onto the values FC 0x49 reports on baremetal +// targets (0 = STOPPED, 1 = RUNNING, 2 = ERROR) so vendor code driving a +// status LED can share one mapping across both target types. INIT and EMPTY +// are v4-only and have no physical meaning for an indicator, so they report as +// STOPPED — the PLC is not executing. +static int plugin_get_plc_state(void) +{ + switch (plc_get_state()) + { + case PLC_STATE_RUNNING: + return 1; + case PLC_STATE_ERROR: + return 2; + default: + return 0; + } +} + // Python capsule destructor for runtime args // Breakpoint here to debug capsule issues @@ -1025,6 +1075,13 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * // Plugin-initiated async PLC stop (for unrecoverable hardware faults). args->request_plc_stop = plugin_request_plc_stop; + // Run/stop control: the same transition path the socket START / STOP + // commands drive, plus the mode-switch store the runtime gates starts on + // and the state read a plugin uses to drive a status LED. + args->request_plc_start = plugin_request_plc_start; + args->set_switch_position = plugin_set_switch_position; + args->get_plc_state = plugin_get_plc_state; + // PLC base tick time. Runtime owns base_tick_ns; on first plugin init // (before symbols_init) it carries the 20 ms default, so plugins must // guard against the value being smaller than their needed resolution. diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 36e3f424..62ca61e6 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -100,6 +100,77 @@ typedef uint8_t (*plugin_debug_write_func_t)(uint8_t arr, uint16_t elem, */ typedef void (*plugin_request_plc_stop_func_t)(const char *reason); +/** + * @brief PLC start request from a plugin + * + * Mirror of plugin_request_plc_stop: both are thin wrappers over the same + * transition path the socket START / STOP commands drive, so a plugin-initiated + * run/stop is byte-for-byte the flow the editor already exercises. + * + * Refused while the stored mode-switch position is STOP (see + * plugin_set_switch_position_func_t) -- hardware is authoritative regardless of + * who asks. Non-blocking; `reason` is logged. Safe to call from any plugin + * thread, including while the PLC is stopped: the runtime keeps every plugin + * mapped across a stop, so this is how a hardware STOP -> RUN edge starts the + * PLC again. + */ +typedef void (*plugin_request_plc_start_func_t)(const char *reason); + +/** + * @brief Report the run/stop mode-switch position + * + * THIS is the hardware interface on Linux. The runtime never reads hardware and + * never polls: the package decides how and how often it samples (GPIO + * interrupt, sysfs poll, fieldbus callback, its own thread) and calls this + * whenever the position changes. + * + * Stores only -- it starts nothing and stops nothing. On a switch change the + * plugin calls this and THEN asks for the matching transition: + * + * args->set_switch_position(PLC_SWITCH_STOP); + * args->request_plc_stop("mode switch moved to STOP"); + * + * Always store the position BEFORE requesting the transition. On a falling edge + * that closes the window where a start could slip in; on a rising edge it stops + * the request being refused by the guard still reading the stale STOP. + * + * `position` is a plc_switch_t (0 = STOP, 1 = RUN). Declared as int here so + * this header stays free of the plc_app include tree. Idempotent, and safe to + * call from any plugin thread. + * + * Sample the switch from a thread started in init() and torn down in cleanup(), + * NOT in start_loop() / stop_loop(): the runtime calls stop_loop when the PLC + * stops, which is precisely when the position matters most. Make that start + * idempotent, because init() is re-entered on each PLC start. Reporting the + * initial position from init() also gates the boot auto-start, so a device + * powered up with its switch in STOP never starts and then bounces. + */ +typedef void (*plugin_set_switch_position_func_t)(int position); + +/** + * @brief Read the current PLC state + * + * Returns 0 = STOPPED, 1 = RUNNING, 2 = ERROR (matching the values FC 0x49 + * reports on baremetal targets, so vendor code can share one mapping). + * + * There is no state-change callback: a plugin driving a panel LED polls this. + * A device with no LED never calls it and the runtime is unaware. + * + * Poll it from the plugin's OWN thread -- the same sampler that reads the mode + * switch is the natural home. Do NOT poll it from cycle_end: cycle_end fires + * once per scan, so it stops being called the moment the PLC stops, and an LED + * driven from there would freeze showing RUNNING exactly when it needs to show + * STOPPED. + * + * IMPORTANT (applies to every field of plugin_runtime_args_t): the runtime + * frees the args pointer as soon as init() returns. Copy the struct BY VALUE + * during init -- `memcpy(&my_args, args, sizeof(plugin_runtime_args_t))` -- + * and call through the copy. Caching the pointer is a use-after-free; calling + * a function pointer read back out of the freed struct crashes. See the + * s7comm plugin for the established pattern. + */ +typedef int (*plugin_get_plc_state_func_t)(void); + /** * @brief Runtime buffer access structure for plugins * @@ -182,6 +253,25 @@ typedef struct * Populated when the runtime initializes the plugin; may be 0 if * symbols are not yet resolved (plugin must guard against zero). */ unsigned long long base_tick_ns; + + /* --------------------------------------------------------------------- + * Run/stop control. Appended at the end of the struct so plugin binaries + * compiled against an earlier layout keep their field offsets. + * + * A plugin that ignores all three behaves exactly as before: the switch + * position stays at its RUN default, so every start path is unguarded. + * ------------------------------------------------------------------- */ + + /* Async request to run — see plugin_request_plc_start_func_t. */ + plugin_request_plc_start_func_t request_plc_start; + + /* Report the mode-switch position — see + * plugin_set_switch_position_func_t. The hardware interface on Linux. */ + plugin_set_switch_position_func_t set_switch_position; + + /* Current PLC state, for driving a status LED — see + * plugin_get_plc_state_func_t. */ + plugin_get_plc_state_func_t get_plc_state; } plugin_runtime_args_t; #endif /* PLUGIN_TYPES_H */ diff --git a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py index 05de87a8..3514fcc8 100644 --- a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py +++ b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py @@ -81,8 +81,24 @@ class PluginRuntimeArgs(ctypes.Structure): ("journal_write_int", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int)), ("journal_write_dint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_uint)), ("journal_write_lint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_ulonglong)), + # Async request to stop the whole PLC: void (*)(const char *reason). + # + # This entry was missing while the C struct had the field, so every + # field after it was shifted by one pointer -- `base_tick_ns` was + # actually reading the request_plc_stop pointer. Keep this list in + # lockstep with plugin_types.h; the offsets are load-bearing. + ("request_plc_stop", ctypes.CFUNCTYPE(None, ctypes.c_char_p)), # PLC base tick time in nanoseconds (mirrors C-side base_tick_ns). ("base_tick_ns", ctypes.c_ulonglong), + # --- Run/stop control (appended in C, so appended here too) --------- + # Async request to run: void (*)(const char *reason). Refused while the + # mode switch reads STOP. + ("request_plc_start", ctypes.CFUNCTYPE(None, ctypes.c_char_p)), + # Report the mode-switch position: void (*)(int position), + # 0 = STOP, 1 = RUN. Store the position BEFORE requesting a transition. + ("set_switch_position", ctypes.CFUNCTYPE(None, ctypes.c_int)), + # Current PLC state: int (*)(void), 0 = STOPPED, 1 = RUNNING, 2 = ERROR. + ("get_plc_state", ctypes.CFUNCTYPE(ctypes.c_int)), ] def validate_pointers(self): diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index ed8e3dfc..49fd0d12 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -15,6 +15,7 @@ #include "../drivers/plugin_driver.h" #include "image_tables.h" #include "plc_state_manager.h" +#include "plc_switch.h" #include "plcapp_manager.h" #include "unix_socket.h" #include "utils/log.h" @@ -166,6 +167,15 @@ int main(int argc, char *argv[]) log_info("Runtime started in SAFE MODE - PLC program will not be loaded"); log_info("Upload a corrected program to recover"); } + // Same gate as any other start. Plugin init ran above, so a VPP that owns a + // physical mode switch has already reported its position — a device powered + // up with the switch in STOP is gated here rather than starting and then + // being stopped a moment later. + else if (!plc_switch_allows_run()) + { + log_info("Hardware mode switch is in STOP - PLC left stopped"); + log_info("Move the switch to RUN to start the PLC"); + } else if (!plc_begin_transition(PLC_STATE_RUNNING)) { log_error("Failed to initiate PLC start"); diff --git a/core/src/plc_app/plc_switch.c b/core/src/plc_app/plc_switch.c new file mode 100644 index 00000000..d7c46285 --- /dev/null +++ b/core/src/plc_app/plc_switch.c @@ -0,0 +1,47 @@ +/** + * @file plc_switch.c + * @brief Storage for the run/stop mode-switch position. + * + * Deliberately tiny: one atomic and three accessors. The runtime never reads + * hardware and never polls -- a VPP plugin pushes the position whenever it + * changes, on whatever schedule that package decides (GPIO interrupt, sysfs + * poll, fieldbus callback, its own thread). The runtime's only use for the + * value is to refuse a start while the switch reads STOP, and to report the + * position to the editor. + * + * The default is RUN so that a runtime with no switch-aware plugin behaves + * exactly as it did before this file existed: the boot auto-start is + * unguarded and every START succeeds. + */ + +#include "plc_switch.h" + +#include + +#include "utils/log.h" + +/* Default RUN: no plugin implementing the interface means no gating. */ +static atomic_int switch_position = PLC_SWITCH_RUN; + +void plc_set_switch_position(plc_switch_t position) +{ + const int normalized = (position == PLC_SWITCH_STOP) ? PLC_SWITCH_STOP : PLC_SWITCH_RUN; + const int previous = atomic_exchange(&switch_position, normalized); + + /* Log edges only. A plugin sampling a GPIO on a fast timer may call this + * on every sample; logging each one would flood the journal. */ + if (previous != normalized) + { + log_info("Mode switch moved to %s", normalized == PLC_SWITCH_RUN ? "RUN" : "STOP"); + } +} + +plc_switch_t plc_get_switch_position(void) +{ + return (plc_switch_t)atomic_load(&switch_position); +} + +bool plc_switch_allows_run(void) +{ + return atomic_load(&switch_position) == PLC_SWITCH_RUN; +} diff --git a/core/src/plc_app/plc_switch.h b/core/src/plc_app/plc_switch.h new file mode 100644 index 00000000..28a2739a --- /dev/null +++ b/core/src/plc_app/plc_switch.h @@ -0,0 +1,64 @@ +#ifndef PLC_SWITCH_H +#define PLC_SWITCH_H + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * @brief Physical run/stop mode-switch position. + * + * Every platform has a mode switch. Devices with no physical switch never call + * plc_set_switch_position(), so the stored value stays at its RUN default and + * every gate below is transparent -- identical to the behaviour before this + * interface existed. + */ +typedef enum +{ + PLC_SWITCH_STOP = 0, + PLC_SWITCH_RUN = 1 +} plc_switch_t; + +/** + * @brief Store the mode-switch position. + * + * Stores only -- it starts nothing and stops nothing. A VPP plugin that owns a + * physical switch calls this on each change and then asks for the matching + * transition through request_plc_start / request_plc_stop, so the transition + * flow stays the same one the editor's START / STOP commands drive. + * + * The ordering matters and is part of the plugin contract: store the position + * BEFORE requesting the transition. On a falling edge that closes the window + * where a start could slip in; on a rising edge it stops the request being + * refused by the guard still reading the stale STOP. + * + * A plain atomic store. Safe to call from any thread, including while the PLC + * is stopped (the runtime keeps every plugin mapped across a stop). + * + * @param position PLC_SWITCH_STOP or PLC_SWITCH_RUN + */ +void plc_set_switch_position(plc_switch_t position); + +/** + * @brief Read the stored mode-switch position. Serves the SWITCH socket + * command and the REST status field. + */ +plc_switch_t plc_get_switch_position(void); + +/** + * @brief Whether a start is permitted right now. + * + * Consulted by every start path: the socket START handler, the boot auto-start, + * and the plugin-facing request_plc_start. False while the switch reads STOP -- + * hardware is authoritative, and a start is refused rather than queued so the + * editor can tell the user to flip the switch. + */ +bool plc_switch_allows_run(void); + +#ifdef __cplusplus +} +#endif + +#endif // PLC_SWITCH_H diff --git a/core/src/plc_app/unix_socket.c b/core/src/plc_app/unix_socket.c index 3bfd347d..74ebea83 100644 --- a/core/src/plc_app/unix_socket.c +++ b/core/src/plc_app/unix_socket.c @@ -12,6 +12,7 @@ #include "../drivers/plugin_driver.h" #include "debug_handler.h" #include "plc_state_manager.h" +#include "plc_switch.h" #include "scan_cycle_manager.h" #include "unix_socket.h" #include "utils/log.h" @@ -44,6 +45,28 @@ static void *transition_worker(void *arg) } atomic_store(&is_transitioning, 0); + + // Converge on the mode switch. + // + // plc_begin_transition() returns false while another transition is in + // flight, which means a stop request CAN be dropped: an editor START may + // win the gate, then a plugin's set_switch_position(STOP) + + // request_plc_stop() arrives and the stop is collapsed as "already + // transitioning". That would leave the PLC running with the switch in + // STOP, with nobody retrying. + // + // Re-checking here — after is_transitioning clears, so this call can win + // the gate — makes every interleaving converge on STOPPED without asking + // the requester to implement retry logic. The failure direction is the + // safe one, which is the property to preserve in any change to either + // caller. (The same drop was already possible for a plugin reporting a + // hardware fault during a start; this fixes that too.) + if (plc_get_state() == PLC_STATE_RUNNING && !plc_switch_allows_run()) + { + log_warn("Mode switch reads STOP but the PLC ended up RUNNING — stopping"); + plc_begin_transition(PLC_STATE_STOPPED); + } + return NULL; } @@ -212,10 +235,27 @@ void handle_unix_socket_commands(const char *command, char *response, size_t res strncpy(response, "STOP:ERROR\n", response_size); } } + else if (strcmp(command, "SWITCH") == 0) + { + // Report the mode-switch position a VPP plugin last stored. Devices + // with no switch-aware plugin always answer RUN. + if (plc_get_switch_position() == PLC_SWITCH_RUN) + strncpy(response, "SWITCH:RUN\n", response_size); + else + strncpy(response, "SWITCH:STOP\n", response_size); + } else if (strcmp(command, "START") == 0) { PLCState current_state = plc_get_state(); - if (current_state != PLC_STATE_RUNNING) + // Hardware is authoritative: refuse rather than queue, so the editor + // can tell the user to flip the switch instead of leaving a start + // pending. Checked before the transition is ever begun. + if (!plc_switch_allows_run()) + { + strncpy(response, "START:ERROR_SWITCH_STOP\n", response_size); + log_warn("Received START command but the mode switch is in STOP"); + } + else if (current_state != PLC_STATE_RUNNING) { if (plc_begin_transition(PLC_STATE_RUNNING)) strncpy(response, "START:OK\n", response_size); diff --git a/webserver/app.py b/webserver/app.py index d6a1c0ac..a05505e5 100644 --- a/webserver/app.py +++ b/webserver/app.py @@ -130,6 +130,22 @@ def parse_timing_stats(stats_response: Optional[str]) -> Optional[dict]: return None +def parse_switch_position(switch_response: Optional[str]) -> Optional[str]: + """ + Parse the SWITCH response from the runtime. + Expected format: ``SWITCH:RUN`` / ``SWITCH:STOP``. + Returns ``"run"`` / ``"stop"``, or None when the response is unusable. + """ + if switch_response is None: + return None + value = switch_response.strip() + if value == "SWITCH:RUN": + return "run" + if value == "SWITCH:STOP": + return "stop" + return None + + def handle_status(data: dict) -> dict: response = runtime_manager.status_plc() if response is None: @@ -137,6 +153,14 @@ def handle_status(data: dict) -> dict: result: dict = {"status": response} + # Mode-switch position, so the editor can block a start before sending it + # rather than relying on the runtime's refusal alone. Additive: the existing + # `status` key is untouched, and an older editor simply ignores this field. + # A runtime with no switch-aware plugin always reports "run". + switch_position = parse_switch_position(runtime_manager.switch_plc()) + if switch_position is not None: + result["switchPosition"] = switch_position + # Only fetch timing stats if explicitly requested via include_stats parameter. # This avoids acquiring the stats mutex on every status poll, which could # introduce latency to the critical PLC scan cycle. @@ -186,6 +210,16 @@ def handle_list_serial_ports(data: dict) -> dict: return {"error": str(e), "ports": []} +def handle_switch(data: dict) -> dict: + """ + Report the run/stop mode-switch position on its own, for callers that want + it without a full status poll. Devices with no switch-aware VPP plugin + always answer "run". + """ + position = parse_switch_position(runtime_manager.switch_plc()) + return {"switchPosition": position if position is not None else "unknown"} + + GET_HANDLERS: dict[str, Callable[[dict], dict]] = { "start-plc": handle_start_plc, "stop-plc": handle_stop_plc, @@ -194,6 +228,7 @@ def handle_list_serial_ports(data: dict) -> dict: "status": handle_status, "ping": handle_ping, "serial-ports": handle_list_serial_ports, + "switch": handle_switch, } diff --git a/webserver/runtimemanager.py b/webserver/runtimemanager.py index fc734567..39e2e7a8 100644 --- a/webserver/runtimemanager.py +++ b/webserver/runtimemanager.py @@ -329,6 +329,22 @@ def status_plc(self): logger.error("Failed to get PLC status (unexpected): %s", e) return "STATUS:ERROR\n" + def switch_plc(self) -> str: + """ + Send SWITCH command to read the run/stop mode-switch position. + + Answers ``SWITCH:RUN`` or ``SWITCH:STOP``. Devices with no switch-aware + VPP plugin always report RUN, so callers need no special case for them. + """ + try: + return self.runtime_socket.send_and_receive("SWITCH\n") + except (OSError, socket.error) as e: + logger.error("Failed to get mode switch position: %s", e) + return "SWITCH:ERROR\n" + except Exception as e: + logger.error("Failed to get mode switch position (unexpected): %s", e) + return "SWITCH:ERROR\n" + def stats_plc(self): """ Send STATS command to get timing statistics From fb4cec0a6ed315f36dde161cc26ce862e26fcd0e Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 3 Aug 2026 16:30:08 -0400 Subject: [PATCH 2/9] feat(runtime): advertise the mode-switch plugin ABI with a feature macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- core/src/drivers/plugin_types.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 62ca61e6..0fe49755 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -15,6 +15,22 @@ #ifndef PLUGIN_TYPES_H #define PLUGIN_TYPES_H +/** + * @brief This runtime's plugin args carry the run/stop mode-switch interface: + * `request_plc_start`, `set_switch_position` and `get_plc_state`. + * + * Defined so a VPP plugin — which ships as SOURCE and is compiled on the device + * against the installed runtime's headers — can support both vintages: + * + * #if defined(PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH) + * args->set_switch_position(position); + * #endif + * + * Without it, a plugin using those members simply fails to build on a runtime that + * predates them, which is a confusing failure a long way from its cause. + */ +#define PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH 1 + #include "../lib/iec_types.h" #include #include @@ -260,6 +276,12 @@ typedef struct * * A plugin that ignores all three behaves exactly as before: the switch * position stays at its RUN default, so every start path is unguarded. + * + * Appending keeps BINARY compatibility, but a VPP plugin is shipped as source + * and compiled on the device against whatever runtime is installed there — so + * source compatibility needs a feature test, which is what + * PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH above is for. Referring to these members + * unguarded fails to compile on an older runtime. * ------------------------------------------------------------------- */ /* Async request to run — see plugin_request_plc_start_func_t. */ From da408b90185c0baf46ba14a3845262255c951192 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 4 Aug 2026 13:26:38 -0400 Subject: [PATCH 3/9] Revert "feat(runtime): advertise the mode-switch plugin ABI with a feature macro" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- core/src/drivers/plugin_types.h | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 0fe49755..62ca61e6 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -15,22 +15,6 @@ #ifndef PLUGIN_TYPES_H #define PLUGIN_TYPES_H -/** - * @brief This runtime's plugin args carry the run/stop mode-switch interface: - * `request_plc_start`, `set_switch_position` and `get_plc_state`. - * - * Defined so a VPP plugin — which ships as SOURCE and is compiled on the device - * against the installed runtime's headers — can support both vintages: - * - * #if defined(PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH) - * args->set_switch_position(position); - * #endif - * - * Without it, a plugin using those members simply fails to build on a runtime that - * predates them, which is a confusing failure a long way from its cause. - */ -#define PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH 1 - #include "../lib/iec_types.h" #include #include @@ -276,12 +260,6 @@ typedef struct * * A plugin that ignores all three behaves exactly as before: the switch * position stays at its RUN default, so every start path is unguarded. - * - * Appending keeps BINARY compatibility, but a VPP plugin is shipped as source - * and compiled on the device against whatever runtime is installed there — so - * source compatibility needs a feature test, which is what - * PLUGIN_RUNTIME_ARGS_HAS_MODE_SWITCH above is for. Referring to these members - * unguarded fails to compile on an older runtime. * ------------------------------------------------------------------- */ /* Async request to run — see plugin_request_plc_start_func_t. */ From 3d160f50f436e09f499561786993642deed03cf4 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 4 Aug 2026 14:54:23 -0400 Subject: [PATCH 4/9] fix(runtime): let dlclose actually unload the program on stop 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//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. --- scripts/Makefile.strucpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/scripts/Makefile.strucpp b/scripts/Makefile.strucpp index c2de33d5..cebd1ccb 100644 --- a/scripts/Makefile.strucpp +++ b/scripts/Makefile.strucpp @@ -56,7 +56,24 @@ CC := $(if $(CCACHE),$(CCACHE) gcc,gcc) # when the per-upload generated.hpp changes, leaving a stale locatedVarsCount # (a constexpr baked in at compile time) in the shim object -> ABI/count # mismatch against the freshly compiled program -> runtime crash. -CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -MMD -MP \ +# -fno-gnu-unique: THIS FLAG IS WHAT MAKES STOP ACTUALLY STOP. The program .so is +# dlopen'd on start and dlclose'd on stop, but glibc refuses to unmap any DSO +# that defines STB_GNU_UNIQUE symbols -- and strucpp's headers emit them for +# template statics (strucpp::GLOBALVAR, GLOBALBOOL), the debug type_ops table, +# and statics inside inline functions. dlclose() therefore returned success, +# logged "PLC program unloaded successfully", and left the mapping resident with +# every static still holding the previous run's values. Because find_libplc_file +# hands dlopen the SAME path on the next start, glibc matched the still-loaded +# object by name and handed it straight back: the program resumed mid-state +# instead of restarting. A CTU that had counted to 100 stayed at 100 across +# stop/start, so the program looked alive in the debugger but did nothing. +# Suppressing the unique attribute costs nothing here -- these symbols are +# private to this one .so (plc_main exports only the ext_strucpp_* hook +# pointers, never the mangled statics), so there is no cross-DSO identity to +# preserve. Verified on an SLM-RP4: with the flag the .so leaves +# /proc//maps on every stop and re-maps fresh on every start; without it, +# it never leaves -- one build's image stays pinned for the life of the process. +CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique -MMD -MP \ -Wno-unknown-pragmas -Wno-deprecated-declarations \ -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) From b93d27b0a5397f0399f2ee51023e5ad3a484491d Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 4 Aug 2026 19:21:53 -0400 Subject: [PATCH 5/9] refactor(runtime): make the state itself the transition interlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/src/plc_app/plc_main.c | 7 +- core/src/plc_app/plc_state_manager.cpp | 177 +++++++++++++++++++------ core/src/plc_app/plc_state_manager.h | 51 ++++++- core/src/plc_app/plc_switch.c | 26 +++- core/src/plc_app/plc_switch.h | 16 +++ core/src/plc_app/scan_cycle_manager.c | 2 +- core/src/plc_app/unix_socket.c | 161 +++++++++++----------- core/src/plc_app/unix_socket.h | 2 +- core/src/plc_app/utils/watchdog.c | 33 ++++- 9 files changed, 352 insertions(+), 123 deletions(-) diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index 49fd0d12..c2a6f79e 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -104,8 +104,9 @@ int main(int argc, char *argv[]) wake_sa.sa_flags = 0; sigaction(SIGUSR1, &wake_sa, NULL); - // Make sure PLC starts in STOP state - plc_set_state(PLC_STATE_STOPPED); + // No need to force STOPPED here: plc_state is statically initialised to it, + // and plc_set_state() is now the body of a claimed transition rather than a + // setter -- calling it with nothing loaded would just log a failed unload. // Initialize watchdog if (watchdog_init() != 0) @@ -156,7 +157,7 @@ int main(int argc, char *argv[]) // faulty program that may have caused repeated crashes). // // Use plc_begin_transition() rather than plc_set_state() directly so the - // auto-start goes through the same is_transitioning CAS gate as any + // auto-start is arbitrated by plc_claim_transition() like any // socket-originated START command. This prevents a race where the socket // listener (already running above) accepts a START before the auto-start // finishes, causing two concurrent load_plc_program() calls — and two diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index a4c976cc..3dc25cb4 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -442,10 +442,16 @@ void *plc_cycle_thread(void *arg) log_info("Starting main loop"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_RUNNING; - pthread_mutex_unlock(&state_mutex); - log_info("PLC State: RUNNING"); + /* NOT where RUNNING is published. The state stays TRANSITIONING_TO_RUN until + * the task threads exist and the dispatcher is about to release the first + * scan -- see the publish below the "Spawned N PLC task thread(s)" log. Two + * writes used to happen before this point (here, and in plc_set_state before + * this thread was even created), and both claimed RUNNING while nothing was + * scanning yet. The one here was also a deadlock: a stop landing in the + * window before this thread was first scheduled wrote STOPPED and joined us, + * and this line put RUNNING back -- after which no loop below would ever + * exit, so the join never returned and the runtime refused every command for + * the rest of the process's life. */ clock_gettime(CLOCK_MONOTONIC, &timer_start); @@ -708,6 +714,18 @@ void *plc_cycle_thread(void *arg) } log_info("Spawned %zu PLC task thread(s)", plc_task_count); + /* RUNNING, at last, and this is the earliest point it is true: the workers + * exist and the very next thing that happens is the dispatcher releasing the + * first scan. Nothing observes RUNNING too early as a result -- the workers + * are parked in sem_wait and are only ever posted from the loop below, and + * the loop itself needs RUNNING visible to run at all. + * + * This is also what ends the transition claimed by plc_claim_transition, so + * every path out of this thread from here on must land a final state: the + * crash recovery above publishes ERROR, and the stop path publishes STOPPED + * once teardown joins. */ + plc_publish_final_state(PLC_STATE_RUNNING); + /* --------------------------------------------------------------------- * GCD master-tick dispatcher. * @@ -948,13 +966,15 @@ extern "C" int load_plc_program(PluginManager *pm) if (plugin_manager_load(pm)) { + /* Progress, not a state change. This used to publish PLC_STATE_INIT, + * which is now actively wrong: it overwrites TRANSITIONING_TO_RUN, so the + * runtime would stop reporting a transition in flight and let a stop be + * claimed while the start was still landing -- the same hole the + * TRANSITIONING states exist to close. (It is also what made STATUS + * flicker to INIT mid-start.) INIT survives in the enum as a startup + * value; nothing writes it during a transition. */ log_info("Loading PLC application"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_INIT; - pthread_mutex_unlock(&state_mutex); - log_info("PLC State: INIT"); - if (plugin_driver) { if (plugin_driver_update_config(plugin_driver, "./plugins.conf") != 0) @@ -1047,14 +1067,18 @@ extern "C" int unload_plc_program(PluginManager *pm) { if (pm && pm == plc_program) { - PLCState prev_state = plc_get_state(); - - if (prev_state != PLC_STATE_ERROR) + /* The dispatcher and its workers leave their loops on anything that is + * not RUNNING, and the join below depends on that. A claimed stop has + * already published TRANSITIONING_TO_STOP, which is that signal. This + * also covers the shutdown path (plc_state_manager_cleanup), which tears + * down without claiming a transition first: publish the direction here so + * the loops still exit. ERROR is left alone -- it must survive teardown. */ + pthread_mutex_lock(&state_mutex); + if (plc_state == PLC_STATE_RUNNING) { - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_STOPPED; - pthread_mutex_unlock(&state_mutex); + plc_state = PLC_STATE_TRANSITIONING_TO_STOP; } + pthread_mutex_unlock(&state_mutex); pthread_join(plc_thread, NULL); @@ -1077,7 +1101,10 @@ extern "C" int unload_plc_program(PluginManager *pm) plc_program = NULL; log_info("PLC program unloaded successfully"); - log_info("PLC State: STOPPED"); + + /* The teardown is done, so this is the moment STOPPED becomes true. + * plc_publish_final_state keeps ERROR if a task crashed on the way out. */ + plc_publish_final_state(PLC_STATE_STOPPED); return 0; } else @@ -1095,25 +1122,89 @@ extern "C" PLCState plc_get_state(void) return s; } -extern "C" bool plc_set_state(PLCState new_state) +extern "C" bool plc_state_is_transitioning(void) { + const PLCState s = plc_get_state(); + return s == PLC_STATE_TRANSITIONING_TO_RUN || s == PLC_STATE_TRANSITIONING_TO_STOP; +} + +extern "C" bool plc_claim_transition(PLCState target) +{ + if (target != PLC_STATE_RUNNING && target != PLC_STATE_STOPPED) + { + log_error("Refusing to transition to state %d: only RUNNING and STOPPED are" + " requestable targets", (int)target); + return false; + } + pthread_mutex_lock(&state_mutex); - if (plc_state == new_state) + + /* Drop, don't queue. Requests are dropped while a transition is in flight, + * and the switch's intent is recovered afterwards by reconciliation (see + * plc_switch_take_movement), so nothing has to be remembered here. */ + if (plc_state == PLC_STATE_TRANSITIONING_TO_RUN || plc_state == PLC_STATE_TRANSITIONING_TO_STOP) { pthread_mutex_unlock(&state_mutex); return false; } - plc_state = new_state; + + if (plc_state == target) + { + pthread_mutex_unlock(&state_mutex); + return false; + } + + plc_state = (target == PLC_STATE_RUNNING) ? PLC_STATE_TRANSITIONING_TO_RUN + : PLC_STATE_TRANSITIONING_TO_STOP; + pthread_mutex_unlock(&state_mutex); + + log_info("PLC State: %s", target == PLC_STATE_RUNNING ? "TRANSITIONING_TO_RUN" + : "TRANSITIONING_TO_STOP"); + return true; +} + +extern "C" void plc_publish_final_state(PLCState final_state) +{ + const char *name = "UNKNOWN"; + switch (final_state) + { + case PLC_STATE_RUNNING: name = "RUNNING"; break; + case PLC_STATE_STOPPED: name = "STOPPED"; break; + case PLC_STATE_ERROR: name = "ERROR"; break; + case PLC_STATE_EMPTY: name = "EMPTY"; break; + default: break; + } + + pthread_mutex_lock(&state_mutex); + + /* ERROR outranks a STOPPED landing: a task that crashed mid-teardown recorded + * the fact that matters, and the teardown completing must not erase it. */ + if (plc_state == PLC_STATE_ERROR && final_state == PLC_STATE_STOPPED) + { + pthread_mutex_unlock(&state_mutex); + log_info("Transition finished in ERROR — keeping ERROR rather than STOPPED"); + return; + } + + plc_state = final_state; pthread_mutex_unlock(&state_mutex); + log_info("PLC State: %s", name); +} - // Note: plc_state must flip BEFORE load/unload runs. Task threads - // exit their scan loops via `while (plc_get_state() == RUNNING)`, - // and unload_plc_program() depends on that signal to join them. - // The "STATUS reports STOPPED while teardown is still in flight" - // window this opens is gated externally via is_transitioning in - // unix_socket.c — STATUS returns STATUS:TRANSITIONING for the - // duration of the worker, so external pollers don't see the - // stale STOPPED. +extern "C" bool plc_set_state(PLCState new_state) +{ + // Performs a transition already claimed via plc_claim_transition(), which + // published TRANSITIONING_TO_RUN or TRANSITIONING_TO_STOP. No state is + // written here: writing the target up front is what used to let a stop's + // STOPPED be resurrected by a start still landing. The final state is + // published by whoever knows the transition actually finished -- + // plc_cycle_thread for RUNNING (just before it releases the first scan) and + // unload_plc_program for STOPPED (after the teardown joins) -- with the + // failure paths below publishing ERROR or EMPTY. + // + // The current TRANSITIONING state is itself the signal the task and + // dispatcher loops need: they run while plc_get_state() == RUNNING, so + // TRANSITIONING_TO_STOP breaks them exactly as the old early STOPPED did. if (new_state == PLC_STATE_RUNNING) { @@ -1123,9 +1214,7 @@ extern "C" bool plc_set_state(PLCState new_state) if (libplc_path == NULL) { log_error("Failed to find libplc file"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_EMPTY; - pthread_mutex_unlock(&state_mutex); + plc_publish_final_state(PLC_STATE_EMPTY); return false; } @@ -1135,17 +1224,17 @@ extern "C" bool plc_set_state(PLCState new_state) if (plc_program == NULL) { log_error("Failed to create PluginManager"); - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_EMPTY; - pthread_mutex_unlock(&state_mutex); + plc_publish_final_state(PLC_STATE_EMPTY); return false; } } if (load_plc_program(plc_program) < 0) { - pthread_mutex_lock(&state_mutex); - plc_state = PLC_STATE_ERROR; - pthread_mutex_unlock(&state_mutex); + /* load_plc_program publishes ERROR or EMPTY itself on the paths it + * knows about; this covers anything it does not, so a claimed + * transition can never end without a final state. Re-publishing the + * same value is harmless. */ + if (plc_state_is_transitioning()) plc_publish_final_state(PLC_STATE_ERROR); return false; } } @@ -1153,7 +1242,21 @@ extern "C" bool plc_set_state(PLCState new_state) { if (plc_program) { - if (unload_plc_program(plc_program) < 0) return false; + if (unload_plc_program(plc_program) < 0) + { + /* Teardown failed. STOPPED is still the honest landing -- there + * is no program running -- and leaving TRANSITIONING set would + * make the runtime refuse every command from here on. */ + if (plc_state_is_transitioning()) plc_publish_final_state(PLC_STATE_STOPPED); + return false; + } + } + else + { + /* Nothing loaded, so the stop is already true. Still has to be + * published: the transition was claimed, and only a final state + * ends it. */ + plc_publish_final_state(PLC_STATE_STOPPED); } } diff --git a/core/src/plc_app/plc_state_manager.h b/core/src/plc_app/plc_state_manager.h index e59acda5..d368c2ce 100644 --- a/core/src/plc_app/plc_state_manager.h +++ b/core/src/plc_app/plc_state_manager.h @@ -25,13 +25,34 @@ typedef atomic_long plc_atomic_long_t; typedef atomic_uint_least64_t plc_atomic_u64_t; #endif +/** + * Runtime states. + * + * The two TRANSITIONING values are APPENDED, never inserted: the first five are + * wire-visible (FC 0x46 reports them, and plugin_get_plc_state maps them for + * vendor status indicators), so renumbering would quietly change what boards + * report. + * + * RUNNING means running -- it is published at the moment the dispatcher is about + * to release the first scan, not when a start is requested. Everything in + * between is a TRANSITIONING state, and because both compare unequal to + * PLC_STATE_RUNNING, every loop that gates on RUNNING treats them correctly + * without modification: a stop's teardown still gets its exit signal, and a + * half-started PLC cannot scan. + * + * The direction is carried (rather than one flat TRANSITIONING) so that any code + * found to need the target state before it lands can test for the specific + * direction instead of having to reintroduce a premature RUNNING. + */ typedef enum { PLC_STATE_INIT, PLC_STATE_RUNNING, PLC_STATE_STOPPED, PLC_STATE_ERROR, - PLC_STATE_EMPTY + PLC_STATE_EMPTY, + PLC_STATE_TRANSITIONING_TO_RUN, + PLC_STATE_TRANSITIONING_TO_STOP } PLCState; /* ----------------------------------------------------------------------- @@ -109,7 +130,7 @@ extern size_t plc_task_count; * The plc_cycle_thread owns the array — it allocates after walking the * configuration (load) and frees after joining task threads (stop). * Concurrently, the unix-socket thread services STATS by iterating the - * array under format_timing_stats_response. is_transitioning gates new + * array under format_timing_stats_response. The TRANSITIONING state gates new * commands but doesn't bracket an in-flight STATS call: a plugin-initiated * stop can fire mid-iteration, free plc_tasks, and the STATS reader * dereferences freed memory. @@ -136,6 +157,32 @@ PLCState plc_get_state(void); */ bool plc_set_state(PLCState new_state); +/** + * @brief Claim the right to transition, publishing the matching TRANSITIONING state. + * + * The single arbiter for "may this change start". Under the state lock: a request + * arriving while a TRANSITIONING state is current is DROPPED -- you cannot change + * state in the middle of changing state -- and so is a request for the state the + * runtime is already in. Otherwise the direction is published and the caller owns + * the transition until it publishes a final state. + * + * @param target PLC_STATE_RUNNING or PLC_STATE_STOPPED + * @return true when the transition is claimed and the caller must complete it + */ +bool plc_claim_transition(PLCState target); + +/** + * @brief Publish the state a transition landed on: RUNNING, STOPPED, ERROR or EMPTY. + * + * Ends the transition. ERROR is sticky against STOPPED: a task that crashed while + * a stop was tearing down has already recorded the more important fact, and the + * teardown finishing must not paper over it. + */ +void plc_publish_final_state(PLCState final_state); + +/** @brief True while a transition is in flight (either direction). */ +bool plc_state_is_transitioning(void); + /** * @brief Cleanup the PLC state manager and unloads the plugin manager. * @return void diff --git a/core/src/plc_app/plc_switch.c b/core/src/plc_app/plc_switch.c index d7c46285..474074ce 100644 --- a/core/src/plc_app/plc_switch.c +++ b/core/src/plc_app/plc_switch.c @@ -23,19 +23,41 @@ /* Default RUN: no plugin implementing the interface means no gating. */ static atomic_int switch_position = PLC_SWITCH_RUN; +/** + * Set when the switch moves, cleared when someone acts on it. + * + * Exists because state-change requests are dropped while a transition is in + * flight: a flip during a start or stop is refused, and without a record of it + * the switch and the PLC end up disagreeing with nobody retrying. Only the fact + * of movement is kept, never a queue of requests -- `switch_position` above + * already holds where the switch came to rest, which is the only position that + * matters once the dust settles. + * + * Deliberately platform-agnostic: every VPP that owns a switch reports through + * plc_set_switch_position(), so no plugin needs to know reconciliation exists. + */ +static atomic_bool switch_moved = false; + void plc_set_switch_position(plc_switch_t position) { const int normalized = (position == PLC_SWITCH_STOP) ? PLC_SWITCH_STOP : PLC_SWITCH_RUN; const int previous = atomic_exchange(&switch_position, normalized); - /* Log edges only. A plugin sampling a GPIO on a fast timer may call this - * on every sample; logging each one would flood the journal. */ + /* Log and record edges only. A plugin sampling a GPIO on a fast timer may + * call this on every sample; logging each one would flood the journal, and + * treating each one as movement would make the runtime reconcile forever. */ if (previous != normalized) { + atomic_store(&switch_moved, true); log_info("Mode switch moved to %s", normalized == PLC_SWITCH_RUN ? "RUN" : "STOP"); } } +bool plc_switch_take_movement(void) +{ + return atomic_exchange(&switch_moved, false); +} + plc_switch_t plc_get_switch_position(void) { return (plc_switch_t)atomic_load(&switch_position); diff --git a/core/src/plc_app/plc_switch.h b/core/src/plc_app/plc_switch.h index 28a2739a..d8203bb1 100644 --- a/core/src/plc_app/plc_switch.h +++ b/core/src/plc_app/plc_switch.h @@ -57,6 +57,22 @@ plc_switch_t plc_get_switch_position(void); */ bool plc_switch_allows_run(void); +/** + * @brief Consume the "switch has moved" record. True if it moved since the last call. + * + * State-change requests are dropped while a transition is in flight, so a flip + * during a start or stop is refused outright. This is how that intent survives: + * the runtime notes only that the switch moved, and the transition-completion + * path compares where it came to rest against the state actually reached, + * correcting a mismatch. Consuming the record is what stops it correcting the + * same movement twice. + * + * Movement-gated on purpose. An editor stop with the switch untouched records no + * movement, so nothing reverses it; comparing position against state + * unconditionally would make Stop impossible whenever the switch sits in RUN. + */ +bool plc_switch_take_movement(void); + #ifdef __cplusplus } #endif diff --git a/core/src/plc_app/scan_cycle_manager.c b/core/src/plc_app/scan_cycle_manager.c index 60dcab43..aa6a441e 100644 --- a/core/src/plc_app/scan_cycle_manager.c +++ b/core/src/plc_app/scan_cycle_manager.c @@ -189,7 +189,7 @@ int format_timing_stats_response(char *buffer, size_t buffer_size) if (n < 0) return 0; offset += (size_t)n; - /* Hold plc_tasks_reader_lock for the whole iteration. is_transitioning + /* Hold plc_tasks_reader_lock for the whole iteration. the TRANSITIONING state * gates new commands but does not bracket an in-flight STATS call — * a plugin-initiated STOP can fire while we're mid-loop, the bootstrap * thread joins task threads and frees plc_tasks[], and we'd then read diff --git a/core/src/plc_app/unix_socket.c b/core/src/plc_app/unix_socket.c index 74ebea83..bde0f2bc 100644 --- a/core/src/plc_app/unix_socket.c +++ b/core/src/plc_app/unix_socket.c @@ -1,10 +1,10 @@ #include #include #include -#include #include #include #include +#include #include #include #include @@ -23,15 +23,18 @@ extern PLCState plc_state; static plugin_driver_t *g_plugin_driver = NULL; +/* How long transition_worker waits to observe the landing before reconciling + * with the mode switch, and how often it looks. The bound is generous because a + * start includes plugin bring-up (SPI base scans, fieldbus probes); exceeding it + * means the transition is not going to land, which the watchdog reports. */ +#define LANDING_WAIT_MS 90000 +#define LANDING_POLL_MS 20 + void unix_socket_set_plugin_driver(void *driver) { g_plugin_driver = (plugin_driver_t *)driver; } -// Flag to prevent overlapping state transitions (e.g. START while STOP is in progress). -// Set before spawning the transition thread, cleared when the transition completes. -static atomic_int is_transitioning = 0; - static void *transition_worker(void *arg) { PLCState target = *(PLCState *)arg; @@ -44,70 +47,84 @@ static void *transition_worker(void *arg) target == PLC_STATE_RUNNING ? "RUNNING" : "STOPPED"); } - atomic_store(&is_transitioning, 0); + // Wait for the landing before reconciling. + // + // plc_set_state(RUNNING) returns as soon as load_plc_program() has spawned + // the PLC thread; that thread publishes RUNNING later, once the workers exist + // (measured at ~4 s on an SLM-RP4, most of it plugin bring-up). Reconciling + // straight after plc_set_state() therefore ran while the state was still + // TRANSITIONING_TO_RUN and threw the switch movement away without acting on + // it -- losing precisely the flip-during-a-start this exists to catch. + // + // Polling rather than a condvar handshake: the state IS the interlock, so + // nothing else can begin a transition while we wait, and there is no lock + // held here. The bound only exists so a transition that never lands cannot + // strand this thread -- the watchdog is what turns that into a reported + // fault. + for (int waited_ms = 0; plc_state_is_transitioning() && waited_ms < LANDING_WAIT_MS; + waited_ms += LANDING_POLL_MS) + { + struct timespec poll = { .tv_sec = 0, .tv_nsec = LANDING_POLL_MS * 1000000L }; + nanosleep(&poll, NULL); + } - // Converge on the mode switch. + // Reconcile with the mode switch. // - // plc_begin_transition() returns false while another transition is in - // flight, which means a stop request CAN be dropped: an editor START may - // win the gate, then a plugin's set_switch_position(STOP) + - // request_plc_stop() arrives and the stop is collapsed as "already - // transitioning". That would leave the PLC running with the switch in - // STOP, with nobody retrying. + // Requests are DROPPED while a transition is in flight, which on its own + // loses the switch's intent: flip to STOP during a start and the stop + // vanishes, leaving the PLC running with the switch in STOP and nobody + // retrying. Rather than queueing requests, the runtime remembers only + // whether the switch MOVED (plc_switch, so this works for every platform's + // plugin) and compares the position it came to rest at against the state we + // actually landed on. Several flips during one transition collapse to the + // final position, which is the only one that matters. // - // Re-checking here — after is_transitioning clears, so this call can win - // the gate — makes every interleaving converge on STOPPED without asking - // the requester to implement retry logic. The failure direction is the - // safe one, which is the property to preserve in any change to either - // caller. (The same drop was already possible for a plugin reporting a - // hardware fault during a start; this fixes that too.) - if (plc_get_state() == PLC_STATE_RUNNING && !plc_switch_allows_run()) + // Gated on movement, deliberately: an editor stop with the switch untouched + // records no movement, so nothing reconciles it away. Comparing position to + // state unconditionally would make Stop impossible whenever the switch sits + // in RUN. It also cannot ping-pong — each pass consumes the movement, and + // only the switch physically moving sets it again. + if (plc_switch_take_movement()) { - log_warn("Mode switch reads STOP but the PLC ended up RUNNING — stopping"); - plc_begin_transition(PLC_STATE_STOPPED); + const PLCState landed = plc_get_state(); + const PLCState wanted = plc_switch_allows_run() ? PLC_STATE_RUNNING : PLC_STATE_STOPPED; + + // Only reconcile from a clean landing. ERROR and EMPTY are not states to + // "correct" — restarting a faulted or programless PLC because a switch + // moved would fight the fault rather than report it. + if ((landed == PLC_STATE_RUNNING || landed == PLC_STATE_STOPPED) && landed != wanted) + { + log_warn("Mode switch came to rest in %s but the PLC landed on %s — correcting", + wanted == PLC_STATE_RUNNING ? "RUN" : "STOP", + landed == PLC_STATE_RUNNING ? "RUNNING" : "STOPPED"); + plc_begin_transition(wanted); + } } return NULL; } // Start a background thread that performs the (potentially slow) state -// transition. Returns true if the thread was spawned, false otherwise. +// transition. Returns true if the transition was claimed and a worker spawned. // -// This is the single authoritative entry point for all state transitions: -// socket START/STOP commands, plugin-initiated stops, AND the boot auto-start -// in plc_main.c (which calls this instead of plc_set_state() directly so that -// all paths share the same guard). That last point closes the race where the -// socket listener accepts a START while the auto-start is still in flight, -// which previously could spawn two concurrent load_plc_program() calls. +// The single authoritative entry point for every state change: socket +// START/STOP, plugin-initiated requests from a mode switch, and the boot +// auto-start in plc_main.c. All of them come here rather than calling +// plc_set_state() directly, so one arbiter decides what may begin. // -// Two safety guards on the entry path: +// That arbiter is plc_claim_transition(), which under the state lock refuses a +// request while the state is TRANSITIONING_TO_RUN or TRANSITIONING_TO_STOP, and +// refuses a request for the state the runtime is already in. There is no second +// flag to keep in step with plc_state -- the state IS the interlock, which is +// what makes "you cannot change state while changing state" true by +// construction rather than by two variables agreeing. // -// 1. CAS on `is_transitioning` 0→1: collapses concurrent calls. A -// misbehaving plugin spinning on plugin_request_plc_stop, or the boot -// path racing with a socket START, would otherwise pile up detached -// pthread workers. The CAS means only the first call fires the worker; -// everything else is a cheap return. -// -// 2. Re-check current state AFTER the CAS wins: closes the -// check-then-call race where the caller sees RUNNING, calls in, -// and the state flips to STOPPED before we spawn the worker. We'd -// otherwise dispatch a no-op transition, leaving STATUS reporting -// TRANSITIONING for the worker's lifetime for nothing. +// Requests refused here are dropped, not queued. The switch's intent survives +// via the movement reconciliation in transition_worker above. bool plc_begin_transition(PLCState target) { - int expected = 0; - if (!atomic_compare_exchange_strong(&is_transitioning, &expected, 1)) + if (!plc_claim_transition(target)) { - // Another transition is already in flight. Don't pile on. - return false; - } - - if (plc_get_state() == target) - { - // State already at target — release the gate and bail. No - // worker needed; reporting STATUS:TRANSITIONING for a no-op - // would just confuse external pollers. - atomic_store(&is_transitioning, 0); return false; } @@ -115,7 +132,9 @@ bool plc_begin_transition(PLCState target) if (!arg) { log_error("Failed to allocate transition argument"); - atomic_store(&is_transitioning, 0); + // Claimed but cannot run it: land a final state, or the runtime would sit + // in TRANSITIONING forever refusing every command. + plc_publish_final_state(PLC_STATE_STOPPED); return false; } *arg = target; @@ -125,7 +144,7 @@ bool plc_begin_transition(PLCState target) { log_error("Failed to create transition thread: %s", strerror(errno)); free(arg); - atomic_store(&is_transitioning, 0); + plc_publish_final_state(PLC_STATE_STOPPED); return false; } pthread_detach(tid); @@ -156,27 +175,16 @@ static ssize_t read_line(int fd, char *buffer, size_t max_length) static void format_status_response(char *response, size_t response_size) { - // While a transition is in progress, plc_state has already flipped - // to the target value (so the running task threads can exit their - // `while (plc_get_state() == RUNNING)` loops) but the actual - // load/unload work is still happening on the transition worker - // thread. Reporting the bare state here would tell external - // pollers STATUS:STOPPED while the runtime can't yet accept a - // START — they'd race ahead and get COMMAND:BUSY. - // - // Surfacing TRANSITIONING for the duration of the worker keeps - // _wait_for_plc_state(STOPPED) on the webserver side honest: - // STATUS:STOPPED is reported only after the worker has completed - // and is_transitioning has cleared. - if (atomic_load(&is_transitioning)) - { - strncpy(response, "STATUS:TRANSITIONING\n", response_size); - return; - } - PLCState current_state = plc_get_state(); - if (current_state == PLC_STATE_INIT) + // Both directions report as the one TRANSITIONING string that external + // callers already know. The distinction is internal (intent), and the + // webserver's _wait_for_plc_idle plus the editor both key off this wire + // value, so it stays exactly as it was. + if (current_state == PLC_STATE_TRANSITIONING_TO_RUN || + current_state == PLC_STATE_TRANSITIONING_TO_STOP) + strncpy(response, "STATUS:TRANSITIONING\n", response_size); + else if (current_state == PLC_STATE_INIT) strncpy(response, "STATUS:INIT\n", response_size); else if (current_state == PLC_STATE_RUNNING) strncpy(response, "STATUS:RUNNING\n", response_size); @@ -193,8 +201,9 @@ static void format_status_response(char *response, size_t response_size) void handle_unix_socket_commands(const char *command, char *response, size_t response_size) { // While a state transition is in progress, only allow PING and STATUS. - // Everything else gets COMMAND:BUSY so commands cannot overlap. - if (atomic_load(&is_transitioning)) + // Everything else gets COMMAND:BUSY: you cannot change state, or read state + // that is mid-change, while it is changing. + if (plc_state_is_transitioning()) { if (strcmp(command, "PING") == 0) { diff --git a/core/src/plc_app/unix_socket.h b/core/src/plc_app/unix_socket.h index 08c98f3a..2c8c0c10 100644 --- a/core/src/plc_app/unix_socket.h +++ b/core/src/plc_app/unix_socket.h @@ -20,7 +20,7 @@ void unix_socket_set_plugin_driver(void *driver); // Spawn a detached worker thread that transitions the PLC to `target`. // Shared with plugin_driver so a plugin's request_plc_stop callback goes // through the same transition-guarded path as an external STOP command -// (same overlap protection via the internal is_transitioning flag). +// (same overlap protection: plc_claim_transition refuses while TRANSITIONING). bool plc_begin_transition(PLCState target); #endif // UNIX_SOCKET_H diff --git a/core/src/plc_app/utils/watchdog.c b/core/src/plc_app/utils/watchdog.c index 3d91670e..ac0b655d 100644 --- a/core/src/plc_app/utils/watchdog.c +++ b/core/src/plc_app/utils/watchdog.c @@ -12,16 +12,47 @@ atomic_long plc_heartbeat; +/* Watchdog loop period, and how long a state change may plausibly take before + * something is wrong. A start brings plugins up (SPI base scans, fieldbus + * probes) and a stop joins task threads, so the bound is generous: it is here to + * catch a transition that will never finish, not to police a slow one. */ +#define WATCHDOG_TICK_S 2 +#define TRANSITION_STUCK_S 60 + void *watchdog_thread(void *arg) { (void)arg; long last = atomic_load(&plc_heartbeat); + int transitioning_ticks = 0; while (1) { - sleep(2); // Watch every 2 seconds + sleep(WATCHDOG_TICK_S); PLCState current_state = plc_get_state(); + + // A transition that never publishes a final state would leave the + // runtime in TRANSITIONING forever, refusing every command but PING and + // STATUS — the state is the interlock now, so there is no flag anyone + // could clear to recover. Every path is meant to land a final state; + // this is the backstop for the one that doesn't, turning a silent + // permanent wedge into a reported fault the webserver can act on. + if (current_state == PLC_STATE_TRANSITIONING_TO_RUN || + current_state == PLC_STATE_TRANSITIONING_TO_STOP) + { + transitioning_ticks++; + if (transitioning_ticks * WATCHDOG_TICK_S > TRANSITION_STUCK_S) + { + log_error("Watchdog: state change stuck in progress for over %d s — " + "forcing ERROR so the runtime accepts commands again", + TRANSITION_STUCK_S); + plc_force_error_state(); + transitioning_ticks = 0; + } + continue; + } + transitioning_ticks = 0; + if (current_state != PLC_STATE_RUNNING) { // Reset tracking when not running so we get a fresh From 147b131eb433afcb33a02b9585054553f33c3bed Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 4 Aug 2026 19:51:14 -0400 Subject: [PATCH 6/9] chore(runtime): stop logging a routine mode-switch stop as an error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/src/drivers/plugin_driver.c | 17 +++++++++-------- core/src/plc_app/plc_main.c | 14 ++++++++++---- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 06138319..5dc3743a 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -159,21 +159,22 @@ static uint8_t plugin_debug_write(uint8_t arr, uint16_t elem, // plugin's own stop_loop is invoked. Plugins that enter fault-stopped state // are expected to short-circuit their I/O during that window. // -// No pre-check on plc_get_state() here: plc_begin_transition does the -// check atomically (under the same gate that prevents concurrent -// transitions), so doing it again outside would just re-introduce the -// check-then-act race the gate is there to close. +// No pre-check on plc_get_state() here: plc_claim_transition does the check +// atomically under the state lock (the state being TRANSITIONING is itself what +// prevents concurrent transitions), so doing it again outside would just +// re-introduce the check-then-act race that interlock exists to close. static void plugin_request_plc_stop(const char *reason) { - log_error("[PLUGIN] stop requested: %s", reason ? reason : "(no reason given)"); + log_info("[PLUGIN] stop requested: %s", reason ? reason : "(no reason given)"); if (!plc_begin_transition(PLC_STATE_STOPPED)) { // Either the PLC is already stopping/stopped or another stop // is already in flight — either way, nothing to do. // - // A stop dropped because another transition was in flight is picked up - // by the converge-on-switch re-check at the end of transition_worker - // (unix_socket.c), so a switch-driven stop can't be lost here. + // A stop dropped because another transition was in flight is recovered + // by the switch-movement reconciliation once that transition lands (see + // transition_worker in unix_socket.c), so a switch-driven stop cannot be + // lost here. log_warn("[PLUGIN] stop request collapsed (already transitioning or not running)"); } } diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index c2a6f79e..c19d234b 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -168,10 +168,16 @@ int main(int argc, char *argv[]) log_info("Runtime started in SAFE MODE - PLC program will not be loaded"); log_info("Upload a corrected program to recover"); } - // Same gate as any other start. Plugin init ran above, so a VPP that owns a - // physical mode switch has already reported its position — a device powered - // up with the switch in STOP is gated here rather than starting and then - // being stopped a moment later. + // Same gate as any other start, but note what it can and cannot see. A VPP + // plugin that owns a physical mode switch is initialised as part of loading + // the program — inside the start transition below — so at this point the + // switch has usually NOT been reported yet and the gate reads the default + // (RUN). A device powered up with the switch in STOP therefore does start, + // and is then corrected: the plugin reports STOP during init, that request is + // dropped because a transition is in flight, and the switch-movement + // reconciliation stops the PLC as soon as the start lands. Safe, but the gate + // only bites here for a switch position already known at this point (e.g. one + // reported by a plugin the runtime loaded independently of the program). else if (!plc_switch_allows_run()) { log_info("Hardware mode switch is in STOP - PLC left stopped"); From eea93f18fe2314c0c62fc3328ef723212058e8cb Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 11 Aug 2026 12:48:05 -0400 Subject: [PATCH 7/9] fix(state/switch/socket): address PR #162 review findings 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) --- core/src/plc_app/plc_main.c | 17 +++- core/src/plc_app/plc_state_manager.cpp | 134 ++++++++++++++++++------- core/src/plc_app/plc_state_manager.h | 47 ++++++++- core/src/plc_app/plc_switch.c | 5 + core/src/plc_app/plc_switch.h | 14 +++ core/src/plc_app/unix_socket.c | 114 +++++++++++++++------ core/src/plc_app/utils/watchdog.c | 19 +++- 7 files changed, 270 insertions(+), 80 deletions(-) diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index c19d234b..be6cbb2d 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -194,14 +194,21 @@ int main(int argc, char *argv[]) sleep(1); } - // Cleanup plugin driver system + log_info("Shutting down..."); + + // Stop the program BEFORE destroying the driver, not after. + // + // plc_state_manager_cleanup() tears the program down, and its teardown calls + // plugin_driver_stop(plugin_driver) -- so destroying the driver first left that + // call reading freed memory, and the cycle thread could still be running plugin + // cycle hooks through the same pointer while it wound down. The order here is + // the dependency order: no program, then no driver. + plc_state_manager_cleanup(); + if (plugin_driver) { plugin_driver_destroy(plugin_driver); + plugin_driver = NULL; } - - // Cleanup - log_info("Shutting down..."); - plc_state_manager_cleanup(); return 0; } diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 3dc25cb4..7668dfca 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -194,9 +194,15 @@ static void *plc_task_thread(void *arg) pthread_setname_np(pthread_self(), ctx->name); + /* 99 is reserved for the dispatcher, which has to be strictly above every + * worker for its tick never to be delayed by a busy one. A worker allowed to + * reach 99 would only TIE it, and SCHED_FIFO does not time-slice between equal + * priorities: a task that never blocks (an unbounded loop in IEC code) would + * then keep the dispatcher off that CPU entirely, along with anything else + * trying to bring the PLC down. */ int rt = ctx->priority; if (rt < 1) rt = 1; - if (rt > 99) rt = 99; + if (rt > 98) rt = 98; sched_param sp{}; sp.sched_priority = rt; if (pthread_setschedparam(pthread_self(), SCHED_FIFO, &sp) != 0) @@ -363,6 +369,46 @@ static void *plc_task_thread(void *arg) return nullptr; } +/* Wake every worker, join them, and destroy the task array. + * + * Two callers: the normal end of the dispatcher loop, and the early-out below + * when bring-up finished but this start is no longer the transition in flight. + * The second one exists so that path tears its workers down instead of leaving + * them parked on a semaphore nobody will ever post again. */ +static void reap_task_threads(void) +{ + log_info("Stopping %zu PLC task thread(s)", plc_task_count); + /* Wake every worker: post its release semaphore (breaks sem_wait) and + * SIGUSR1 (breaks a syscall). A worker mid-scan finishes, loops to + * sem_wait, consumes the post, observes state != RUNNING, and exits. */ + for (size_t i = 0; i < plc_task_count; ++i) + { + sem_post(&plc_tasks[i].go); + pthread_kill(plc_tasks[i].thread, SIGUSR1); + } + for (size_t i = 0; i < plc_task_count; ++i) + { + pthread_join(plc_tasks[i].thread, nullptr); + } + + /* Take plc_tasks_lock for the tracker-cleanup + free. A STATS reader + * that started iterating before STOP arrived will block briefly + * waiting for this critical section, then exit because plc_task_count + * is observed as 0. Without the lock, the reader could be midway + * through scan_cycle_tracker_snapshot when we pthread_mutex_destroy + * the tracker's own mutex below — undefined behaviour. */ + pthread_mutex_lock(&plc_tasks_lock); + for (size_t i = 0; i < plc_task_count; ++i) + { + scan_cycle_tracker_cleanup(&plc_tasks[i].tracker); + sem_destroy(&plc_tasks[i].go); + } + std::free(plc_tasks); + plc_tasks = nullptr; + plc_task_count = 0; + pthread_mutex_unlock(&plc_tasks_lock); +} + void *plc_cycle_thread(void *arg) { PluginManager *pm = (PluginManager *)arg; @@ -723,8 +769,25 @@ void *plc_cycle_thread(void *arg) * This is also what ends the transition claimed by plc_claim_transition, so * every path out of this thread from here on must land a final state: the * crash recovery above publishes ERROR, and the stop path publishes STOPPED - * once teardown joins. */ - plc_publish_final_state(PLC_STATE_RUNNING); + * once teardown joins. + * + * Conditional, because ending a transition is only ours to do while it is + * still the one in flight. Two paths get here otherwise: the watchdog forced + * ERROR because this start exceeded its bound, and plc_state_manager_cleanup + * published TRANSITIONING_TO_STOP on shutdown and is now blocked joining this + * very thread. Publishing RUNNING in either case erases a state someone else + * landed, and in the second it hangs the process -- the dispatcher loop below + * would never see a non-RUNNING state and the join would never return. */ + if (!plc_publish_running_if_claimed()) + { + log_warn("PLC bring-up finished but the start is no longer the transition in " + "flight (state is %d) — not releasing the first scan", + (int)plc_get_state()); + reap_task_threads(); + signal(SIGFPE, SIG_DFL); + signal(SIGSEGV, SIG_DFL); + return NULL; + } /* --------------------------------------------------------------------- * GCD master-tick dispatcher. @@ -915,36 +978,7 @@ void *plc_cycle_thread(void *arg) pthread_mutex_unlock(&done_mutex); } - log_info("Stopping %zu PLC task thread(s)", plc_task_count); - /* Wake every worker: post its release semaphore (breaks sem_wait) and - * SIGUSR1 (breaks a syscall). A worker mid-scan finishes, loops to - * sem_wait, consumes the post, observes state != RUNNING, and exits. */ - for (size_t i = 0; i < plc_task_count; ++i) - { - sem_post(&plc_tasks[i].go); - pthread_kill(plc_tasks[i].thread, SIGUSR1); - } - for (size_t i = 0; i < plc_task_count; ++i) - { - pthread_join(plc_tasks[i].thread, nullptr); - } - - /* Take plc_tasks_lock for the tracker-cleanup + free. A STATS reader - * that started iterating before STOP arrived will block briefly - * waiting for this critical section, then exit because plc_task_count - * is observed as 0. Without the lock, the reader could be midway - * through scan_cycle_tracker_snapshot when we pthread_mutex_destroy - * the tracker's own mutex below — undefined behaviour. */ - pthread_mutex_lock(&plc_tasks_lock); - for (size_t i = 0; i < plc_task_count; ++i) - { - scan_cycle_tracker_cleanup(&plc_tasks[i].tracker); - sem_destroy(&plc_tasks[i].go); - } - std::free(plc_tasks); - plc_tasks = nullptr; - plc_task_count = 0; - pthread_mutex_unlock(&plc_tasks_lock); + reap_task_threads(); signal(SIGFPE, SIG_DFL); signal(SIGSEGV, SIG_DFL); @@ -1069,12 +1103,18 @@ extern "C" int unload_plc_program(PluginManager *pm) { /* The dispatcher and its workers leave their loops on anything that is * not RUNNING, and the join below depends on that. A claimed stop has - * already published TRANSITIONING_TO_STOP, which is that signal. This - * also covers the shutdown path (plc_state_manager_cleanup), which tears - * down without claiming a transition first: publish the direction here so - * the loops still exit. ERROR is left alone -- it must survive teardown. */ + * already published TRANSITIONING_TO_STOP, which is that signal. + * + * Anything that is not ERROR is overwritten, not just RUNNING, because + * this also covers the shutdown path (plc_state_manager_cleanup), which + * tears down without claiming a transition first. On SIGTERM during a + * start the state is TRANSITIONING_TO_RUN, and a guard that only matched + * RUNNING published nothing at all -- so the cycle thread went on to + * publish RUNNING and the join below never returned. Pairs with the + * conditional publish in plc_cycle_thread: this write is what makes that + * one decline. ERROR is left alone -- it must survive teardown. */ pthread_mutex_lock(&state_mutex); - if (plc_state == PLC_STATE_RUNNING) + if (plc_state != PLC_STATE_ERROR) { plc_state = PLC_STATE_TRANSITIONING_TO_STOP; } @@ -1191,6 +1231,24 @@ extern "C" void plc_publish_final_state(PLCState final_state) log_info("PLC State: %s", name); } +extern "C" bool plc_publish_running_if_claimed(void) +{ + /* Land RUNNING only while the start we are completing is still the transition + * in flight. The check and the write share one critical section: reading the + * state and then publishing in two steps would let a stop be claimed in + * between, and RUNNING would go down on top of it. */ + pthread_mutex_lock(&state_mutex); + if (plc_state != PLC_STATE_TRANSITIONING_TO_RUN) + { + pthread_mutex_unlock(&state_mutex); + return false; + } + plc_state = PLC_STATE_RUNNING; + pthread_mutex_unlock(&state_mutex); + log_info("PLC State: RUNNING"); + return true; +} + extern "C" bool plc_set_state(PLCState new_state) { // Performs a transition already claimed via plc_claim_transition(), which diff --git a/core/src/plc_app/plc_state_manager.h b/core/src/plc_app/plc_state_manager.h index d368c2ce..9cfe24be 100644 --- a/core/src/plc_app/plc_state_manager.h +++ b/core/src/plc_app/plc_state_manager.h @@ -151,9 +151,22 @@ void plc_tasks_reader_unlock(void); PLCState plc_get_state(void); /** - * @brief Set the PLC state. In case of a state change, it will load or unload the PLC program as needed. - * @param new_state The new PLC state to set - * @return true if the state was changed, false if it was already in the desired state + * @brief Body of a transition that has ALREADY been claimed. Not a setter. + * + * PRECONDITION: plc_claim_transition(new_state) returned true, so a TRANSITIONING + * state is current. Calling this without a claim tears the program down with no + * TRANSITIONING_TO_STOP for the scan loops to observe and then publishes a final + * state nobody claimed, corrupting the interlock the TRANSITIONING states are. + * + * Writes no state of its own: the landing is published by whoever knows the work + * finished -- plc_cycle_thread for RUNNING, unload_plc_program for STOPPED -- with + * the failure paths here publishing ERROR or EMPTY. + * + * Go through plc_begin_transition() instead. It is the only intended entry point + * for a state change and it does the claiming for you. + * + * @param new_state PLC_STATE_RUNNING or PLC_STATE_STOPPED + * @return true if the transition body completed without error */ bool plc_set_state(PLCState new_state); @@ -180,9 +193,37 @@ bool plc_claim_transition(PLCState target); */ void plc_publish_final_state(PLCState final_state); +/** + * @brief Publish RUNNING, but only if the start being completed is still in flight. + * + * Same landing as plc_publish_final_state(PLC_STATE_RUNNING), refused when the + * current state is not TRANSITIONING_TO_RUN. Ending a transition is only the + * caller's to do while it owns it: a watchdog-forced ERROR or a shutdown that + * published TRANSITIONING_TO_STOP has taken ownership away, and overwriting either + * with RUNNING erases a landing (or, on the shutdown path, hangs the join waiting + * for a state that is never written again). + * + * @return true when RUNNING was published and the caller may start scanning + */ +bool plc_publish_running_if_claimed(void); + /** @brief True while a transition is in flight (either direction). */ bool plc_state_is_transitioning(void); +/* How long a state change may plausibly take before something is wrong. + * + * ONE bound, two consumers, deliberately ordered: transition_worker stops waiting + * to observe the landing at PLC_TRANSITION_LANDING_TIMEOUT_MS, and the watchdog + * forces ERROR strictly later. Two independent numbers is how the watchdog came to + * fire 30 s before the runtime itself had given up -- ending a transition while its + * worker was still executing it. + * + * Generous on purpose: a start brings plugins up (SPI base scans, fieldbus probes, + * certificate generation) and a stop joins task threads. The bound is here to catch + * a transition that will never finish, not to police a slow one. */ +#define PLC_TRANSITION_LANDING_TIMEOUT_MS 90000 +#define PLC_TRANSITION_STUCK_TIMEOUT_MS (PLC_TRANSITION_LANDING_TIMEOUT_MS + 30000) + /** * @brief Cleanup the PLC state manager and unloads the plugin manager. * @return void diff --git a/core/src/plc_app/plc_switch.c b/core/src/plc_app/plc_switch.c index 474074ce..c744efc0 100644 --- a/core/src/plc_app/plc_switch.c +++ b/core/src/plc_app/plc_switch.c @@ -58,6 +58,11 @@ bool plc_switch_take_movement(void) return atomic_exchange(&switch_moved, false); } +void plc_switch_note_movement(void) +{ + atomic_store(&switch_moved, true); +} + plc_switch_t plc_get_switch_position(void) { return (plc_switch_t)atomic_load(&switch_position); diff --git a/core/src/plc_app/plc_switch.h b/core/src/plc_app/plc_switch.h index d8203bb1..5dcb967b 100644 --- a/core/src/plc_app/plc_switch.h +++ b/core/src/plc_app/plc_switch.h @@ -73,6 +73,20 @@ bool plc_switch_allows_run(void); */ bool plc_switch_take_movement(void); +/** + * @brief Re-arm the "switch has moved" record after a correction could not start. + * + * The counterpart to plc_switch_take_movement(): consuming the record commits the + * caller to acting on it, and this hands it back when that action was refused -- + * another transition claimed in the gap, or a spawn that failed. Without it the + * switch's intent is lost silently and the PLC stays in the state the switch + * disagrees with, which is the exact failure the record exists to prevent. + * + * Not for plugins: a plugin reporting a position calls plc_set_switch_position(), + * which records movement on an edge by itself. + */ +void plc_switch_note_movement(void); + #ifdef __cplusplus } #endif diff --git a/core/src/plc_app/unix_socket.c b/core/src/plc_app/unix_socket.c index bde0f2bc..0244c278 100644 --- a/core/src/plc_app/unix_socket.c +++ b/core/src/plc_app/unix_socket.c @@ -19,15 +19,14 @@ #include "utils/utils.h" extern volatile sig_atomic_t keep_running; -extern PLCState plc_state; static plugin_driver_t *g_plugin_driver = NULL; -/* How long transition_worker waits to observe the landing before reconciling - * with the mode switch, and how often it looks. The bound is generous because a - * start includes plugin bring-up (SPI base scans, fieldbus probes); exceeding it - * means the transition is not going to land, which the watchdog reports. */ -#define LANDING_WAIT_MS 90000 +/* How long run_transition waits to observe the landing before reconciling with + * the mode switch, and how often it looks. The bound comes from + * plc_state_manager.h so that the watchdog's stuck-transition bound is derived + * from the same number and can only fire strictly later. */ +#define LANDING_WAIT_MS PLC_TRANSITION_LANDING_TIMEOUT_MS #define LANDING_POLL_MS 20 void unix_socket_set_plugin_driver(void *driver) @@ -35,11 +34,12 @@ void unix_socket_set_plugin_driver(void *driver) g_plugin_driver = (plugin_driver_t *)driver; } -static void *transition_worker(void *arg) +// Body of a claimed transition: perform it, wait for it to land, then reconcile +// with the mode switch. Normally runs on the detached worker spawned below, but +// is called directly when that worker cannot be spawned -- the transition has +// already been claimed by then, and it has to be completed by somebody. +static bool run_transition(PLCState target) { - PLCState target = *(PLCState *)arg; - free(arg); - bool result = plc_set_state(target); if (!result) { @@ -97,15 +97,40 @@ static void *transition_worker(void *arg) log_warn("Mode switch came to rest in %s but the PLC landed on %s — correcting", wanted == PLC_STATE_RUNNING ? "RUN" : "STOP", landed == PLC_STATE_RUNNING ? "RUNNING" : "STOPPED"); - plc_begin_transition(wanted); + + // The movement record was consumed above, so a refusal here would + // throw the switch's intent away for good: the state is already + // final, which leaves the socket thread free to claim a transition in + // the gap, and the spawn paths below can fail too. Put the record + // back so the next landing reconciles instead. This cannot ping-pong + // -- the retry compares position against state again, and a landing + // that agrees with the switch just consumes the record. + if (!plc_begin_transition(wanted)) + { + plc_switch_note_movement(); + log_warn("Correction to %s did not go through — re-armed for the " + "next landing", + wanted == PLC_STATE_RUNNING ? "RUN" : "STOP"); + } } } + return result; +} + +static void *transition_worker(void *arg) +{ + PLCState target = *(PLCState *)arg; + free(arg); + + run_transition(target); return NULL; } // Start a background thread that performs the (potentially slow) state -// transition. Returns true if the transition was claimed and a worker spawned. +// transition. Returns false when the request was refused; otherwise the +// transition is under way (or, if the worker could not be spawned, has already +// been completed on this thread -- see below). // // The single authoritative entry point for every state change: socket // START/STOP, plugin-initiated requests from a mode switch, and the boot @@ -128,24 +153,39 @@ bool plc_begin_transition(PLCState target) return false; } + // Claimed but the worker cannot be spawned: run the transition on this thread + // rather than publishing a landing. + // + // Publishing STOPPED here used to look like the safe way out, and it is the + // opposite. The claim has already published TRANSITIONING_TO_STOP, which is + // what makes the dispatcher and workers leave their loops -- so on a stop from + // RUNNING the scan really does end, but unload_plc_program never runs: + // journal_cleanup, plugin_driver_stop, plugin_manager_destroy and the dlclose + // are all skipped, plc_program stays non-NULL, plc_thread is never joined, and + // STATUS reports a stop that tore nothing down. The next start then re-enters + // plugin_driver_init on live plugin state and re-runs a program whose statics + // were never reinitialised. (For a start from EMPTY it also reported "no + // program" as "stopped".) + // + // Completing it here blocks this caller for the duration -- the socket is + // single-client, so the editor waits -- which on a thread-or-memory exhaustion + // path is the cheaper of the two costs by a wide margin. PLCState *arg = malloc(sizeof(PLCState)); if (!arg) { - log_error("Failed to allocate transition argument"); - // Claimed but cannot run it: land a final state, or the runtime would sit - // in TRANSITIONING forever refusing every command. - plc_publish_final_state(PLC_STATE_STOPPED); - return false; + log_error("Failed to allocate transition argument — completing the " + "transition on the calling thread"); + return run_transition(target); } *arg = target; pthread_t tid; if (pthread_create(&tid, NULL, transition_worker, arg) != 0) { - log_error("Failed to create transition thread: %s", strerror(errno)); + log_error("Failed to create transition thread (%s) — completing the " + "transition on the calling thread", strerror(errno)); free(arg); - plc_publish_final_state(PLC_STATE_STOPPED); - return false; + return run_transition(target); } pthread_detach(tid); return true; @@ -198,11 +238,28 @@ static void format_status_response(char *response, size_t response_size) strncpy(response, "STATUS:UNKNOWN\n", response_size); } +static void format_switch_response(char *response, size_t response_size) +{ + // Report the mode-switch position a VPP plugin last stored. Devices with no + // switch-aware plugin always answer RUN. + if (plc_get_switch_position() == PLC_SWITCH_RUN) + strncpy(response, "SWITCH:RUN\n", response_size); + else + strncpy(response, "SWITCH:STOP\n", response_size); +} + void handle_unix_socket_commands(const char *command, char *response, size_t response_size) { - // While a state transition is in progress, only allow PING and STATUS. - // Everything else gets COMMAND:BUSY: you cannot change state, or read state - // that is mid-change, while it is changing. + // While a state transition is in progress, only allow the reads: you cannot + // change state while it is changing, and everything else gets COMMAND:BUSY. + // + // SWITCH belongs here with PING and STATUS. It is a plain atomic load of + // plc_switch with no coupling to plc_state, so there is nothing mid-change for + // it to expose -- and answering BUSY meant the webserver dropped + // switchPosition from every status response for the whole duration of a start + // or stop (parse_switch_position returns None) and GET /switch reported + // "unknown". An editor that decides whether a start is allowed from that field + // lost it precisely while polling through the transition it had just asked for. if (plc_state_is_transitioning()) { if (strcmp(command, "PING") == 0) @@ -213,6 +270,10 @@ void handle_unix_socket_commands(const char *command, char *response, size_t res { format_status_response(response, response_size); } + else if (strcmp(command, "SWITCH") == 0) + { + format_switch_response(response, response_size); + } else { strncpy(response, "COMMAND:BUSY\n", response_size); @@ -246,12 +307,7 @@ void handle_unix_socket_commands(const char *command, char *response, size_t res } else if (strcmp(command, "SWITCH") == 0) { - // Report the mode-switch position a VPP plugin last stored. Devices - // with no switch-aware plugin always answer RUN. - if (plc_get_switch_position() == PLC_SWITCH_RUN) - strncpy(response, "SWITCH:RUN\n", response_size); - else - strncpy(response, "SWITCH:STOP\n", response_size); + format_switch_response(response, response_size); } else if (strcmp(command, "START") == 0) { diff --git a/core/src/plc_app/utils/watchdog.c b/core/src/plc_app/utils/watchdog.c index ac0b655d..11c43bf3 100644 --- a/core/src/plc_app/utils/watchdog.c +++ b/core/src/plc_app/utils/watchdog.c @@ -12,12 +12,12 @@ atomic_long plc_heartbeat; -/* Watchdog loop period, and how long a state change may plausibly take before - * something is wrong. A start brings plugins up (SPI base scans, fieldbus - * probes) and a stop joins task threads, so the bound is generous: it is here to - * catch a transition that will never finish, not to police a slow one. */ +/* Watchdog loop period. The stuck-transition bound is NOT defined here: it comes + * from plc_state_manager.h, where it is derived from the same constant the + * transition worker waits on, so this can never fire while the runtime still + * considers the transition to be progressing normally. */ #define WATCHDOG_TICK_S 2 -#define TRANSITION_STUCK_S 60 +#define TRANSITION_STUCK_S (PLC_TRANSITION_STUCK_TIMEOUT_MS / 1000) void *watchdog_thread(void *arg) { @@ -37,6 +37,15 @@ void *watchdog_thread(void *arg) // could clear to recover. Every path is meant to land a final state; // this is the backstop for the one that doesn't, turning a silent // permanent wedge into a reported fault the webserver can act on. + // + // KNOWN LIMITATION: forcing ERROR releases the interlock but does not + // abort the transition, so the worker that failed to land is still + // running -- and a START accepted from ERROR would begin a second one + // over the top of it. Reaching this point at all now takes longer than + // the runtime's own landing bound (see PLC_TRANSITION_STUCK_TIMEOUT_MS), + // which removes the realistic trigger; closing it properly needs the + // transition owner to be able to abort its own work, which is the + // lifecycle-executor refactor and not this function's job. if (current_state == PLC_STATE_TRANSITIONING_TO_RUN || current_state == PLC_STATE_TRANSITIONING_TO_STOP) { From d7f30adb69f3ba985dbfcfb9733f617c01db5626 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 11 Aug 2026 14:26:18 -0400 Subject: [PATCH 8/9] fix(runtime): make graceful shutdown actually graceful 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) --- core/src/drivers/plugin_driver.c | 39 ++++++++++++++++-- core/src/drivers/plugin_driver.h | 13 ++++++ core/src/plc_app/plc_main.c | 55 ++++++++++++++++++++------ core/src/plc_app/plc_state_manager.cpp | 32 +++++++++++++++ webserver/runtimemanager.py | 25 +++++++++++- 5 files changed, 146 insertions(+), 18 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 5dc3743a..8cac385a 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -738,6 +738,21 @@ int plugin_driver_cleanup_init(plugin_driver_t *driver) return cleaned; } +void plugin_driver_release_gil(void) +{ + if (!Py_IsInitialized()) + { + return; + } + /* Idempotent: a second call with the GIL already released would be calling + * PyEval_SaveThread() without holding it. */ + if (main_tstate != NULL) + { + return; + } + main_tstate = PyEval_SaveThread(); +} + // Call the thread function for each plugin int plugin_driver_start(plugin_driver_t *driver) { @@ -752,11 +767,19 @@ int plugin_driver_start(plugin_driver_t *driver) return 0; } - // Only manage Python GIL if we have Python plugins and Python is initialized + // Only manage Python GIL if we have Python plugins and Python is initialized. + // + // Acquire-then-save leaves this thread without the GIL, which is the point: + // the plugin threads started below need it. The saved state is deliberately + // NOT stored in main_tstate -- this runs on the PLC cycle thread, and + // main_tstate is what plugin_driver_destroy restores before Py_FinalizeEx(), + // which must be the MAIN thread's state. Overwriting it here meant a shutdown + // after a start restored a state belonging to a thread that no longer exists. + // plugin_driver_release_gil() owns that value. if (has_python_plugin && Py_IsInitialized()) { - gstate = PyGILState_Ensure(); - main_tstate = PyEval_SaveThread(); + gstate = PyGILState_Ensure(); + PyEval_SaveThread(); } for (int i = 0; i < driver->plugin_count; i++) @@ -971,10 +994,18 @@ void plugin_driver_destroy(plugin_driver_t *driver) if (python_initialized) { - PyGILState_Release(local_gstate); + /* Py_FinalizeEx() requires the GIL, and getting there with it released is + * what used to segfault the runtime on every graceful shutdown where the + * PLC had never run (Py_FinalizeEx -> PyImport_GetModule with no thread + * state). main_tstate is only non-NULL once the GIL has been saved by the + * main thread, so when it is NULL the right move is to KEEP the state + * PyGILState_Ensure() gave us above rather than dropping it. */ if (main_tstate != NULL) { + PyGILState_Release(local_gstate); PyEval_RestoreThread(main_tstate); + /* Consumed: a second destroy must not restore a stale state. */ + main_tstate = NULL; } Py_FinalizeEx(); } diff --git a/core/src/drivers/plugin_driver.h b/core/src/drivers/plugin_driver.h index 62994081..b834ee3f 100644 --- a/core/src/drivers/plugin_driver.h +++ b/core/src/drivers/plugin_driver.h @@ -100,6 +100,19 @@ int plugin_driver_start(plugin_driver_t *driver); int plugin_driver_stop(plugin_driver_t *driver); void plugin_driver_destroy(plugin_driver_t *driver); +/* Release the Python GIL held by the calling thread after plugin loading, and + * remember the thread state so plugin_driver_destroy() can restore it before + * Py_FinalizeEx(). + * + * Call from the MAIN thread, once, after plugins are loaded. The runtime used to + * call PyEval_SaveThread() directly and drop the returned state on the floor, + * which left the driver with nothing to restore: shutdown then finalised the + * interpreter with no GIL held and segfaulted -- on every graceful shutdown of a + * runtime whose PLC had never started, safe mode included. Keeping the + * bookkeeping next to the code that consumes it is what makes that + * unrepresentable. No-op when Python was never initialised. */ +void plugin_driver_release_gil(void); + // Cycle hook functions for native plugins (called during PLC scan cycle) // These iterate through all active native plugins and call their cycle hooks // Plugins opt-in by implementing cycle_start/cycle_end; opt-out by not implementing them diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index be6cbb2d..b8ed246b 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -27,7 +27,10 @@ volatile sig_atomic_t keep_running = 1; plugin_driver_t *plugin_driver = NULL; extern bool print_logs; -void handle_sigint(int sig) +/* Graceful shutdown for both signals that mean "stop": SIGINT from an + * interactive Ctrl-C, and SIGTERM from a supervisor. Drops out of the main loop + * so the program is stopped and the plugin driver torn down on the way out. */ +void handle_shutdown_signal(int sig) { (void)sig; keep_running = 0; @@ -83,12 +86,22 @@ int main(int argc, char *argv[]) return -1; } - // Handle SIGINT for graceful shutdown + // Handle SIGINT and SIGTERM for graceful shutdown. + // + // SIGTERM matters as much as SIGINT and was missing: RuntimeManager stops the + // runtime with process.terminate() (SIGTERM) and only escalates to + // process.kill() after a 5 s wait, and systemd's default KillSignal is also + // SIGTERM. With no handler installed the default disposition applied, so every + // supervisor-initiated stop killed the process outright -- the PLC program was + // never unloaded, plugins never stopped, the journal never flushed, and the + // program .so never dlclose'd. The escalation to SIGKILL is the backstop for a + // shutdown that hangs, not the normal path. struct sigaction sa; - sa.sa_handler = handle_sigint; + sa.sa_handler = handle_shutdown_signal; sigemptyset(&sa.sa_mask); sa.sa_flags = 0; sigaction(SIGINT, &sa, NULL); + sigaction(SIGTERM, &sa, NULL); // Install the process-wide SIGUSR1 wake handler exactly once. Task // threads (plc_state_manager.cpp) and EtherCAT bus threads @@ -115,17 +128,20 @@ int main(int argc, char *argv[]) return -1; } - // Start UNIX socket server - if (setup_unix_socket() != 0) - { - log_error("Failed to set up UNIX socket"); - return -1; - } - // Initialize plugin driver system BEFORE loading the PLC program. // plc_set_state(RUNNING) triggers load_plc_program() which uses the plugin // driver to update config and re-init plugins, and plc_cycle_thread() calls // plugin_driver_start() after image tables are populated. + // + // This block runs BEFORE the command socket exists, deliberately. A START + // accepted while it is still executing runs load_plc_program() -> + // plugin_driver_init() on the transition worker at the same time as this + // thread is inside plugin_driver_load_config()/plugin_driver_init(), and + // rebuilding a plugin slot dlcloses the .so -- so a plugin sleeping in its + // own init() on the other thread returns into an unmapped page. Observed as a + // SIGSEGV in the main thread at an address inside the plugin that had just + // been unloaded. Transition arbitration cannot help here: there is only one + // transition, racing driver setup rather than another transition. plugin_driver = plugin_driver_create(); if (plugin_driver) { @@ -146,20 +162,35 @@ int main(int argc, char *argv[]) // This prevents a deadlock where the main thread holds the GIL forever // while sleeping, blocking other threads (like the unix socket thread) // from using Python when handling commands like START. + // + // Through the driver rather than PyEval_SaveThread() directly: the driver + // has to restore this exact thread state before Py_FinalizeEx() at + // shutdown, and the state was previously discarded here. if (Py_IsInitialized()) { - PyEval_SaveThread(); + plugin_driver_release_gil(); log_info("[PLUGIN]: Released Python GIL"); } } + // Start the command socket only now that the plugin driver is fully built. + // Everything the socket can ask for -- START, STOP, PLUGIN_CMD, STATS -- + // reaches into the driver, so serving commands before this point was serving + // them against a half-configured one. The webserver already tolerates the + // socket appearing a moment later: it connects, retries, and polls. + if (setup_unix_socket() != 0) + { + log_error("Failed to set up UNIX socket"); + return -1; + } + // Start PLC (skip in safe mode to allow program upload without loading the // faulty program that may have caused repeated crashes). // // Use plc_begin_transition() rather than plc_set_state() directly so the // auto-start is arbitrated by plc_claim_transition() like any // socket-originated START command. This prevents a race where the socket - // listener (already running above) accepts a START before the auto-start + // listener (started just above) accepts a START before the auto-start // finishes, causing two concurrent load_plc_program() calls — and two // dispatcher threads. plc_begin_transition() also makes the start // asynchronous, which is fine: the main thread just sleeps below. diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 7668dfca..6608d14f 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -1323,6 +1323,38 @@ extern "C" bool plc_set_state(PLCState new_state) extern "C" void plc_state_manager_cleanup(void) { + /* Let an in-flight transition finish before tearing anything down. + * + * Shutdown is the one state change that does not go through + * plc_claim_transition, so it can land on top of a start that is still + * running. Tearing down from there is not safe at any point of it: during + * plugin bring-up load_plc_program has not assigned plc_thread yet, so the + * join below would run on a handle that was never set; a moment later the + * cycle thread is mid-bring-up and would have RUNNING published underneath + * the teardown. Both disappear if the transition is allowed to land first -- + * then this is an ordinary stop of a RUNNING (or ERROR, or EMPTY) runtime. + * + * Bounded by the same constant the transition worker waits on, so a + * transition that will never land cannot hold the process open forever; the + * teardown then proceeds and does what it can. */ + const int poll_ms = 20; + int waited = 0; + while (plc_state_is_transitioning() && waited < PLC_TRANSITION_LANDING_TIMEOUT_MS) + { + struct timespec poll = { 0, (long)poll_ms * 1000000L }; + nanosleep(&poll, nullptr); + waited += poll_ms; + } + if (waited > 0) + { + log_info("Shutdown waited %d ms for the state change in flight to land", waited); + } + if (plc_state_is_transitioning()) + { + log_warn("Shutdown proceeding with a state change still in flight after %d ms", + waited); + } + if (plc_program) unload_plc_program(plc_program); } diff --git a/webserver/runtimemanager.py b/webserver/runtimemanager.py index 39e2e7a8..7a188c98 100644 --- a/webserver/runtimemanager.py +++ b/webserver/runtimemanager.py @@ -28,6 +28,12 @@ MAX_RAPID_CRASHES = 3 RAPID_CRASH_WINDOW = 30 # seconds +# How long to let the runtime shut down gracefully after SIGTERM before killing +# it. Has to exceed the worst-case graceful stop: the runtime waits for a state +# change already in flight to land (a boot start with plugin bring-up is ~4 s on +# an SLM-RP4) and then tears the program and plugins down. +RUNTIME_SHUTDOWN_TIMEOUT_S = 15 + class RuntimeManager: def __init__(self, runtime_path, plc_socket, log_socket, print_debug=False): @@ -244,17 +250,32 @@ def stop(self): self.monitor_thread.join(timeout=5) time.sleep(1) if self.process: + # SIGTERM now reaches a handler in the runtime, so terminate() starts a + # real shutdown: it stops the PLC program, waits out any state change + # already in flight (a boot start is ~4 s on an SLM-RP4), stops the + # plugins and unloads the program. Wait long enough for that to finish, + # or the SIGKILL below would preempt the very cleanup the signal asked + # for -- which is what happened for every stop while SIGTERM had no + # handler at all. The kill stays as the backstop for a hung teardown. if HAS_PSUTIL and isinstance(self.process, psutil.Process): self.process.terminate() try: - self.process.wait(timeout=5) + self.process.wait(timeout=RUNTIME_SHUTDOWN_TIMEOUT_S) except (psutil.TimeoutExpired, psutil.Error): + logger.warning( + "PLC runtime did not exit within %d s of SIGTERM; killing it", + RUNTIME_SHUTDOWN_TIMEOUT_S, + ) self.process.kill() elif isinstance(self.process, subprocess.Popen): self.process.terminate() try: - self.process.wait(timeout=5) + self.process.wait(timeout=RUNTIME_SHUTDOWN_TIMEOUT_S) except (subprocess.TimeoutExpired, subprocess.SubprocessError): + logger.warning( + "PLC runtime did not exit within %d s of SIGTERM; killing it", + RUNTIME_SHUTDOWN_TIMEOUT_S, + ) self.process.kill() self.process = None self._safe_stop_log_server() From c4d88c5f8d0b7cfc45a0c15f03e36931ba30d100 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 11 Aug 2026 14:26:36 -0400 Subject: [PATCH 9/9] test(lifecycle): end-to-end suite for run/stop state control 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//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) --- tests/lifecycle/README.md | 82 ++++++ tests/lifecycle/failinject.c | 48 ++++ tests/lifecycle/fakevpp_plugin.c | 214 +++++++++++++++ tests/lifecycle/harness.sh | 57 ++++ tests/lifecycle/logserver.py | 39 +++ tests/lifecycle/suite.py | 435 +++++++++++++++++++++++++++++++ 6 files changed, 875 insertions(+) create mode 100644 tests/lifecycle/README.md create mode 100644 tests/lifecycle/failinject.c create mode 100644 tests/lifecycle/fakevpp_plugin.c create mode 100644 tests/lifecycle/harness.sh create mode 100644 tests/lifecycle/logserver.py create mode 100644 tests/lifecycle/suite.py diff --git a/tests/lifecycle/README.md b/tests/lifecycle/README.md new file mode 100644 index 00000000..5bff45e9 --- /dev/null +++ b/tests/lifecycle/README.md @@ -0,0 +1,82 @@ +# Lifecycle tests + +End-to-end tests for run/stop state control: what happens when a signal, a +command, or a mode-switch flip lands in the middle of a state change. + +These are not unit tests. Each one boots a real `plc_main` with a real compiled +PLC program, drives it over its command socket, and judges it by what an outside +observer can see — socket replies, the journal, and `/proc//maps`. That +matters because the bugs in this area are not wrong return values; they are a +state that claims a transition finished while the work is still running. Only the +mapping tells you whether a reported stop actually unloaded anything. + +## Running + +Needs Linux (`SCHED_FIFO`, `sem_init`, `/proc`). On macOS, run it in the project's +container image: + +```bash +docker run --rm --cap-add=sys_nice \ + -v "$PWD":/src:ro \ + -v /path/to/editor/payload:/payload:ro \ + -v "$PWD/tests/lifecycle":/fixtures:ro \ + openplc-runtime-runstop:test bash /fixtures/harness.sh +``` + +`--cap-add=sys_nice` is required for the runtime's real-time scheduling. + +`/payload` is a directory of editor-generated Runtime v4 sources — the contents of +`/build/OpenPLC Runtime v4/src`. The harness copies it to +`core/generated` and compiles it with `scripts/compile.sh`, so any program works. +Payloads from editor builds before `defines.h` existed need one added next to the +sources, since `Makefile.strucpp` lists it as an explicit prerequisite: + +```c +#define PROGRAM_MD5 "" +``` + +Run one group at a time with `SUITE_ONLY=crash|shutdown|phantom|switch|misc`, and +get a backtrace out of a crash with `SUITE_GDB=1` (needs `gdb` in the image). + +## What the pieces are for + +| File | Why it exists | +|---|---| +| `suite.py` | The tests. One group per review finding, each with an oracle that fails on the unfixed code. | +| `fakevpp_plugin.c` | Stands in for a board's VPP package: a configurable sleep in `init()`/`stop_loop()` widens a transition enough to aim at, and a file-driven mode switch (`/tmp/fakevpp_switch`, "RUN"/"STOP") drives `set_switch_position` + `request_plc_start`/`request_plc_stop` the way real hardware does. | +| `failinject.c` | `LD_PRELOAD` shim that 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 `/run/runtime/log_runtime.socket`. Without something listening there, `--print-logs` produces nothing on stdout either and every journal assertion silently passes. | +| `harness.sh` | Builds the runtime, the fixtures and the program, writes `plugins.conf`, then runs the suite. | + +## Two traps worth knowing before editing these + +**Keep the Python plugins in `plugins.conf`.** They stay disabled, but *loading* +them is what initialises the interpreter, and `has_python_plugin && +Py_IsInitialized()` is the precondition for the whole class of shutdown bugs +around `Py_FinalizeEx`. A conf containing only the native fixture makes that class +untestable while every test still passes. + +**Never start a thread in a plugin's `init()`.** The contract says so, and the +reason is concrete: `plugin_driver_update_config` rebuilds every slot on each +start, which `dlclose`s the `.so`, so a thread left running from `init()` returns +into an unmapped page. This fixture segfaulted that way before it was written to +use `start_loop`/`stop_loop`. + +## Coverage, and what is still argued rather than observed + +Covered: graceful shutdown with the PLC never started (both signals); shutdown +during a start; a stop whose transition worker cannot be spawned; a mode-switch +flip during a teardown; `SWITCH` answered mid-transition; and 25 rapid start/stop +pairs as a wedge guard. + +Not covered: + +- **The re-arm branch of switch reconciliation.** The tests prove a flip during a + transition is honoured, but the branch that hands the movement record back only + runs when the corrective transition is *refused* by a request that slipped in + first — a race that cannot be forced without a hook in the runtime. +- **The watchdog forcing ERROR on a stuck transition.** Needs a transition that + outlives `PLC_TRANSITION_STUCK_TIMEOUT_MS` (2 minutes), so it belongs in a slow + opt-in run rather than here. +- **A runaway IEC task.** Terminating one needs the forced-abort ladder that does + not exist yet; today a program with an unbounded loop wedges the stop. diff --git a/tests/lifecycle/failinject.c b/tests/lifecycle/failinject.c new file mode 100644 index 00000000..833e62af --- /dev/null +++ b/tests/lifecycle/failinject.c @@ -0,0 +1,48 @@ +/** + * @file failinject.c + * @brief LD_PRELOAD shim that fails exactly one pthread_create, on demand. + * + * Finding 4 lives on the "the transition worker could not be spawned" path, which + * needs thread or memory exhaustion to reach -- not something to induce for real + * inside a container without collateral damage. + * + * Contract: while the file named by FAILINJECT_ARM (default /tmp/failinject_arm) + * exists, the NEXT pthread_create returns EAGAIN, and the shim then unlinks the + * file itself. One-shot and self-disarming, so the failure lands on the call the + * test aimed at and every later thread -- including the ones the recovery path + * needs -- is created normally. + */ + +#define _GNU_SOURCE +#include +#include +#include +#include +#include + +typedef int (*pthread_create_fn)(pthread_t *, const pthread_attr_t *, + void *(*)(void *), void *); + +static const char *arm_path(void) +{ + const char *p = getenv("FAILINJECT_ARM"); + return (p && *p) ? p : "/tmp/failinject_arm"; +} + +int pthread_create(pthread_t *thread, const pthread_attr_t *attr, + void *(*start_routine)(void *), void *arg) +{ + static pthread_create_fn real = NULL; + if (!real) + real = (pthread_create_fn)dlsym(RTLD_NEXT, "pthread_create"); + + const char *p = arm_path(); + if (access(p, F_OK) == 0) + { + /* Disarm first: if unlink fails we must not fail every subsequent call. */ + if (unlink(p) == 0) + return EAGAIN; + } + + return real(thread, attr, start_routine, arg); +} diff --git a/tests/lifecycle/fakevpp_plugin.c b/tests/lifecycle/fakevpp_plugin.c new file mode 100644 index 00000000..953e6b47 --- /dev/null +++ b/tests/lifecycle/fakevpp_plugin.c @@ -0,0 +1,214 @@ +/** + * @file fakevpp_plugin.c + * @brief Test-only native plugin standing in for a board's VPP package. + * + * Exists to make three otherwise-unreachable conditions reproducible: + * + * 1. A SLOW START. init() sleeps FAKEVPP_INIT_MS (default 2500 ms), which is + * what a real board spends on SPI base scans and fieldbus probes. Without + * it, bring-up in a container lands in ~150 ms and there is no window to + * aim a signal or a competing request at. + * + * 2. A MODE SWITCH. A watcher thread polls FAKEVPP_SWITCH_FILE (default + * /tmp/fakevpp_switch) for the text "RUN" or "STOP" and drives the runtime + * exactly as the plugin contract prescribes: store the position first, then + * request the matching transition. Writing that file is the test's way of + * flipping a physical switch. + * + * 3. A RUNAWAY-FREE STOP. stop_loop()/cleanup() return promptly, so a slow + * teardown never confuses a slow start. + * + * Everything is env-driven so one .so covers every scenario, and nothing here + * touches the process image -- it declares no located variables. + */ + +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif + +#include +#include +#include +#include +#include +#include +#include + +/* Keep in step with plugin_types.h. Only the tail matters here, but the whole + * prefix has to be laid out identically for the offsets to line up, so this + * includes the real header rather than re-declaring the struct. */ +#include "plugin_types.h" + +#define SWITCH_RUN 1 +#define SWITCH_STOP 0 + +static plugin_runtime_args_t *g_args = NULL; +static atomic_int g_running = 0; +static pthread_t g_watcher; +static int g_watcher_up = 0; +static int g_last_pos = SWITCH_RUN; + +static void plog(const char *level, const char *msg) +{ + if (g_args && g_args->log_info && strcmp(level, "info") == 0) + g_args->log_info(msg); + else if (g_args && g_args->log_warn && strcmp(level, "warn") == 0) + g_args->log_warn(msg); + else + fprintf(stderr, "[FAKEVPP] %s: %s\n", level, msg); +} + +static long env_long(const char *name, long fallback) +{ + const char *v = getenv(name); + if (!v || !*v) + return fallback; + return atol(v); +} + +static const char *switch_file(void) +{ + const char *v = getenv("FAKEVPP_SWITCH_FILE"); + return (v && *v) ? v : "/tmp/fakevpp_switch"; +} + +/* Read the switch file. Absent or unparseable means RUN, matching the runtime's + * own default for a device with no switch-aware plugin. */ +static int read_switch_file(void) +{ + FILE *f = fopen(switch_file(), "r"); + if (!f) + return SWITCH_RUN; + char buf[16] = {0}; + if (!fgets(buf, sizeof(buf), f)) + { + fclose(f); + return SWITCH_RUN; + } + fclose(f); + return (strncmp(buf, "STOP", 4) == 0) ? SWITCH_STOP : SWITCH_RUN; +} + +static void *watcher_thread(void *arg) +{ + (void)arg; + const long poll_ms = env_long("FAKEVPP_POLL_MS", 5); + struct timespec t = { .tv_sec = poll_ms / 1000, .tv_nsec = (poll_ms % 1000) * 1000000L }; + + while (atomic_load(&g_running)) + { + const int pos = read_switch_file(); + if (pos != g_last_pos) + { + g_last_pos = pos; + /* Contract order: store the position BEFORE requesting, so the start + * gate never reads a stale STOP on a rising edge and no start can slip + * in on a falling one. */ + if (g_args && g_args->set_switch_position) + g_args->set_switch_position(pos); + + if (pos == SWITCH_RUN) + { + plog("info", "fake mode switch moved to RUN"); + if (g_args && g_args->request_plc_start) + g_args->request_plc_start("fake mode switch moved to RUN"); + } + else + { + plog("info", "fake mode switch moved to STOP"); + if (g_args && g_args->request_plc_stop) + g_args->request_plc_stop("fake mode switch moved to STOP"); + } + } + nanosleep(&t, NULL); + } + return NULL; +} + +int init(void *args) +{ + g_args = (plugin_runtime_args_t *)args; + + /* Adopt whatever the switch file already says before anything else, so a test + * can boot a device with the switch in STOP. */ + g_last_pos = read_switch_file(); + if (g_args && g_args->set_switch_position) + g_args->set_switch_position(g_last_pos); + + const long slow_ms = env_long("FAKEVPP_INIT_MS", 2500); + if (slow_ms > 0) + { + char msg[128]; + snprintf(msg, sizeof(msg), "fake VPP bring-up: sleeping %ld ms in init()", slow_ms); + plog("info", msg); + struct timespec t = { .tv_sec = slow_ms / 1000, .tv_nsec = (slow_ms % 1000) * 1000000L }; + nanosleep(&t, NULL); + } + + plog("info", "fake VPP initialised"); + return 0; +} + +/* The watcher lives between start_loop and stop_loop, and NOT a moment longer. + * + * init() is the tempting place for it -- a real mode switch has to be watched + * while the PLC is stopped too -- but the contract forbids threads there for a + * concrete reason: plugin_driver_update_config tears every slot down and + * re-dlopens it on each start, so a thread left running from init() ends up + * executing code that has been unmapped. That is a SIGSEGV, and this fixture + * earned one before being written this way. Everything the tests need still + * works, because the runtime stops plugins only AFTER joining the PLC thread: + * a flip during a stop is still seen. */ +int start_loop(void *args) +{ + (void)args; + if (atomic_exchange(&g_running, 1) == 1) + return 0; + if (pthread_create(&g_watcher, NULL, watcher_thread, NULL) != 0) + { + atomic_store(&g_running, 0); + plog("warn", "failed to start the switch watcher"); + return -1; + } + g_watcher_up = 1; + plog("info", "fake VPP switch watcher running"); + return 0; +} + +int stop_loop(void *args) +{ + (void)args; + + /* Optional slow teardown, BEFORE the watcher is joined, so the switch is + * still being watched while the stop is in flight. That is the only way to + * land a flip inside a stop transition on purpose: a stop is otherwise tens + * of milliseconds and there is nothing to aim at. */ + const long slow_ms = env_long("FAKEVPP_STOP_MS", 0); + if (slow_ms > 0) + { + char msg[128]; + snprintf(msg, sizeof(msg), "fake VPP teardown: sleeping %ld ms in stop_loop()", slow_ms); + plog("info", msg); + struct timespec t = { .tv_sec = slow_ms / 1000, .tv_nsec = (slow_ms % 1000) * 1000000L }; + nanosleep(&t, NULL); + } + + if (atomic_exchange(&g_running, 0) == 1 && g_watcher_up) + { + pthread_join(g_watcher, NULL); + g_watcher_up = 0; + } + return 0; +} + +int cleanup(void *args) +{ + (void)args; + if (atomic_exchange(&g_running, 0) == 1 && g_watcher_up) + { + pthread_join(g_watcher, NULL); + g_watcher_up = 0; + } + g_args = NULL; + return 0; +} diff --git a/tests/lifecycle/harness.sh b/tests/lifecycle/harness.sh new file mode 100644 index 00000000..22ca3e19 --- /dev/null +++ b/tests/lifecycle/harness.sh @@ -0,0 +1,57 @@ +#!/bin/bash +# Builds the runtime, compiles a real PLC program, builds the test fixtures, and +# runs the finding-by-finding suite. Single runtime instance at a time: the +# command socket is single-client, so no webserver competes for it. +set -uo pipefail + +echo "### build runtime" +mkdir -p /work +(cd /src && tar cf - --exclude=build --exclude=venvs --exclude=.git --exclude=core/generated .) | (cd /work && tar xf -) +cp -r /workdir/venvs /work/venvs +cd /work && mkdir -p build && cd build && cmake .. >/tmp/cmake.log 2>&1 && make -j"$(nproc)" >/tmp/make.log 2>&1 \ + || { echo "BUILD FAIL"; tail -25 /tmp/make.log; exit 1; } +cd /work +echo " warnings: $(grep -ci warning /tmp/make.log)" + +echo "### build fixtures" +mkdir -p build/plugins +gcc -shared -fPIC -O1 -Wall -o build/plugins/libfakevpp_plugin.so \ + /fixtures/fakevpp_plugin.c -I core/src/drivers -I "$(python3 -c 'import sysconfig;print(sysconfig.get_paths()["include"])')" \ + -lpthread 2>&1 | head -5 || { echo "FAKEVPP BUILD FAIL"; exit 1; } +gcc -shared -fPIC -O1 -Wall -o /tmp/failinject.so /fixtures/failinject.c -ldl 2>&1 | head -5 +echo '{"name":"fakevpp","protocol":"NONE","config":{}}' > /tmp/fakevpp_config.json +ls -la build/plugins/libfakevpp_plugin.so /tmp/failinject.so | sed 's/^/ /' + +echo "### compile a real PLC program (through the webserver's own compile path)" +mkdir -p /run/runtime core/generated && cp -r /payload/. core/generated/ +bash scripts/compile.sh >/tmp/compile.log 2>&1 +ls build/libplc_*.so >/dev/null 2>&1 || { + # compile.sh leaves the .so as new_libplc.so when invoked outside the webserver + if [ -f build/new_libplc.so ]; then + mv build/new_libplc.so "build/libplc_$(date +%s%N).so" + else + echo "COMPILE FAIL"; tail -12 /tmp/compile.log; exit 1 + fi +} +echo " program: $(ls build/libplc_*.so)" + +echo "### plugins.conf: the shipped set plus the fake VPP" +# The stock Python entries stay, disabled, because loading them is what +# initialises the interpreter -- and has_python_plugin && Py_IsInitialized() is +# the precondition for the Py_FinalizeEx() shutdown crash. A conf with only the +# native fixture in it quietly makes that whole class untestable. +# Native plugin lines whose .so is not built are dropped: they only add warnings. +grep -v 'libs7comm_plugin\|libethercat_plugin' plugins_default.conf > plugins.conf +# name,path,enabled,type,config_json,venv (type 1 = native) +printf 'fakevpp,./build/plugins/libfakevpp_plugin.so,1,1,/tmp/fakevpp_config.json,\n' >> plugins.conf +sed 's/^/ /' plugins.conf + +echo "### start the log-socket server (so --print-logs reaches stdout)" +python3 /fixtures/logserver.py & +LOGSRV=$! +sleep 0.4 + +/work/venvs/runtime/bin/python /fixtures/suite.py +RC=$? +kill $LOGSRV 2>/dev/null +exit $RC diff --git a/tests/lifecycle/logserver.py b/tests/lifecycle/logserver.py new file mode 100644 index 00000000..ad7c110f --- /dev/null +++ b/tests/lifecycle/logserver.py @@ -0,0 +1,39 @@ +"""Minimal stand-in for the webserver's log server. + +plc_main's log_init() connects to /run/runtime/log_runtime.socket; with nothing +listening, --print-logs produces nothing on stdout either, which is how a whole +round of testing ended up with empty journals. +""" + +import os +import socket +import threading + +PATH = "/run/runtime/log_runtime.socket" + +try: + os.unlink(PATH) +except OSError: + pass + +srv = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +srv.bind(PATH) +srv.listen(16) + + +def drain(conn): + with conn: + while True: + try: + if not conn.recv(8192): + return + except OSError: + return + + +while True: + try: + c, _ = srv.accept() + except OSError: + break + threading.Thread(target=drain, args=(c,), daemon=True).start() diff --git a/tests/lifecycle/suite.py b/tests/lifecycle/suite.py new file mode 100644 index 00000000..aa17f0f6 --- /dev/null +++ b/tests/lifecycle/suite.py @@ -0,0 +1,435 @@ +"""PR #162 review findings: one test per finding, run against a real runtime. + +Each test drives plc_main directly over its command socket, with the fake VPP +plugin providing a slow bring-up (so a transition is wide enough to aim at) and a +file-driven mode switch. Oracles are chosen to be observable from outside the +process -- socket replies, the journal, and /proc//maps -- so the same suite +scores the pristine branch and the fixed one. +""" + +import os +import signal +import socket +import subprocess +import sys +import time + +SOCK = "/run/runtime/plc_runtime.socket" +SWITCH = "/tmp/fakevpp_switch" +ARM = "/tmp/failinject_arm" +results = [] + + +# ---------------------------------------------------------------- plumbing +def cmd(c, timeout=20): + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.settimeout(timeout) + s.connect(SOCK) + s.sendall((c + "\n").encode()) + r = s.recv(4096).decode().strip() + s.close() + return r + + +def status(): + try: + return cmd("STATUS") + except OSError: + return "unreachable" + + +def wait_final(limit=120.0): + t0 = time.time() + while time.time() - t0 < limit: + st = status() + if "TRANSITIONING" not in st and st != "unreachable": + return st, time.time() - t0 + time.sleep(0.02) + return status(), time.time() - t0 + + +def wait_transitioning(limit=20.0): + t0 = time.time() + while time.time() - t0 < limit: + if "TRANSITIONING" in status(): + return True + time.sleep(0.005) + return False + + +def ensure_running(limit=25.0): + """Get the PLC to RUNNING, tolerating the boot-auto-start race. + + plc_main creates the command socket before it claims the boot start, so a + STATUS poll right after the socket appears legitimately answers STOPPED -- + which is not the same as "the start is not coming". + """ + t0 = time.time() + while time.time() - t0 < limit: + st = status() + if st == "STATUS:RUNNING": + return st + if "TRANSITIONING" in st: + wait_final() + continue + if st in ("STATUS:STOPPED", "STATUS:EMPTY", "STATUS:ERROR"): + # Either the boot start has not been claimed yet or it will not come; + # asking explicitly settles it either way. + r = cmd("START") + if "OK" in r: + wait_final() + else: + time.sleep(0.05) + continue + time.sleep(0.02) + return status() + + +def set_switch(pos): + with open(SWITCH, "w") as f: + f.write(pos) + + +class Runtime: + """One plc_main process, its log captured.""" + + def __init__(self, tag, init_ms=2500, preload=None, env=None): + self.tag = tag + for p in (SOCK,): + try: + os.unlink(p) + except OSError: + pass + self.logpath = f"/tmp/rt_{tag}.log" + self.log = open(self.logpath, "w") + e = dict(os.environ, FAKEVPP_INIT_MS=str(init_ms), FAKEVPP_SWITCH_FILE=SWITCH) + if preload: + e["LD_PRELOAD"] = preload + if env: + e.update(env) + argv = ["stdbuf", "-oL", "-eL", "./build/plc_main", "--print-logs"] + if os.environ.get("SUITE_GDB"): + argv = ["gdb", "-q", "-batch", + "-ex", "handle SIGINT nostop noprint pass", + "-ex", "handle SIGTERM nostop noprint pass", + "-ex", "handle SIGUSR1 nostop noprint pass", + "-ex", "run", "-ex", "bt 12", "-ex", "thread apply all bt 4", + "--args", "./build/plc_main", "--print-logs"] + self.p = subprocess.Popen( + argv, + stdout=self.log, + stderr=subprocess.STDOUT, + env=e, + ) + self.ready = False + for _ in range(600): + if self.p.poll() is not None: + break + try: + if cmd("PING", timeout=2) == "PING:OK": + self.ready = True + break + except OSError: + pass + time.sleep(0.01) + if not self.ready: + print(f" runtime {tag} never answered PING") + self.dump() + + def journal(self): + self.log.flush() + try: + return open(self.logpath).read() + except OSError: + return "" + + def dump(self, n=25): + print(f" exit code: {self.p.poll()} journal (socket noise removed):") + lines = [l for l in self.journal().strip().splitlines() + if "Unix socket client" not in l] + for line in lines[-n:]: + print(" ", line) + + def maps(self): + try: + return open(f"/proc/{self.p.pid}/maps").read() + except OSError: + return "" + + def program_mapped(self): + return "libplc_" in self.maps() + + def signal_and_wait(self, sig, limit=10.0): + self.p.send_signal(sig) + t0 = time.time() + while time.time() - t0 < limit: + rc = self.p.poll() + if rc is not None: + return rc, time.time() - t0 + time.sleep(0.02) + return None, time.time() - t0 + + def kill(self): + if self.p.poll() is None: + self.p.kill() + self.p.wait() + self.log.close() + + +def record(finding, name, ok, detail=""): + results.append((finding, name, ok)) + tail = f" -- {detail}" if detail else "" + print(f" [{'PASS' if ok else 'FAIL'}] ({finding}) {name}{tail}") + + +def banner(t): + print(f"\n=== {t}") + + +# ------------------------------------------------- findings 2 + 3: shutdown +def test_shutdown_midstart(sig, signame): + """SIGINT/SIGTERM while TRANSITIONING_TO_RUN must complete, not hang. + + Pre-fix: unload_plc_program's guard only matched RUNNING so it published + nothing, plc_cycle_thread then published RUNNING unconditionally, and the + join in the teardown never returned. + """ + banner(f"findings 2+3: {signame} during a start") + set_switch("RUN") + rt = Runtime(f"mid_{signame}", init_ms=2500) + caught = wait_transitioning() + st = status() + record("2/3", f"{signame}: caught the start in flight", caught, f"state={st}") + rc, took = rt.signal_and_wait(sig, limit=12.0) + j = rt.journal() + if rc is None: + record("2/3", f"{signame}: process exits", False, "HUNG, needed SIGKILL") + elif rc < 0: + record("2/3", f"{signame}: process exits", False, f"died on signal {-rc}") + else: + record("2/3", f"{signame}: process exits", True, f"rc={rc} in {took:.2f}s") + # Either the start was allowed to land and then torn down properly, or the + # cycle thread declined to publish RUNNING over the teardown. What must never + # happen is RUNNING published and no unload. + resurrected = ("PLC State: RUNNING" in j) and ("PLC program unloaded successfully" not in j) + record( + "3", + f"{signame}: RUNNING never left standing over a teardown", + not resurrected, + "declined to publish" if "not releasing the first scan" in j + else ("landed then unloaded" if "unloaded successfully" in j else "no RUNNING published"), + ) + record( + "2", + f"{signame}: shutdown waited for the transition", + "Shutdown waited" in j or "not releasing the first scan" in j, + "waited for the landing" if "Shutdown waited" in j else "cycle thread declined", + ) + rt.kill() + + +# ------------------------------------------------ finding 4: phantom stop +def test_phantom_stop(): + """A STOP whose worker cannot be spawned must not report a stop that never ran. + + The claim has already published TRANSITIONING_TO_STOP, which ends the scan; + pre-fix the failure path then published STOPPED while unload_plc_program never + ran, leaving the .so mapped and the program loaded. /proc//maps is the + oracle: STATUS says STOPPED either way, only the mapping tells the truth. + """ + banner("finding 4: STOP with the transition worker unspawnable") + set_switch("RUN") + rt = Runtime("phantom", init_ms=200, preload="/tmp/failinject.so") + st = ensure_running() + record("4", "reached RUNNING before the test", st == "STATUS:RUNNING", st) + if st != "STATUS:RUNNING": + rt.dump() + record("4", "program mapped while RUNNING", False, "skipped, runtime not running") + rt.kill() + return + record("4", "program mapped while RUNNING", rt.program_mapped()) + + open(ARM, "w").close() # next pthread_create fails, one-shot + reply = cmd("STOP") + st, secs = wait_final() + mapped = rt.program_mapped() + j = rt.journal() + record("4", "STOP still lands a final state", "TRANSITIONING" not in st, f"{reply} -> {st}") + record( + "4", + "the stop was real: program unmapped", + not mapped, + "still mapped -- STATUS lied about the stop" if mapped else f"unmapped, {st}", + ) + fired = ("completing the transition on the calling thread" in j + or "Failed to create transition thread" in j) + record("4", "the spawn failure was actually injected", fired, + "injection fired" if fired else "injection MISSED -- test inconclusive") + record( + "4", + "teardown actually ran", + "PLC program unloaded successfully" in j, + "journal shows the unload" if "unloaded successfully" in j else "no unload in journal", + ) + try: + os.unlink(ARM) + except OSError: + pass + rt.kill() + + +# ------------------------------- finding 5: switch intent must not be lost +def test_switch_intent(): + """A switch flip during a transition must not be lost. + + The sequence that matters, and the only one that records movement at all + (plc_set_switch_position only notes EDGES): + + 1. PLC RUNNING, switch RUN. + 2. Flip to STOP. The plugin stores STOP and requests a stop, which is + accepted -- TRANSITIONING_TO_STOP. + 3. Flip back to RUN while that stop is still tearing down. The plugin + stores RUN (an edge, so movement is recorded) and requests a start, + which is DROPPED because a transition is in flight. + 4. The stop lands on STOPPED. Only the movement record can now honour the + switch, and pre-fix that record was consumed before the corrective + transition was known to have started, with the return value discarded. + + Step 3 needs a stop wide enough to aim at, hence FAKEVPP_STOP_MS: the fake + plugin sleeps in stop_loop() while its switch watcher is still alive. + """ + banner("finding 5: switch flipped back during a stop") + set_switch("RUN") + rt = Runtime("switch", init_ms=300, env={"FAKEVPP_STOP_MS": "1500"}) + st = ensure_running() + record("5", "reached RUNNING before the test", st == "STATUS:RUNNING", st) + if st != "STATUS:RUNNING": + rt.dump() + rt.kill() + return + + disagreements = [] + corrections = 0 + for i in range(4): + set_switch("STOP") # step 2 + if not wait_transitioning(limit=10): + record("5", f"round {i}: stop transition started", False, "no transition seen") + break + time.sleep(0.2) # inside the slow teardown + set_switch("RUN") # step 3: the dropped request + st, _ = wait_final(limit=30) + # Reconciliation runs after the landing and starts its own transition. + time.sleep(0.3) + st, _ = wait_final(limit=30) + sw = cmd("SWITCH") + agree = (sw == "SWITCH:RUN" and st == "STATUS:RUNNING") or ( + sw == "SWITCH:STOP" and st == "STATUS:STOPPED" + ) + print(f" round {i}: switch={sw} state={st} {'agree' if agree else 'DISAGREE'}") + if not agree: + disagreements.append((i, sw, st)) + corrections = rt.journal().count("correcting") + if st != "STATUS:RUNNING": + ensure_running() + + j = rt.journal() + record( + "5", + "switch and PLC agree after every flip", + not disagreements, + f"{len(disagreements)} disagreement(s): {disagreements[:3]}" if disagreements + else "4/4 rounds reconciled", + ) + record( + "5", + "reconciliation is what did it", + corrections > 0, + f"{corrections} correction(s) logged", + ) + rt.kill() + + +# ---------------------------------- shutdown with the PLC never having run +def test_shutdown_never_ran(): + """The graceful-shutdown segfault: Py_FinalizeEx() with no GIL held. + + main_tstate was only ever set by plugin_driver_start(), so a runtime whose PLC + never ran had nothing to restore and finalised the interpreter without the + GIL. Reached by booting with the switch in STOP, which is also the safe-mode + and no-program shape. + """ + banner("graceful shutdown when the PLC never ran") + set_switch("STOP") + for sig, name in ((signal.SIGINT, "SIGINT"), (signal.SIGTERM, "SIGTERM")): + rt = Runtime(f"never_{name}", init_ms=100) + st, _ = wait_final(limit=30) + never_ran = "RUNNING" not in st + record("crash", f"{name}: PLC never ran", never_ran, st) + rc, took = rt.signal_and_wait(sig, limit=10.0) + if rc is None: + record("crash", f"{name}: clean shutdown", False, "HUNG") + elif rc < 0: + record("crash", f"{name}: clean shutdown", False, f"died on signal {-rc}") + else: + record("crash", f"{name}: clean shutdown", True, f"rc={rc} in {took:.2f}s") + rt.kill() + set_switch("RUN") + + +# ----------------------------------------- finding 6 + the interlock guard +def test_switch_readable_and_interlock(): + banner("finding 6 + interlock regression guard") + set_switch("RUN") + rt = Runtime("misc", init_ms=1500) + caught = wait_transitioning() + busy = answered = 0 + while "TRANSITIONING" in status(): + r = cmd("SWITCH") + if r.startswith("SWITCH:"): + answered += 1 + elif "BUSY" in r: + busy += 1 + time.sleep(0.01) + record("6", "SWITCH answered mid-transition", caught and answered > 0 and busy == 0, + f"{answered} answered, {busy} BUSY") + wait_final() + + tally = {} + for _ in range(25): + for c in ("START", "STOP"): + r = cmd(c) + tally[r] = tally.get(r, 0) + 1 + time.sleep(0.15) + st, secs = wait_final() + record("2/3", "no wedge after 25 rapid start/stop pairs", "TRANSITIONING" not in st, + f"settled on {st} in {secs:.2f}s") + record("2/3", "still answering after the stress", cmd("PING") == "PING:OK") + rt.kill() + + +ONLY = os.environ.get("SUITE_ONLY", "") + + +def run(name, fn, *a): + if not ONLY or ONLY == name: + fn(*a) + + +run("crash", test_shutdown_never_ran) +run("shutdown", test_shutdown_midstart, signal.SIGINT, "SIGINT") +run("shutdown", test_shutdown_midstart, signal.SIGTERM, "SIGTERM") +run("phantom", test_phantom_stop) +run("switch", test_switch_intent) +run("misc", test_switch_readable_and_interlock) + +print("\n=== summary") +by_finding = {} +for finding, name, ok in results: + d = by_finding.setdefault(finding, [0, 0]) + d[0 if ok else 1] += 1 +for finding in sorted(by_finding): + p, f = by_finding[finding] + print(f" finding {finding:<6} {p} passed, {f} failed") +failed = sum(1 for _, _, ok in results if not ok) +print(f" TOTAL: {len(results) - failed} passed, {failed} failed") +sys.exit(1 if failed else 0)