From a0289b202e7ec96b707e8ee5d3e3f92028323bd4 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 28 Jul 2026 21:28:31 -0400 Subject: [PATCH 01/14] 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 6c2fabd7bc23667840a7f7f98a579d49d3947459 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Sun, 2 Aug 2026 19:46:32 -0300 Subject: [PATCH 02/14] fix(modbus): zero IEC buffers on read failure when the I/O group asks for it --- .../modbus_master/modbus_master_plugin.py | 63 ++++++--- .../modbus_master/modbus_master_utils.py | 35 +++++ .../modbus_master_config_model.py | 45 ++++++- .../test_modbus_master_config_model.py | 40 ++++++ .../test_error_handling_zero_fill.py | 126 ++++++++++++++++++ 5 files changed, 286 insertions(+), 23 deletions(-) create mode 100644 tests/pytest/modbus_master/test_error_handling_zero_fill.py diff --git a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py index c1b1902b..e9d636be 100644 --- a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py +++ b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py @@ -21,6 +21,7 @@ # Import the configuration model from shared.plugin_config_decode.modbus_master_config_model import ( + ERROR_HANDLING_SET_TO_ZERO, ModbusMasterConfig, ) @@ -38,7 +39,8 @@ ) from .modbus_master_utils import ( calculate_gcd_of_cycle_times, - get_modbus_registers_count_for_iec_size, + get_read_count_for_io_point, + get_zero_payload_for_io_point, parse_modbus_offset, ) except ImportError: @@ -52,7 +54,8 @@ ) from modbus_master_utils import ( calculate_gcd_of_cycle_times, - get_modbus_registers_count_for_iec_size, + get_read_count_for_io_point, + get_zero_payload_for_io_point, parse_modbus_offset, ) @@ -66,6 +69,29 @@ # pylint: enable=invalid-name +def queue_zero_fill_on_failure(point: Any, read_results_to_update: List[Any]) -> bool: + """ + Queue a zeroed payload for a read point whose group asks for it. + + A failed read used to just be skipped, which left the point's last good + value in the IEC buffer for as long as the device stayed down — the + editor's "Set to zero" option had no effect anywhere (openplc-editor#691). + Queueing a zero-fill here routes it through the same conversion and + batched mutex write the successful path uses, so the program sees zeros + for that address instead of stale data. + + Points configured to keep the last value are left untouched. + + Returns True when a zero-fill was queued, so callers can log it. + """ + if getattr(point, "error_handling", None) != ERROR_HANDLING_SET_TO_ZERO: + return False + read_results_to_update.append( + (point.iec_location, get_zero_payload_for_io_point(point), point.length) + ) + return True + + class ModbusSlaveDevice(threading.Thread): """ Handles a single Modbus TCP device with its own connection. @@ -148,15 +174,10 @@ def run(self): # pylint: disable=too-many-locals # Parse Modbus offset address = parse_modbus_offset(point.offset) - # Calculate the correct number of Modbus registers/coils needed - if point.fc in [3, 4]: # Register-based operations (FC 3,4) - iec_size = point.iec_location.size - registers_per_iec_element = get_modbus_registers_count_for_iec_size( - iec_size - ) - count = point.length * registers_per_iec_element - else: # Coil/Discrete Input operations (FC 1,2) - count = point.length # 1:1 mapping for boolean operations + # Registers/coils this read asks for. Same helper sizes + # the zero-fill used when the read fails, so request and + # fallback can never disagree. + count = get_read_count_for_io_point(point) # Perform Modbus read based on function code # Note: pymodbus 3.10+ uses device_id parameter (formerly slave) @@ -187,6 +208,7 @@ def run(self): # pylint: disable=too-many-locals f"[{self.name}] Modbus read error " f"(FC {point.fc}, addr {address}): {response}" ) + queue_zero_fill_on_failure(point, read_results_to_update) # Mark as disconnected to force reconnection on next cycle self.connection_manager.mark_disconnected() continue @@ -195,6 +217,7 @@ def run(self): # pylint: disable=too-many-locals f"[{self.name}] Modbus read failed " f"(FC {point.fc}, addr {address}): {response}" ) + queue_zero_fill_on_failure(point, read_results_to_update) # Mark as disconnected to force reconnection on next cycle self.connection_manager.mark_disconnected() continue @@ -220,6 +243,7 @@ def run(self): # pylint: disable=too-many-locals f"[{self.name}] Connection error reading " f"FC {point.fc}, offset {point.offset}: {ce}" ) + queue_zero_fill_on_failure(point, read_results_to_update) # Mark as disconnected to force reconnection self.connection_manager.mark_disconnected() except Exception as e: @@ -227,6 +251,7 @@ def run(self): # pylint: disable=too-many-locals f"[{self.name}] Error reading " f"FC {point.fc}, offset {point.offset}: {e}" ) + queue_zero_fill_on_failure(point, read_results_to_update) # For other errors also mark disconnected as precaution self.connection_manager.mark_disconnected() @@ -567,15 +592,9 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta # Parse Modbus offset address = parse_modbus_offset(point.offset) - # Calculate the correct number of Modbus registers/coils needed - if point.fc in [3, 4]: # Register-based operations - iec_size = point.iec_location.size - registers_per_iec_element = get_modbus_registers_count_for_iec_size( - iec_size - ) - count = point.length * registers_per_iec_element - else: # Coil/Discrete Input operations - count = point.length + # Registers/coils this read asks for — same helper that + # sizes the zero-fill on failure. + count = get_read_count_for_io_point(point) # Perform Modbus read with device-specific slave_id if point.fc == 1: # Read Coils @@ -604,6 +623,7 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta f"[{self.name}] Modbus read error " f"(slave {slave_id}, FC {point.fc}, addr {address}): {response}" ) + queue_zero_fill_on_failure(point, read_results_to_update) self.connection_manager.mark_disconnected() continue if response.isError(): @@ -611,6 +631,7 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta f"[{self.name}] Modbus read failed " f"(slave {slave_id}, FC {point.fc}, addr {address}): {response}" ) + queue_zero_fill_on_failure(point, read_results_to_update) self.connection_manager.mark_disconnected() continue @@ -635,12 +656,14 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta f"[{self.name}] Connection error reading " f"slave {slave_id}, FC {point.fc}, offset {point.offset}: {ce}" ) + queue_zero_fill_on_failure(point, read_results_to_update) self.connection_manager.mark_disconnected() except Exception as e: self.logger.error( f"[{self.name}] Error reading " f"slave {slave_id}, FC {point.fc}, offset {point.offset}: {e}" ) + queue_zero_fill_on_failure(point, read_results_to_update) self.connection_manager.mark_disconnected() # Batch update IEC buffers with single mutex acquisition diff --git a/core/src/drivers/plugins/python/modbus_master/modbus_master_utils.py b/core/src/drivers/plugins/python/modbus_master/modbus_master_utils.py index 967d41a5..47c8a3ff 100644 --- a/core/src/drivers/plugins/python/modbus_master/modbus_master_utils.py +++ b/core/src/drivers/plugins/python/modbus_master/modbus_master_utils.py @@ -202,3 +202,38 @@ def parse_modbus_offset(offset_str: str) -> int: raise ValueError(f"Offset must be non-negative, got: {address}") return address + + +def get_read_count_for_io_point(point: Any) -> int: + """ + Returns how many Modbus registers/coils a read of `point` asks for. + + Registers (FC 3/4) scale with the IEC element width — a %MD point needs + two registers per element — while coils and discrete inputs (FC 1/2) map + 1:1. Callers use this both to size the Modbus request and to size the + zero-fill that replaces a failed read (see `get_zero_payload_for_io_point`), + which is why it derives everything from the point itself. + """ + if point.fc in (3, 4): + registers_per_element = get_modbus_registers_count_for_iec_size(point.iec_location.size) + return point.length * registers_per_element + return point.length + + +def get_zero_payload_for_io_point(point: Any) -> List[Any]: + """ + Returns a zeroed payload shaped like the response a successful read of + `point` would have produced. + + Used to honour the I/O group's "set to zero" error handling: instead of a + separate buffer-writing path, the failed read feeds this through the same + conversion + batched mutex write the happy path uses, so a zeroed value + lands exactly where a real one would. + + Booleans come back as `False` (FC 1/2 responses carry bits), registers as + integer zeros. + """ + count = get_read_count_for_io_point(point) + if point.fc in (1, 2): + return [False] * count + return [0] * count diff --git a/core/src/drivers/plugins/python/shared/plugin_config_decode/modbus_master_config_model.py b/core/src/drivers/plugins/python/shared/plugin_config_decode/modbus_master_config_model.py index a98d3ea2..4099abdf 100644 --- a/core/src/drivers/plugins/python/shared/plugin_config_decode/modbus_master_config_model.py +++ b/core/src/drivers/plugins/python/shared/plugin_config_decode/modbus_master_config_model.py @@ -269,16 +269,36 @@ def __repr__(self) -> str: return f"{self.__class__.__name__}(devices={len(self.devices)})" +# What a read point does with its IEC buffer while communication is down. +# The editor exposes this per I/O group ("Keep last value" / "Set to zero"); +# these are the strings it writes into the plugin config. +ERROR_HANDLING_KEEP_LAST = "keep-last-value" +ERROR_HANDLING_SET_TO_ZERO = "set-to-zero" +ERROR_HANDLING_MODES = (ERROR_HANDLING_KEEP_LAST, ERROR_HANDLING_SET_TO_ZERO) + + class ModbusIoPointConfig: """ Model for a single Modbus I/O point configuration. """ - def __init__(self, fc: int, offset: str, iec_location: str, length: int, cycle_time_ms: int = 1000): + def __init__( + self, + fc: int, + offset: str, + iec_location: str, + length: int, + cycle_time_ms: int = 1000, + error_handling: str = ERROR_HANDLING_KEEP_LAST, + ): self.fc = fc # Function code self.offset = offset # Modbus register offset self.iec_location: IECAddress = parse_iec_address(iec_location) # IEC location (as IECAddress) self.length = length # Length of the data self.cycle_time_ms = cycle_time_ms # Polling cycle time in milliseconds + # Behaviour on communication failure. Defaults to keeping the last + # value, which is what every config written before this field existed + # implies — and what the runtime did unconditionally. + self.error_handling = error_handling @classmethod def from_dict(cls, data: Dict[str, Any]) -> 'ModbusIoPointConfig': @@ -294,7 +314,25 @@ def from_dict(cls, data: Dict[str, Any]) -> 'ModbusIoPointConfig': except KeyError as e: raise ValueError(f"Missing required field in Modbus I/O point config: {e}") - return cls(fc=fc, offset=offset, iec_location=iec_location, length=length, cycle_time_ms=cycle_time_ms) + # An unrecognised mode falls back to the historical behaviour instead + # of failing the load: a config from a newer editor must not take the + # whole device offline over one unknown string. + error_handling = data.get("error_handling", ERROR_HANDLING_KEEP_LAST) + if error_handling not in ERROR_HANDLING_MODES: + print( + f"(WARN) Unknown error_handling '{error_handling}' for I/O point " + f"{iec_location}; falling back to '{ERROR_HANDLING_KEEP_LAST}'" + ) + error_handling = ERROR_HANDLING_KEEP_LAST + + return cls( + fc=fc, + offset=offset, + iec_location=iec_location, + length=length, + cycle_time_ms=cycle_time_ms, + error_handling=error_handling, + ) def to_dict(self) -> Dict[str, Any]: """ @@ -310,7 +348,8 @@ def to_dict(self) -> Dict[str, Any]: "offset": self.offset, "iec_location": iec_str, "len": self.length, - "cycle_time_ms": self.cycle_time_ms + "cycle_time_ms": self.cycle_time_ms, + "error_handling": self.error_handling } def __repr__(self) -> str: diff --git a/core/src/drivers/plugins/python/shared/plugin_config_decode/test_modbus_master_config_model.py b/core/src/drivers/plugins/python/shared/plugin_config_decode/test_modbus_master_config_model.py index 1789ca49..bb72df55 100644 --- a/core/src/drivers/plugins/python/shared/plugin_config_decode/test_modbus_master_config_model.py +++ b/core/src/drivers/plugins/python/shared/plugin_config_decode/test_modbus_master_config_model.py @@ -1,5 +1,7 @@ import pytest from .modbus_master_config_model import ( + ERROR_HANDLING_KEEP_LAST, + ERROR_HANDLING_SET_TO_ZERO, parse_iec_address, ModbusIoPointConfig, ModbusDeviceConfig, @@ -51,6 +53,44 @@ def test_modbus_io_point_from_dict(): assert d["fc"] == 3 assert d["iec_location"].startswith("%I") +def test_modbus_io_point_error_handling_defaults_to_keep_last(): + # Configs written before the field existed must keep behaving as they did: + # the runtime left the last value in the buffer when a read failed. + point = ModbusIoPointConfig.from_dict( + {"fc": 3, "offset": "40001", "iec_location": "%IW0", "len": 2} + ) + assert point.error_handling == ERROR_HANDLING_KEEP_LAST + + +def test_modbus_io_point_parses_set_to_zero(): + point = ModbusIoPointConfig.from_dict( + { + "fc": 3, + "offset": "40001", + "iec_location": "%IW0", + "len": 2, + "error_handling": "set-to-zero", + } + ) + assert point.error_handling == ERROR_HANDLING_SET_TO_ZERO + assert point.to_dict()["error_handling"] == "set-to-zero" + + +def test_modbus_io_point_unknown_error_handling_falls_back(): + # A config from a newer editor must not take the whole device offline over + # one unrecognised string. + point = ModbusIoPointConfig.from_dict( + { + "fc": 3, + "offset": "40001", + "iec_location": "%IW0", + "len": 2, + "error_handling": "explode", + } + ) + assert point.error_handling == ERROR_HANDLING_KEEP_LAST + + def test_modbus_io_point_missing_field(): data = { "offset": "40001", diff --git a/tests/pytest/modbus_master/test_error_handling_zero_fill.py b/tests/pytest/modbus_master/test_error_handling_zero_fill.py new file mode 100644 index 00000000..c7821948 --- /dev/null +++ b/tests/pytest/modbus_master/test_error_handling_zero_fill.py @@ -0,0 +1,126 @@ +""" +Tests for the Modbus master's "set to zero" error handling. + +Regression coverage for openplc-editor#691: a read that fails used to be +skipped outright, so the point's last good value stayed in the IEC buffer for +as long as the device was unreachable — the editor's per-group "Set to zero" +option had no effect anywhere in the stack. + +The behaviour lives in two pure helpers plus one queueing function, so these +tests need no threads, no sockets and no PLC: they assert the payload that +gets handed to the (already covered) buffer-write path. +""" + +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +_plugin_dir = Path(__file__).parent.parent.parent.parent / "core" / "src" / "drivers" / "plugins" / "python" +sys.path.insert(0, str(_plugin_dir / "modbus_master")) +sys.path.insert(0, str(_plugin_dir)) + +from modbus_master_utils import ( + get_read_count_for_io_point, + get_zero_payload_for_io_point, +) +from shared.plugin_config_decode.modbus_master_config_model import ( + ERROR_HANDLING_KEEP_LAST, + ERROR_HANDLING_SET_TO_ZERO, +) + + +def make_point(fc: int, length: int = 4, iec_size: str = "W", error_handling: str = ERROR_HANDLING_KEEP_LAST): + """Minimal stand-in for ModbusIoPointConfig — only what the helpers read.""" + return SimpleNamespace( + fc=fc, + length=length, + iec_location=SimpleNamespace(size=iec_size, area="M", byte=0, bit=None), + error_handling=error_handling, + ) + + +class TestReadCount: + """The request size and the zero-fill size must come from one place.""" + + @pytest.mark.parametrize("fc", [1, 2]) + def test_coils_map_one_to_one(self, fc): + assert get_read_count_for_io_point(make_point(fc, length=6, iec_size="X")) == 6 + + @pytest.mark.parametrize( + "iec_size,registers_per_element", + [("B", 1), ("W", 1), ("D", 2), ("L", 4)], + ) + def test_registers_scale_with_element_width(self, iec_size, registers_per_element): + point = make_point(3, length=3, iec_size=iec_size) + assert get_read_count_for_io_point(point) == 3 * registers_per_element + + +class TestZeroPayload: + """The zero-fill has to look like the response it replaces.""" + + @pytest.mark.parametrize("fc", [1, 2]) + def test_boolean_reads_get_false(self, fc): + payload = get_zero_payload_for_io_point(make_point(fc, length=3, iec_size="X")) + assert payload == [False, False, False] + + def test_register_reads_get_integer_zeros(self): + payload = get_zero_payload_for_io_point(make_point(3, length=2, iec_size="W")) + assert payload == [0, 0] + + def test_wide_registers_get_one_zero_per_register(self): + # A %MD point is two registers per element, so two elements need four. + payload = get_zero_payload_for_io_point(make_point(4, length=2, iec_size="D")) + assert payload == [0, 0, 0, 0] + + +class TestQueueZeroFillOnFailure: + """ + What the read loop does with a failed read. + + Imported lazily: the plugin module pulls in pymodbus, and these are the + only tests here that need it. + """ + + @staticmethod + def _queue(): + from modbus_master_plugin import queue_zero_fill_on_failure + + return queue_zero_fill_on_failure + + def test_set_to_zero_queues_a_zeroed_write(self): + point = make_point(3, length=2, iec_size="W", error_handling=ERROR_HANDLING_SET_TO_ZERO) + queued = [] + + assert self._queue()(point, queued) is True + assert len(queued) == 1 + iec_location, payload, length = queued[0] + assert iec_location is point.iec_location + assert payload == [0, 0] + assert length == 2 + + def test_keep_last_value_queues_nothing(self): + point = make_point(3, error_handling=ERROR_HANDLING_KEEP_LAST) + queued = [] + + assert self._queue()(point, queued) is False + assert queued == [] + + def test_point_without_the_field_keeps_last_value(self): + # Defensive: a point object from an older config path has no attribute + # at all, and must not start zeroing buffers. + point = SimpleNamespace( + fc=3, length=2, iec_location=SimpleNamespace(size="W", area="M", byte=0, bit=None) + ) + queued = [] + + assert self._queue()(point, queued) is False + assert queued == [] + + def test_boolean_point_queues_false_values(self): + point = make_point(1, length=2, iec_size="X", error_handling=ERROR_HANDLING_SET_TO_ZERO) + queued = [] + + self._queue()(point, queued) + assert queued[0][1] == [False, False] From fb4cec0a6ed315f36dde161cc26ce862e26fcd0e Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 3 Aug 2026 16:30:08 -0400 Subject: [PATCH 03/14] 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 60ddab6cf6b5bc5305755052ae5500d53072a7c0 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 3 Aug 2026 16:38:52 -0400 Subject: [PATCH 04/14] fix(runtime): sync located globals from strucpp's locatedGlobals[] image_tables_bind_located_vars() derived the config-scope located set positionally: it assumed strucpp emitted program-local entries before CONFIGURATION VAR_GLOBAL ... AT entries, and took the tail not covered by any program's located_range() as the globals. strucpp emits config globals FIRST, so the program-local block is always the tail. covered_end therefore reached locatedVarsCount as soon as ANY POU declared a located variable, g_config_located_count collapsed to 0, and image_tables_copy_config_globals_in/out became no-ops -- every located configuration global silently stopped being synced. %MX coils written over Modbus never reached the program, and %IX/%QX/%MW globals stopped being read and committed. Reported on the forum as "%MX locations now invalid - Runtime v4": the reporter's %MX writes worked until he added one unrelated located variable to a POU, and downgrading to v4.1.3 (which predates the threaded process-image model) fixed it. The runtime cannot recover the scope on its own: globals are file-scope GlobalVar singletons that appear nowhere in the configuration object graph, so ConfigurationInstance -> Resource -> Task -> Program never reaches them. STruC++ now states it instead, emitting locatedGlobals[] with the canonical storage pointer of every located VAR_GLOBAL -- the same raw_ptr() value it writes into locatedVars[].pointer. Replace the inferred slice with an explicit index list built once at load by joining locatedVars[].pointer against locatedGlobals[] on pointer identity. No ordering rule, no layout assumption, and no false positives since distinct objects have distinct addresses. The join lives in located_globals.c behind a pointer-accessor callback so it carries no strucpp layout knowledge and is unit-testable; tests/test_located_globals.c locks order-independence, the multi-program shape, the degradation paths, inconsistency detection and unbound descriptors. Backwards compatible in both directions. strucpp emits the extern "C" accessors beside its own array, so nothing here references a symbol an older program lacks -- important because scripts/compile.sh rebuilds the shim against each upload's generated header, where a shim-side reference would be a COMPILE error. The accessors are resolved with required=false (same pattern as strucpp_debug_locate): an older program resolves NULL, in which case located configuration globals are not synced at all and a warning naming the fix is logged once at load. The program still runs and POU-local located I/O is unaffected. An older runtime never looks the symbols up and is bit-for-bit unaffected. Two validations guard the result. Join totality: every locatedGlobals[] entry must match some located variable, otherwise the generated arrays disagree. And the program-range coverage bitmap is retained purely as an independent cross-check witness -- it can flag a config global that a task also claims, but it never decides a scope. Co-Authored-By: Claude Opus 5 (1M context) --- core/src/CMakeLists.txt | 1 + core/src/plc_app/image_tables.cpp | 281 +++++++++++++++++++++-------- core/src/plc_app/image_tables.h | 12 +- core/src/plc_app/located_globals.c | 62 +++++++ core/src/plc_app/located_globals.h | 78 ++++++++ tests/test_located_globals.c | 264 +++++++++++++++++++++++++++ 6 files changed, 620 insertions(+), 78 deletions(-) create mode 100644 core/src/plc_app/located_globals.c create mode 100644 core/src/plc_app/located_globals.h create mode 100644 tests/test_located_globals.c diff --git a/core/src/CMakeLists.txt b/core/src/CMakeLists.txt index 64527aad..8b637a0a 100644 --- a/core/src/CMakeLists.txt +++ b/core/src/CMakeLists.txt @@ -40,6 +40,7 @@ add_executable(plc_main ${CMAKE_SOURCE_DIR}/core/src/plc_app/utils/utils.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/utils/watchdog.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/image_tables.cpp + ${CMAKE_SOURCE_DIR}/core/src/plc_app/located_globals.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/journal_buffer.c ${CMAKE_SOURCE_DIR}/core/src/plc_app/debug_write_journal.cpp ${CMAKE_SOURCE_DIR}/core/src/plc_app/plc_io_cycle.cpp diff --git a/core/src/plc_app/image_tables.cpp b/core/src/plc_app/image_tables.cpp index 4a319641..22663175 100644 --- a/core/src/plc_app/image_tables.cpp +++ b/core/src/plc_app/image_tables.cpp @@ -13,6 +13,7 @@ extern "C" { #include "include/iec_python.h" +#include "located_globals.h" } // Layout-compatible mirror of the strucpp ABI. The runtime executable @@ -58,9 +59,18 @@ IEC_BOOL *bool_memory[BUFFER_SIZE][8]; namespace { using GetLocatedVarsFn = const strucpp::LocatedVar *(*)(void); using GetLocatedCountFn = uint32_t (*)(void); + // Located CONFIGURATION VAR_GLOBALs. strucpp emits these accessors beside + // the array in the generated configuration TU (not in our shim), so an older + // program simply does not export them -- see the OPTIONAL note in + // image_tables.h and the fallback in image_tables_bind_located_vars(). + using GetLocatedGlobalsFn = void *const *(*)(void); + using GetLocatedGlobalCountFn = uint32_t (*)(void); GetLocatedVarsFn ext_strucpp_get_located_vars = nullptr; GetLocatedCountFn ext_strucpp_get_located_var_count = nullptr; + + GetLocatedGlobalsFn ext_strucpp_get_located_globals = nullptr; + GetLocatedGlobalCountFn ext_strucpp_get_located_global_count = nullptr; } // --------------------------------------------------------------------------- @@ -94,14 +104,23 @@ namespace { uint64_t *g_located_snapshot = nullptr; uint32_t g_located_count = 0; - // Config-scope located slice: the tail of locatedVars[] that belongs to - // CONFIGURATION VAR_GLOBAL ... AT (strucpp emits program-local located vars - // first, then the shared globals). These are NOT covered by any program's - // located_range(), so the per-task copy-in/out never touches them; the - // dispatcher copies this slice at the quiescent frame boundary instead (see - // image_tables_copy_config_globals_in/out). [offset, offset+count). - uint32_t g_config_located_offset = 0; - uint32_t g_config_located_count = 0; + // Indices of the locatedVars[] entries that are CONFIGURATION VAR_GLOBAL + // ... AT. No program's located_range() covers them, so the per-task + // copy-in/out never touches them; the dispatcher copies them at the + // quiescent frame boundary instead (image_tables_copy_config_globals_in/out). + // + // Built once at program load by joining locatedVars[].pointer against the + // .so's locatedGlobals[] on POINTER IDENTITY. strucpp states which storage + // belongs to a configuration global; we never infer it. + // + // This deliberately replaces an earlier "[offset, count) tail not covered by + // any program range" slice. That assumed strucpp emitted program-local + // entries before config globals; the real order is the reverse, so the + // computed count collapsed to zero as soon as any POU declared a located + // variable and every located global silently stopped being serviced. Do not + // reintroduce any rule based on an entry's position in the array. + uint32_t *g_located_globals_idx = nullptr; + uint32_t g_located_globals_n = 0; int init_recursive_pi_mutex(pthread_mutex_t *m) { @@ -215,6 +234,17 @@ extern "C" int symbols_init(PluginManager *pm) *(void **)&ext_strucpp_get_located_vars = resolve(pm, "strucpp_get_located_vars", true); *(void **)&ext_strucpp_get_located_var_count = resolve(pm, "strucpp_get_located_var_count", true); + /* Located CONFIGURATION VAR_GLOBALs — OPTIONAL. strucpp emits these + * accessors beside locatedGlobals[] in the generated configuration TU, so a + * program exported by an editor that predates them simply does not have the + * symbols. When absent the runtime cannot tell which located entries are + * config-scope, so it services none of them (the program still runs, and + * POU-local located I/O is unaffected) and warns once at load. */ + *(void **)&ext_strucpp_get_located_globals = + resolve(pm, "strucpp_get_located_globals", false); + *(void **)&ext_strucpp_get_located_global_count = + resolve(pm, "strucpp_get_located_global_count", false); + *(void **)&ext_strucpp_debug_array_count = resolve(pm, "strucpp_debug_array_count", true); *(void **)&ext_strucpp_debug_elem_count = resolve(pm, "strucpp_debug_elem_count", true); *(void **)&ext_strucpp_debug_size = resolve(pm, "strucpp_debug_size", true); @@ -280,6 +310,14 @@ extern "C" int symbols_init(PluginManager *pm) return 0; } +// Adapter letting located_globals.c read locatedVars[i].pointer without knowing +// the strucpp LocatedVar layout (that mirror lives only in this TU). +static const void *located_pointer_at(const void *located_vars, uint32_t index) +{ + const strucpp::LocatedVar *lv = (const strucpp::LocatedVar *)located_vars; + return lv[index].pointer; +} + void image_tables_bind_located_vars(void) { if (!ext_strucpp_get_located_vars || !ext_strucpp_get_located_var_count) @@ -299,14 +337,53 @@ void image_tables_bind_located_vars(void) g_located_snapshot = (uint64_t *)calloc(lv_count ? lv_count : 1, sizeof(uint64_t)); - // Determine the config-scope located slice = the entries not covered by any - // program's located_range(). strucpp lays out locatedVars[] as - // [program-local ... ][config globals ... ], so the covered part is the - // prefix [0, covered_end) and the config globals are the tail - // [covered_end, lv_count). Compute covered_end as the max end over every - // program's range (robust to a program declaring zero located vars). - uint32_t covered_end = 0; - if (g_config_ptr) + // Build the config-scope index list. Authority is the .so's + // locatedGlobals[]: strucpp records there the canonical storage pointer of + // every located CONFIGURATION VAR_GLOBAL — the same raw_ptr() value it writes + // into locatedVars[].pointer — so an entry is config-scope exactly when its + // pointer appears in that array. Pointer identity, no layout or ordering + // assumption, and distinct objects have distinct addresses so there are no + // false positives. + free(g_located_globals_idx); + g_located_globals_idx = nullptr; + g_located_globals_n = 0; + + if (!ext_strucpp_get_located_globals || !ext_strucpp_get_located_global_count) + { + // Program exported before strucpp emitted locatedGlobals[]. We cannot + // identify the config-scope entries, and we will not guess: service none + // of them. The program still runs and POU-local located I/O is + // unaffected — only located VAR_GLOBALs are inert. + log_warn("[image_tables] this program does not export locatedGlobals[] " + "(built by an older editor/STruC++) — located CONFIGURATION " + "VAR_GLOBALs (%%IX/%%QX/%%MX/%%MW ... AT on a global) will NOT " + "be synced; re-export the project from a current OpenPLC Editor"); + log_info("[image_tables] %u located var(s) via copy-in/out " + "(%u program-local, 0 config-scope shared globals)", + lv_count, lv_count); + return; + } + + const strucpp::LocatedVar *lv = ext_strucpp_get_located_vars(); + void *const *lg = ext_strucpp_get_located_globals(); + uint32_t lg_count = ext_strucpp_get_located_global_count(); + + if (lv_count) + { + g_located_globals_idx = (uint32_t *)calloc(lv_count, sizeof(uint32_t)); + if (!g_located_globals_idx) + { + log_error("[image_tables] out of memory building the config-located " + "index — located configuration globals will NOT be synced"); + return; + } + } + + // Independent cross-check witness: which indices a task actually claims via + // located_range(). Used only to detect disagreement with the authoritative + // classification, never to derive it. + uint8_t *claimed = (uint8_t *)calloc(lv_count ? lv_count : 1, 1); + if (claimed && g_config_ptr) { strucpp::ResourceInstance *res = g_config_ptr->get_resources(); size_t rc = g_config_ptr->get_resource_count(); @@ -319,18 +396,47 @@ void image_tables_bind_located_vars(void) { uint32_t off = 0, cnt = 0; tk.programs[p]->located_range(&off, &cnt); - if (cnt && off + cnt > covered_end) covered_end = off + cnt; + for (uint32_t i = off; i < off + cnt && i < lv_count; ++i) + claimed[i] = 1; } } } } - if (covered_end > lv_count) covered_end = lv_count; - g_config_located_offset = covered_end; - g_config_located_count = lv_count - covered_end; + + uint32_t matched_globals = 0; + g_located_globals_n = located_globals_join_ex(lv_count, lv, + located_pointer_at, + lg, lg_count, + g_located_globals_idx, + &matched_globals); + + // Every entry in locatedGlobals[] must correspond to some locatedVars[] + // entry; a shortfall means the two arrays disagree, i.e. a codegen bug. + if (matched_globals != lg_count) + { + log_error("[image_tables] only %u of %u locatedGlobals[] entries matched " + "a located variable — generated code is inconsistent", + matched_globals, lg_count); + } + + // A config global that a task's located_range() also claims would be copied + // twice, by the dispatcher and by that task. + if (claimed) + { + for (uint32_t j = 0; j < g_located_globals_n; ++j) + { + uint32_t k = g_located_globals_idx[j]; + if (k < lv_count && claimed[k]) + log_error("[image_tables] locatedVars[%u] is a configuration " + "global but a task's located_range() also claims it — " + "double-serviced slot", k); + } + } + free(claimed); log_info("[image_tables] %u located var(s) via copy-in/out " "(%u program-local, %u config-scope shared globals)", - lv_count, covered_end, g_config_located_count); + lv_count, lv_count - g_located_globals_n, g_located_globals_n); } // --------------------------------------------------------------------------- @@ -415,6 +521,49 @@ uint64_t threaded_image_read(const strucpp::LocatedVar &v) return 0; } +// Copy a SINGLE located entry. Both the per-task range walk and the config-scope +// index walk go through these, so the two callers can never drift apart in how an +// entry is actually moved. +void copy_in_one(const strucpp::LocatedVar *lv, uint32_t k) +{ + uint64_t v = threaded_image_read(lv[k]); + threaded_member_write(lv[k], v); + if (g_located_snapshot) g_located_snapshot[k] = v; +} + +void copy_out_one(const strucpp::LocatedVar *lv, uint32_t k) +{ + const strucpp::LocatedVar &v = lv[k]; + if (v.area == strucpp::LocatedArea::Input) return; // %I is never committed + uint64_t cur = threaded_member_read(v); + if (g_located_snapshot && cur == g_located_snapshot[k]) return; // unchanged + if (g_located_snapshot) g_located_snapshot[k] = cur; + uint16_t idx = v.byte_index; + bool out = (v.area == strucpp::LocatedArea::Output); + switch (v.size) + { + case strucpp::LocatedSize::Bit: + journal_write_bool(out ? JOURNAL_BOOL_OUTPUT : JOURNAL_BOOL_MEMORY, + idx, v.bit_index, cur != 0); + break; + case strucpp::LocatedSize::Byte: + journal_write_byte(JOURNAL_BYTE_OUTPUT, idx, (uint8_t)cur); + break; + case strucpp::LocatedSize::Word: + journal_write_int(out ? JOURNAL_INT_OUTPUT : JOURNAL_INT_MEMORY, + idx, (uint16_t)cur); + break; + case strucpp::LocatedSize::DWord: + journal_write_dint(out ? JOURNAL_DINT_OUTPUT : JOURNAL_DINT_MEMORY, + idx, (uint32_t)cur); + break; + case strucpp::LocatedSize::LWord: + journal_write_lint(out ? JOURNAL_LINT_OUTPUT : JOURNAL_LINT_MEMORY, + idx, cur); + break; + } +} + } // namespace extern "C" void image_tables_threaded_copy_in(uint32_t offset, uint32_t count) @@ -423,12 +572,7 @@ extern "C" void image_tables_threaded_copy_in(uint32_t offset, uint32_t count) const strucpp::LocatedVar *lv = ext_strucpp_get_located_vars(); uint32_t end = offset + count; if (end > g_located_count) end = g_located_count; - for (uint32_t k = offset; k < end; ++k) - { - uint64_t v = threaded_image_read(lv[k]); - threaded_member_write(lv[k], v); - if (g_located_snapshot) g_located_snapshot[k] = v; - } + for (uint32_t k = offset; k < end; ++k) copy_in_one(lv, k); } extern "C" void image_tables_threaded_copy_out(uint32_t offset, uint32_t count) @@ -437,61 +581,45 @@ extern "C" void image_tables_threaded_copy_out(uint32_t offset, uint32_t count) const strucpp::LocatedVar *lv = ext_strucpp_get_located_vars(); uint32_t end = offset + count; if (end > g_located_count) end = g_located_count; - for (uint32_t k = offset; k < end; ++k) - { - const strucpp::LocatedVar &v = lv[k]; - if (v.area == strucpp::LocatedArea::Input) continue; // %I is never committed - uint64_t cur = threaded_member_read(v); - if (g_located_snapshot && cur == g_located_snapshot[k]) continue; // unchanged - if (g_located_snapshot) g_located_snapshot[k] = cur; - uint16_t idx = v.byte_index; - bool out = (v.area == strucpp::LocatedArea::Output); - switch (v.size) - { - case strucpp::LocatedSize::Bit: - journal_write_bool(out ? JOURNAL_BOOL_OUTPUT : JOURNAL_BOOL_MEMORY, - idx, v.bit_index, cur != 0); - break; - case strucpp::LocatedSize::Byte: - journal_write_byte(JOURNAL_BYTE_OUTPUT, idx, (uint8_t)cur); - break; - case strucpp::LocatedSize::Word: - journal_write_int(out ? JOURNAL_INT_OUTPUT : JOURNAL_INT_MEMORY, - idx, (uint16_t)cur); - break; - case strucpp::LocatedSize::DWord: - journal_write_dint(out ? JOURNAL_DINT_OUTPUT : JOURNAL_DINT_MEMORY, - idx, (uint32_t)cur); - break; - case strucpp::LocatedSize::LWord: - journal_write_lint(out ? JOURNAL_LINT_OUTPUT : JOURNAL_LINT_MEMORY, - idx, cur); - break; - } - } + for (uint32_t k = offset; k < end; ++k) copy_out_one(lv, k); } -// Config-scope located globals (CONFIGURATION VAR_GLOBAL ... AT). These are the -// tail slice of locatedVars[] that no program's located_range() covers, so the -// per-task copy-in/out never reaches them. The dispatcher calls these at the -// quiescent frame boundary (g_tasks_running == 0) so there is no concurrent -// task access to the shared canonical storage — the copy is safe WITHOUT the -// per-global mutex (quiescence is the synchronization). copy_in primes the -// canonical globals from the image (inputs get fresh hardware values); copy_out -// journals changed output/memory globals back to the image (drained by the -// dispatcher). No-ops when there are no located globals. +// Config-scope located globals (CONFIGURATION VAR_GLOBAL ... AT). No program's +// located_range() covers these, so the per-task copy-in/out never reaches them. +// The dispatcher calls these at the quiescent frame boundary +// (g_tasks_running == 0) so there is no concurrent task access to the shared +// canonical storage — the copy is safe WITHOUT the per-global mutex (quiescence +// is the synchronization). copy_in primes the canonical globals from the image +// (inputs get fresh hardware values); copy_out journals changed output/memory +// globals back to the image (drained by the dispatcher). +// +// The entries are an explicit index list built at load by +// image_tables_bind_located_vars() from the .so's locatedGlobals[]; they are NOT +// a contiguous slice, so these walk the list rather than calling the range-based +// helpers above. No-ops when the program has no located globals, or when it +// predates locatedGlobals[] (a warning is logged once at load). extern "C" void image_tables_copy_config_globals_in(void) { - if (g_config_located_count) - image_tables_threaded_copy_in(g_config_located_offset, - g_config_located_count); + if (!g_located_globals_n || !g_located_globals_idx) return; + if (!ext_strucpp_get_located_vars) return; + const strucpp::LocatedVar *lv = ext_strucpp_get_located_vars(); + for (uint32_t j = 0; j < g_located_globals_n; ++j) + { + uint32_t k = g_located_globals_idx[j]; + if (k < g_located_count) copy_in_one(lv, k); + } } extern "C" void image_tables_copy_config_globals_out(void) { - if (g_config_located_count) - image_tables_threaded_copy_out(g_config_located_offset, - g_config_located_count); + if (!g_located_globals_n || !g_located_globals_idx) return; + if (!ext_strucpp_get_located_vars) return; + const strucpp::LocatedVar *lv = ext_strucpp_get_located_vars(); + for (uint32_t j = 0; j < g_located_globals_n; ++j) + { + uint32_t k = g_located_globals_idx[j]; + if (k < g_located_count) copy_out_one(lv, k); + } } // --------------------------------------------------------------------------- @@ -545,8 +673,9 @@ void image_tables_clear_null_pointers(void) free(g_located_snapshot); g_located_snapshot = nullptr; g_located_count = 0; - g_config_located_offset = 0; - g_config_located_count = 0; + free(g_located_globals_idx); + g_located_globals_idx = nullptr; + g_located_globals_n = 0; std::memset(bool_input, 0, sizeof(bool_input)); std::memset(bool_output, 0, sizeof(bool_output)); diff --git a/core/src/plc_app/image_tables.h b/core/src/plc_app/image_tables.h index a6d548b0..6d2aa1f2 100644 --- a/core/src/plc_app/image_tables.h +++ b/core/src/plc_app/image_tables.h @@ -163,8 +163,16 @@ extern "C" * * Shared globals no longer use a runtime-owned mutex — each carries its own * std::mutex inside the .so (strucpp GlobalVar), taken per access in - * run(). The config-scope helpers copy the shared-global tail slice at the - * dispatcher's quiescent frame boundary (see image_tables.cpp). + * run(). The config-scope helpers copy the located CONFIGURATION VAR_GLOBALs + * at the dispatcher's quiescent frame boundary. Those entries are identified + * at load by joining locatedVars[].pointer against the .so's locatedGlobals[] + * on pointer identity — NOT by their position in locatedVars[], and NOT as a + * contiguous slice (see image_tables.cpp). + * + * locatedGlobals[] is OPTIONAL: a program exported before STruC++ emitted it + * does not export the accessors, in which case located configuration globals + * are not synced at all (the program still runs, POU-local located I/O is + * unaffected) and a warning is logged once at load. * --------------------------------------------------------------------- */ void image_tables_threaded_copy_in(uint32_t offset, uint32_t count); void image_tables_threaded_copy_out(uint32_t offset, uint32_t count); diff --git a/core/src/plc_app/located_globals.c b/core/src/plc_app/located_globals.c new file mode 100644 index 00000000..062a603d --- /dev/null +++ b/core/src/plc_app/located_globals.c @@ -0,0 +1,62 @@ +/* ----------------------------------------------------------------------------- + * located_globals.c — see located_globals.h for the rationale. + * -------------------------------------------------------------------------- */ + +#include + +#include "located_globals.h" + +uint32_t located_globals_join_ex(uint32_t lv_count, + const void *located_vars, + located_pointer_at_fn pointer_at, + const void *const *globals, + uint32_t globals_count, + uint32_t *out_idx, + uint32_t *out_matched) +{ + uint32_t n = 0; + + if (out_matched) *out_matched = 0; + if (!pointer_at || !out_idx) return 0; + if (!globals || globals_count == 0) return 0; + + for (uint32_t i = 0; i < lv_count; ++i) + { + const void *p = pointer_at(located_vars, i); + /* An unbound descriptor can't be matched, and must not be guessed at. */ + if (p == NULL) continue; + + for (uint32_t g = 0; g < globals_count; ++g) + { + if (globals[g] == p) + { + out_idx[n++] = i; + break; + } + } + } + + /* Report how many locatedGlobals[] entries found a home, so the caller can + * detect the two generated arrays disagreeing. Counted separately because a + * single global could in principle be referenced by more than one located + * descriptor (aliasing), which would make the index count misleading. */ + if (out_matched) + { + uint32_t matched = 0; + for (uint32_t g = 0; g < globals_count; ++g) + { + if (globals[g] == NULL) continue; + for (uint32_t i = 0; i < lv_count; ++i) + { + if (pointer_at(located_vars, i) == globals[g]) + { + ++matched; + break; + } + } + } + *out_matched = matched; + } + + return n; +} diff --git a/core/src/plc_app/located_globals.h b/core/src/plc_app/located_globals.h new file mode 100644 index 00000000..b3659118 --- /dev/null +++ b/core/src/plc_app/located_globals.h @@ -0,0 +1,78 @@ +/* ----------------------------------------------------------------------------- + * located_globals.h — resolve which locatedVars[] entries are CONFIGURATION + * VAR_GLOBAL ... AT, by joining against the .so's locatedGlobals[] array. + * + * Pure logic with no runtime dependencies, so it can be unit-tested directly + * (tests/test_located_globals.c). + * + * Why a join, and why this module exists at all: + * + * locatedVars[] mixes two ownership classes. POU-local `VAR ... AT` is serviced + * by the owning IEC task around run(); CONFIGURATION `VAR_GLOBAL ... AT` is + * owned by no task, so the dispatcher copies it at the quiescent frame boundary. + * The LocatedVar descriptors carry no scope, and the runtime cannot recover it + * from the configuration tree — globals are file-scope singletons that appear + * nowhere in ConfigurationInstance -> Resource -> Task -> Program. + * + * The runtime used to INFER the split from position, assuming the layout was + * [program-local ...][config globals ...] and treating the tail uncovered by any + * program's located_range() as the globals. strucpp actually emits config + * globals FIRST, so the program-local block is always the tail: one POU-local + * located variable made the computed count collapse to zero and every located + * global silently stopped being serviced (forum: "%MX locations now invalid"). + * + * strucpp now states the answer. locatedGlobals[] holds the canonical storage + * pointer of each located VAR_GLOBAL — the same raw_ptr() value written into + * locatedVars[].pointer — so an entry is config-scope exactly when its pointer + * appears in that array. Pointer identity: no ordering rule, no layout + * assumption, and no false positives, since distinct objects have distinct + * addresses. + * -------------------------------------------------------------------------- */ + +#ifndef OPENPLC_LOCATED_GLOBALS_H +#define OPENPLC_LOCATED_GLOBALS_H + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/* Pointer accessor for a located variable. Kept as a callback so this module + * needs no knowledge of the strucpp LocatedVar layout and stays unit-testable. + * Returns the entry's canonical storage pointer, or NULL when unbound. */ +typedef const void *(*located_pointer_at_fn)(const void *located_vars, + uint32_t index); + +/* ----------------------------------------------------------------------------- + * Join locatedVars[] against locatedGlobals[] and collect the config-scope + * indices, ascending. + * + * lv_count number of locatedVars[] entries + * located_vars opaque handle passed back to pointer_at + * pointer_at accessor yielding locatedVars[i]'s storage pointer + * globals locatedGlobals[] — storage pointers of the located globals + * globals_count number of entries in globals + * out_idx receives the config-scope indices; room for lv_count + * out_matched optional; receives how many `globals` entries matched some + * located variable. A value below globals_count means the two + * generated arrays disagree, which the caller should report. + * + * Returns the number of indices written to out_idx. + * + * Entries with a NULL pointer are skipped: an unbound descriptor cannot be + * matched, and guessing a scope for it is exactly the failure this replaces. + * -------------------------------------------------------------------------- */ +uint32_t located_globals_join_ex(uint32_t lv_count, + const void *located_vars, + located_pointer_at_fn pointer_at, + const void *const *globals, + uint32_t globals_count, + uint32_t *out_idx, + uint32_t *out_matched); + +#ifdef __cplusplus +} +#endif + +#endif /* OPENPLC_LOCATED_GLOBALS_H */ diff --git a/tests/test_located_globals.c b/tests/test_located_globals.c new file mode 100644 index 00000000..00ec5bda --- /dev/null +++ b/tests/test_located_globals.c @@ -0,0 +1,264 @@ +/* + * test_located_globals.c — unit tests for located_globals_join_ex(), which + * resolves which locatedVars[] entries are CONFIGURATION VAR_GLOBAL ... AT by + * joining against the .so's locatedGlobals[] on pointer identity. + * + * Why this exists: the runtime used to derive the config-scope set from POSITION, + * assuming strucpp emitted [program-local ...][config globals ...] and treating + * the tail uncovered by any program's located_range() as the globals. strucpp + * emits config globals FIRST, so the program-local block is always the tail: + * `covered_end` reached locatedVarsCount as soon as any POU declared a located + * variable, the count collapsed to 0, and EVERY located configuration global + * (%MX, %QX, %MW alike) silently stopped being synced. Reported on the forum as + * "%MX locations now invalid - Runtime v4". + * + * The behaviour locked here: + * - the join follows pointer identity only, so the result is identical whether + * globals come first, last, or interleaved (ordering_* cases); + * - out_matched reports how many locatedGlobals[] entries found a located + * variable, so the caller can detect the two generated arrays disagreeing; + * - unbound (NULL) descriptors are skipped rather than guessed at; + * - an absent/empty globals array yields zero config-scope entries, which is + * the "older program" degradation path. + * + * Build (standalone, no runtime deps): + * cc -std=c11 -Wall -Wextra -I core/src/plc_app \ + * tests/test_located_globals.c core/src/plc_app/located_globals.c \ + * -o /tmp/test_located_globals && /tmp/test_located_globals + */ + +#include +#include + +#include "located_globals.h" + +static int g_failures = 0; + +#define CHECK(cond, msg, ...) \ + do { \ + if (!(cond)) { \ + printf(" FAIL: " msg "\n", ##__VA_ARGS__); \ + ++g_failures; \ + } \ + } while (0) + +/* --------------------------------------------------------------------------- + * Stand-in for locatedVars[]: an array of storage pointers. Distinct dummy + * objects give distinct addresses, exactly as real IEC storage does. + * ------------------------------------------------------------------------- */ +#define MAX_VARS 8 +static char g_storage[MAX_VARS]; /* distinct addresses */ +static const void *g_lv[MAX_VARS]; /* the "locatedVars[i].pointer" values */ + +static const void *pointer_at(const void *handle, uint32_t index) +{ + const void *const *lv = (const void *const *)handle; + return lv[index]; +} + +/* Convenience: address of dummy storage slot n. */ +#define S(n) ((const void *)&g_storage[n]) + +static void reset(void) +{ + for (uint32_t i = 0; i < MAX_VARS; ++i) g_lv[i] = NULL; +} + +/* --------------------------------------------------------------------------- + * The regression: config globals FIRST, one program-local var last. Under the + * old positional rule this produced zero config entries. + * ------------------------------------------------------------------------- */ +static void test_globals_first(void) +{ + printf("test_globals_first (the reported regression)\n"); + reset(); + g_lv[0] = S(0); /* GLOBALVAR AT %MD0 — config global */ + g_lv[1] = S(1); /* GLOBALBOOL AT %MX0.0 — config global */ + g_lv[2] = S(2); /* HW_IN AT %IX0.1 — program-local */ + const void *globals[] = { S(0), S(1) }; + + uint32_t idx[3], matched = 0; + uint32_t n = located_globals_join_ex(3, g_lv, pointer_at, globals, 2, + idx, &matched); + + CHECK(n == 2, "expected 2 config globals, got %u", n); + CHECK(idx[0] == 0 && idx[1] == 1, "expected [0,1], got [%u,%u]", idx[0], idx[1]); + CHECK(matched == 2, "expected 2 matched globals, got %u", matched); +} + +/* --------------------------------------------------------------------------- + * Order independence — the property the old code lacked. + * ------------------------------------------------------------------------- */ +static void test_ordering_independence(void) +{ + printf("test_ordering_independence\n"); + + /* (a) globals last */ + { + reset(); + g_lv[0] = S(0); /* program-local */ + g_lv[1] = S(1); + g_lv[2] = S(2); + const void *globals[] = { S(1), S(2) }; + uint32_t idx[3], matched = 0; + uint32_t n = located_globals_join_ex(3, g_lv, pointer_at, globals, 2, + idx, &matched); + CHECK(n == 2, "(globals last) expected 2, got %u", n); + CHECK(idx[0] == 1 && idx[1] == 2, "(globals last) got [%u,%u]", idx[0], idx[1]); + } + + /* (b) interleaved */ + { + reset(); + for (uint32_t i = 0; i < 4; ++i) g_lv[i] = S(i); + const void *globals[] = { S(0), S(2) }; + uint32_t idx[4], matched = 0; + uint32_t n = located_globals_join_ex(4, g_lv, pointer_at, globals, 2, + idx, &matched); + CHECK(n == 2, "(interleaved) expected 2, got %u", n); + CHECK(idx[0] == 0 && idx[1] == 2, "(interleaved) got [%u,%u]", idx[0], idx[1]); + } +} + +/* --------------------------------------------------------------------------- + * Multi-program shape that also collapsed to zero under the old rule: + * 3 globals, then two programs with two located vars each. + * ------------------------------------------------------------------------- */ +static void test_multi_program(void) +{ + printf("test_multi_program\n"); + reset(); + for (uint32_t i = 0; i < 7; ++i) g_lv[i] = S(i); + const void *globals[] = { S(0), S(1), S(2) }; + + uint32_t idx[7], matched = 0; + uint32_t n = located_globals_join_ex(7, g_lv, pointer_at, globals, 3, + idx, &matched); + CHECK(n == 3, "expected 3 config globals, got %u", n); + CHECK(idx[0] == 0 && idx[1] == 1 && idx[2] == 2, + "expected [0,1,2], got [%u,%u,%u]", idx[0], idx[1], idx[2]); + CHECK(matched == 3, "expected 3 matched, got %u", matched); +} + +/* ------------------------------------------------------------------------- */ +static void test_degradation_and_edges(void) +{ + printf("test_degradation_and_edges\n"); + + /* Older program: no globals array at all (accessors absent). The caller + * warns and services no globals; the join must simply report zero. */ + { + reset(); + for (uint32_t i = 0; i < 3; ++i) g_lv[i] = S(i); + uint32_t idx[3], matched = 99; + CHECK(located_globals_join_ex(3, g_lv, pointer_at, NULL, 0, idx, &matched) == 0, + "NULL globals array should yield 0"); + CHECK(matched == 0, "matched should be 0 for a NULL globals array"); + } + + /* Present but empty: project genuinely has no located globals. Distinct from + * the case above at the caller (no warning), identical here. */ + { + reset(); + for (uint32_t i = 0; i < 3; ++i) g_lv[i] = S(i); + const void *globals[] = { NULL }; + uint32_t idx[3], matched = 99; + CHECK(located_globals_join_ex(3, g_lv, pointer_at, globals, 0, idx, &matched) == 0, + "count 0 should yield 0"); + CHECK(matched == 0, "matched should be 0 when count is 0"); + } + + /* No located variables at all, but globals declared — nothing to join. */ + { + reset(); + const void *globals[] = { S(0) }; + uint32_t idx[1], matched = 99; + CHECK(located_globals_join_ex(0, g_lv, pointer_at, globals, 1, idx, &matched) == 0, + "no located vars should yield 0"); + CHECK(matched == 0, "no located vars means no global can match"); + } + + /* Every located var is a global (no POU declares one) — the shape that + * happened to work under the old positional rule. */ + { + reset(); + g_lv[0] = S(0); g_lv[1] = S(1); + const void *globals[] = { S(0), S(1) }; + uint32_t idx[2], matched = 0; + CHECK(located_globals_join_ex(2, g_lv, pointer_at, globals, 2, idx, &matched) == 2, + "all-globals should yield 2"); + } + + /* Defensive: NULL accessor / NULL output must not crash and must yield 0. */ + { + reset(); + const void *globals[] = { S(0) }; + uint32_t idx[2]; + CHECK(located_globals_join_ex(2, g_lv, NULL, globals, 1, idx, NULL) == 0, + "NULL pointer_at should yield 0"); + CHECK(located_globals_join_ex(2, g_lv, pointer_at, globals, 1, NULL, NULL) == 0, + "NULL out_idx should yield 0"); + } +} + +/* --------------------------------------------------------------------------- + * Inconsistency detection: a locatedGlobals[] entry that matches no located + * variable means the two generated arrays disagree. The join must report it via + * out_matched rather than silently dropping it. + * ------------------------------------------------------------------------- */ +static void test_inconsistency_detected(void) +{ + printf("test_inconsistency_detected\n"); + reset(); + g_lv[0] = S(0); + g_lv[1] = S(1); + /* S(5) belongs to no located variable — codegen bug. */ + const void *globals[] = { S(0), S(5) }; + + uint32_t idx[2], matched = 0; + uint32_t n = located_globals_join_ex(2, g_lv, pointer_at, globals, 2, + idx, &matched); + CHECK(n == 1, "expected 1 config global, got %u", n); + CHECK(idx[0] == 0, "expected index 0, got %u", idx[0]); + CHECK(matched == 1, "expected matched=1 (< count 2) to flag the mismatch, got %u", + matched); +} + +/* --------------------------------------------------------------------------- + * Unbound descriptors: a located variable whose pointer was never populated + * (e.g. a program never instantiated) must be skipped, not guessed at. + * ------------------------------------------------------------------------- */ +static void test_unbound_skipped(void) +{ + printf("test_unbound_skipped\n"); + reset(); + g_lv[0] = S(0); + g_lv[1] = NULL; /* never bound */ + g_lv[2] = S(2); + const void *globals[] = { S(0), S(2) }; + + uint32_t idx[3], matched = 0; + uint32_t n = located_globals_join_ex(3, g_lv, pointer_at, globals, 2, + idx, &matched); + CHECK(n == 2, "expected 2 config globals, got %u", n); + CHECK(idx[0] == 0 && idx[1] == 2, "expected [0,2], got [%u,%u]", idx[0], idx[1]); +} + +int main(void) +{ + printf("=== located_globals_join_ex unit tests ===\n"); + test_globals_first(); + test_ordering_independence(); + test_multi_program(); + test_degradation_and_edges(); + test_inconsistency_detected(); + test_unbound_skipped(); + + if (g_failures == 0) + { + printf("=== all tests passed ===\n"); + return 0; + } + printf("=== %d check(s) FAILED ===\n", g_failures); + return 1; +} From da408b90185c0baf46ba14a3845262255c951192 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 4 Aug 2026 13:26:38 -0400 Subject: [PATCH 05/14] 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 06/14] 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 07/14] 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 08/14] 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 2f9f8df75189e9afa1ca8679b94ef4413eb403fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Thu, 6 Aug 2026 10:56:09 +0200 Subject: [PATCH 09/14] feat(runtime): publish minEditorVersion at /api/capabilities (DOPE-448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Until now nothing let a runtime state what it needs from an editor: the upload endpoint accepts any ZIP that passes analyze_zip, and the bundle carries no editor identity at all. So "this runtime must not accept programs from an older editor" was not expressible. The new endpoint declares it. Unauthenticated, like /api/version — an editor calls it before login, because an editor too old to log in must still be able to find out why. The runtime only ADVERTISES this value; the editor compares it and refuses to upload. That is deliberate. Nothing on the upload path enforces it, so shipping a runtime release can never lock out an editor already installed in the field, and there is one place to debug when a push is refused. It is not a security control and should not be described as one. MIN_EDITOR_VERSION starts at 4.1.0 because that is where the STruC++ pipeline landed — 4.0.x editors emitted MatIEC artefacts this runtime cannot build at all. It is not a build counter: raising it for a release that merely changed something locks out working editors for no reason. Editors predating the endpoint get a 401 from the / catch-all (not a 404) and fall back to /api/version, seeing no floor — exactly their previous behaviour. Verified against a real pre-change container. Co-Authored-By: Claude Opus 5 --- docs/EDITOR_INTEGRATION.md | 14 ++++- tests/pytest/restapi/test_capabilities.py | 74 +++++++++++++++++++++++ webserver/restapi.py | 45 +++++++++++++- webserver/version.py | 18 ++++++ 4 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 tests/pytest/restapi/test_capabilities.py diff --git a/docs/EDITOR_INTEGRATION.md b/docs/EDITOR_INTEGRATION.md index b619f83c..7a7d4926 100644 --- a/docs/EDITOR_INTEGRATION.md +++ b/docs/EDITOR_INTEGRATION.md @@ -206,6 +206,18 @@ The runtime uses self-signed TLS certificates by default. The OpenPLC Editor han ## API Endpoints Summary +### Version and compatibility +- `GET /api/version` - Runtime version string +- `GET /api/capabilities` - Runtime version plus `minEditorVersion`, the oldest + editor this runtime accepts programs from + +Both are unauthenticated: the Editor calls them before login to decide whether +it may talk to this device. `minEditorVersion` is **advertised, not enforced** — +the Editor compares it against its own version and refuses to upload, so a +runtime release can never lock out an editor already installed in the field. +Editors that predate `/api/capabilities` get a 404 and fall back to +`/api/version`, seeing no editor floor (their previous behaviour). + ### Authentication - `POST /api/create-user` - Create user account - `POST /api/login` - Login and get JWT token @@ -227,7 +239,7 @@ The runtime uses self-signed TLS certificates by default. The OpenPLC Editor han ### Debug Interface - `wss://host:8443/api/debug` - WebSocket debug interface -All endpoints except `/api/create-user` (first user only), `/api/login`, and `/api/get-users-info` require JWT authentication. +All endpoints except `/api/version`, `/api/capabilities`, `/api/create-user` (first user only), `/api/login`, and `/api/get-users-info` require JWT authentication. ## Error Handling diff --git a/tests/pytest/restapi/test_capabilities.py b/tests/pytest/restapi/test_capabilities.py new file mode 100644 index 00000000..43b5c1a0 --- /dev/null +++ b/tests/pytest/restapi/test_capabilities.py @@ -0,0 +1,74 @@ +"""Behavioural tests for GET /api/version and GET /api/capabilities. + +Both endpoints exist so an editor can decide, before login, whether it may +talk to this runtime at all (DOPE-448). The contract these tests pin down: + + * both are reachable WITHOUT a token, even once users exist — an editor that + cannot authenticate must still be able to tell why; + * ``/api/capabilities`` reports the same version string as ``/api/version``, + so the two can never disagree about what runtime this is; + * ``minEditorVersion`` is present and parseable as a version, because the + editor compares it numerically and a malformed value would either block + every editor or none. +""" + +import re + +from webserver.version import MIN_EDITOR_VERSION, RUNTIME_VERSION + +from conftest import create_user + +_VERSION_RE = re.compile(r"^\d+\.\d+\.\d+$") + + +# --- reachability --------------------------------------------------------- + + +def test_capabilities_is_reachable_without_a_token(client): + resp = client.get("/api/capabilities") + assert resp.status_code == 200 + + +def test_capabilities_stays_unauthenticated_once_users_exist(client): + # The editor needs the compatibility answer even when it holds no + # credentials for this device, so creating a user must not close the door. + create_user(client, "admin", "admin-pass") + assert client.get("/api/capabilities").status_code == 200 + + +def test_version_is_reachable_without_a_token(client): + assert client.get("/api/version").status_code == 200 + + +# --- payload -------------------------------------------------------------- + + +def test_capabilities_reports_runtime_version_and_editor_floor(client): + body = client.get("/api/capabilities").get_json() + assert body == { + "runtimeVersion": RUNTIME_VERSION, + "minEditorVersion": MIN_EDITOR_VERSION, + } + + +def test_capabilities_and_version_agree_on_the_runtime_version(client): + capabilities = client.get("/api/capabilities").get_json() + version = client.get("/api/version").get_json() + assert capabilities["runtimeVersion"] == version["version"] + + +def test_min_editor_version_is_a_plain_three_part_version(): + # The editor parses this and compares it against its own APP_VERSION. A + # tag-style value ("v4.1.0") or a partial one ("4.1") would make that + # comparison ambiguous, so the published floor stays a bare x.y.z. + assert _VERSION_RE.match(MIN_EDITOR_VERSION), MIN_EDITOR_VERSION + + +# --- headers -------------------------------------------------------------- + + +def test_runtime_version_header_is_present_on_capabilities(client): + # Older editors read the version off this header rather than a body; the + # after_request hook must cover the new route too. + resp = client.get("/api/capabilities") + assert resp.headers["X-OpenPLC-Runtime-Version"] == RUNTIME_VERSION diff --git a/webserver/restapi.py b/webserver/restapi.py index 2b7701e7..4cee074c 100644 --- a/webserver/restapi.py +++ b/webserver/restapi.py @@ -17,7 +17,7 @@ import webserver.config from webserver.logger import get_logger -from webserver.version import RUNTIME_VERSION +from webserver.version import MIN_EDITOR_VERSION, RUNTIME_VERSION logger, buffer = get_logger("logger", use_buffer=True) @@ -72,6 +72,49 @@ def restapi_version(): return jsonify({"version": RUNTIME_VERSION}), 200 +@restapi_bp.route("/capabilities", methods=["GET"]) +def restapi_capabilities(): + """Return what this runtime is and what it requires of an editor. + + Unauthenticated, like ``/version`` — the editor calls this before + login to decide whether it may talk to this device at all. + + ``minEditorVersion`` is the oldest editor this runtime accepts + programs from. The runtime only ADVERTISES it: the editor compares + the value against its own version and refuses to upload. Nothing on + the upload path enforces it, so shipping a new runtime can never lock + out an editor already installed in the field (DOPE-448). + + Editors that predate this endpoint get a 404 and fall back to + ``/version``; they simply see no editor floor, which is exactly the + behaviour they had before. + --- + tags: + - Runtime + responses: + 200: + description: Runtime capabilities retrieved + schema: + type: object + properties: + runtimeVersion: + type: string + description: Runtime version string (GitHub release tag) + minEditorVersion: + type: string + description: Oldest OpenPLC Editor version this runtime accepts programs from + """ + return ( + jsonify( + { + "runtimeVersion": RUNTIME_VERSION, + "minEditorVersion": MIN_EDITOR_VERSION, + } + ), + 200, + ) + + jwt = JWTManager(app_restapi) db = SQLAlchemy(app_restapi) diff --git a/webserver/version.py b/webserver/version.py index c96647ce..fc1c2b45 100644 --- a/webserver/version.py +++ b/webserver/version.py @@ -42,6 +42,24 @@ import os from pathlib import Path +# Oldest OpenPLC Editor this runtime accepts programs from, published at +# ``GET /api/capabilities`` as ``minEditorVersion``. +# +# The EDITOR is what compares this value against its own version and refuses +# to upload — the runtime only advertises it (see DOPE-448). That is +# deliberate: an editor already installed in the field can never be locked out +# by a runtime release, because nothing on the upload path enforces this. +# +# Raise this ONLY when an older editor genuinely produces a bundle this +# runtime would mis-compile — a changed file layout, a renamed generated +# artefact, a conf the compile step can no longer read. It is NOT a build +# counter: bumping it for a release that merely "changed something" locks out +# working editors for no reason. When in doubt, leave it alone. +# +# 4.1.0 is the floor because the STruC++ compile pipeline landed there; the +# 4.0.x editors emitted MatIEC artefacts this runtime cannot build at all. +MIN_EDITOR_VERSION = "4.1.0" + def _resolve_runtime_version() -> str: # 1. Env var (CI Docker build-arg path). From a745a195ede429412480b54601af9200f5edac67 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Sun, 9 Aug 2026 21:05:40 -0300 Subject: [PATCH 10/14] fix(opcua): write REAL/LREAL values, not their bit pattern --- .../plugins/python/opcua/opcua_utils.py | 59 +++---- .../plugins/opcua/test_type_conversions.py | 110 +++++++------ tests/pytest/plugins/opcua/test_write_path.py | 155 ++++++++++++++++++ 3 files changed, 234 insertions(+), 90 deletions(-) create mode 100644 tests/pytest/plugins/opcua/test_write_path.py diff --git a/core/src/drivers/plugins/python/opcua/opcua_utils.py b/core/src/drivers/plugins/python/opcua/opcua_utils.py index 8b283e0e..bab74b43 100644 --- a/core/src/drivers/plugins/python/opcua/opcua_utils.py +++ b/core/src/drivers/plugins/python/opcua/opcua_utils.py @@ -3,7 +3,6 @@ import ctypes import os import sys -import struct from typing import Any from asyncua import ua @@ -140,24 +139,13 @@ def convert_value_for_opcua(datatype: str, value: Any) -> Any: clamped_value = max(0, min(18446744073709551615, int(value))) return ctypes.c_uint64(clamped_value).value - elif datatype.upper() in ["FLOAT", "REAL"]: - # Float/Real values are stored as integers in debug variables - # Convert back to float if it's an integer representation - if isinstance(value, int): - try: - return struct.unpack('f', struct.pack('I', value))[0] - except struct.error: - return float(value) - return float(value) - - elif datatype.upper() == "LREAL": - # LREAL (64-bit float) values are stored as integers in debug variables - # Convert back to double if it's an integer representation - if isinstance(value, int): - try: - return struct.unpack('d', struct.pack('Q', value))[0] - except struct.error: - return float(value) + elif datatype.upper() in ["FLOAT", "REAL", "LREAL"]: + # debug_read_value casts the raw bytes to c_float / c_double, so + # what arrives here is the number itself, never its bit pattern. + # The MatIEC-era debug protocol did hand over raw integers, and the + # struct.unpack that decoded them outlived it -- on the STruC++ + # debug surface it would turn an integer-valued REAL of 1 into + # 1.4e-45. Mirror of the write-side fix below. return float(value) elif datatype.upper() == "STRING": @@ -312,25 +300,13 @@ def convert_value_for_plc(datatype: str, value: Any) -> Any: clamped_value = max(0, min(18446744073709551615, int(value))) return ctypes.c_uint64(clamped_value).value - elif datatype.upper() in ["FLOAT", "REAL"]: - # Convert float to int representation for storage - if isinstance(value, float): - try: - return struct.unpack('I', struct.pack('f', value))[0] - except struct.error: - return int(value) - else: - return int(float(value)) - - elif datatype.upper() == "LREAL": - # Convert double to int representation for storage (64-bit) - if isinstance(value, float): - try: - return struct.unpack('Q', struct.pack('d', value))[0] - except struct.error: - return int(value) - else: - return int(float(value)) + elif datatype.upper() in ["FLOAT", "REAL", "LREAL"]: + # debug_write_value packs this through c_float / c_double, so hand + # it the number. Packing the bit pattern here -- correct back when + # the debug protocol exchanged raw integers -- meant a client + # writing 42.0 stored 1109917696.0 in the PLC (forum thread + # "Error changing values via OPC UA"). + return float(value) elif datatype.upper() == "STRING": return str(value) @@ -391,8 +367,11 @@ def convert_value_for_plc(datatype: str, value: Any) -> Any: log_warn(f"Failed to convert value {value} to {datatype}, using default: {e}") if datatype.upper() == "BOOL": return 0 - elif datatype.upper() in ["FLOAT", "REAL"]: - return 0 + elif datatype.upper() in ["FLOAT", "REAL", "LREAL"]: + # Float default, matching the c_float / c_double the write path + # encodes with -- an int default would be a (harmless, but + # misleading) type mismatch. + return 0.0 elif datatype.upper() == "STRING": return "" elif datatype.upper() in TIME_DATATYPES: diff --git a/tests/pytest/plugins/opcua/test_type_conversions.py b/tests/pytest/plugins/opcua/test_type_conversions.py index ef893ba6..06ed5058 100644 --- a/tests/pytest/plugins/opcua/test_type_conversions.py +++ b/tests/pytest/plugins/opcua/test_type_conversions.py @@ -9,7 +9,6 @@ """ import pytest -import struct import sys from pathlib import Path @@ -247,12 +246,16 @@ def test_float_from_float(self): result = convert_value_for_opcua("FLOAT", 3.14159) assert abs(result - 3.14159) < 0.0001 - def test_float_from_int_representation(self): - """Float stored as int representation should be unpacked.""" - # Pack 3.14159 as int representation - int_repr = struct.unpack('I', struct.pack('f', 3.14159))[0] - result = convert_value_for_opcua("FLOAT", int_repr) - assert abs(result - 3.14159) < 0.0001 + def test_float_from_int_is_the_number_not_a_bit_pattern(self): + """An int reaching a FLOAT node is that number, widened. + + debug_read_value hands over a real float (c_float / c_double), so + there is no bit pattern to decode. The MatIEC-era code reinterpreted + ints through struct.unpack, which turned a REAL of 1 into 1.4e-45. + """ + assert convert_value_for_opcua("FLOAT", 1) == 1.0 + assert convert_value_for_opcua("FLOAT", -7) == -7.0 + assert convert_value_for_opcua("FLOAT", 1109917696) == 1109917696.0 def test_float_zero(self): """Zero float should work correctly.""" @@ -270,12 +273,16 @@ def test_real_from_float(self): result = convert_value_for_opcua("REAL", 3.14159) assert abs(result - 3.14159) < 0.0001 - def test_real_from_int_representation(self): - """REAL stored as int representation should be unpacked.""" - # Pack 3.14159 as int representation - int_repr = struct.unpack('I', struct.pack('f', 3.14159))[0] - result = convert_value_for_opcua("REAL", int_repr) - assert abs(result - 3.14159) < 0.0001 + def test_real_from_int_is_the_number_not_a_bit_pattern(self): + """Same contract as FLOAT, for both REAL and LREAL.""" + assert convert_value_for_opcua("REAL", 1) == 1.0 + assert convert_value_for_opcua("LREAL", 42) == 42.0 + + # LREAL conversions (IEC 61131-3 LREAL = 64-bit float) + def test_lreal_from_float(self): + """LREAL values should pass through as float.""" + result = convert_value_for_opcua("LREAL", -273.15) + assert abs(result - (-273.15)) < 0.0001 # STRING conversions def test_string_normal(self): @@ -396,26 +403,40 @@ def test_lint_normal_values(self): assert convert_value_for_plc("LINT", 1000000000) == 1000000000 # FLOAT conversions - def test_float_to_int_representation(self): - """Float should be packed to int representation for PLC storage.""" - result = convert_value_for_plc("FLOAT", 3.14159) - # Verify by unpacking back - unpacked = struct.unpack('f', struct.pack('I', result))[0] - assert abs(unpacked - 3.14159) < 0.0001 + def test_float_stays_a_float(self): + """Floats are handed over as numbers, not as their bit pattern. + + debug_write_value encodes with c_float / c_double, so packing the bit + pattern here would store the pattern as the value. + """ + assert convert_value_for_plc("FLOAT", 3.5) == 3.5 + assert convert_value_for_plc("FLOAT", -273.15) == -273.15 def test_float_zero(self): - """Zero float should pack correctly.""" - result = convert_value_for_plc("FLOAT", 0.0) - unpacked = struct.unpack('f', struct.pack('I', result))[0] - assert unpacked == 0.0 + """Zero float should stay zero.""" + assert convert_value_for_plc("FLOAT", 0.0) == 0.0 + + def test_float_from_int_is_widened_not_reinterpreted(self): + """A client sending an int to a Float node means that number.""" + assert convert_value_for_plc("FLOAT", 3) == 3.0 # REAL conversions (IEC 61131-3 REAL = 32-bit float) - def test_real_to_int_representation(self): - """REAL should be packed to int representation for PLC storage.""" - result = convert_value_for_plc("REAL", 3.14159) - # Verify by unpacking back - unpacked = struct.unpack('f', struct.pack('I', result))[0] - assert abs(unpacked - 3.14159) < 0.0001 + def test_real_stays_a_float(self): + """REAL and LREAL follow the same contract as FLOAT.""" + assert convert_value_for_plc("REAL", 3.5) == 3.5 + assert convert_value_for_plc("LREAL", -273.15) == -273.15 + + def test_real_write_of_42_is_not_a_bit_pattern(self): + """Regression: forum thread "Error changing values via OPC UA". + + A client writing 42 saw 1.109918e+09 on a REAL and + 4.6311077918204232e+18 on an LREAL -- the IEEE-754 bit patterns of + 42.0 stored as the value itself. + """ + assert convert_value_for_plc("REAL", 42.0) == 42.0 + assert convert_value_for_plc("REAL", 42.0) != 1109917696 + assert convert_value_for_plc("LREAL", 42.0) == 42.0 + assert convert_value_for_plc("LREAL", 42.0) != 4631107791820423168 # STRING conversions def test_string_normal(self): @@ -536,30 +557,19 @@ def test_lint_roundtrip(self): assert plc_val == val def test_float_roundtrip(self): - """FLOAT values should survive round-trip conversion (with float precision).""" - for val in [0.0, 3.14159, -273.15, 1000000.5]: - # First convert float to int representation (as stored in PLC) - int_repr = struct.unpack('I', struct.pack('f', val))[0] - # Convert to OPC-UA - opcua_val = convert_value_for_opcua("FLOAT", int_repr) - # Convert back to PLC + """FLOAT values should survive round-trip conversion.""" + for val in [0.0, 3.14159, -273.15, 1000000.5, 42.0]: + opcua_val = convert_value_for_opcua("FLOAT", val) plc_val = convert_value_for_plc("FLOAT", opcua_val) - # Unpack and compare - result = struct.unpack('f', struct.pack('I', plc_val))[0] - assert abs(result - val) < 0.0001 + assert abs(plc_val - val) < 0.0001 def test_real_roundtrip(self): - """REAL values should survive round-trip conversion (same as FLOAT).""" - for val in [0.0, 3.14159, -273.15, 1000000.5]: - # First convert float to int representation (as stored in PLC) - int_repr = struct.unpack('I', struct.pack('f', val))[0] - # Convert to OPC-UA - opcua_val = convert_value_for_opcua("REAL", int_repr) - # Convert back to PLC - plc_val = convert_value_for_plc("REAL", opcua_val) - # Unpack and compare - result = struct.unpack('f', struct.pack('I', plc_val))[0] - assert abs(result - val) < 0.0001 + """REAL and LREAL values should survive round-trip conversion.""" + for datatype in ("REAL", "LREAL"): + for val in [0.0, 3.14159, -273.15, 1000000.5, 42.0]: + opcua_val = convert_value_for_opcua(datatype, val) + plc_val = convert_value_for_plc(datatype, opcua_val) + assert abs(plc_val - val) < 0.0001 def test_string_roundtrip(self): """STRING values should survive round-trip conversion.""" diff --git a/tests/pytest/plugins/opcua/test_write_path.py b/tests/pytest/plugins/opcua/test_write_path.py new file mode 100644 index 00000000..6c36e37d --- /dev/null +++ b/tests/pytest/plugins/opcua/test_write_path.py @@ -0,0 +1,155 @@ +""" +Round-trip tests for the OPC-UA write path. + +A client write travels + + client value + -> convert_value_for_plc() (opcua_utils.py) + -> debug_write_value() (opcua_memory.py, encodes with ctypes) + -> PLC memory + +and comes back through debug_read_value() -> convert_value_for_opcua(). +The two ends were tested in isolation but never as a pair, which is how a +float could be packed into its integer bit pattern on the way in and read +back as that pattern: a client writing 42 to a REAL stored 1109917696.0 +(forum thread "Error changing values via OPC UA"). + +These tests drive the real functions against a fake `args` whose debug_read / +debug_write keep the bytes in a dict, exactly like the runtime keeps them in +PLC memory. +""" + +import ctypes +import sys +from pathlib import Path + +import pytest + +# Add plugin path for imports +_plugin_dir = ( + Path(__file__).parent.parent.parent.parent.parent + / "core" + / "src" + / "drivers" + / "plugins" + / "python" +) +sys.path.insert(0, str(_plugin_dir / "opcua")) + +from opcua_memory import STATUS_OK, debug_read_value, debug_write_value +from opcua_utils import convert_value_for_opcua, convert_value_for_plc + + +class FakePlcMemory: + """Stands in for the runtime's debug surface. + + Stores whatever bytes debug_write hands over, keyed by (arr, elem), and + replays them on debug_read -- the same contract plc_main implements. + """ + + def __init__(self): + self.cells = {} + + def debug_write(self, arr, elem, buf, size): + key = ( + int(arr.value if hasattr(arr, "value") else arr), + int(elem.value if hasattr(elem, "value") else elem), + ) + length = int(size.value if hasattr(size, "value") else size) + self.cells[key] = bytes(buf[i] for i in range(length)) + return STATUS_OK + + def debug_read(self, arr, elem, buf): + key = ( + int(arr.value if hasattr(arr, "value") else arr), + int(elem.value if hasattr(elem, "value") else elem), + ) + raw = self.cells.get(key) + if raw is None: + return 0 + for i, byte in enumerate(raw): + buf[i] = byte + return len(raw) + + +@pytest.fixture +def plc(): + return FakePlcMemory() + + +def write_then_read(plc, datatype, client_value): + """Full client-write -> PLC -> client-read cycle for one variable.""" + plc_value = convert_value_for_plc(datatype, client_value) + assert debug_write_value(plc, 0, 0, datatype, plc_value) is True + stored = debug_read_value(plc, 0, 0, datatype) + return convert_value_for_opcua(datatype, stored) + + +class TestFloatingPointWrites: + """Regression: forum thread "Error changing values via OPC UA".""" + + def test_real_write_of_42_round_trips(self, plc): + assert write_then_read(plc, "REAL", 42.0) == 42.0 + + def test_lreal_write_of_42_round_trips(self, plc): + assert write_then_read(plc, "LREAL", 42.0) == 42.0 + + def test_real_does_not_store_the_bit_pattern(self, plc): + """The exact failure mode: 42.0 stored as 1109917696.0.""" + assert write_then_read(plc, "REAL", 42.0) != 1109917696.0 + + def test_lreal_does_not_store_the_bit_pattern(self, plc): + assert write_then_read(plc, "LREAL", 42.0) != 4.6311077918204232e18 + + @pytest.mark.parametrize("value", [0.0, 1.5, -273.15, 1000000.5]) + def test_lreal_keeps_full_double_precision(self, plc, value): + assert write_then_read(plc, "LREAL", value) == value + + @pytest.mark.parametrize("value", [0.0, 1.5, -273.0, 42.0]) + def test_real_keeps_float32_representable_values(self, plc, value): + # Values chosen to be exact in float32, so the comparison is exact. + assert write_then_read(plc, "REAL", value) == value + + def test_real_narrows_to_float32(self, plc): + """A REAL is 32-bit: the stored value is the float32 rounding of the + client's double, not the double itself.""" + result = write_then_read(plc, "REAL", 3.14159) + assert result == pytest.approx(3.14159, abs=1e-6) + + def test_int_written_to_a_float_node_is_the_number(self, plc): + """A client may send an integer variant to a Float node.""" + assert write_then_read(plc, "REAL", 42) == 42.0 + + +class TestIntegerWritesStillWork: + """INT was never affected; guard against fixing floats by breaking ints.""" + + @pytest.mark.parametrize( + "datatype,value", + [ + ("INT", 42), + ("INT", -32768), + ("INT", 32767), + ("DINT", -2147483648), + ("UINT", 65535), + ("LINT", 9223372036854775807), + ("BOOL", True), + ("BOOL", False), + ], + ) + def test_integer_round_trip(self, plc, datatype, value): + result = write_then_read(plc, datatype, value) + expected = bool(value) if datatype == "BOOL" else value + assert result == expected + + +class TestEncoding: + """The bytes that reach PLC memory are the IEC-typed encoding.""" + + def test_real_writes_four_bytes_of_float32(self, plc): + debug_write_value(plc, 0, 0, "REAL", convert_value_for_plc("REAL", 42.0)) + assert plc.cells[(0, 0)] == bytes(ctypes.c_float(42.0)) + + def test_lreal_writes_eight_bytes_of_float64(self, plc): + debug_write_value(plc, 0, 0, "LREAL", convert_value_for_plc("LREAL", 42.0)) + assert plc.cells[(0, 0)] == bytes(ctypes.c_double(42.0)) From eea93f18fe2314c0c62fc3328ef723212058e8cb Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 11 Aug 2026 12:48:05 -0400 Subject: [PATCH 11/14] 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 12/14] 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 13/14] 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) From 1e7482310afd8f1b59ddb48fab6ae8883ded4c9b Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Tue, 11 Aug 2026 16:19:52 -0400 Subject: [PATCH 14/14] chore(release): bump VERSION to v4.1.10 Local installs and the Windows installer payload read the repo-root VERSION file to answer /api/version and the discovery scan, so it has to be in sync with the release tag before the tag is pushed. Co-Authored-By: Claude Opus 5 (1M context) --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index e9120e14..23ad5eb3 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v4.1.9 +v4.1.10