From 10af15966b6d00f2466b9d77a80709033b3cf1d5 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 12:43:01 +0200 Subject: [PATCH 1/8] fix(opcua): recover the OPC UA session after a bad start Two failures share the same shape: the plugin decides something once, at startup, and can never revise it while it runs. Re-scan while no session is up. Config-less discovery (#544, #509) ran a single scan a couple of seconds after start. A gateway that boots alongside its PLC scans while the PLC is still coming up, finds nothing, falls back to opc.tcp://localhost:4840 and retries that endpoint for as long as it runs. Only a restart found the PLC. The poller's reconnect arm now asks the plugin for a fresh scan, rate limited by discovery.interval_s (default 30 s), and adopts a newly found server for its next connect attempt, resetting the backoff so the new endpoint is tried at once rather than after the dead one's accumulated wait. The rules that made discovery safe are unchanged: an explicitly configured endpoint_url still wins and is never rescanned, the scan stays a bounded read-only TCP sweep plus GetEndpoints, and nothing is scanned while a session is up. interval_s stops being an accepted-but-ignored knob. Clear PLC_COMMS_LOST on every successful connect. The fault raised for a sustained outage (#496) was cleared only when the running process still remembered raising it. The fault manager keys faults by fault code and persists them, so a fault raised before a gateway restart is standing in the store with nothing in memory to remember it, and the arm that would clear it is never entered when the first connect succeeds. The fault then stayed CONFIRMED for good. Both connect paths, the initial one and every reconnect, now send the clear regardless of what this process raised. The clear is fire and forget, so a clear for a fault that is not there costs nothing. The debounce that governs raising is untouched. Tests: the discovery pass and the endpoint adoption rule are exercised with injected probes, including the positive control that a configured endpoint is refused the very server an unconfigured one accepts. The comms-lost heal runs against the live test server, because only a connect that actually succeeds reaches that arm. --- .../ros2_medkit_opcua/README.md | 33 +++- .../ros2_medkit_opcua/network_discovery.hpp | 10 +- .../ros2_medkit_opcua/opcua_plugin.hpp | 58 +++++++ .../ros2_medkit_opcua/opcua_poller.hpp | 25 +++ .../ros2_medkit_opcua/src/opcua_plugin.cpp | 148 +++++++++++++++--- .../ros2_medkit_opcua/src/opcua_poller.cpp | 44 +++++- .../test/test_opcua_identity.cpp | 82 +++++++++- .../test/test_opcua_plugin.cpp | 141 +++++++++++++++++ 8 files changed, 499 insertions(+), 42 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 0381ef765..17cc1115e 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -548,7 +548,7 @@ ros2_medkit_gateway: | `subscription_interval_ms` | `500` | Publishing interval for OPC-UA subscriptions when `prefer_subscriptions: true` | | `condition_replay_strategy` | `auto` | Active-condition replay on reconnect: `method`, `read`, `auto`, `off` (see below) | | `require_confirm_for_clear` | `true` | Require both Acknowledge AND Confirm before a native alarm auto-clears. Set `false` for Confirm-less servers (e.g. Siemens S7-1500) so alarms clear on Acknowledge alone (see below) | -| `comms_lost_fault_enabled` | `true` | Raise a component-scoped `PLC_COMMS_LOST` fault when the connection stays down (issue #496) | +| `comms_lost_fault_enabled` | `true` | Raise a component-scoped `PLC_COMMS_LOST` fault when the connection stays down, and clear it on every successful connect (issue #496) | | `comms_lost_debounce_ms` | `5000` | Continuous down time before `PLC_COMMS_LOST` is raised (debounces reconnect blips; clamped to [0, 3600000] ms) | | `comms_lost_severity` | `ERROR` | SOVD severity bucket for the `PLC_COMMS_LOST` fault | | `discovery.enabled` | `false` | Opt-in read-only PLC network discovery (auto endpoint). See below | @@ -604,7 +604,7 @@ plugins.opcua.discovery: connect_timeout_ms: 600 # per-port TCP connect timeout scan_concurrency: 100 # bounded, polite concurrent connect count identify_timeout_ms: 6000 # per GetEndpoints identify - interval_s: 0 # 0 = one-shot at startup (periodic re-scan: TODO) + interval_s: 0 # re-scan cadence while disconnected (0 = default 30 s) anonymous_none_only: true # only auto-connect None/Anonymous servers ``` @@ -625,6 +625,15 @@ How it works: auto-selects the best None/Anonymous data server (deterministic, lowest ip:port) and connects to the **scanned ip:port** - not the advertised EndpointUrl, which a server may report as a non-resolvable hostname. +5. While no session is established, the reconnect loop scans again every + `interval_s` (default 30 s) and adopts a newly found server for its next + connect attempt, logging the swap at INFO. This is what covers the common + race where the gateway and the PLC boot together: the startup scan finds + nothing because the PLC is still coming up, and without a re-scan the plugin + would retry the fallback endpoint until someone restarted it. + +Re-scanning stops as soon as a session is up, and never starts at all when an +`endpoint_url` is configured. Safety / OT posture: - Everything is read-only: TCP connect + `GetEndpoints` only. No writes, no @@ -640,9 +649,9 @@ Safety / OT posture: Note on passive discovery: a stock Siemens S7-1500 neither multicast-announces (mDNS `_opcua-tcp._tcp`) nor registers with an OPC-UA LDS, so passive sources find nothing there; the active scan is what discovers it. Passive mDNS / LDS -`FindServers` sources (useful on Kepware / Prosys / GDS estates) and periodic -re-scan + multi-endpoint registration are planned follow-ups; this iteration -delivers the active-scan core and single "auto endpoint" mode. +`FindServers` sources (useful on Kepware / Prosys / GDS estates) and +multi-endpoint registration are planned follow-ups. This iteration delivers the +active-scan core and a single "auto endpoint" mode. ### Active-condition replay on reconnect (issue #389/#478) @@ -694,6 +703,20 @@ so the alarm clears on `Acknowledge` alone. The default (`true`) is unchanged and spec-strict; the relaxed path still requires acknowledgement and needs real-S7-1500 validation. +### Connection loss and `PLC_COMMS_LOST` (issue #496) + +When the OPC-UA connection stays down for `comms_lost_debounce_ms` continuously, +the plugin raises one component-scoped `PLC_COMMS_LOST` fault (a shorter blip +during a normal reconnect does not flap it). + +The fault is cleared on **every** successful connect, both the initial one and +every later reconnect, whether or not this process was the one that raised it. +The fault manager keys faults by fault code and persists them, so a fault raised +before a gateway restart is still standing while the new process has no memory +of it. Clearing only what the running process remembered left exactly that fault +CONFIRMED for good. The clear is fire-and-forget, so a clear for a fault that is +not there is harmless. + Node map entries also support an optional `ros2_topic` field to override the auto-generated ROS 2 topic name for the PLC value bridge: ```yaml diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp index ed68d33e5..56c31a8ae 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp @@ -105,9 +105,13 @@ struct OpcuaDiscoveryConfig { int scan_concurrency{100}; ///< bounded, polite concurrent connect count int identify_timeout_ms{6000}; - /// Re-scan cadence. 0 = one-shot at startup (the only mode implemented in - /// this iteration); a positive value is accepted and validated but periodic - /// re-scan is a documented follow-up. + /// Re-scan cadence, in seconds, while no OPC-UA session is established. 0 + /// selects the built-in default (see OpcuaPlugin::effective_rescan_interval_s). + /// The startup scan always runs once. The cadence only governs how often the + /// disconnected reconnect loop scans again, so a gateway that started before + /// its PLC finished booting adopts the PLC when it appears instead of retrying + /// the fallback endpoint forever. Never used once an endpoint is configured + /// explicitly, and never while a session is up. int interval_s{0}; /// Only auto-register endpoints that expose a None + Anonymous endpoint (what diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index e77a3a519..95d2415e3 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -31,10 +31,12 @@ #include #include +#include #include #include #include #include +#include #include #include #include @@ -141,6 +143,42 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, static void apply_auto_alarms_param(const nlohmann::json & value, AutoAlarmsConfig & cfg, const std::function & warn); + // Run one read-only discovery pass and return the endpoint URL to adopt. + // + // Returns nullopt - meaning "keep the endpoint you have" - when discovery is + // disabled, when an endpoint was configured explicitly, when no subnet could + // be resolved, or when the pass found no auto-connectable None/Anonymous data + // server. Both the startup scan and the reconnect rescan go through here, so + // the two cannot drift apart. Static with injected probes and log sinks so + // both are unit-testable without a network. + // + // @param config discovery configuration (subnets, ports, timeouts, ...) + // @param endpoint_configured true when the operator pinned endpoint_url, so + // discovery then selects nothing and can neither override the + // operator's target nor open a second session on an already polled PLC + // @param scan injected TCP port probe + // @param identify injected OPC-UA GetEndpoints identify + // @param log_info operator-visible info sink + // @param log_warn operator-visible warning sink + static std::optional discover_endpoint(const OpcuaDiscoveryConfig & config, bool endpoint_configured, + const PortScanFn & scan, const IdentifyFn & identify, + const std::function & log_info, + const std::function & log_warn); + + // Seconds between reconnect rescans, or 0 when the reconnect loop must never + // rescan (discovery disabled, or an endpoint configured explicitly). A + // configured ``interval_s`` wins. interval_s = 0 means "discovery is on but no + // cadence was stated" and takes the built-in default rather than never + // rescanning: a config-less deployment is the one that cannot name a cadence + // and the one that most needs its PLC adopted once it finishes booting. + static int effective_rescan_interval_s(const OpcuaDiscoveryConfig & config, bool endpoint_configured); + + // Default reconnect rescan cadence, in seconds, when discovery is enabled with + // no explicit ``interval_s``. Long enough that a bounded subnet sweep stays a + // background cost on the poll thread, short enough that a PLC finishing its + // boot is picked up in well under a minute. + static constexpr int kDefaultRescanIntervalS = 30; + private: // Route handlers void handle_plc_data(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); @@ -160,6 +198,14 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, const std::string & severity_str, const std::string & message); void send_clear_fault(const std::string & fault_code); + // Clear PLC_COMMS_LOST after the initial connect in set_context() succeeded. + // Unconditional on purpose: the fault manager keys faults by fault_code and + // persists them, so a comms-lost fault raised before a gateway restart is + // still standing in the store while this process has no memory of raising it. + // The poller's own reconnect clear can never reach that case, because a + // successful first connect means the reconnect arm is never entered. + void clear_comms_lost_on_connect(); + // Dispatch now if the fault_manager service is matched, else buffer the // dispatch (bounded, order-preserving) to be flushed once it appears. void send_or_buffer(std::function dispatch); @@ -206,6 +252,14 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // an endpoint is already configured. void run_startup_discovery(); + // Poll-thread hook bound into PollerConfig::rediscover_endpoint whenever + // discovery runs without a configured endpoint. Called from the poller's + // reconnect arm, so only while no session is up, and rate-limited to one scan + // per effective_rescan_interval_s(). Returns the newly selected endpoint when + // a rescan found a different server (and logs the swap at INFO), nullopt when + // the rescan is not due yet or changed nothing. + std::optional rescan_endpoint_for_reconnect(); + // Build JSON response for data endpoint nlohmann::json build_data_response(const std::string & entity_id) const; @@ -231,6 +285,10 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // a network. Set in configure(), consumed in run_startup_discovery(). PortScanFn discovery_scan_fn_; IdentifyFn discovery_identify_fn_; + // When the last discovery pass ran, so the reconnect rescan honours the + // cadence instead of sweeping the subnet on every reconnect attempt. Stamped + // by the startup scan, then only ever read/written on the poll thread. + std::chrono::steady_clock::time_point last_discovery_scan_{}; std::unique_ptr client_; NodeMap node_map_; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp index 711c3ec47..73f670f72 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -133,8 +134,21 @@ struct PollerConfig { /// fire-and-forget report is never dropped-and-forgotten while the sink is /// unmatched - it retries on the next poll instead. Empty => assume ready. std::function report_sink_ready; + /// Optional endpoint rediscovery, bound by a plugin running config-less + /// network discovery with no endpoint configured. Called from the reconnect + /// arm - so only while no session is up - and expected to rate-limit itself. + /// Returns a new endpoint URL to reconnect against, nullopt to keep the + /// current one. Without it the reconnect loop retries the same endpoint + /// forever, which strands a gateway that scanned before its PLC had booted. + std::function()> rediscover_endpoint; }; +/// Fault code of the component-scoped OPC-UA connection fault the poller raises +/// on a sustained outage and clears on the next successful connect (issue #496). +/// Named here so the plugin can clear the same code from its own connect path +/// without keeping a second copy of the literal. +inline constexpr const char * kCommsLostFaultCode = "PLC_COMMS_LOST"; + /// Manages OPC-UA data collection via subscriptions (preferred) or polling class OpcuaPoller { public: @@ -237,6 +251,17 @@ class OpcuaPoller { std::chrono::steady_clock::time_point down_since, std::chrono::steady_clock::time_point now, std::chrono::milliseconds debounce); + /// Endpoint the next reconnect attempt should target. Asks + /// ``rediscover_endpoint`` (when bound) for a freshly discovered server and + /// returns it only when it names a DIFFERENT endpoint than ``current``. + /// nullopt means "keep the current one", which is also the answer when no + /// callback is bound, when the callback declines, or when it hands back an + /// empty string. Pure and static (the callback is injected) so the adoption + /// rule is unit-testable without a network. + static std::optional + adopt_rediscovered_endpoint(const std::string & current, + const std::function()> & rediscover); + /// Zero-config native A&C (``auto_alarms``): the alarm sources that should /// actually be subscribed / replayed, i.e. every explicit ``event_alarms`` /// entry plus (when ``auto_cfg.enabled`` and no explicit entry already diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index aebb164f9..884d599ae 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -585,6 +585,7 @@ void OpcuaPlugin::set_context(PluginContext & context) { const bool connected = client_->connect(client_config_); if (connected) { log_info("Connected to OPC-UA server: " + client_config_.endpoint_url); + clear_comms_lost_on_connect(); } else { log_warn("Failed to connect to OPC-UA server: " + client_config_.endpoint_url); } @@ -660,6 +661,16 @@ void OpcuaPlugin::set_context(PluginContext & context) { poller_config_.report_sink_ready = [this]() { return fault_clients_->report && fault_clients_->report->service_is_ready(); }; + // Config-less discovery with no configured endpoint: let the poller's + // reconnect arm ask for a fresh scan while it is down. Without this the + // startup scan is the only one that ever runs, so a gateway that scanned + // while its PLC was still booting retries the fallback endpoint forever and + // only a restart finds the PLC. + if (effective_rescan_interval_s(discovery_config_, endpoint_configured_) > 0) { + poller_config_.rediscover_endpoint = [this]() { + return rescan_endpoint_for_reconnect(); + }; + } poller_->start(poller_config_); log_info("OPC-UA poller started (mode: " + std::string(poller_->using_subscriptions() ? "subscription" : "poll") + ")"); @@ -733,7 +744,11 @@ IntrospectionResult OpcuaPlugin::introspect(const IntrospectionInput & /*input*/ // Fault scope grants bare-id ownership only to external entities; the poller // reports PLC_COMMS_LOST under this component's own id. comp.external = true; - comp.description = "PLC runtime connected at " + client_config_.endpoint_url; + // Read the endpoint off the client, not off client_config_: a reconnect + // rescan can adopt a different server after startup, and the client is the + // one that holds the endpoint actually being connected to. + const std::string live_endpoint = client_ ? client_->endpoint_url() : client_config_.endpoint_url; + comp.description = "PLC runtime connected at " + live_endpoint; // INV2: fill the asset-identity nameplate from the live server's device-info // (ServerStatus/BuildInfo + optional OPC-UA DI nameplate). Read once per @@ -744,7 +759,7 @@ IntrospectionResult OpcuaPlugin::introspect(const IntrospectionInput & /*input*/ if (client_ && client_->is_connected()) { const uint64_t session_generation = client_->connection_generation(); if (session_generation != device_identity_generation_) { - device_identity_ = opcua_device_info_to_identity(client_->read_device_info(), client_config_.endpoint_url); + device_identity_ = opcua_device_info_to_identity(client_->read_device_info(), live_endpoint); device_identity_generation_ = session_generation; if (!device_identity_.empty()) { log_info("Populated asset identity from OPC-UA device-info (manufacturer='" + device_identity_.manufacturer + @@ -1253,6 +1268,17 @@ void OpcuaPlugin::send_clear_fault(const std::string & fault_code) { }); } +void OpcuaPlugin::clear_comms_lost_on_connect() { + if (!poller_config_.comms_lost_fault_enabled) { + return; + } + // ClearFault is idempotent from this side: send_clear_fault is + // fire-and-forget, so a "Fault not found" answer for a code that was never + // raised costs nothing here and is the normal case on a healthy start. + log_info(std::string("OPC-UA connection established; clearing any standing ") + kCommsLostFaultCode); + send_clear_fault(kCommsLostFaultCode); +} + void OpcuaPlugin::send_or_buffer(std::function dispatch) { // Bound the buffer so a deployment with no fault_manager cannot grow it // without limit; drop the oldest (least relevant) pending dispatch. @@ -1466,35 +1492,39 @@ void OpcuaPlugin::log_security_profile() const { } } -void OpcuaPlugin::run_startup_discovery() { - if (!discovery_config_.enabled) { - return; +int OpcuaPlugin::effective_rescan_interval_s(const OpcuaDiscoveryConfig & config, bool endpoint_configured) { + if (!config.enabled || endpoint_configured) { + return 0; + } + return config.interval_s > 0 ? config.interval_s : kDefaultRescanIntervalS; +} + +std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryConfig & config, bool endpoint_configured, + const PortScanFn & scan, const IdentifyFn & identify, + const std::function & log_info, + const std::function & log_warn) { + if (!config.enabled) { + return std::nullopt; } // Never override an explicitly configured endpoint: discovery must not open a // second session on a PLC the operator already targets (and already polls). - if (endpoint_configured_) { - log_info("OPC-UA discovery enabled but endpoint_url is explicitly configured (" + client_config_.endpoint_url + - "); skipping auto-discovery to avoid a second session."); - return; - } - if (discovery_config_.interval_s > 0) { - log_warn("OPC-UA discovery interval_s=" + std::to_string(discovery_config_.interval_s) + - " set, but periodic re-scan is not implemented yet; running a one-shot scan at startup."); + if (endpoint_configured) { + return std::nullopt; } - NetworkDiscovery discovery(discovery_config_, discovery_scan_fn_, discovery_identify_fn_); + NetworkDiscovery discovery(config, scan, identify); const auto subnets = discovery.resolve_subnets(); if (subnets.empty()) { log_warn("OPC-UA discovery: no subnet configured and could not derive a local /24; nothing to scan."); - return; + return std::nullopt; } std::string subnet_list; for (const auto & s : subnets) { subnet_list += (subnet_list.empty() ? "" : ", ") + s; } log_info("OPC-UA discovery: read-only active scan of [" + subnet_list + "] on " + - std::to_string(discovery_config_.ports.size()) + " port(s)..."); + std::to_string(config.ports.size()) + " port(s)..."); const std::vector found = discovery.run(); @@ -1528,18 +1558,92 @@ void OpcuaPlugin::run_startup_discovery() { std::to_string(discovery_servers) + " discovery server(s)/LDS, " + std::to_string(secured_only) + " secured-only (need credentials), " + std::to_string(leads) + " non-OPC-UA/unidentified lead(s)."); - const DiscoveredEndpoint * chosen = - NetworkDiscovery::select_auto_endpoint(found, discovery_config_.anonymous_none_only); + const DiscoveredEndpoint * chosen = NetworkDiscovery::select_auto_endpoint(found, config.anonymous_none_only); if (chosen == nullptr) { log_warn( - "OPC-UA discovery: no auto-connectable None/Anonymous data server found; leaving endpoint at default. " + "OPC-UA discovery: no auto-connectable None/Anonymous data server found; leaving the endpoint unchanged. " "Secured-only servers require operator credentials."); + return std::nullopt; + } + + log_info("OPC-UA discovery: selected endpoint " + chosen->endpoint_url + " (uri='" + chosen->application_uri + "')"); + return chosen->endpoint_url; +} + +void OpcuaPlugin::run_startup_discovery() { + if (!discovery_config_.enabled) { return; } + if (endpoint_configured_) { + log_info("OPC-UA discovery enabled but endpoint_url is explicitly configured (" + client_config_.endpoint_url + + "); skipping auto-discovery to avoid a second session."); + return; + } + + // Stamp the scan before running it: the rescan cadence measures the gap + // between the START of two sweeps, so a slow sweep does not immediately earn + // another one. + last_discovery_scan_ = std::chrono::steady_clock::now(); + const auto chosen = discover_endpoint( + discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, + [this](const std::string & m) { + log_info(m); + }, + [this](const std::string & m) { + log_warn(m); + }); + + if (!chosen) { + // The startup scan can legitimately find nothing - a gateway that boots + // alongside its PLC routinely scans while the PLC is still coming up. The + // endpoint stays at its default and the poller's reconnect arm rescans on + // the cadence below, so this is a delay rather than a dead end. + log_info("OPC-UA discovery: startup scan selected no endpoint; the reconnect loop rescans every " + + std::to_string(effective_rescan_interval_s(discovery_config_, endpoint_configured_)) + "s while down."); + return; + } + + client_config_.endpoint_url = *chosen; + log_info("OPC-UA discovery: auto-selected endpoint " + *chosen + " - handing to the connect + introspect path."); +} + +std::optional OpcuaPlugin::rescan_endpoint_for_reconnect() { + // A sweep is a bounded but multi-second blocking call on the poll thread, and + // stop() has to wait for whatever it is in the middle of. Do not start one the + // shutdown is going to throw away. + if (shutdown_requested_.load()) { + return std::nullopt; + } + const int interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); + if (interval_s <= 0) { + return std::nullopt; + } + + const auto now = std::chrono::steady_clock::now(); + if (now - last_discovery_scan_ < std::chrono::seconds(interval_s)) { + return std::nullopt; + } + last_discovery_scan_ = now; + + const auto chosen = discover_endpoint( + discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, + [this](const std::string & m) { + log_info(m); + }, + [this](const std::string & m) { + log_warn(m); + }); + // The live client config, not client_config_: this runs on the poll thread + // and client_config_ is read by the refresh thread in introspect(). The + // client owns the endpoint once connect() has been called with it, and its + // accessors are mutex-guarded. + const std::string current = client_ ? client_->endpoint_url() : client_config_.endpoint_url; + if (!chosen || *chosen == current) { + return std::nullopt; + } - client_config_.endpoint_url = chosen->endpoint_url; - log_info("OPC-UA discovery: auto-selected endpoint " + chosen->endpoint_url + " (uri='" + chosen->application_uri + - "') - handing to the connect + introspect path."); + log_info("OPC-UA discovery: rescan while disconnected adopted endpoint " + *chosen + " (was " + current + ")"); + return chosen; } nlohmann::json OpcuaPlugin::build_data_response(const std::string & entity_id) const { diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp index c24f9ca0b..d972a3e05 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp @@ -1142,9 +1142,22 @@ bool OpcuaPoller::comms_lost_should_raise(bool enabled, bool already_raised, return (now - down_since) >= debounce; } +std::optional +OpcuaPoller::adopt_rediscovered_endpoint(const std::string & current, + const std::function()> & rediscover) { + if (!rediscover) { + return std::nullopt; + } + const std::optional found = rediscover(); + if (!found || found->empty() || *found == current) { + return std::nullopt; + } + return found; +} + void OpcuaPoller::emit_comms_lost(bool active) { ros2_medkit::fault_detection::FaultSignal signal; - signal.fault_code = "PLC_COMMS_LOST"; + signal.fault_code = kCommsLostFaultCode; signal.severity = config_.comms_lost_severity; signal.message = active ? ("OPC-UA connection lost to " + client_.endpoint_url()) : ("OPC-UA connection restored to " + client_.endpoint_url()); @@ -1173,15 +1186,32 @@ void OpcuaPoller::poll_loop() { comms_down_since_ = std::chrono::steady_clock::now(); } - // Attempt reconnect with original config (preserves timeout, etc.) - if (client_.connect(client_.current_config())) { + // Reconnect with the original config (preserves timeout, security, ...). + // The endpoint is the one exception: when a rediscovery callback is bound + // and offers a different server, adopt it for this attempt. connect() + // stores the config it is given, so current_config() carries the adopted + // endpoint from here on and every later retry targets the new server. + OpcuaClientConfig reconnect_config = client_.current_config(); + if (auto adopted = adopt_rediscovered_endpoint(reconnect_config.endpoint_url, config_.rediscover_endpoint)) { + reconnect_config.endpoint_url = *adopted; + // A freshly discovered server deserves a prompt attempt: without this + // reset the backoff (up to 60 s) would keep the newly found PLC waiting + // for as long as the old dead endpoint had earned. + reconnect_wait = config_.reconnect_interval; + } + + if (client_.connect(reconnect_config)) { reconnect_wait = config_.reconnect_interval; // reset on success - // Issue #496: connection restored - clear the comms-lost fault if it - // was raised, then reset the debounce timer. - if (comms_lost_raised_) { + // Issue #496: connection restored - clear the comms-lost fault. Sent on + // EVERY successful reconnect, not only when this process raised it: the + // fault manager keys faults by fault_code and persists them, so a fault + // raised before a restart is standing in the store with nothing in + // memory to remember it. The clear is fire-and-forget and the store + // answers "not found" harmlessly when there is nothing to clear. + if (config_.comms_lost_fault_enabled) { emit_comms_lost(/*active=*/false); - comms_lost_raised_ = false; } + comms_lost_raised_ = false; comms_down_since_.reset(); if (config_.prefer_subscriptions) { setup_subscriptions(); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp index 48294503f..93b84ee1f 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp @@ -12,15 +12,22 @@ // See the License for the specific language governing permissions and // limitations under the License. -// INV2 end-to-end (no HW): boot the test_alarm_server OPC-UA fixture, connect, -// and prove the asset-identity nameplate is filled from the server's device-info -// (ServerStatus/BuildInfo + the OPC-UA DI DeviceSet nameplate) with no manual -// entry. Exercises both the raw OpcuaClient::read_device_info read and the full -// OpcuaPlugin::introspect() path that lands identity on the SOVD Component. +// End-to-end against a live OPC-UA server (no HW): boot the test_alarm_server +// fixture and exercise the paths that only a real session can reach. +// +// INV2 identity: prove the asset-identity nameplate is filled from the server's +// device-info (ServerStatus/BuildInfo + the OPC-UA DI DeviceSet nameplate) with +// no manual entry, through both the raw OpcuaClient::read_device_info read and +// the full OpcuaPlugin::introspect() path that lands identity on the SOVD +// Component. +// +// Connection lifecycle: prove a successful connect clears the standing +// PLC_COMMS_LOST fault, which needs a connect that actually succeeds. #include "ros2_medkit_opcua/device_identity.hpp" #include "ros2_medkit_opcua/opcua_client.hpp" #include "ros2_medkit_opcua/opcua_plugin.hpp" +#include "ros2_medkit_opcua/opcua_poller.hpp" #include @@ -33,13 +40,16 @@ #include #include +#include #include #include #include #include +#include #include #include #include +#include #include #include @@ -559,4 +569,66 @@ TEST_F(OpcuaIdentityE2ETest, DiNameplateReadFollowsBrowseContinuationPoints) { client.disconnect(); } +// A gateway that restarts after a comms outage never raised PLC_COMMS_LOST in +// THIS process, yet the fault manager keys faults by fault_code alone and +// persists them, so the fault raised before the restart is still standing. +// The reconnect arm used to clear only when its own in-memory +// ``comms_lost_raised_`` flag was set, which no restart can satisfy, so the +// fault stayed CONFIRMED for good. The clear now goes out on every successful +// connect. Driven against the live fixture because the arm can only be reached +// by a connect that actually succeeds. +TEST_F(OpcuaIdentityE2ETest, SuccessfulConnectClearsCommsLostNeverRaisedHere) { + OpcuaClient client; + OpcuaClientConfig config; + config.endpoint_url = endpoint_; + config.connect_timeout = std::chrono::milliseconds(5000); + // Connect once to seed the client's stored config (what the poller reconnects + // with), then drop the session so the poll loop starts in its reconnect arm - + // the state a freshly started gateway is in while the PLC is already up. + ASSERT_TRUE(client.connect(config)); + client.disconnect(); + ASSERT_FALSE(client.is_connected()); + + NodeMap node_map; // config-less: no entries, nothing to poll + OpcuaPoller poller(client, node_map); + + std::mutex signals_mutex; + std::vector> signals; // (fault_code, active) + poller.set_alarm_callback( + [&signals_mutex, &signals](const std::string &, const ros2_medkit::fault_detection::FaultSignal & signal) { + std::lock_guard lock(signals_mutex); + signals.emplace_back(signal.fault_code, signal.active); + }); + + PollerConfig poller_config; + poller_config.poll_interval = std::chrono::milliseconds(100); + poller_config.reconnect_interval = std::chrono::milliseconds(100); + poller_config.comms_lost_fault_enabled = true; + poller.start(poller_config); + + bool cleared = false; + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(15); + while (!cleared && std::chrono::steady_clock::now() < deadline) { + { + std::lock_guard lock(signals_mutex); + cleared = std::find(signals.begin(), signals.end(), std::make_pair(std::string(kCommsLostFaultCode), false)) != + signals.end(); + } + if (!cleared) { + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + } + poller.stop(); + + EXPECT_TRUE(cleared) << "a successful connect must clear PLC_COMMS_LOST even when this process never raised it"; + + // Absence control on the same harness: the connect succeeded, so nothing may + // have RAISED the fault. Without this a clear-everything-always regression + // would still pass the assertion above. + std::lock_guard lock(signals_mutex); + EXPECT_EQ(std::find(signals.begin(), signals.end(), std::make_pair(std::string(kCommsLostFaultCode), true)), + signals.end()) + << "comms-lost must not be raised while the connection is up"; +} + } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index 4bfdf4bc6..f47108005 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -27,7 +27,11 @@ #include #include #include +#include +#include #include +#include +#include #include #include #include @@ -638,6 +642,143 @@ TEST(CommsLostShouldRaise, IdempotentAndDisabled) { EXPECT_FALSE(OpcuaPoller::comms_lost_should_raise(/*enabled=*/false, false, t0, late, debounce)); } +// --------------------------------------------------------------------------- +// Endpoint rediscovery while disconnected (config-less discovery) +// --------------------------------------------------------------------------- + +namespace { + +// Fake port scanner backed by a set of open "ip:port" hosts. +PortScanFn fake_scan(std::set open) { + return [open = std::move(open)](const std::string & ip, uint16_t port, int) { + return open.count(ip + ":" + std::to_string(port)) > 0; + }; +} + +// Fake GetEndpoints identify keyed by connect URL. Anything else is unreachable. +IdentifyFn fake_identify(std::map table) { + return [table = std::move(table)](const std::string & url, int) -> IdentifyResult { + const auto it = table.find(url); + if (it != table.end()) { + return it->second; + } + IdentifyResult r; + r.error = "unreachable"; + return r; + }; +} + +IdentifyResult plc_identity() { + IdentifyResult r; + r.ok = true; + r.advertised_url = "opc.tcp://192.168.1.10:4840"; + r.application_uri = "urn:SIMATIC.S7-1500.OPC-UA.Application:Software PLC_1"; + r.product_uri = "https://www.siemens.com/s7-1500"; + r.application_name = "SIMATIC.S7-1500"; + r.application_type = 0; // Server + r.security_policies = {{"None", 1}}; + r.anonymous_none_available = true; + return r; +} + +OpcuaDiscoveryConfig rescan_cfg() { + OpcuaDiscoveryConfig cfg; + cfg.enabled = true; + cfg.subnets = {"192.168.1.0/24"}; // explicit, so no local-interface derivation + cfg.ports = {4840}; + return cfg; +} + +// Discards log output. The tests assert on the selected endpoint, not the text. +const std::function kSilent = [](const std::string &) {}; + +} // namespace + +TEST(DiscoverEndpoint, ScanBeforeThePlcIsUpSelectsNothingAndALaterRescanAdoptsIt) { + // The field race: the gateway scans 2 s after start while the PLC is still + // booting. Nothing answers, so nothing is selected and the caller keeps the + // default endpoint. + const auto empty_pass = OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({}), + fake_identify({}), kSilent, kSilent); + EXPECT_FALSE(empty_pass.has_value()); + + // The PLC finishes booting. The same call with the same config now finds it, + // which is what the reconnect arm applies to the next connect attempt. + const auto later_pass = OpcuaPlugin::discover_endpoint( + rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), kSilent, kSilent); + ASSERT_TRUE(later_pass.has_value()); + EXPECT_EQ(*later_pass, "opc.tcp://192.168.1.10:4840"); +} + +TEST(DiscoverEndpoint, AnExplicitEndpointIsNeverRescanned) { + // Positive control: the very scan that DOES find a server above finds the + // same server here, and is still refused because the operator pinned an + // endpoint. Discovery must not open a second session on a polled PLC. + const auto chosen = OpcuaPlugin::discover_endpoint( + rescan_cfg(), /*endpoint_configured=*/true, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), kSilent, kSilent); + EXPECT_FALSE(chosen.has_value()); +} + +TEST(DiscoverEndpoint, DisabledDiscoveryScansNothing) { + OpcuaDiscoveryConfig cfg = rescan_cfg(); + cfg.enabled = false; + bool scanned = false; + auto counting_scan = [&scanned](const std::string &, uint16_t, int) { + scanned = true; + return true; + }; + const auto chosen = OpcuaPlugin::discover_endpoint(cfg, /*endpoint_configured=*/false, counting_scan, + fake_identify({}), kSilent, kSilent); + EXPECT_FALSE(chosen.has_value()); + EXPECT_FALSE(scanned) << "a disabled discovery must not touch the network"; +} + +TEST(EffectiveRescanInterval, DefaultsWhenDiscoveryIsOnWithNoCadenceAndIsOffOtherwise) { + OpcuaDiscoveryConfig cfg = rescan_cfg(); + // Config-less: discovery on, no interval stated -> the built-in cadence, not + // "never rescan". This is the deployment that most needs the rescan. + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(cfg, /*endpoint_configured=*/false), + OpcuaPlugin::kDefaultRescanIntervalS); + // An operator-stated cadence wins. + cfg.interval_s = 120; + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(cfg, false), 120); + // An explicit endpoint, or discovery off, means no rescan at all. + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(cfg, /*endpoint_configured=*/true), 0); + cfg.enabled = false; + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(cfg, false), 0); +} + +TEST(AdoptRediscoveredEndpoint, AdoptsOnlyADifferentNonEmptyUrl) { + const std::string current = "opc.tcp://localhost:4840"; + + // No callback bound (an explicit endpoint, or discovery off) -> keep current. + EXPECT_FALSE(OpcuaPoller::adopt_rediscovered_endpoint(current, nullptr).has_value()); + + // Rescan not due, or found nothing -> keep current. + EXPECT_FALSE(OpcuaPoller::adopt_rediscovered_endpoint(current, [] { + return std::optional{}; + }).has_value()); + + // Same server as before -> nothing to adopt, so no needless reconnect churn. + EXPECT_FALSE(OpcuaPoller::adopt_rediscovered_endpoint(current, [¤t] { + return std::optional{current}; + }).has_value()); + + // An empty URL is not an endpoint. + EXPECT_FALSE(OpcuaPoller::adopt_rediscovered_endpoint(current, [] { + return std::optional{""}; + }).has_value()); + + // A different server -> adopt it for the next connect attempt. + const auto adopted = OpcuaPoller::adopt_rediscovered_endpoint(current, [] { + return std::optional{"opc.tcp://192.168.1.10:4840"}; + }); + ASSERT_TRUE(adopted.has_value()); + EXPECT_EQ(*adopted, "opc.tcp://192.168.1.10:4840"); +} + // Issue #478 safety-gate: an empty scan from a source that has NEVER yielded a // condition instance node (EventNotifier-only server, e.g. S7-1500) must NOT // clear the still-active tracked fault. This is the single most important From 67b072a7619143072ba5d472f7d537db73c82ded Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 12:43:01 +0200 Subject: [PATCH 2/8] fix(gateway): say where a plugin entity's freeze-frame values came from A freeze-frame captured for a plugin-backed entity (#564) reaches a client with an empty topic and an empty message_type. That is correct, the values are the plugin's live entity data and not a ROS message, but it leaves the snapshot with no field at all naming its origin. Two of these frames from different bridges are indistinguishable, and a frame is indistinguishable from a topic capture whose metadata went missing. Frame now carries the capture path that read it, and it is served as x-medkit.source: plugin_data_provider for a read through the owning plugin's DataProvider, plugin_x_plc_data_route for the in-process dispatch of the plugin's own x-plc-data route (bridges that export no DataProvider). topic and message_type are left empty rather than overloaded, since neither names a ROS topic here. The field is omitted, not emptied, when the capture named no path, so a fault-manager freeze-frame taken from a real topic is unaffected and carries its topic and message_type as before. --- docs/tutorials/snapshots.rst | 21 ++++++++ .../entity_freeze_frame_capture.hpp | 19 +++++++- .../src/entity_freeze_frame_capture.cpp | 7 +-- .../src/http/handlers/fault_handlers.cpp | 13 +++++ .../test/test_entity_freeze_frame_capture.cpp | 31 ++++++++++++ .../test/test_fault_handlers.cpp | 48 +++++++++++++++++++ 6 files changed, 134 insertions(+), 5 deletions(-) diff --git a/docs/tutorials/snapshots.rst b/docs/tutorials/snapshots.rst index f17d55d7e..c23e845ef 100644 --- a/docs/tutorials/snapshots.rst +++ b/docs/tutorials/snapshots.rst @@ -216,6 +216,26 @@ with: ros2 run ros2_medkit_gateway gateway_node --ros-args \ -p entity_freeze_frame.enabled:=false +A plugin entity's values are not a ROS message, so ``topic`` and +``message_type`` are empty on these frames. ``x-medkit.source`` names the +capture path instead, so a consumer can still tell where the values came +from: + +.. list-table:: + :header-rows: 1 + :widths: 30 70 + + * - ``x-medkit.source`` + - Meaning + * - ``plugin_data_provider`` + - Read through the owning plugin's ``DataProvider::list_data``. + * - ``plugin_x_plc_data_route`` + - Read by dispatching the owning plugin's own ``x-plc-data`` route + in-process (plugins that export no ``DataProvider``). + +The field is absent on freeze-frames captured by the fault manager from a ROS +topic. Those carry a real ``topic`` and ``message_type`` instead. + Example plugin-entity freeze-frame in the fault response: .. code-block:: json @@ -227,6 +247,7 @@ Example plugin-entity freeze-frame in the fault response: "x-medkit": { "topic": "", "message_type": "", + "source": "plugin_x_plc_data_route", "full_data": {"tank_level": 87.5, "pump_running": true}, "captured_at": "2026-07-14T12:00:00.000Z" } diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/entity_freeze_frame_capture.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/entity_freeze_frame_capture.hpp index 7dc081dc0..d3c041ee4 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/entity_freeze_frame_capture.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/entity_freeze_frame_capture.hpp @@ -60,6 +60,13 @@ namespace ros2_medkit_gateway { */ class EntityFreezeFrameCapture { public: + /// Capture-path identifiers stored in Frame::source and served as + /// ``x-medkit.source``. The plugin's own DataProvider, and the in-process + /// dispatch of the plugin's `x-plc-data` route for plugins that export no + /// DataProvider. + static constexpr const char * kSourceDataProvider = "plugin_data_provider"; + static constexpr const char * kSourceXPlcDataRoute = "plugin_x_plc_data_route"; + /// One captured frame: the entity's data values at fault-confirm time. /// captured_at_ns dates the capture, not the values - a disconnected entity /// serves its last known values, whose age is bounded only by the outage. @@ -72,6 +79,13 @@ class EntityFreezeFrameCapture { bool startup_catchup{false}; std::optional connected; ///< payload's top-level link flag, when reported nlohmann::json source_timestamp; ///< payload's own "timestamp" field verbatim (null when absent) + /// Which capture path read the values (kSourceDataProvider / + /// kSourceXPlcDataRoute), served as ``x-medkit.source``. These values are + /// entity data, not a ROS message, so ``topic`` and ``message_type`` are + /// empty on the wire and would otherwise leave a consumer with nothing at + /// all saying where the numbers came from. Empty when the caller named no + /// path. + std::string source; }; /// Resolves an entity id to its owning plugin's DataProvider (nullptr when @@ -187,9 +201,10 @@ class EntityFreezeFrameCapture { bool capture_for_event(const ros2_medkit_msgs::msg::FaultEvent & event, bool startup_catchup = false); /// Build a frame from list-data-shaped content, enforcing the shared - /// no-row-of-nulls invariant on both capture paths. + /// no-row-of-nulls invariant on both capture paths. @p source names the path + /// that read the content and is stored verbatim in Frame::source. std::optional frame_from_content(const std::string & entity_id, const std::string & fault_code, - const nlohmann::json & content); + const nlohmann::json & content, const std::string & source); /// Capture via the plugin's own x-plc-data route (no DataProvider exported). /// Returns nullopt when the route yields nothing usable. diff --git a/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp b/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp index fe060826c..01bde9674 100644 --- a/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp +++ b/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp @@ -188,13 +188,14 @@ EntityFreezeFrameCapture::standing_faults_from_list_reply(const nlohmann::json & std::optional EntityFreezeFrameCapture::frame_from_content(const std::string & entity_id, const std::string & fault_code, - const nlohmann::json & content) { + const nlohmann::json & content, const std::string & source) { if (!content_has_live_data(content)) { log_fallback_failure_once(fault_code, "entity '" + entity_id + "' returned no data items"); return std::nullopt; } Frame frame; frame.entity_id = entity_id; + frame.source = source; frame.values = values_from_list_content(content); if (!values_have_data(frame.values)) { // Items present but nothing usable in them (all-null values, or no usable @@ -228,7 +229,7 @@ EntityFreezeFrameCapture::capture_via_route(const std::string & entity_id, const if (!content) { return std::nullopt; // not plugin-owned, no x-plc-data route, or handler error } - return frame_from_content(entity_id, fault_code, *content); + return frame_from_content(entity_id, fault_code, *content, kSourceXPlcDataRoute); } void EntityFreezeFrameCapture::log_fallback_failure_once(const std::string & fault_code, const std::string & message) { @@ -410,7 +411,7 @@ bool EntityFreezeFrameCapture::capture_for_event(const ros2_medkit_msgs::msg::Fa log_fallback_failure_once(fault_code, "list_data('" + source + "') failed: " + result.error().message); continue; } - if (auto frame = frame_from_content(source, fault_code, result->content)) { + if (auto frame = frame_from_content(source, fault_code, result->content, kSourceDataProvider)) { frames.push_back(std::move(*frame)); } } catch (const std::exception & e) { diff --git a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp index b235acc53..c7282fd7b 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp @@ -263,6 +263,13 @@ json FaultHandlers::merge_entity_freeze_frames(json env_data, snap["topic"] = ""; // entity data values, not a ROS topic snap["message_type"] = ""; snap["captured_at_ns"] = frame.captured_at_ns; + // Capture provenance. topic/message_type stay empty because these values + // are not a ROS message, which leaves "source" as the only field naming + // where the numbers came from - so carry it whenever the capture named a + // path. + if (!frame.source.empty()) { + snap["source"] = frame.source; + } if (frame.startup_catchup) { // Values were read at gateway start, not when the fault confirmed; // absent marker = captured on the confirm edge. @@ -344,6 +351,12 @@ dto::FaultDetail FaultHandlers::build_sovd_fault_response(const json & fault_jso snap["x-medkit"]["capture_origin"] = s["capture_origin"]; } // Entity-frame provenance (merge_entity_freeze_frames), only when known. + // "source" names the capture path (a plugin DataProvider or the + // plugin's x-plc-data route). A consumer reads it instead of the + // empty topic/message_type an entity frame necessarily carries. + if (s.contains("source") && s["source"].is_string()) { + snap["x-medkit"]["source"] = s["source"]; + } if (s.contains("connected") && s["connected"].is_boolean()) { snap["x-medkit"]["connected"] = s["connected"]; } diff --git a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp index b9a44b2e0..152297ba0 100644 --- a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp +++ b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp @@ -787,6 +787,37 @@ TEST(MergeEntityFreezeFrames, AppendsWhenNoConfiguredFreezeFrame) { EXPECT_FALSE(snap.contains("capture_origin")); // confirm-edge frames carry no marker } +TEST(MergeEntityFreezeFrames, CarriesCapturePathAsSource) { + // An entity frame has no ROS topic, so topic/message_type are necessarily + // empty, so "source" is the only field left saying where they came from. + json env_data = {{"snapshots", json::array()}}; + EntityFreezeFrameCapture::Frame frame; + frame.entity_id = "plc_app"; + frame.values = {{"temperature", 42.5}}; + frame.captured_at_ns = 1234; + frame.source = EntityFreezeFrameCapture::kSourceXPlcDataRoute; + + auto merged = FaultHandlers::merge_entity_freeze_frames(env_data, {frame}); + ASSERT_EQ(merged["snapshots"].size(), 1u); + const auto & snap = merged["snapshots"][0]; + EXPECT_EQ(snap["source"], EntityFreezeFrameCapture::kSourceXPlcDataRoute); + EXPECT_EQ(snap["topic"], ""); + EXPECT_EQ(snap["message_type"], ""); +} + +TEST(MergeEntityFreezeFrames, OmitsSourceWhenTheCaptureNamedNoPath) { + // Absence control for the test above, on the same harness: a frame whose + // capture path is unknown must not have one invented for it. + json env_data = {{"snapshots", json::array()}}; + EntityFreezeFrameCapture::Frame frame; + frame.entity_id = "plc_app"; + frame.values = {{"temperature", 42.5}}; + + auto merged = FaultHandlers::merge_entity_freeze_frames(env_data, {frame}); + ASSERT_EQ(merged["snapshots"].size(), 1u); + EXPECT_FALSE(merged["snapshots"][0].contains("source")); +} + TEST(MergeEntityFreezeFrames, StartupCatchUpFrameCarriesCaptureOrigin) { json env_data = {{"snapshots", json::array()}}; EntityFreezeFrameCapture::Frame frame; diff --git a/src/ros2_medkit_gateway/test/test_fault_handlers.cpp b/src/ros2_medkit_gateway/test/test_fault_handlers.cpp index 9c7f08a91..29b53ef22 100644 --- a/src/ros2_medkit_gateway/test/test_fault_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_fault_handlers.cpp @@ -142,6 +142,54 @@ TEST_F(FaultHandlersTest, BuildSovdFaultResponsePropagatesCaptureOrigin) { EXPECT_EQ(snap["x-medkit"]["capture_origin"], "startup"); } +TEST_F(FaultHandlersTest, BuildSovdFaultResponseServesEntityFrameSource) { + // A plugin-captured entity frame reaches the wire with an empty topic and + // message_type (the values are not a ROS message) plus x-medkit.source + // naming the capture path that read them. + ros2_medkit_msgs::msg::Fault fault; + fault.fault_code = "PLC_ALARM"; + + json env_data = {{"snapshots", json::array({{{"type", "freeze_frame"}, + {"snapshot_type", "freeze_frame"}, + {"name", "plc_app"}, + {"data", R"({"tank_level": 87.5})"}, + {"topic", ""}, + {"message_type", ""}, + {"captured_at_ns", 1234}, + {"source", "plugin_x_plc_data_route"}}})}}; + + auto response = to_json(FaultHandlers::build_sovd_fault_response(fault_json(fault), env_data, "/apps/plc_app")); + + auto & snap = response["environment_data"]["snapshots"][0]; + EXPECT_EQ(snap["x-medkit"]["source"], "plugin_x_plc_data_route"); + EXPECT_EQ(snap["x-medkit"]["topic"], ""); + EXPECT_EQ(snap["x-medkit"]["message_type"], ""); +} + +TEST_F(FaultHandlersTest, BuildSovdFaultResponseOmitsSourceWhenTheSnapshotHasNone) { + // Absence control for the test above, on the same harness: a topic-captured + // freeze frame (the fault_manager's own) carries no source, and none is + // invented for it. + ros2_medkit_msgs::msg::Fault fault; + fault.fault_code = "TEMP_FAULT"; + + ros2_medkit_msgs::msg::EnvironmentData env_data; + ros2_medkit_msgs::msg::Snapshot freeze_frame; + freeze_frame.type = "freeze_frame"; + freeze_frame.name = "temperature"; + freeze_frame.data = R"({"temperature": 85.5})"; + freeze_frame.topic = "/motor/temperature"; + freeze_frame.message_type = "sensor_msgs/msg/Temperature"; + env_data.snapshots.push_back(freeze_frame); + + auto response = + to_json(FaultHandlers::build_sovd_fault_response(fault_json(fault), env_json(env_data), "/apps/motor")); + + auto & snap = response["environment_data"]["snapshots"][0]; + EXPECT_FALSE(snap["x-medkit"].contains("source")); + EXPECT_EQ(snap["x-medkit"]["topic"], "/motor/temperature"); +} + // Conversion layer must emit an explicit "snapshot_type" discriminator so // downstream consumers (handler, SSE, MCP) can dispatch on a single key // regardless of which optional payload fields are present. From f9e6aa55dc601543dd7be1a41538ebe763b93b2d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 12:43:01 +0200 Subject: [PATCH 3/8] fix(gateway): stop listing the gateway's own nodes as apps The gateway runs four nodes inside its own process, all named after itself: the gateway node, "_sub" for the subscription executor, "_fault_clients" for the fault-service transport, and "_lifecycle_state_reader" for the lifecycle reader. None of them starts with an underscore, so the ROS 2 hidden-node convention does not cover them, and runtime introspection returned all four as ordinary Apps. The gateway advertised its own plumbing as diagnosable entities, and an operator browsing /api/v1/apps saw four entries that answer nothing useful. There were two half-answers to the same question. count_peer_nodes knew the gateway's own FQN plus "_sub" and "_fault_clients" but not the lifecycle reader, and the app filter knew only the underscore rule, so it dropped none of the four. Both now go through one predicate, is_own_gateway_node, so a fifth helper is declared in one place instead of two. The match is exact per suffix, never a prefix test: a genuine peer named "_monitor" or "2" must stay visible, and hiding a real node is the worse error. A fault_manager sharing the process is not ours either and stays listed. Remote entities are left alone: a peer's helper nodes carry the same fully qualified names and are the peer's own filter's business. --- .../ros2_medkit_gateway/gateway_node.hpp | 30 ++++++- src/ros2_medkit_gateway/src/gateway_node.cpp | 47 ++++++++-- .../test/test_handler_context.cpp | 90 +++++++++++++++++-- 3 files changed, 150 insertions(+), 17 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp index a46a11bf5..d4585d99a 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp @@ -497,6 +497,27 @@ class GatewayNode : public rclcpp::Node { std::unique_ptr server_thread_; }; +/** + * @brief Is this node FQN the gateway's own, rather than a diagnosable peer? + * + * True for the gateway node itself and for the helper nodes it creates inside + * its own process: the subscription executor's `_sub`, the fault-service + * transport's `_fault_clients`, and the lifecycle reader's + * `_lifecycle_state_reader`. None of these begins with '_', so the ROS 2 + * hidden-node convention does not cover them and the gateway would otherwise + * count them as peers and list them as diagnosable Apps - reporting on itself. + * + * A fault_manager node sharing the process is NOT ours: it is a separate, + * diagnosable component and stays visible. + * + * Exact matches only. A prefix test would also claim a genuine peer named + * `_monitor` or `2`, and dropping a real node is the worse error. + * + * @param node_fqn Fully qualified node name to test ("/ns/node") + * @param self_fqn The gateway node's own FQN; an empty value matches nothing + */ +bool is_own_gateway_node(const std::string & node_fqn, const std::string & self_fqn); + /** * @brief Filter ROS 2 internal nodes from an app list * @@ -505,11 +526,18 @@ class GatewayNode : public rclcpp::Node { * before checking for the underscore prefix, using the routing table for precise * prefix detection. * + * Also removes local apps bound to one of the gateway's own nodes + * (is_own_gateway_node), which the underscore rule cannot see. The test is on + * the bound node FQN, and only for apps with no routing-table entry: a peer's + * helper nodes are the peer's business and are left to the peer's own filter. + * * @param apps App vector to filter in place * @param peer_routing_table Maps entity_id -> peer_name for remote entities + * @param self_fqn The gateway node's own FQN; empty disables the self check * @return Number of apps removed */ size_t filter_internal_node_apps(std::vector & apps, - const std::unordered_map & peer_routing_table); + const std::unordered_map & peer_routing_table, + const std::string & self_fqn); } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index a8f32986e..0d4e8c5ff 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -15,6 +15,7 @@ #include "ros2_medkit_gateway/gateway_node.hpp" #include +#include #include #include #include @@ -1596,6 +1597,27 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki }); } +bool is_own_gateway_node(const std::string & node_fqn, const std::string & self_fqn) { + if (self_fqn.empty() || node_fqn.empty()) { + return false; + } + if (node_fqn == self_fqn) { + return true; + } + // The helper nodes the gateway creates inside its own process, each named + // after this node plus a fixed suffix. Where each one is set: + // "_sub" Ros2SubscriptionExecutor::Config + // (subscription_node_name_suffix) + // "_fault_clients" Ros2FaultServiceTransport + // "_lifecycle_state_reader" Ros2LifecycleStateReader + // Exact matches only: a prefix test would also claim a genuine peer named + // "_monitor" or "2", and hiding a real node is the worse error. + static constexpr std::array kHelperSuffixes{"_sub", "_fault_clients", "_lifecycle_state_reader"}; + return std::any_of(kHelperSuffixes.begin(), kHelperSuffixes.end(), [&](const char * suffix) { + return node_fqn == self_fqn + suffix; + }); +} + size_t GatewayNode::count_peer_nodes(const std::vector> & nodes_and_namespaces, const std::string & self_fqn) { size_t count = 0; @@ -1608,10 +1630,7 @@ size_t GatewayNode::count_peer_nodes(const std::vector_monitor" or "2"). - if (fqn == self_fqn || fqn == self_fqn + "_sub" || fqn == self_fqn + "_fault_clients") { + if (is_own_gateway_node(fqn, self_fqn)) { continue; } ++count; @@ -2457,14 +2476,15 @@ void GatewayNode::refresh_cache() { } } - // Filter ROS 2 internal nodes (underscore prefix convention). + // Filter ROS 2 internal nodes (underscore prefix convention) and this + // gateway's own helper nodes. // Controlled by discovery.runtime.filter_internal_nodes parameter (default: true). // Covers local heuristic apps (which bypass the merge pipeline orphan filter // in runtime_only mode) and any peer apps that slipped through fetch_entities. if (filter_internal_nodes_) { - auto removed = filter_internal_node_apps(apps, peer_routing_table); + auto removed = filter_internal_node_apps(apps, peer_routing_table, get_fully_qualified_name()); if (removed > 0) { - RCLCPP_DEBUG(get_logger(), "Filtered %zu internal node apps (_ prefix)", removed); + RCLCPP_DEBUG(get_logger(), "Filtered %zu internal node apps (_ prefix or own helper node)", removed); } } @@ -2561,9 +2581,10 @@ void GatewayNode::stop_rest_server() { } size_t filter_internal_node_apps(std::vector & apps, - const std::unordered_map & peer_routing_table) { + const std::unordered_map & peer_routing_table, + const std::string & self_fqn) { auto before = apps.size(); - auto end = std::remove_if(apps.begin(), apps.end(), [&peer_routing_table](const App & app) { + auto end = std::remove_if(apps.begin(), apps.end(), [&peer_routing_table, &self_fqn](const App & app) { std::string original_id = app.id; auto rt_it = peer_routing_table.find(app.id); if (rt_it != peer_routing_table.end()) { @@ -2573,6 +2594,14 @@ size_t filter_internal_node_apps(std::vector & apps, if (original_id.size() > prefix.size() && original_id.compare(0, prefix.size(), prefix) == 0) { original_id = original_id.substr(prefix.size()); } + } else if (is_own_gateway_node(app.effective_fqn(), self_fqn)) { + // A local app bound to one of this gateway's own nodes. Those names do + // not start with '_' ("_sub", "_fault_clients", ...), + // so only the FQN test catches them, and without it the gateway + // advertises its own plumbing as diagnosable apps. Remote entities are + // skipped deliberately: a peer's helper nodes carry the same FQNs and are + // the peer's own filter's business. + return true; } // ROS 2 internal nodes use _ prefix convention return !original_id.empty() && original_id[0] == '_'; diff --git a/src/ros2_medkit_gateway/test/test_handler_context.cpp b/src/ros2_medkit_gateway/test/test_handler_context.cpp index 968f29469..573c356c0 100644 --- a/src/ros2_medkit_gateway/test/test_handler_context.cpp +++ b/src/ros2_medkit_gateway/test/test_handler_context.cpp @@ -874,7 +874,7 @@ TEST(FilterInternalNodeAppsTest, FiltersLocalInternalNodes) { apps.push_back(another_internal); std::unordered_map routing; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 2u); ASSERT_EQ(apps.size(), 1u); @@ -896,7 +896,7 @@ TEST(FilterInternalNodeAppsTest, PreservesAllNormalNodes) { apps.push_back(a3); std::unordered_map routing; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 0u); EXPECT_EQ(apps.size(), 3u); @@ -921,7 +921,7 @@ TEST(FilterInternalNodeAppsTest, FiltersPeerPrefixedInternalNodes) { routing["peer_subsystem___ros2cli_daemon"] = "peer_subsystem"; routing["peer_subsystem__lidar_driver"] = "peer_subsystem"; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 1u); ASSERT_EQ(apps.size(), 1u); @@ -939,7 +939,7 @@ TEST(FilterInternalNodeAppsTest, DoesNotStripPrefixWithoutRoutingEntry) { apps.push_back(ambiguous); std::unordered_map routing; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 0u); ASSERT_EQ(apps.size(), 1u); @@ -950,7 +950,7 @@ TEST(FilterInternalNodeAppsTest, HandlesEmptyAppList) { std::vector apps; std::unordered_map routing; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 0u); EXPECT_TRUE(apps.empty()); @@ -981,7 +981,7 @@ TEST(FilterInternalNodeAppsTest, MixedLocalAndRemoteInternalNodes) { routing["sub_b__actuator"] = "sub_b"; routing["sub_b___parameter_bridge"] = "sub_b"; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 2u); ASSERT_EQ(apps.size(), 2u); @@ -1006,12 +1006,88 @@ TEST(FilterInternalNodeAppsTest, PeerPrefixMatchMustBeExact) { std::unordered_map routing; routing["my_peer__sensor"] = "my_peer"; - auto removed = filter_internal_node_apps(apps, routing); + auto removed = filter_internal_node_apps(apps, routing, "/ros2_medkit_gateway"); EXPECT_EQ(removed, 0u); ASSERT_EQ(apps.size(), 1u); } +namespace { + +App bound_app(const std::string & id, const std::string & fqn) { + App app; + app.id = id; + app.name = id; + app.bound_fqn = fqn; + return app; +} + +} // namespace + +TEST(FilterInternalNodeAppsTest, DropsTheGatewaysOwnHelperNodes) { + // The gateway creates "_sub", "_fault_clients" and + // "_lifecycle_state_reader" in its own process. None starts with '_', + // so runtime introspection returns them as ordinary apps and the gateway ends + // up listing its own plumbing as diagnosable. + const std::string self_fqn = "/ros2_medkit_gateway"; + std::vector apps{ + bound_app("ros2_medkit_gateway", self_fqn), + bound_app("ros2_medkit_gateway_sub", self_fqn + "_sub"), + bound_app("ros2_medkit_gateway_fault_clients", self_fqn + "_fault_clients"), + bound_app("ros2_medkit_gateway_lifecycle_state_reader", self_fqn + "_lifecycle_state_reader"), + // Positive controls on the same harness: a similarly suffixed FOREIGN + // node, a node whose name merely extends the gateway's, and the + // fault_manager, which is a separate diagnosable component even when it + // shares the process. + bound_app("other_gateway_sub", "/other_gateway_sub"), + bound_app("ros2_medkit_gateway_monitor", self_fqn + "_monitor"), + bound_app("fault_manager", "/fault_manager"), + }; + + std::unordered_map routing; + auto removed = filter_internal_node_apps(apps, routing, self_fqn); + + EXPECT_EQ(removed, 4u); + std::set remaining; + for (const auto & app : apps) { + remaining.insert(app.id); + } + EXPECT_EQ(remaining, (std::set{"other_gateway_sub", "ros2_medkit_gateway_monitor", "fault_manager"})); +} + +TEST(FilterInternalNodeAppsTest, LeavesPeerHelperNodesToThePeer) { + // A remote entity carrying the same FQN belongs to the peer that reported + // it, so this gateway must not reach across and filter it. + const std::string self_fqn = "/ros2_medkit_gateway"; + std::vector apps{bound_app("sub_b__ros2_medkit_gateway_sub", self_fqn + "_sub")}; + + std::unordered_map routing; + routing["sub_b__ros2_medkit_gateway_sub"] = "sub_b"; + + auto removed = filter_internal_node_apps(apps, routing, self_fqn); + + EXPECT_EQ(removed, 0u); + ASSERT_EQ(apps.size(), 1u); +} + +TEST(IsOwnGatewayNodeTest, MatchesSelfAndHelpersExactlyAndNothingElse) { + const std::string self_fqn = "/ros2_medkit_gateway"; + EXPECT_TRUE(is_own_gateway_node(self_fqn, self_fqn)); + EXPECT_TRUE(is_own_gateway_node(self_fqn + "_sub", self_fqn)); + EXPECT_TRUE(is_own_gateway_node(self_fqn + "_fault_clients", self_fqn)); + EXPECT_TRUE(is_own_gateway_node(self_fqn + "_lifecycle_state_reader", self_fqn)); + + // Prefix neighbours are genuine peers, not ours. + EXPECT_FALSE(is_own_gateway_node(self_fqn + "_monitor", self_fqn)); + EXPECT_FALSE(is_own_gateway_node(self_fqn + "2", self_fqn)); + EXPECT_FALSE(is_own_gateway_node("/other" + self_fqn + "_sub", self_fqn)); + EXPECT_FALSE(is_own_gateway_node("/fault_manager", self_fqn)); + + // An unknown self FQN must claim nothing rather than everything. + EXPECT_FALSE(is_own_gateway_node(self_fqn, "")); + EXPECT_FALSE(is_own_gateway_node("", self_fqn)); +} + // ============================================================================= // Area fault/log aggregation handler tests (via REST API) // ============================================================================= From 7b11924ef82b2c95e174929e72a1f78abc2574df Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 13:25:05 +0200 Subject: [PATCH 4/8] test(opcua): cover the config-less discovery start-up race in docker Every existing opcua docker scenario pins OPCUA_ENDPOINT_URL, which short-circuits discovery, so none of them can reach the failure this covers: the gateway and the PLC power on together, the start-up scan runs while the PLC is still booting, and the plugin is left retrying its fallback endpoint. The scenario starts the gateway first with discovery on and no endpoint configured, asserts it settled on the fallback endpoint with no session, then brings an OPC-UA server up on the same subnet and asserts the endpoint is adopted within two re-scan intervals. It also asserts the container never restarted, since a restart would satisfy the endpoint check while proving nothing: a restart is exactly what used to be needed. The network is created with an explicit /24 so the read-only sweep stays 254 hosts and finishes in seconds. --- .../ros2_medkit_opcua/README.md | 10 + .../docker/scripts/run_discovery_race_test.sh | 183 ++++++++++++++++++ 2 files changed, 193 insertions(+) create mode 100755 src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 17cc1115e..7f1f8e4a6 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -850,6 +850,16 @@ bash scripts/run_integration_tests.sh bash scripts/stop.sh ``` +A separate scenario covers the config-less discovery start-up race, which the +suite above cannot see because it pins `OPCUA_ENDPOINT_URL` and so +short-circuits discovery. It starts the gateway before any server, with +discovery on and no endpoint configured, then brings a server up and asserts +the gateway adopts it without a restart: + +```bash +bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh +``` + ### Test Coverage | Category | Tests | What it validates | diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh new file mode 100755 index 000000000..66fcad32f --- /dev/null +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh @@ -0,0 +1,183 @@ +#!/usr/bin/env bash +# Copyright 2026 mfaferek93 +# +# Integration test for the config-less discovery start-up race. +# +# The field failure this reproduces: the gateway and the PLC power on +# together, the gateway's start-up scan runs while the PLC is still booting +# and finds nothing, and without a re-scan the plugin retries its fallback +# endpoint for as long as it runs. Only a restart ever found the PLC. +# +# So this scenario starts the gateway FIRST, with discovery on and no +# OPCUA_ENDPOINT_URL, asserts it settled on the fallback endpoint with no +# session, then starts an OPC-UA server and asserts the gateway adopts it +# within two re-scan intervals without being restarted. +# +# Every other opcua docker scenario pins OPCUA_ENDPOINT_URL, which +# short-circuits discovery, so none of them can see this. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/../../../../.." && pwd)" +NET_NAME=discovery-race-net +NET_SUBNET=172.31.77.0/24 +SERVER_NAME=discovery-race-server +GATEWAY_NAME=discovery-race-gateway +SERVER_PORT=4840 +GATEWAY_PORT=8089 +RESCAN_INTERVAL_S=5 +CONFIG_DIR=/tmp/discovery_race_config + +# The fallback the plugin keeps when a scan selects nothing (OpcuaClientConfig). +FALLBACK_ENDPOINT="opc.tcp://localhost:4840" + +cleanup() { + local rc=$? + if [[ ${rc} -ne 0 ]]; then + for c in "${SERVER_NAME}" "${GATEWAY_NAME}"; do + echo "=== ${c} logs (cleanup trap) ===" >&2 + docker logs "${c}" >&2 2>&1 || true + done + fi + docker rm -f "${SERVER_NAME}" "${GATEWAY_NAME}" >/dev/null 2>&1 || true + docker network rm "${NET_NAME}" >/dev/null 2>&1 || true + rm -rf "${CONFIG_DIR}" +} +trap cleanup EXIT + +fail() { + echo " FAIL: $*" >&2 + exit 1 +} + +status_json() { + curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components/discovery_race_runtime/x-plc-status" || echo '{}' +} + +status_field() { + status_json | python3 -c "import json,sys; print(json.load(sys.stdin).get('$1', ''))" +} + +cd "${REPO_ROOT}" + +# Idempotent teardown of anything a hard-killed earlier run left behind. +docker rm -f "${SERVER_NAME}" "${GATEWAY_NAME}" >/dev/null 2>&1 || true +docker network rm "${NET_NAME}" >/dev/null 2>&1 || true + +echo "[1/6] Build images" +docker build --network=host \ + -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile \ + -t ros2_medkit_alarm_test_server:dev . >/dev/null +docker build --network=host \ + -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway \ + -t gateway-opcua:discovery-race . >/dev/null + +# A /24 keeps the read-only sweep to 254 hosts, so a scan finishes in seconds. +# Discovery rejects anything wider than /16 outright. +docker network create --subnet "${NET_SUBNET}" "${NET_NAME}" >/dev/null + +echo "[2/6] Start the gateway BEFORE any server, discovery on, no endpoint pinned" +mkdir -p "${CONFIG_DIR}" +cat >"${CONFIG_DIR}/discovery_nodes.yaml" <<'EOF' +area_id: plc_systems +component_id: discovery_race_runtime +# One node is enough: the assertion is on the session, not on any value. The +# node id need not resolve on the server, since a failed read does not drop +# the connection. +nodes: + - node_id: "ns=2;s=StatusWord" + entity_id: tank_process + data_name: status_word + data_type: int +EOF +cat >"${CONFIG_DIR}/manifest.yaml" <<'EOF' +manifest_version: "1.0" +EOF +cp src/ros2_medkit_plugins/ros2_medkit_opcua/docker/gateway_params.yaml \ + "${CONFIG_DIR}/gateway_params.yaml" + +docker run -d --name "${GATEWAY_NAME}" --network "${NET_NAME}" \ + -p "${GATEWAY_PORT}:8080" \ + -v "${CONFIG_DIR}:/config:ro" \ + -e ROS_DOMAIN_ID=67 \ + -e OPCUA_DISCOVERY_ENABLED=1 \ + -e OPCUA_DISCOVERY_SUBNETS="${NET_SUBNET}" \ + -e OPCUA_DISCOVERY_INTERVAL_S="${RESCAN_INTERVAL_S}" \ + -e OPCUA_NODE_MAP_PATH=/config/discovery_nodes.yaml \ + gateway-opcua:discovery-race \ + bash -c ' + set -e + mkdir -p /var/lib/ros2_medkit/rosbags + source /opt/ros/jazzy/setup.bash + source /root/ws/install/setup.bash + ros2 run ros2_medkit_fault_manager fault_manager_node \ + > /var/lib/ros2_medkit/fault_manager.log 2>&1 & + PLUGIN_PATH=$(find /root/ws/install -name "libros2_medkit_opcua_plugin.so" | head -1) + exec ros2 run ros2_medkit_gateway gateway_node \ + --ros-args --params-file /config/gateway_params.yaml \ + -p plugins.opcua.path:="${PLUGIN_PATH}" \ + -p discovery.mode:=hybrid \ + -p discovery.manifest_path:=/config/manifest.yaml \ + -p discovery.manifest_strict_validation:=false' >/dev/null + +echo "[3/6] Wait for the REST API" +for _ in $(seq 1 60); do + if curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null 2>&1; then + break + fi + sleep 1 +done +curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null \ + || fail "gateway REST API never came up" + +echo "[4/6] Assert the start-up scan found nothing and left the fallback endpoint" +# The scan runs during set_context(), so by the time the API answers it has +# already completed against an empty network. +endpoint="$(status_field endpoint_url)" +connected="$(status_field connected)" +[[ "${endpoint}" == "${FALLBACK_ENDPOINT}" ]] \ + || fail "expected the fallback endpoint before the server exists, got '${endpoint}'" +[[ "${connected}" == "False" ]] \ + || fail "expected no session before the server exists, got connected='${connected}'" +echo " OK no server found at start-up, endpoint left at ${FALLBACK_ENDPOINT}" + +echo "[5/6] Start the OPC-UA server (the PLC finishing its boot)" +docker run -d --name "${SERVER_NAME}" --network "${NET_NAME}" \ + ros2_medkit_alarm_test_server:dev --port "${SERVER_PORT}" >/dev/null +for _ in $(seq 1 30); do + if docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY '; then + break + fi + sleep 1 +done +docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY ' || fail "test server never became ready" +SERVER_IP="$(docker inspect -f "{{(index .NetworkSettings.Networks \"${NET_NAME}\").IPAddress}}" "${SERVER_NAME}")" +echo " server up at ${SERVER_IP}:${SERVER_PORT}" + +echo "[6/6] Assert the gateway adopts it within two re-scan intervals, unrestarted" +# Budget: two intervals for the re-scan to come round, plus the sweep and +# connect themselves. Generous enough not to flake, far short of "never", +# which is what the bug did. +DEADLINE=$((SECONDS + 2 * RESCAN_INTERVAL_S + 40)) +adopted="" +while [[ ${SECONDS} -lt ${DEADLINE} ]]; do + endpoint="$(status_field endpoint_url)" + connected="$(status_field connected)" + if [[ "${endpoint}" == "opc.tcp://${SERVER_IP}:${SERVER_PORT}" && "${connected}" == "True" ]]; then + adopted="${endpoint}" + break + fi + sleep 2 +done +[[ -n "${adopted}" ]] \ + || fail "endpoint still '${endpoint}' (connected='${connected}') after $((2 * RESCAN_INTERVAL_S + 40))s" +echo " OK re-scan adopted ${adopted} without a gateway restart" + +# The gateway must have adopted the server in the process that started before +# it, not in a fresh one: a restarted container would pass the check above +# while proving nothing. +restarts="$(docker inspect -f '{{.RestartCount}}' "${GATEWAY_NAME}")" +[[ "${restarts}" == "0" ]] || fail "gateway restarted ${restarts} time(s) during the run" +echo " OK gateway never restarted" + +echo "Discovery race scenario passed." From 054e5c6559a45b92035c4753232c1fa264aa6011 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 16:35:01 +0200 Subject: [PATCH 5/8] fix(opcua): keep config-less discovery honest while it stays disconnected A gateway that starts before its PLC runs the discovery rescan for the life of the outage, and several things it does in that state were wrong. Component identity. With no node map the SOVD component is named from the device. When the start-up connect fails there is no device to ask, so the name comes from the fallback endpoint and an empty DeviceInfo, and it was then pinned forever: after discovery adopted the real PLC the component still served opcua- while introspect() reported the adopted endpoint. The identity is now re-derived on the first poll of a new session (config-less mode only, an explicit node map still owns the name), the derived alarms entity follows the rename, and the change is logged at INFO. The docker race scenario gained a config-less pass that asserts the rename against a real server. Rescan cadence. The cadence was stamped when a sweep STARTED, so a sweep of a legal /16 (minutes at the defaults) made the next one due the moment it returned: the poll thread swept back to back and the reconnect attempt dropped to one per sweep. It is now stamped when the sweep ends. NetworkDiscovery::run() also takes a cancel predicate, bound to the shutdown flag and checked before each probe and between the sweep and identify phases, so shutdown() no longer has to wait out a sweep. Reconnect backoff. The rescan is consulted once per reconnect attempt and attempts are spaced by the exponential backoff, so the real cadence was max(interval_s, backoff) while the README, the header and the start-up log all said "every interval_s". The backoff ceiling is now capped at the rescan cadence while discovery is rescanning. interval_s. An unset interval and an explicit 0 both mapped to the 30 s default, so there was no way to keep discovery on and stop rescanning. They are now distinct: unset takes the default, an explicit 0 leaves the start-up scan one-shot, and a negative value is refused with a warning that no longer claims the kept default is one-shot. The start-up line no longer promises a loop that rescans "every 0s" either. Discovery report. A pass re-emitted its whole report every rescan, so a site with a secured-only server logged the same WARN every 30 s for the life of the process. A pass whose outcome matches the previous one now reports at DEBUG. The first pass, and every changed outcome, still reports at INFO/WARN. Connect-time clear. It is a link-state clear, not an operator resolving a root cause, so it now sets skip_correlation_auto_clear and cannot cascade-clear the symptom faults a rule attributes to PLC_COMMS_LOST. The poller's own clear on a successful reconnect is the same event and does the same. Those clears were also buffered unconditionally while the fault manager was unmatched, so a flapping link pushed real alarm reports out of the bounded buffer: the buffer now keeps at most one pending clear per fault code, evicts a clear before a report, and refuses a clear rather than dropping a report. --- .../ros2_medkit_opcua/README.md | 38 +- .../docker/scripts/run_discovery_race_test.sh | 174 +++++++- .../ros2_medkit_opcua/network_discovery.hpp | 20 +- .../ros2_medkit_opcua/opcua_plugin.hpp | 169 +++++++- .../ros2_medkit_opcua/opcua_poller.hpp | 19 +- .../src/network_discovery.cpp | 33 +- .../ros2_medkit_opcua/src/opcua_plugin.cpp | 365 +++++++++++++---- .../ros2_medkit_opcua/src/opcua_poller.cpp | 12 +- .../test/test_network_discovery.cpp | 116 +++++- .../test/test_opcua_plugin.cpp | 383 +++++++++++++++++- 10 files changed, 1194 insertions(+), 135 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 7f1f8e4a6..65c3d2225 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -604,12 +604,18 @@ plugins.opcua.discovery: connect_timeout_ms: 600 # per-port TCP connect timeout scan_concurrency: 100 # bounded, polite concurrent connect count identify_timeout_ms: 6000 # per GetEndpoints identify - interval_s: 0 # re-scan cadence while disconnected (0 = default 30 s) + # re-scan cadence while disconnected. Omit the key for the built-in 30 s; + # set it to 0 to keep discovery on but never re-scan (start-up scan only). + interval_s: 30 anonymous_none_only: true # only auto-connect None/Anonymous servers ``` Environment overrides (Docker / appliance): `OPCUA_DISCOVERY_ENABLED`, `OPCUA_DISCOVERY_SUBNETS` (comma-separated CIDRs), `OPCUA_DISCOVERY_INTERVAL_S`. +Leaving `interval_s` (and `OPCUA_DISCOVERY_INTERVAL_S`) unset means "no cadence +stated" and takes the 30 s default; an explicit `0` is honoured as written and +turns the recurring sweep off. A negative value is refused with a warning and +leaves the cadence unset. How it works: 1. Bounded concurrent TCP connect sweep of the configured ports across the @@ -626,11 +632,19 @@ How it works: ip:port) and connects to the **scanned ip:port** - not the advertised EndpointUrl, which a server may report as a non-resolvable hostname. 5. While no session is established, the reconnect loop scans again every - `interval_s` (default 30 s) and adopts a newly found server for its next - connect attempt, logging the swap at INFO. This is what covers the common - race where the gateway and the PLC boot together: the startup scan finds - nothing because the PLC is still coming up, and without a re-scan the plugin - would retry the fallback endpoint until someone restarted it. + `interval_s` (default 30 s), measured from the END of the previous sweep, and + adopts a newly found server for its next connect attempt, logging the swap at + INFO. The re-scan is consulted once per reconnect attempt, and those are + spaced by an exponential backoff, so the backoff ceiling is capped at + `interval_s` while discovery is re-scanning - otherwise the real cadence + would be `max(interval_s, backoff)` rather than the stated one. This covers + the common race where the gateway and the PLC boot together: the startup scan + finds nothing because the PLC is still coming up, and without a re-scan the + plugin would retry the fallback endpoint until someone restarted it. +6. On the first session after such an adoption, a config-less deployment (no + node map) re-derives the SOVD component identity from the device itself, so + the component stops being served under the provisional `opcua-` name it + got when nothing answered. The change is logged at INFO. Re-scanning stops as soon as a session is up, and never starts at all when an `endpoint_url` is configured. @@ -638,11 +652,19 @@ Re-scanning stops as soon as a session is up, and never starts at all when an Safety / OT posture: - Everything is read-only: TCP connect + `GetEndpoints` only. No writes, no subscriptions, no second long-lived session. +- The scan is NOT one-shot: while the plugin has no session it repeats every + `interval_s` (default 30 s) for as long as it stays disconnected. Set + `interval_s: 0` (or `OPCUA_DISCOVERY_INTERVAL_S=0`) to keep discovery on with + the start-up scan only, or `enabled: false` to switch it off entirely. +- A sweep is cancelled when the plugin shuts down, so a stop does not have to + wait out a subnet the size of a /16. - An explicitly configured `endpoint_url` (or `OPCUA_ENDPOINT_URL`) always wins; discovery then does nothing, so it never opens a second session on a PLC the plugin already polls. -- Secured-only servers (no None/Anonymous endpoint) are surfaced in the startup - log as leads requiring operator credentials - never auto-connected or probed. +- Secured-only servers (no None/Anonymous endpoint) are surfaced in the log as + leads requiring operator credentials - never auto-connected or probed. A + re-scan whose outcome has not changed reports at DEBUG instead of repeating + the whole report, so a recurring sweep does not bury the rest of the log. - The scan is bounded (short connect timeout, capped concurrency) and CIDRs wider than /16 are rejected to prevent an accidental broad sweep. diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh index 66fcad32f..dc52d3823 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh @@ -13,6 +13,12 @@ # session, then starts an OPC-UA server and asserts the gateway adopts it # within two re-scan intervals without being restarted. # +# It then repeats the race with NO node map at all (the config-less deployment). +# There the component identity is derived from the device, and a gateway that +# scanned before its PLC existed can only name it after the fallback endpoint - +# so the second pass asserts the SOVD component stops being served under that +# provisional name once the PLC is adopted. +# # Every other opcua docker scenario pins OPCUA_ENDPOINT_URL, which # short-circuits discovery, so none of them can see this. @@ -30,6 +36,12 @@ CONFIG_DIR=/tmp/discovery_race_config # The fallback the plugin keeps when a scan selects nothing (OpcuaClientConfig). FALLBACK_ENDPOINT="opc.tcp://localhost:4840" +# Config-less naming: with no node map the component id is derived from the +# device. Before any server exists that can only be the fallback endpoint's host; +# after adoption it is the test server's DI nameplate (Manufacturer "SelfPatch +# Devices" + Model "SPX-1000"), slugified. +FALLBACK_COMPONENT_ID="opcua-localhost" +DEVICE_COMPONENT_ID="selfpatch_devices_spx_1000" cleanup() { local rc=$? @@ -50,21 +62,53 @@ fail() { exit 1 } +# x-plc-status of a named component (the node-map pass pins the id; the +# config-less pass has to look it up first). +status_json_for() { + curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components/$1/x-plc-status" || echo '{}' +} + status_json() { - curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components/discovery_race_runtime/x-plc-status" || echo '{}' + status_json_for discovery_race_runtime } status_field() { status_json | python3 -c "import json,sys; print(json.load(sys.stdin).get('$1', ''))" } +status_field_for() { + status_json_for "$1" | python3 -c "import json,sys; print(json.load(sys.stdin).get('$2', ''))" +} + +component_ids() { + curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" 2>/dev/null | + python3 -c " +import json,sys +try: + print(' '.join(c.get('id','') for c in json.load(sys.stdin).get('items', []))) +except Exception: + print('') +" +} + +wait_for_rest_api() { + for _ in $(seq 1 60); do + if curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null 2>&1; then + return 0 + fi + sleep 1 + done + curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null \ + || fail "gateway REST API never came up" +} + cd "${REPO_ROOT}" # Idempotent teardown of anything a hard-killed earlier run left behind. docker rm -f "${SERVER_NAME}" "${GATEWAY_NAME}" >/dev/null 2>&1 || true docker network rm "${NET_NAME}" >/dev/null 2>&1 || true -echo "[1/6] Build images" +echo "[1/9] Build images" docker build --network=host \ -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile \ -t ros2_medkit_alarm_test_server:dev . >/dev/null @@ -76,7 +120,7 @@ docker build --network=host \ # Discovery rejects anything wider than /16 outright. docker network create --subnet "${NET_SUBNET}" "${NET_NAME}" >/dev/null -echo "[2/6] Start the gateway BEFORE any server, discovery on, no endpoint pinned" +echo "[2/9] Start the gateway BEFORE any server, discovery on, no endpoint pinned" mkdir -p "${CONFIG_DIR}" cat >"${CONFIG_DIR}/discovery_nodes.yaml" <<'EOF' area_id: plc_systems @@ -120,17 +164,10 @@ docker run -d --name "${GATEWAY_NAME}" --network "${NET_NAME}" \ -p discovery.manifest_path:=/config/manifest.yaml \ -p discovery.manifest_strict_validation:=false' >/dev/null -echo "[3/6] Wait for the REST API" -for _ in $(seq 1 60); do - if curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null 2>&1; then - break - fi - sleep 1 -done -curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components" >/dev/null \ - || fail "gateway REST API never came up" +echo "[3/9] Wait for the REST API" +wait_for_rest_api -echo "[4/6] Assert the start-up scan found nothing and left the fallback endpoint" +echo "[4/9] Assert the start-up scan found nothing and left the fallback endpoint" # The scan runs during set_context(), so by the time the API answers it has # already completed against an empty network. endpoint="$(status_field endpoint_url)" @@ -141,7 +178,7 @@ connected="$(status_field connected)" || fail "expected no session before the server exists, got connected='${connected}'" echo " OK no server found at start-up, endpoint left at ${FALLBACK_ENDPOINT}" -echo "[5/6] Start the OPC-UA server (the PLC finishing its boot)" +echo "[5/9] Start the OPC-UA server (the PLC finishing its boot)" docker run -d --name "${SERVER_NAME}" --network "${NET_NAME}" \ ros2_medkit_alarm_test_server:dev --port "${SERVER_PORT}" >/dev/null for _ in $(seq 1 30); do @@ -154,10 +191,15 @@ docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY ' || fail "test server never SERVER_IP="$(docker inspect -f "{{(index .NetworkSettings.Networks \"${NET_NAME}\").IPAddress}}" "${SERVER_NAME}")" echo " server up at ${SERVER_IP}:${SERVER_PORT}" -echo "[6/6] Assert the gateway adopts it within two re-scan intervals, unrestarted" -# Budget: two intervals for the re-scan to come round, plus the sweep and -# connect themselves. Generous enough not to flake, far short of "never", -# which is what the bug did. +echo "[6/9] Assert the gateway adopts it within two re-scan intervals, unrestarted" +# Budget arithmetic. The re-scan is consulted once per reconnect attempt, and +# those are spaced by the reconnect backoff, so the adoption cadence is +# max(interval_s, backoff). The backoff ceiling is capped at interval_s while +# discovery is re-scanning, which is what keeps this budget in terms of +# interval_s alone: worst case is one full interval before the sweep is due plus +# one for the attempt that follows it, and the +40 s covers the sweep of a /24, +# the connect and the REST refresh. Generous enough not to flake, far short of +# "never", which is what the bug did. DEADLINE=$((SECONDS + 2 * RESCAN_INTERVAL_S + 40)) adopted="" while [[ ${SECONDS} -lt ${DEADLINE} ]]; do @@ -180,4 +222,100 @@ restarts="$(docker inspect -f '{{.RestartCount}}' "${GATEWAY_NAME}")" [[ "${restarts}" == "0" ]] || fail "gateway restarted ${restarts} time(s) during the run" echo " OK gateway never restarted" +# --------------------------------------------------------------------------- +# Config-less variant: the same race with NO node map. +# +# Without a node map the SOVD component is named from the device itself. A +# gateway that scanned before its PLC existed has no device to ask, so it can +# only name the component after the fallback endpoint. The defect this covers is +# that identity being pinned once and never revisited: the component kept +# serving opcua-localhost for the life of the process while the plugin was +# happily polling the adopted PLC. +# --------------------------------------------------------------------------- + +echo "[7/9] Config-less pass: stop everything, start the gateway with no node map" +docker rm -f "${SERVER_NAME}" "${GATEWAY_NAME}" >/dev/null 2>&1 || true + +docker run -d --name "${GATEWAY_NAME}" --network "${NET_NAME}" \ + -p "${GATEWAY_PORT}:8080" \ + -v "${CONFIG_DIR}:/config:ro" \ + -e ROS_DOMAIN_ID=67 \ + -e OPCUA_DISCOVERY_ENABLED=1 \ + -e OPCUA_DISCOVERY_SUBNETS="${NET_SUBNET}" \ + -e OPCUA_DISCOVERY_INTERVAL_S="${RESCAN_INTERVAL_S}" \ + gateway-opcua:discovery-race \ + bash -c ' + set -e + mkdir -p /var/lib/ros2_medkit/rosbags + source /opt/ros/jazzy/setup.bash + source /root/ws/install/setup.bash + ros2 run ros2_medkit_fault_manager fault_manager_node \ + > /var/lib/ros2_medkit/fault_manager.log 2>&1 & + PLUGIN_PATH=$(find /root/ws/install -name "libros2_medkit_opcua_plugin.so" | head -1) + exec ros2 run ros2_medkit_gateway gateway_node \ + --ros-args --params-file /config/gateway_params.yaml \ + -p plugins.opcua.path:="${PLUGIN_PATH}" \ + -p discovery.mode:=hybrid \ + -p discovery.manifest_path:=/config/manifest.yaml \ + -p discovery.manifest_strict_validation:=false' >/dev/null + +wait_for_rest_api + +echo "[8/9] Assert the component is named after the fallback while nothing answers" +ids="" +DEADLINE=$((SECONDS + 30)) +while [[ ${SECONDS} -lt ${DEADLINE} ]]; do + ids="$(component_ids)" + if [[ " ${ids} " == *" ${FALLBACK_COMPONENT_ID} "* ]]; then + break + fi + sleep 2 +done +[[ " ${ids} " == *" ${FALLBACK_COMPONENT_ID} "* ]] \ + || fail "expected the provisional component '${FALLBACK_COMPONENT_ID}' before any server, got '${ids}'" +echo " OK config-less component provisionally named ${FALLBACK_COMPONENT_ID}" + +echo "[9/9] Start the server and assert the component is renamed from the device" +docker run -d --name "${SERVER_NAME}" --network "${NET_NAME}" \ + ros2_medkit_alarm_test_server:dev --port "${SERVER_PORT}" >/dev/null +for _ in $(seq 1 30); do + if docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY '; then + break + fi + sleep 1 +done +docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY ' || fail "test server never became ready" +SERVER_IP="$(docker inspect -f "{{(index .NetworkSettings.Networks \"${NET_NAME}\").IPAddress}}" "${SERVER_NAME}")" +echo " server up at ${SERVER_IP}:${SERVER_PORT}" + +# Same budget as step 6, plus the discovery refresh that republishes entities. +DEADLINE=$((SECONDS + 2 * RESCAN_INTERVAL_S + 60)) +renamed="" +while [[ ${SECONDS} -lt ${DEADLINE} ]]; do + ids="$(component_ids)" + if [[ " ${ids} " == *" ${DEVICE_COMPONENT_ID} "* ]]; then + renamed="${DEVICE_COMPONENT_ID}" + break + fi + sleep 2 +done +[[ -n "${renamed}" ]] \ + || fail "component still '${ids}' after adoption - expected the device-derived '${DEVICE_COMPONENT_ID}'" + +# The renamed component is the one actually polling the adopted PLC, so the id +# the operator sees is not a second, stale entity next to a live opcua-localhost. +endpoint="$(status_field_for "${renamed}" endpoint_url)" +connected="$(status_field_for "${renamed}" connected)" +[[ "${endpoint}" == "opc.tcp://${SERVER_IP}:${SERVER_PORT}" ]] \ + || fail "renamed component reports endpoint '${endpoint}', expected the adopted server" +[[ "${connected}" == "True" ]] \ + || fail "renamed component reports connected='${connected}', expected a live session" +[[ "${renamed}" != "${FALLBACK_COMPONENT_ID}" ]] \ + || fail "component id never moved off ${FALLBACK_COMPONENT_ID}" +echo " OK component renamed to ${renamed}, connected at ${endpoint}" + +restarts="$(docker inspect -f '{{.RestartCount}}' "${GATEWAY_NAME}")" +[[ "${restarts}" == "0" ]] || fail "gateway restarted ${restarts} time(s) during the config-less run" +echo " OK gateway never restarted" + echo "Discovery race scenario passed." diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp index 56c31a8ae..9f73d21d2 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/network_discovery.hpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -105,14 +106,17 @@ struct OpcuaDiscoveryConfig { int scan_concurrency{100}; ///< bounded, polite concurrent connect count int identify_timeout_ms{6000}; - /// Re-scan cadence, in seconds, while no OPC-UA session is established. 0 - /// selects the built-in default (see OpcuaPlugin::effective_rescan_interval_s). + /// Re-scan cadence, in seconds, while no OPC-UA session is established. + /// Unset (the key absent) selects the built-in default, an explicit 0 turns + /// re-scanning off and keeps the startup scan one-shot - the two are + /// deliberately distinct, so a deployment can keep discovery on and still + /// stop the recurring sweep (see OpcuaPlugin::effective_rescan_interval_s). /// The startup scan always runs once. The cadence only governs how often the /// disconnected reconnect loop scans again, so a gateway that started before /// its PLC finished booting adopts the PLC when it appears instead of retrying /// the fallback endpoint forever. Never used once an endpoint is configured /// explicitly, and never while a session is up. - int interval_s{0}; + std::optional interval_s; /// Only auto-register endpoints that expose a None + Anonymous endpoint (what /// the plugin connects with today). Secured-only servers are surfaced as @@ -158,7 +162,15 @@ class NetworkDiscovery { /// Run one full discovery pass (blocking). Read-only: TCP connect + /// GetEndpoints only. Deduplicated by ApplicationUri (fallback ip:port), /// sorted deterministically by ip:port. - std::vector run(); + /// + /// @param cancelled optional abort predicate, polled before every probe and + /// between the sweep and identify phases. A sweep of a legal /16 is + /// tens of thousands of probes and takes minutes, and the caller runs + /// it on the poll thread that a shutdown has to join, so without this + /// a ``docker stop`` grace period would expire mid-sweep. A pass still + /// cancelled at the next phase boundary returns an empty result rather + /// than a partial one, within one in-flight probe per worker. + std::vector run(const std::function & cancelled = {}); /// Resolve the subnets to scan: configured ``subnets`` if any, else the /// derived local /24. Exposed for logging / tests. diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index 95d2415e3..a646bfccd 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -15,11 +15,14 @@ #pragma once #include "ros2_medkit_opcua/address_space_browser.hpp" +#include "ros2_medkit_opcua/device_identity.hpp" #include "ros2_medkit_opcua/network_discovery.hpp" #include "ros2_medkit_opcua/node_map.hpp" #include "ros2_medkit_opcua/opcua_client.hpp" #include "ros2_medkit_opcua/opcua_poller.hpp" +#include + #include #include #include @@ -143,6 +146,23 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, static void apply_auto_alarms_param(const nlohmann::json & value, AutoAlarmsConfig & cfg, const std::function & warn); + // Where one discovery pass reports to, plus the memory that keeps a repeated + // identical pass quiet. A rescan runs every ``interval_s`` for the life of a + // disconnected process, so re-emitting the same scan line, per-server lines, + // summary and "no auto-connectable server" WARN each time buries every other + // message in the log. ``previous_outcome`` is owned by the caller (the plugin + // keeps one across rescans): when it is non-null and the pass reaches the same + // outcome as the pass before it, the whole report goes to ``debug`` instead. + // The first pass, and every pass whose outcome changed, is always reported at + // info/warn. A null ``previous_outcome`` (the startup scan, and tests that do + // not care) reports every pass. + struct DiscoveryReporter { + std::function info; + std::function warn; + std::function debug; + std::string * previous_outcome{nullptr}; + }; + // Run one read-only discovery pass and return the endpoint URL to adopt. // // Returns nullopt - meaning "keep the endpoint you have" - when discovery is @@ -158,19 +178,21 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // operator's target nor open a second session on an already polled PLC // @param scan injected TCP port probe // @param identify injected OPC-UA GetEndpoints identify - // @param log_info operator-visible info sink - // @param log_warn operator-visible warning sink + // @param reporter operator-visible log sinks + repeat-suppression memory + // @param cancelled abort predicate handed to NetworkDiscovery::run, so a + // shutdown does not have to wait out a full sweep static std::optional discover_endpoint(const OpcuaDiscoveryConfig & config, bool endpoint_configured, const PortScanFn & scan, const IdentifyFn & identify, - const std::function & log_info, - const std::function & log_warn); + const DiscoveryReporter & reporter, + const std::function & cancelled = {}); // Seconds between reconnect rescans, or 0 when the reconnect loop must never - // rescan (discovery disabled, or an endpoint configured explicitly). A - // configured ``interval_s`` wins. interval_s = 0 means "discovery is on but no - // cadence was stated" and takes the built-in default rather than never - // rescanning: a config-less deployment is the one that cannot name a cadence - // and the one that most needs its PLC adopted once it finishes booting. + // rescan. That is the answer when discovery is disabled, when an endpoint was + // configured explicitly, and when the operator set ``interval_s: 0``, which + // means "keep discovery on but leave the startup scan one-shot". An UNSET + // interval is the config-less case - it cannot name a cadence and is the one + // that most needs its PLC adopted once it finishes booting - so it takes the + // built-in default instead of never rescanning. static int effective_rescan_interval_s(const OpcuaDiscoveryConfig & config, bool endpoint_configured); // Default reconnect rescan cadence, in seconds, when discovery is enabled with @@ -179,6 +201,93 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // boot is picked up in well under a minute. static constexpr int kDefaultRescanIntervalS = 30; + // One rate-limited rescan step: run ``sweep`` when the cadence is due, + // otherwise do nothing. + // + // The cadence is measured from the END of the previous sweep, which + // ``*last_scan_end`` stores. A sweep of a legal /16 takes minutes, so + // stamping its start would make the next one due the moment it returned: the + // poll thread would sweep back to back and the reconnect attempt would drop to + // one per sweep. ``now`` is injected so the spacing is unit-testable without + // sleeping. + // + // @return whatever ``sweep`` returned, or nullopt when it was not due yet. + static std::optional rescan_step(int interval_s, + const std::function & now, + std::chrono::steady_clock::time_point * last_scan_end, + const std::function()> & sweep); + + // Ceiling for the poller's exponential reconnect backoff. + // + // Without discovery this is ``default_ceiling`` (60 s). While the reconnect + // loop is rescanning, the rescan is only consulted once per reconnect attempt, + // so the real adoption cadence is max(interval_s, backoff) - a documented + // "every 30 s" would silently become every 60 s. Capping the backoff at the + // rescan interval makes the documented cadence the true one. Never shorter + // than ``base`` (the configured reconnect interval), so a tiny interval cannot + // turn the backoff into a hot retry loop. + static std::chrono::milliseconds effective_max_reconnect_wait(std::chrono::milliseconds base, + std::chrono::milliseconds default_ceiling, + int rescan_interval_s); + + // The component identity a config-less deployment should serve after a + // connect, or nullopt when the identity it already has still holds. + // + // A gateway that starts before its PLC connects to nothing, so the identity + // derived at start-up comes from an empty DeviceInfo and the fallback + // endpoint: the neutral ``opcua-`` placeholder. Once discovery adopts + // the real server, the device can finally name itself, and the SOVD component + // must stop serving the placeholder. Pure / static so the rule is testable + // without a server. + static std::optional rederived_component_identity(const std::string & current_id, + const OpcuaClient::DeviceInfo & info, + const std::string & endpoint_url); + + // Build the ClearFault request for one fault code. ``link_state`` marks a + // clear that only reports the OPC-UA link came back (the connect-time + // ``PLC_COMMS_LOST`` clear). Such a clear must not cascade: a correlation rule + // may name PLC_COMMS_LOST as the root cause of every symptom the outage + // produced, and the link returning is not an operator resolving those. An + // operator-driven clear (the SOVD DELETE route) leaves the flag off and keeps + // the cascade. Static so the wire field is assertable without a fault manager. + static ros2_medkit_msgs::srv::ClearFault::Request make_clear_fault_request(const std::string & fault_code, + bool link_state); + + // One entry in the bounded buffer of fault dispatches held while the + // fault_manager service is unmatched. + struct PendingFaultDispatch { + enum class Kind { Report, Clear }; + Kind kind{Kind::Report}; + std::string fault_code; ///< dedup key for a Clear; diagnostic for a Report + std::function dispatch; + }; + + // What ``enqueue_pending_dispatch`` did, so the caller can log it. + enum class PendingEnqueueOutcome { + Buffered, ///< appended, nothing lost + ReplacedClear, ///< superseded the pending clear for the same fault code + EvictedClear, ///< buffer was full: dropped a pending clear to make room + EvictedReport, ///< buffer was full of reports and a report arrived + Refused ///< buffer was full of reports and a clear arrived + }; + + // Enqueue policy for the bounded pending-dispatch buffer. + // + // Reports outrank clears. A report is a one-shot edge from the PLC that + // nothing will re-send, while a clear is re-derivable: the link state is + // re-observed on the next reconnect. So at most ONE clear per fault code is + // ever pending (a newer one moves to the back, keeping report-then-clear + // order), a full buffer gives up its oldest pending clear first, and a clear + // arriving at a buffer full of reports is refused rather than evicting one. + // Without this a flapping link enqueued one connect-time clear per reconnect + // attempt and pushed real alarm reports out of the buffer. + static PendingEnqueueOutcome enqueue_pending_dispatch(std::vector & buffer, size_t max_size, + PendingFaultDispatch entry); + + // Bound on the pending-dispatch buffer, so a deployment with no fault_manager + // cannot grow it without limit. + static constexpr size_t kMaxPendingDispatches = 256; + private: // Route handlers void handle_plc_data(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); @@ -196,7 +305,9 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // Report/clear fault via ROS 2 service (private helpers, not the FaultProvider overrides) void send_report_fault(const std::string & entity_id, const std::string & fault_code, const std::string & severity_str, const std::string & message); - void send_clear_fault(const std::string & fault_code); + // ``link_state`` marks a clear that reports the OPC-UA link came back rather + // than an operator resolving a root cause; see make_clear_fault_request. + void send_clear_fault(const std::string & fault_code, bool link_state = false); // Clear PLC_COMMS_LOST after the initial connect in set_context() succeeded. // Unconditional on purpose: the fault manager keys faults by fault_code and @@ -208,7 +319,7 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // Dispatch now if the fault_manager service is matched, else buffer the // dispatch (bounded, order-preserving) to be flushed once it appears. - void send_or_buffer(std::function dispatch); + void send_or_buffer(PendingFaultDispatch entry); // Flush buffered fault dispatches when the fault_manager service is ready. void flush_pending_reports(); @@ -231,6 +342,16 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // node_map_mutex_. No-op (never called) when auto_browse is disabled. void run_auto_browse(); + // Poll-thread hook (from publish_values): re-derive the SOVD component + // identity from the device once a NEW session is up, in config-less mode only + // (an explicit node map owns the name). This is what stops a gateway that + // started before its PLC from serving the ``opcua-`` + // placeholder for the life of the process after discovery adopted the real + // server. Logs the change at INFO and rebuilds every derived reference (the + // ``_alarms`` entity, entity_defs) under the node-map lock. + // No-op when the identity is unchanged. + void maybe_rederive_component_identity(); + // Poll-thread hook (from publish_values): re-run auto_browse when the client // has established a new session since the last walk. Covers the field case // where the gateway starts before the PLC is reachable (initial connect @@ -252,6 +373,12 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // an endpoint is already configured. void run_startup_discovery(); + // Log sinks for a discovery pass: the plugin's operator-visible info/warn + // plus the named ``opcua.plugin`` debug logger a repeated identical pass falls + // back to. ``previous_outcome`` is the caller's repeat-suppression memory + // (null to report every pass in full). + DiscoveryReporter discovery_reporter(std::string * previous_outcome) const; + // Poll-thread hook bound into PollerConfig::rediscover_endpoint whenever // discovery runs without a configured endpoint. Called from the poller's // reconnect arm, so only while no session is up, and rate-limited to one scan @@ -285,10 +412,15 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // a network. Set in configure(), consumed in run_startup_discovery(). PortScanFn discovery_scan_fn_; IdentifyFn discovery_identify_fn_; - // When the last discovery pass ran, so the reconnect rescan honours the + // When the last discovery pass FINISHED, so the reconnect rescan honours the // cadence instead of sweeping the subnet on every reconnect attempt. Stamped - // by the startup scan, then only ever read/written on the poll thread. - std::chrono::steady_clock::time_point last_discovery_scan_{}; + // by the startup scan, then only ever read/written on the poll thread. See + // rescan_step for why the end of the sweep is the reference point. + std::chrono::steady_clock::time_point last_discovery_scan_end_{}; + // Outcome digest of the previous discovery pass, so an unchanged rescan + // reports at DEBUG instead of repeating the whole report every interval_s. + // Poll thread only (the startup scan runs before the poller exists). + std::string last_discovery_outcome_; std::unique_ptr client_; NodeMap node_map_; @@ -317,6 +449,13 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, AssetIdentity device_identity_; uint64_t device_identity_generation_{0}; + // OpcuaClient::connection_generation the config-less component identity was + // derived at (0 = derived with no session, i.e. from the fallback endpoint and + // an empty DeviceInfo). The poll thread re-derives when the live generation + // differs, mirroring device_identity_generation_. Written on the set_context + // thread (happens-before the poller starts) then only on the poll thread. + uint64_t component_identity_generation_{0}; + // ROS 2 service clients for fault reporting struct FaultClients; std::unique_ptr fault_clients_; @@ -336,7 +475,7 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // never held across the actual dispatch (async_send_request) to keep ROS I/O out // of the critical section. std::mutex pending_reports_mutex_; - std::vector> pending_reports_; + std::vector pending_reports_; // Tracks which non-numeric nodes have already been warned about (avoids log spam). // Instance member instead of static to survive plugin reload (dlclose/dlopen). diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp index 73f670f72..5e3c183fd 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp @@ -103,6 +103,13 @@ struct PollerConfig { double subscription_interval_ms{500.0}; std::chrono::milliseconds poll_interval{1000}; std::chrono::milliseconds reconnect_interval{5000}; + /// Ceiling for the exponential reconnect backoff (it doubles from + /// ``reconnect_interval`` up to this). A plugin whose reconnect arm also + /// rescans for a new endpoint lowers this to the rescan cadence: the rescan is + /// consulted once per reconnect attempt, so a backoff longer than the cadence + /// would silently stretch the documented "re-scan every interval_s" to the + /// backoff instead (see OpcuaPlugin::effective_max_reconnect_wait). + std::chrono::milliseconds max_reconnect_interval{60000}; /// Active-condition replay strategy on (re)subscribe (issue #389). /// Default Auto: ConditionRefresh with a read-based fallback so hardened /// servers that reject the method still recover their active alarms. @@ -262,6 +269,13 @@ class OpcuaPoller { adopt_rediscovered_endpoint(const std::string & current, const std::function()> & rediscover); + /// Wait before the next reconnect attempt: the current wait doubled, clamped + /// to ``max_wait`` (a wait already above the cap comes back down to it). Pure + /// and static so the backoff - and the cap that keeps a rescanning reconnect + /// loop on its documented cadence - is unit-testable. + static std::chrono::milliseconds next_reconnect_wait(std::chrono::milliseconds current, + std::chrono::milliseconds max_wait); + /// Zero-config native A&C (``auto_alarms``): the alarm sources that should /// actually be subscribed / replayed, i.e. every explicit ``event_alarms`` /// entry plus (when ``auto_cfg.enabled`` and no explicit entry already @@ -455,7 +469,10 @@ class OpcuaPoller { // Issue #496: comms-lost debounce state, touched only on the poll thread. // ``comms_down_since_`` is set the first poll iteration the connection is // observed down and cleared on reconnect; ``comms_lost_raised_`` guards the - // one-shot raise / matching clear so the fault is idempotent. + // one-shot RAISE only. The clear is deliberately not guarded by it: it is sent + // on every successful reconnect, because the fault manager persists faults by + // fault_code and a comms-lost fault raised before a restart is standing in the + // store with nothing in this process's memory to remember it. std::optional comms_down_since_; bool comms_lost_raised_{false}; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/network_discovery.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/network_discovery.cpp index 40f7dd2e5..f3eaceea7 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/network_discovery.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/network_discovery.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -71,8 +72,12 @@ bool ipv4_less(const std::string & a, const std::string & b) { // ``max_workers`` threads (never more than ``count``) that pull indices off a // shared atomic cursor. The single primitive backs both the connect sweep and // the GetEndpoints identify so they share the same concurrency bound. +// +// ``cancelled`` is polled by every worker before it takes the next index, so an +// abort stops the fan-out after at most one more probe per worker instead of +// running the remaining tens of thousands. An empty predicate never cancels. template -void parallel_for(size_t count, int max_workers, Body && body) { +void parallel_for(size_t count, int max_workers, const std::function & cancelled, Body && body) { if (count == 0) { return; } @@ -80,6 +85,9 @@ void parallel_for(size_t count, int max_workers, Body && body) { std::atomic next{0}; const auto worker = [&]() { for (;;) { + if (cancelled && cancelled()) { + return; + } const size_t i = next.fetch_add(1); if (i >= count) { return; @@ -306,9 +314,12 @@ OpcuaDiscoveryConfig parse_discovery_config(const nlohmann::json & j, if (j.contains("interval_s") && j["interval_s"].is_number_integer()) { const int v = j["interval_s"].get(); if (v >= 0) { + // An explicit 0 is kept as an explicit 0: it means "keep discovery on but + // never re-scan", which an unset interval (the built-in cadence) cannot + // express. cfg.interval_s = v; } else { - warn_fn("discovery: interval_s must be >= 0 - keeping default (0 = one-shot)"); + warn_fn("discovery: interval_s must be >= 0 (0 disables re-scanning) - keeping the default cadence"); } } if (j.contains("anonymous_none_only") && j["anonymous_none_only"].is_boolean()) { @@ -333,7 +344,7 @@ std::vector NetworkDiscovery::resolve_subnets() const { return {local}; } -std::vector NetworkDiscovery::run() { +std::vector NetworkDiscovery::run(const std::function & cancelled) { // "passive" has no active scan implementation yet (mDNS / LDS FindServers // are a documented follow-up) - a no-op stub rather than silently running // the active scan mode wasn't asked for. parse_discovery_config() already @@ -365,7 +376,7 @@ std::vector NetworkDiscovery::run() { // only the polite fan-out lives here. std::vector open_hits; std::mutex hits_mu; - parallel_for(targets.size(), cfg_.scan_concurrency, [&](size_t i) { + parallel_for(targets.size(), cfg_.scan_concurrency, cancelled, [&](size_t i) { const Target & t = targets[i]; if (scan_(t.ip, t.port, cfg_.connect_timeout_ms)) { std::lock_guard lk(hits_mu); @@ -373,6 +384,12 @@ std::vector NetworkDiscovery::run() { } }); + // Cancelled mid-sweep: the hit list is partial, so do not spend an identify + // round-trip per hit on a pass whose caller is shutting down. + if (cancelled && cancelled()) { + return {}; + } + // Deterministic identify order (numerically lowest ip:port first). std::sort(open_hits.begin(), open_hits.end(), [](const Target & a, const Target & b) { if (a.ip != b.ip) { @@ -388,7 +405,7 @@ std::vector NetworkDiscovery::run() { // sweep, writing each result by index so the deterministic ip:port order // (open_hits was sorted above) survives regardless of completion order. std::vector built(open_hits.size()); - parallel_for(open_hits.size(), cfg_.scan_concurrency, [&](size_t i) { + parallel_for(open_hits.size(), cfg_.scan_concurrency, cancelled, [&](size_t i) { const Target & t = open_hits[i]; DiscoveredEndpoint ep; ep.ip = t.ip; @@ -415,6 +432,12 @@ std::vector NetworkDiscovery::run() { built[i] = std::move(ep); }); + // Cancelled during the identify phase: ``built`` holds default-constructed + // (empty) entries for the hits no worker reached, which is not a result set. + if (cancelled && cancelled()) { + return {}; + } + // 4. Dedup by ApplicationUri (fallback ip:port), sequentially over the // deterministic order so the lowest ip:port always wins. std::vector ordered; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index 884d599ae..355570fa4 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -604,6 +604,13 @@ void OpcuaPlugin::set_context(PluginContext & context) { node_map_.set_component_identity(ci.id, ci.name); log_info("Component identity derived from device: id='" + ci.id + "', name='" + ci.name + "'"); } + // Which session this identity speaks for. 0 when the connect failed: the id + // above then came from the fallback endpoint and an empty DeviceInfo, so it + // is provisional and the poll thread re-derives it on the first session + // (maybe_rederive_component_identity). Without that a gateway which started + // before its PLC would serve the placeholder for the life of the process, + // even after discovery adopted the real server. + component_identity_generation_ = connected ? client_->connection_generation() : 0; // Zero-config native A&C: with no node map and no explicit auto_alarms // block, subscribe the Server EventNotifier by default so discovered @@ -666,10 +673,16 @@ void OpcuaPlugin::set_context(PluginContext & context) { // startup scan is the only one that ever runs, so a gateway that scanned // while its PLC was still booting retries the fallback endpoint forever and // only a restart finds the PLC. - if (effective_rescan_interval_s(discovery_config_, endpoint_configured_) > 0) { + const int rescan_interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); + if (rescan_interval_s > 0) { poller_config_.rediscover_endpoint = [this]() { return rescan_endpoint_for_reconnect(); }; + // The rescan is consulted once per reconnect attempt, so the real adoption + // cadence is max(interval_s, backoff). Cap the backoff at the cadence so + // the documented "re-scan every interval_s while down" is the true one. + poller_config_.max_reconnect_interval = effective_max_reconnect_wait( + poller_config_.reconnect_interval, poller_config_.max_reconnect_interval, rescan_interval_s); } poller_->start(poller_config_); log_info("OPC-UA poller started (mode: " + std::string(poller_->using_subscriptions() ? "subscription" : "poll") + @@ -1035,7 +1048,11 @@ void OpcuaPlugin::on_alarm_change(const std::string & entity_id, send_report_fault(entity_id, signal.fault_code, signal.severity, signal.message); } else { log_info("Alarm cleared: " + signal.fault_code + " on " + entity_id); - send_clear_fault(signal.fault_code); + // The poller's own comms-lost clear on a successful reconnect is the same + // link-state clear as the connect-time one, so it does not cascade either. + // Every other code is a real alarm going inactive on the device and keeps + // the default correlation behaviour. + send_clear_fault(signal.fault_code, /*link_state=*/signal.fault_code == kCommsLostFaultCode); } } @@ -1249,23 +1266,36 @@ void OpcuaPlugin::send_report_fault(const std::string & entity_id, const std::st request->severity = ros2_medkit_msgs::msg::Fault::SEVERITY_INFO; } - send_or_buffer([this, request]() { - fault_clients_->report->async_send_request(request); - }); + send_or_buffer({PendingFaultDispatch::Kind::Report, fault_code, [this, request]() { + fault_clients_->report->async_send_request(request); + }}); +} + +ros2_medkit_msgs::srv::ClearFault::Request OpcuaPlugin::make_clear_fault_request(const std::string & fault_code, + bool link_state) { + ros2_medkit_msgs::srv::ClearFault::Request request; + request.fault_code = fault_code; + // A link-state clear reports that the OPC-UA session came back. It is not an + // operator resolving a root cause, so it must not trip the correlation + // engine's auto_clear_with_root cascade: a rule naming PLC_COMMS_LOST as the + // root cause would otherwise clear every symptom fault the outage produced, + // none of which this plugin has any evidence about. + request.skip_correlation_auto_clear = link_state; + return request; } -void OpcuaPlugin::send_clear_fault(const std::string & fault_code) { +void OpcuaPlugin::send_clear_fault(const std::string & fault_code, bool link_state) { if (!fault_clients_->clear) { log_warn("ClearFault service client not available"); return; } - auto request = std::make_shared(); - request->fault_code = fault_code; + auto request = + std::make_shared(make_clear_fault_request(fault_code, link_state)); - send_or_buffer([this, request]() { - fault_clients_->clear->async_send_request(request); - }); + send_or_buffer({PendingFaultDispatch::Kind::Clear, fault_code, [this, request]() { + fault_clients_->clear->async_send_request(request); + }}); } void OpcuaPlugin::clear_comms_lost_on_connect() { @@ -1276,26 +1306,69 @@ void OpcuaPlugin::clear_comms_lost_on_connect() { // fire-and-forget, so a "Fault not found" answer for a code that was never // raised costs nothing here and is the normal case on a healthy start. log_info(std::string("OPC-UA connection established; clearing any standing ") + kCommsLostFaultCode); - send_clear_fault(kCommsLostFaultCode); + send_clear_fault(kCommsLostFaultCode, /*link_state=*/true); } -void OpcuaPlugin::send_or_buffer(std::function dispatch) { +OpcuaPlugin::PendingEnqueueOutcome OpcuaPlugin::enqueue_pending_dispatch(std::vector & buffer, + size_t max_size, PendingFaultDispatch entry) { + const bool is_clear = entry.kind == PendingFaultDispatch::Kind::Clear; + + // At most one pending clear per fault code. A repeat moves to the BACK rather + // than overwriting in place, so an interleaved report-then-clear for the same + // code still flushes in the order the PLC produced it. + bool replaced = false; + if (is_clear) { + const auto same_code = std::find_if(buffer.begin(), buffer.end(), [&entry](const PendingFaultDispatch & pending) { + return pending.kind == PendingFaultDispatch::Kind::Clear && pending.fault_code == entry.fault_code; + }); + if (same_code != buffer.end()) { + buffer.erase(same_code); + replaced = true; + } + } + + PendingEnqueueOutcome outcome = replaced ? PendingEnqueueOutcome::ReplacedClear : PendingEnqueueOutcome::Buffered; + if (buffer.size() >= max_size) { + // A report is a one-shot edge from the PLC that nothing will re-send; a + // clear is re-derivable from the next reconnect. So a full buffer gives up a + // pending clear first, and refuses an incoming clear rather than evicting a + // report for it. + const auto oldest_clear = std::find_if(buffer.begin(), buffer.end(), [](const PendingFaultDispatch & pending) { + return pending.kind == PendingFaultDispatch::Kind::Clear; + }); + if (oldest_clear != buffer.end()) { + buffer.erase(oldest_clear); + outcome = PendingEnqueueOutcome::EvictedClear; + } else if (is_clear) { + return PendingEnqueueOutcome::Refused; + } else { + buffer.erase(buffer.begin()); + outcome = PendingEnqueueOutcome::EvictedReport; + } + } + + buffer.push_back(std::move(entry)); + return outcome; +} + +void OpcuaPlugin::send_or_buffer(PendingFaultDispatch entry) { // Bound the buffer so a deployment with no fault_manager cannot grow it - // without limit; drop the oldest (least relevant) pending dispatch. - // Runs on both the poll thread and the REST clear_fault thread, so the vector - // mutation is serialised by pending_reports_mutex_. - constexpr size_t kMaxPendingReports = 256; - bool dropped_oldest = false; + // without limit. Runs on both the poll thread and the REST clear_fault thread, + // so the vector mutation is serialised by pending_reports_mutex_. + PendingEnqueueOutcome outcome = PendingEnqueueOutcome::Buffered; { std::lock_guard lock(pending_reports_mutex_); - if (pending_reports_.size() >= kMaxPendingReports) { - pending_reports_.erase(pending_reports_.begin()); - dropped_oldest = true; - } - pending_reports_.push_back(std::move(dispatch)); - } - if (dropped_oldest) { - log_warn("pending fault report buffer full (" + std::to_string(kMaxPendingReports) + "), dropping oldest"); + outcome = enqueue_pending_dispatch(pending_reports_, kMaxPendingDispatches, std::move(entry)); + } + if (outcome == PendingEnqueueOutcome::EvictedReport) { + log_warn("pending fault dispatch buffer full (" + std::to_string(kMaxPendingDispatches) + + "), dropping the oldest report"); + } else if (outcome == PendingEnqueueOutcome::EvictedClear) { + log_warn("pending fault dispatch buffer full (" + std::to_string(kMaxPendingDispatches) + + "), dropping the oldest pending clear"); + } else if (outcome == PendingEnqueueOutcome::Refused) { + log_warn("pending fault dispatch buffer full of reports (" + std::to_string(kMaxPendingDispatches) + + "), dropping this clear instead of a report"); } // Drains immediately (in order) if the sink is already matched. flush_pending_reports(); @@ -1311,7 +1384,7 @@ void OpcuaPlugin::flush_pending_reports() { // the vector is never being reallocated by a concurrent send_or_buffer while it // is iterated here (the use-after-free that corrupted the heap), and so the ROS // service call never runs under the mutex. - std::vector> batch; + std::vector batch; { std::lock_guard lock(pending_reports_mutex_); if (pending_reports_.empty()) { @@ -1319,8 +1392,8 @@ void OpcuaPlugin::flush_pending_reports() { } batch.swap(pending_reports_); } - for (auto & dispatch : batch) { - dispatch(); + for (auto & entry : batch) { + entry.dispatch(); } } @@ -1380,6 +1453,56 @@ void OpcuaPlugin::run_auto_browse() { (result.depth_cap_hit ? " [depth cap reached on at least one branch]" : "")); } +std::optional OpcuaPlugin::rederived_component_identity(const std::string & current_id, + const OpcuaClient::DeviceInfo & info, + const std::string & endpoint_url) { + const ComponentIdentity ci = derive_component_identity(info, endpoint_url); + if (ci.id.empty() || ci.id == current_id) { + return std::nullopt; + } + return ci; +} + +void OpcuaPlugin::maybe_rederive_component_identity() { + // An explicit node map owns the component name; only the config-less path + // derives it from the device. + if (!node_map_path_.empty() || !client_ || !client_->is_connected()) { + return; + } + const uint64_t generation = client_->connection_generation(); + if (generation == component_identity_generation_) { + return; // identity already speaks for this session + } + + const std::string live_endpoint = client_->endpoint_url(); + const auto rederived = + rederived_component_identity(node_map_.component_id(), client_->read_device_info(), live_endpoint); + component_identity_generation_ = generation; + if (!rederived) { + return; + } + + const std::string previous_id = node_map_.component_id(); + { + // Serialize against the REST read paths, which hold references into + // node_map_ (entity_defs) while they answer. + std::unique_lock lock(node_map_mutex_); + // The auto_alarms fallback entity is derived from the component id, so a + // default-derived one has to follow the rename. An operator-chosen entity_id + // does not match the derived form and is left alone. + auto & auto_alarms = node_map_.mutable_auto_alarms(); + if (auto_alarms.entity_id == previous_id + "_alarms") { + auto_alarms.entity_id.clear(); + } + node_map_.set_component_identity(rederived->id, rederived->name); + // Re-derives the fallback entity id and rebuilds entity_defs, so every + // reference to the component id moves together. + node_map_.finalize_auto_alarms_overlay(); + } + log_info("Component identity re-derived from the adopted device at " + live_endpoint + ": id='" + rederived->id + + "', name='" + rederived->name + "' (was '" + previous_id + "')"); +} + void OpcuaPlugin::maybe_rebrowse_on_reconnect() { if (!node_map_.auto_browse_config().enabled || !client_ || !client_->is_connected()) { return; @@ -1398,6 +1521,11 @@ void OpcuaPlugin::publish_values(const PollSnapshot & snap) { // Poll-thread hook: drain any fault reports buffered before fault_manager was // discovered, so a late sink still receives them. flush_pending_reports(); + // Poll-thread hook: re-derive the config-less component identity from the + // device once a session is up, so an adopted PLC stops being served under the + // provisional endpoint-derived id. Runs BEFORE the re-walk below, which + // rebuilds entity_defs off the component id. + maybe_rederive_component_identity(); // Poll-thread hook: (re)run auto_browse after a fresh session so a PLC that // came up (or restarted) after the initial connect still gets walked. maybe_rebrowse_on_reconnect(); @@ -1496,13 +1624,44 @@ int OpcuaPlugin::effective_rescan_interval_s(const OpcuaDiscoveryConfig & config if (!config.enabled || endpoint_configured) { return 0; } - return config.interval_s > 0 ? config.interval_s : kDefaultRescanIntervalS; + // Unset means "discovery is on but no cadence was stated" - the config-less + // deployment, which takes the built-in default. An explicit 0 is an operator + // saying "do not re-scan", and is honoured as written. + return config.interval_s.value_or(kDefaultRescanIntervalS); +} + +std::chrono::milliseconds OpcuaPlugin::effective_max_reconnect_wait(std::chrono::milliseconds base, + std::chrono::milliseconds default_ceiling, + int rescan_interval_s) { + if (rescan_interval_s <= 0) { + return default_ceiling; // no rescan: the plain backoff ceiling applies + } + const auto cadence = std::chrono::milliseconds(static_cast(rescan_interval_s) * 1000); + return std::max(base, std::min(default_ceiling, cadence)); +} + +std::optional OpcuaPlugin::rescan_step(int interval_s, + const std::function & now, + std::chrono::steady_clock::time_point * last_scan_end, + const std::function()> & sweep) { + if (interval_s <= 0 || !now || last_scan_end == nullptr || !sweep) { + return std::nullopt; + } + if (now() - *last_scan_end < std::chrono::seconds(interval_s)) { + return std::nullopt; + } + const auto result = sweep(); + // Stamp the END of the sweep: a legal /16 runs for minutes, and stamping its + // start would make the next one due the moment this one returned - the poll + // thread would sweep back to back and only attempt a reconnect once a sweep. + *last_scan_end = now(); + return result; } std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryConfig & config, bool endpoint_configured, const PortScanFn & scan, const IdentifyFn & identify, - const std::function & log_info, - const std::function & log_warn) { + const DiscoveryReporter & reporter, + const std::function & cancelled) { if (!config.enabled) { return std::nullopt; } @@ -1512,21 +1671,56 @@ std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryCo return std::nullopt; } + // The pass reports through a buffer rather than straight to the log: whether + // the report is operator-visible or a DEBUG trace depends on the outcome, + // which is only known once the sweep is done. A rescan runs for the life of a + // disconnected process, so an unchanged outcome must not repeat its whole + // report (a secured-only site would log the same WARN every interval_s). + struct ReportLine { + bool warning; + std::string text; + }; + std::vector report; + std::string outcome; + const auto info_line = [&report, &outcome](const std::string & text) { + report.push_back({false, text}); + outcome += text; + outcome += '\n'; + }; + const auto warn_line = [&report, &outcome](const std::string & text) { + report.push_back({true, text}); + outcome += text; + outcome += '\n'; + }; + const auto emit = [&report, &outcome, &reporter]() { + const bool repeat = reporter.previous_outcome != nullptr && *reporter.previous_outcome == outcome; + if (reporter.previous_outcome != nullptr) { + *reporter.previous_outcome = outcome; + } + for (const auto & line : report) { + const auto & sink = repeat ? reporter.debug : (line.warning ? reporter.warn : reporter.info); + if (sink) { + sink(line.text); + } + } + }; + NetworkDiscovery discovery(config, scan, identify); const auto subnets = discovery.resolve_subnets(); if (subnets.empty()) { - log_warn("OPC-UA discovery: no subnet configured and could not derive a local /24; nothing to scan."); + warn_line("OPC-UA discovery: no subnet configured and could not derive a local /24; nothing to scan."); + emit(); return std::nullopt; } std::string subnet_list; for (const auto & s : subnets) { subnet_list += (subnet_list.empty() ? "" : ", ") + s; } - log_info("OPC-UA discovery: read-only active scan of [" + subnet_list + "] on " + - std::to_string(config.ports.size()) + " port(s)..."); + info_line("OPC-UA discovery: read-only active scan of [" + subnet_list + "] on " + + std::to_string(config.ports.size()) + " port(s)..."); - const std::vector found = discovery.run(); + const std::vector found = discovery.run(cancelled); // Summarize what was found and what was skipped (leads, LDS, secured-only). size_t data_servers = 0; @@ -1550,23 +1744,25 @@ std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryCo if (!ep.anonymous_none_available) { ++secured_only; } - log_info("OPC-UA discovery: found data server " + ep.endpoint_url + " (uri='" + ep.application_uri + - "', product='" + ep.product_uri + "', None/Anonymous=" + (ep.anonymous_none_available ? "yes" : "no") + - ")"); + info_line("OPC-UA discovery: found data server " + ep.endpoint_url + " (uri='" + ep.application_uri + + "', product='" + ep.product_uri + "', None/Anonymous=" + (ep.anonymous_none_available ? "yes" : "no") + + ")"); } - log_info("OPC-UA discovery summary: " + std::to_string(data_servers) + " data server(s), " + - std::to_string(discovery_servers) + " discovery server(s)/LDS, " + std::to_string(secured_only) + - " secured-only (need credentials), " + std::to_string(leads) + " non-OPC-UA/unidentified lead(s)."); + info_line("OPC-UA discovery summary: " + std::to_string(data_servers) + " data server(s), " + + std::to_string(discovery_servers) + " discovery server(s)/LDS, " + std::to_string(secured_only) + + " secured-only (need credentials), " + std::to_string(leads) + " non-OPC-UA/unidentified lead(s)."); const DiscoveredEndpoint * chosen = NetworkDiscovery::select_auto_endpoint(found, config.anonymous_none_only); if (chosen == nullptr) { - log_warn( + warn_line( "OPC-UA discovery: no auto-connectable None/Anonymous data server found; leaving the endpoint unchanged. " "Secured-only servers require operator credentials."); + emit(); return std::nullopt; } - log_info("OPC-UA discovery: selected endpoint " + chosen->endpoint_url + " (uri='" + chosen->application_uri + "')"); + info_line("OPC-UA discovery: selected endpoint " + chosen->endpoint_url + " (uri='" + chosen->application_uri + "')"); + emit(); return chosen->endpoint_url; } @@ -1580,26 +1776,35 @@ void OpcuaPlugin::run_startup_discovery() { return; } - // Stamp the scan before running it: the rescan cadence measures the gap - // between the START of two sweeps, so a slow sweep does not immediately earn + // The startup scan is always reported in full (no previous outcome to compare + // against) and always cancellable, so a shutdown during set_context does not + // wait out a whole sweep. + const auto chosen = discover_endpoint(discovery_config_, endpoint_configured_, discovery_scan_fn_, + discovery_identify_fn_, discovery_reporter(&last_discovery_outcome_), [this]() { + return shutdown_requested_.load(); + }); + // Stamp when the sweep FINISHED: the rescan cadence is measured from the end + // of the previous sweep, so a long sweep is not immediately followed by // another one. - last_discovery_scan_ = std::chrono::steady_clock::now(); - const auto chosen = discover_endpoint( - discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, - [this](const std::string & m) { - log_info(m); - }, - [this](const std::string & m) { - log_warn(m); - }); + last_discovery_scan_end_ = std::chrono::steady_clock::now(); if (!chosen) { // The startup scan can legitimately find nothing - a gateway that boots - // alongside its PLC routinely scans while the PLC is still coming up. The - // endpoint stays at its default and the poller's reconnect arm rescans on - // the cadence below, so this is a delay rather than a dead end. - log_info("OPC-UA discovery: startup scan selected no endpoint; the reconnect loop rescans every " + - std::to_string(effective_rescan_interval_s(discovery_config_, endpoint_configured_)) + "s while down."); + // alongside its PLC routinely scans while the PLC is still coming up. With a + // cadence the endpoint stays at its default and the reconnect arm rescans, + // so this is a delay rather than a dead end. With re-scanning switched off + // (an explicit interval_s: 0) it IS the end, and the operator has to be told + // which of the two they configured. + const int startup_interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); + if (startup_interval_s > 0) { + log_info("OPC-UA discovery: startup scan selected no endpoint; the reconnect loop rescans every " + + std::to_string(startup_interval_s) + "s while down."); + } else { + log_warn( + "OPC-UA discovery: startup scan selected no endpoint and re-scanning is off (interval_s: 0); the endpoint " + "stays at " + + client_config_.endpoint_url + " until the plugin is restarted."); + } return; } @@ -1607,6 +1812,21 @@ void OpcuaPlugin::run_startup_discovery() { log_info("OPC-UA discovery: auto-selected endpoint " + *chosen + " - handing to the connect + introspect path."); } +OpcuaPlugin::DiscoveryReporter OpcuaPlugin::discovery_reporter(std::string * previous_outcome) const { + DiscoveryReporter reporter; + reporter.info = [this](const std::string & m) { + log_info(m); + }; + reporter.warn = [this](const std::string & m) { + log_warn(m); + }; + reporter.debug = [](const std::string & m) { + RCLCPP_DEBUG(opcua_plugin_logger(), "%s", m.c_str()); + }; + reporter.previous_outcome = previous_outcome; + return reporter; +} + std::optional OpcuaPlugin::rescan_endpoint_for_reconnect() { // A sweep is a bounded but multi-second blocking call on the poll thread, and // stop() has to wait for whatever it is in the middle of. Do not start one the @@ -1615,23 +1835,18 @@ std::optional OpcuaPlugin::rescan_endpoint_for_reconnect() { return std::nullopt; } const int interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); - if (interval_s <= 0) { - return std::nullopt; - } - - const auto now = std::chrono::steady_clock::now(); - if (now - last_discovery_scan_ < std::chrono::seconds(interval_s)) { - return std::nullopt; - } - last_discovery_scan_ = now; - const auto chosen = discover_endpoint( - discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, - [this](const std::string & m) { - log_info(m); + const auto chosen = rescan_step( + interval_s, + []() { + return std::chrono::steady_clock::now(); }, - [this](const std::string & m) { - log_warn(m); + &last_discovery_scan_end_, + [this]() { + return discover_endpoint(discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, + discovery_reporter(&last_discovery_outcome_), [this]() { + return shutdown_requested_.load(); + }); }); // The live client config, not client_config_: this runs on the poll thread // and client_config_ is read by the refresh thread in introspect(). The diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp index d972a3e05..019d0604c 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp @@ -1155,6 +1155,11 @@ OpcuaPoller::adopt_rediscovered_endpoint(const std::string & current, return found; } +std::chrono::milliseconds OpcuaPoller::next_reconnect_wait(std::chrono::milliseconds current, + std::chrono::milliseconds max_wait) { + return std::min(current * 2, max_wait); +} + void OpcuaPoller::emit_comms_lost(bool active) { ros2_medkit::fault_detection::FaultSignal signal; signal.fault_code = kCommsLostFaultCode; @@ -1170,7 +1175,6 @@ void OpcuaPoller::emit_comms_lost(bool active) { void OpcuaPoller::poll_loop() { auto reconnect_wait = config_.reconnect_interval; - constexpr auto max_reconnect_wait = std::chrono::milliseconds(60000); while (running_.load()) { // Handle reconnection @@ -1244,14 +1248,16 @@ void OpcuaPoller::poll_loop() { comms_lost_raised_ = true; } } - // Exponential backoff capped at 60s. condition_variable so stop() wakes immediately. + // Exponential backoff, capped at config_.max_reconnect_interval (60 s by + // default, the rescan cadence while the reconnect arm also rescans). + // condition_variable so stop() wakes immediately. { std::unique_lock lock(stop_mutex_); stop_cv_.wait_for(lock, reconnect_wait, [this] { return !running_.load(); }); } - reconnect_wait = std::min(reconnect_wait * 2, max_reconnect_wait); + reconnect_wait = next_reconnect_wait(reconnect_wait, config_.max_reconnect_interval); continue; } } diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp index 03643beb3..96c32e339 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -106,7 +107,32 @@ TEST(ParseDiscoveryConfig, DefaultsDisabled) { ASSERT_EQ(cfg.ports.size(), 1u); EXPECT_EQ(cfg.ports[0], 4840); EXPECT_TRUE(cfg.anonymous_none_only); - EXPECT_EQ(cfg.interval_s, 0); + // Unset, NOT 0: an absent key means "no cadence stated" (the caller takes its + // built-in default), while an explicit 0 means "never re-scan". + EXPECT_FALSE(cfg.interval_s.has_value()); +} + +TEST(ParseDiscoveryConfig, ExplicitZeroIntervalIsKeptAsAnExplicitZero) { + std::vector warnings; + const auto cfg = parse_discovery_config(nlohmann::json{{"interval_s", 0}}, [&](const std::string & m) { + warnings.push_back(m); + }); + ASSERT_TRUE(cfg.interval_s.has_value()); + EXPECT_EQ(*cfg.interval_s, 0); + EXPECT_TRUE(warnings.empty()); +} + +TEST(ParseDiscoveryConfig, NegativeIntervalWarnsAndLeavesTheCadenceUnset) { + std::vector warnings; + const auto cfg = parse_discovery_config(nlohmann::json{{"interval_s", -5}}, [&](const std::string & m) { + warnings.push_back(m); + }); + EXPECT_FALSE(cfg.interval_s.has_value()); + ASSERT_EQ(warnings.size(), 1u); + EXPECT_NE(warnings[0].find("interval_s"), std::string::npos); + // The warning must not tell the operator the kept default is one-shot: an + // unset interval re-scans on the built-in cadence, only an explicit 0 stops. + EXPECT_EQ(warnings[0].find("0 = one-shot"), std::string::npos) << warnings[0]; } TEST(ParseDiscoveryConfig, ReadsAllKnownKeys) { @@ -133,7 +159,8 @@ TEST(ParseDiscoveryConfig, ReadsAllKnownKeys) { EXPECT_EQ(cfg.connect_timeout_ms, 300); EXPECT_EQ(cfg.scan_concurrency, 64); EXPECT_EQ(cfg.identify_timeout_ms, 2000); - EXPECT_EQ(cfg.interval_s, 900); + ASSERT_TRUE(cfg.interval_s.has_value()); + EXPECT_EQ(*cfg.interval_s, 900); EXPECT_FALSE(cfg.anonymous_none_only); EXPECT_TRUE(warnings.empty()); } @@ -377,6 +404,91 @@ TEST(NetworkDiscoveryRun, IdentifyFailureRecordedAsLead) { EXPECT_EQ(NetworkDiscovery::select_auto_endpoint(eps, true), nullptr); } +// --------------------------------------------------------------------------- // +// run(cancelled): a shutdown must not wait out a whole sweep +// --------------------------------------------------------------------------- // +TEST(NetworkDiscoveryRun, CancelStopsTheSweepInsteadOfProbingEveryHost) { + // A /24 is 254 probes and a legal /16 is 65k; the caller runs them on the poll + // thread a shutdown has to join. With scan_concurrency 1 the sweep is + // sequential, so the probe count is exactly what the cancel predicate allowed. + std::atomic probes{0}; + std::atomic stop{false}; + auto scan = [&probes, &stop](const std::string &, uint16_t, int) { + if (probes.fetch_add(1) + 1 >= 5) { + stop.store(true); // the shutdown flag flipping mid-sweep + } + return false; + }; + OpcuaDiscoveryConfig cfg; + cfg.enabled = true; + cfg.subnets = {"192.168.1.0/24"}; + cfg.ports = {4840}; + cfg.scan_concurrency = 1; + + NetworkDiscovery disc(cfg, scan, make_identify({})); + const auto eps = disc.run([&stop]() { + return stop.load(); + }); + + EXPECT_TRUE(eps.empty()); + // One in-flight probe per worker may still complete after the flag flips. + EXPECT_GE(probes.load(), 5); + EXPECT_LE(probes.load(), 6) << "the sweep kept probing after it was cancelled"; +} + +TEST(NetworkDiscoveryRun, WithoutCancellationEveryHostIsStillProbed) { + // Positive control for the test above on the same harness: the identical + // sweep with no cancel predicate visits all 254 hosts, so a low probe count + // there is the cancellation and not a broken fake. + std::atomic probes{0}; + auto scan = [&probes](const std::string &, uint16_t, int) { + probes.fetch_add(1); + return false; + }; + OpcuaDiscoveryConfig cfg; + cfg.enabled = true; + cfg.subnets = {"192.168.1.0/24"}; + cfg.ports = {4840}; + cfg.scan_concurrency = 1; + + NetworkDiscovery disc(cfg, scan, make_identify({})); + const auto eps = disc.run(); + EXPECT_TRUE(eps.empty()); + EXPECT_EQ(probes.load(), 254); +} + +TEST(NetworkDiscoveryRun, CancelBetweenSweepAndIdentifySkipsTheIdentifyRoundTrips) { + // The identify phase is a separate batch, and each GetEndpoints blocks for up + // to identify_timeout_ms. A cancel that arrives once the sweep is done must + // not still pay for one round-trip per hit. + std::atomic stop{false}; + auto scan = [&stop](const std::string & ip, uint16_t, int) { + const bool hit = ip == "192.168.1.10" || ip == "192.168.1.11"; + if (ip == "192.168.1.254") { + stop.store(true); // sweep finished, shutdown requested + } + return hit; + }; + std::atomic identifies{0}; + auto identify = [&identifies](const std::string &, int) { + identifies.fetch_add(1); + return IdentifyResult{}; + }; + + OpcuaDiscoveryConfig cfg; + cfg.enabled = true; + cfg.subnets = {"192.168.1.0/24"}; + cfg.ports = {4840}; + cfg.scan_concurrency = 1; + + NetworkDiscovery disc(cfg, scan, identify); + const auto eps = disc.run([&stop]() { + return stop.load(); + }); + EXPECT_TRUE(eps.empty()); + EXPECT_EQ(identifies.load(), 0); +} + // --------------------------------------------------------------------------- // // select_auto_endpoint // --------------------------------------------------------------------------- // diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index f47108005..9984cef85 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -692,6 +692,15 @@ OpcuaDiscoveryConfig rescan_cfg() { // Discards log output. The tests assert on the selected endpoint, not the text. const std::function kSilent = [](const std::string &) {}; +// Silent reporter: no repeat-suppression memory, so every pass reports in full +// (into the void). Tests that assert on the log build their own. +OpcuaPlugin::DiscoveryReporter silent_reporter() { + OpcuaPlugin::DiscoveryReporter reporter; + reporter.info = kSilent; + reporter.warn = kSilent; + return reporter; +} + } // namespace TEST(DiscoverEndpoint, ScanBeforeThePlcIsUpSelectsNothingAndALaterRescanAdoptsIt) { @@ -699,14 +708,14 @@ TEST(DiscoverEndpoint, ScanBeforeThePlcIsUpSelectsNothingAndALaterRescanAdoptsIt // booting. Nothing answers, so nothing is selected and the caller keeps the // default endpoint. const auto empty_pass = OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({}), - fake_identify({}), kSilent, kSilent); + fake_identify({}), silent_reporter()); EXPECT_FALSE(empty_pass.has_value()); // The PLC finishes booting. The same call with the same config now finds it, // which is what the reconnect arm applies to the next connect attempt. const auto later_pass = OpcuaPlugin::discover_endpoint( rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), - fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), kSilent, kSilent); + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), silent_reporter()); ASSERT_TRUE(later_pass.has_value()); EXPECT_EQ(*later_pass, "opc.tcp://192.168.1.10:4840"); } @@ -717,7 +726,7 @@ TEST(DiscoverEndpoint, AnExplicitEndpointIsNeverRescanned) { // endpoint. Discovery must not open a second session on a polled PLC. const auto chosen = OpcuaPlugin::discover_endpoint( rescan_cfg(), /*endpoint_configured=*/true, fake_scan({"192.168.1.10:4840"}), - fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), kSilent, kSilent); + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), silent_reporter()); EXPECT_FALSE(chosen.has_value()); } @@ -730,7 +739,7 @@ TEST(DiscoverEndpoint, DisabledDiscoveryScansNothing) { return true; }; const auto chosen = OpcuaPlugin::discover_endpoint(cfg, /*endpoint_configured=*/false, counting_scan, - fake_identify({}), kSilent, kSilent); + fake_identify({}), silent_reporter()); EXPECT_FALSE(chosen.has_value()); EXPECT_FALSE(scanned) << "a disabled discovery must not touch the network"; } @@ -750,6 +759,372 @@ TEST(EffectiveRescanInterval, DefaultsWhenDiscoveryIsOnWithNoCadenceAndIsOffOthe EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(cfg, false), 0); } +TEST(EffectiveRescanInterval, ExplicitZeroKeepsDiscoveryOnAndStopsRescanning) { + // The three states an operator can be in, all with discovery enabled and no + // endpoint pinned. + OpcuaDiscoveryConfig unset = rescan_cfg(); // (1) unset -> the built-in cadence + EXPECT_FALSE(unset.interval_s.has_value()); + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(unset, false), OpcuaPlugin::kDefaultRescanIntervalS); + + OpcuaDiscoveryConfig explicit_zero = rescan_cfg(); // (2) explicit 0 -> one-shot + explicit_zero.interval_s = 0; + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(explicit_zero, false), 0) + << "an explicit interval_s: 0 must stop the rescan, not fall back to the default"; + + // (3) A negative value never reaches here: the parse warns and leaves the + // cadence unset, so what arrives is case (1). + std::vector warnings; + const auto parsed = + parse_discovery_config(nlohmann::json{{"enabled", true}, {"interval_s", -1}}, [&warnings](const std::string & m) { + warnings.push_back(m); + }); + EXPECT_EQ(warnings.size(), 1u); + EXPECT_EQ(OpcuaPlugin::effective_rescan_interval_s(parsed, false), OpcuaPlugin::kDefaultRescanIntervalS); +} + +// --------------------------------------------------------------------------- +// Rescan cadence: measured from the END of the previous sweep +// --------------------------------------------------------------------------- + +TEST(RescanStep, SpacesSweepsFromTheEndOfThePreviousOne) { + // A legal /16 sweep runs for minutes. With the cadence stamped at the START, + // the next sweep is due the instant the current one returns, so the poll + // thread sweeps back to back and the reconnect attempt drops to one a sweep. + const auto t0 = std::chrono::steady_clock::time_point{}; + const auto sweep_duration = std::chrono::seconds(390); // a /16 at the defaults + auto clock_now = t0; + const auto now = [&clock_now]() { + return clock_now; + }; + + int sweeps = 0; + const auto sweep = [&sweeps, &clock_now, sweep_duration]() -> std::optional { + ++sweeps; + clock_now += sweep_duration; // the sweep blocks for its whole duration + return std::nullopt; + }; + + auto last_end = t0; + clock_now = t0 + std::chrono::seconds(30); + OpcuaPlugin::rescan_step(30, now, &last_end, sweep); + ASSERT_EQ(sweeps, 1); + EXPECT_EQ(last_end, clock_now) << "the cadence must be stamped when the sweep finished"; + + // One second after the sweep returned: not due, even though it STARTED 391 s + // ago. + clock_now += std::chrono::seconds(1); + OpcuaPlugin::rescan_step(30, now, &last_end, sweep); + EXPECT_EQ(sweeps, 1) << "a rescan ran less than one interval after the previous sweep ended"; + + // A full interval after the end: due again. + clock_now += std::chrono::seconds(29); + OpcuaPlugin::rescan_step(30, now, &last_end, sweep); + EXPECT_EQ(sweeps, 2); +} + +TEST(RescanStep, DoesNothingWithoutACadence) { + const auto t0 = std::chrono::steady_clock::time_point{}; + auto last_end = t0; + int sweeps = 0; + const auto now = [t0]() { + return t0 + std::chrono::hours(1); + }; + const auto sweep = [&sweeps]() -> std::optional { + ++sweeps; + return std::string("opc.tcp://192.168.1.10:4840"); + }; + // 0 is the operator's "do not re-scan" (and also discovery off / endpoint + // pinned, both of which effective_rescan_interval_s maps to 0). + EXPECT_FALSE(OpcuaPlugin::rescan_step(0, now, &last_end, sweep).has_value()); + EXPECT_EQ(sweeps, 0); + // Positive control on the same harness: with a cadence the very same call + // sweeps and hands the endpoint back. + const auto adopted = OpcuaPlugin::rescan_step(30, now, &last_end, sweep); + ASSERT_TRUE(adopted.has_value()); + EXPECT_EQ(*adopted, "opc.tcp://192.168.1.10:4840"); + EXPECT_EQ(sweeps, 1); +} + +// --------------------------------------------------------------------------- +// Reconnect backoff ceiling while the reconnect arm also rescans +// --------------------------------------------------------------------------- + +TEST(EffectiveMaxReconnectWait, CapsTheBackoffAtTheRescanCadence) { + using namespace std::chrono_literals; + // No rescan: the plain 60 s ceiling. + EXPECT_EQ(OpcuaPlugin::effective_max_reconnect_wait(5000ms, 60000ms, /*rescan_interval_s=*/0), 60000ms); + // Rescanning every 30 s: an uncapped backoff would make the real adoption + // cadence max(30 s, 60 s), not the documented 30 s. + EXPECT_EQ(OpcuaPlugin::effective_max_reconnect_wait(5000ms, 60000ms, 30), 30000ms); + // A cadence longer than the ceiling does not raise the ceiling. + EXPECT_EQ(OpcuaPlugin::effective_max_reconnect_wait(5000ms, 60000ms, 900), 60000ms); + // A cadence shorter than the configured reconnect interval does not turn the + // backoff into a hot retry loop. + EXPECT_EQ(OpcuaPlugin::effective_max_reconnect_wait(5000ms, 60000ms, 1), 5000ms); +} + +TEST(NextReconnectWait, DoublesUpToTheCeiling) { + using namespace std::chrono_literals; + EXPECT_EQ(OpcuaPoller::next_reconnect_wait(5000ms, 60000ms), 10000ms); + EXPECT_EQ(OpcuaPoller::next_reconnect_wait(40000ms, 60000ms), 60000ms); + EXPECT_EQ(OpcuaPoller::next_reconnect_wait(60000ms, 60000ms), 60000ms); + // Capped at a 30 s rescan cadence: the wait never exceeds it, so the rescan is + // consulted every cadence instead of every max(cadence, backoff). + EXPECT_EQ(OpcuaPoller::next_reconnect_wait(20000ms, 30000ms), 30000ms); + EXPECT_EQ(OpcuaPoller::next_reconnect_wait(30000ms, 30000ms), 30000ms); +} + +// --------------------------------------------------------------------------- +// Discovery report: quiet while the outcome does not change +// --------------------------------------------------------------------------- + +TEST(DiscoverEndpoint, AnUnchangedRescanReportsAtDebugInsteadOfRepeatingItself) { + // A secured-only site rescans for the life of the process and would otherwise + // log the whole report - scan line, per-server line, summary and the + // "no auto-connectable server" WARN - every interval_s. + IdentifyResult secured = plc_identity(); + secured.anonymous_none_available = false; + + std::vector info; + std::vector warn; + std::vector debug; + std::string outcome; + OpcuaPlugin::DiscoveryReporter reporter; + reporter.info = [&info](const std::string & m) { + info.push_back(m); + }; + reporter.warn = [&warn](const std::string & m) { + warn.push_back(m); + }; + reporter.debug = [&debug](const std::string & m) { + debug.push_back(m); + }; + reporter.previous_outcome = &outcome; + + const auto pass = [&]() { + return OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", secured}}), reporter); + }; + + EXPECT_FALSE(pass().has_value()); + const size_t first_info = info.size(); + const size_t first_warn = warn.size(); + EXPECT_GT(first_info, 0u); + EXPECT_EQ(first_warn, 1u) << "the first pass always reports the secured-only outcome"; + EXPECT_TRUE(debug.empty()); + + // Same network, same outcome: nothing new at INFO/WARN, the report goes to + // the debug logger instead. + EXPECT_FALSE(pass().has_value()); + EXPECT_EQ(info.size(), first_info) << "an unchanged rescan repeated its report at INFO"; + EXPECT_EQ(warn.size(), first_warn) << "an unchanged rescan repeated its WARN"; + EXPECT_EQ(debug.size(), first_info + first_warn) << "the repeated report must still be traceable at DEBUG"; + + // The server opens up an anonymous endpoint: the outcome changed, so the + // operator hears about it at INFO again. + const auto chosen = + OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), reporter); + ASSERT_TRUE(chosen.has_value()); + EXPECT_GT(info.size(), first_info) << "a changed outcome must be reported at INFO"; +} + +TEST(DiscoverEndpoint, WithNoRepeatMemoryEveryPassIsReported) { + // Positive control for the test above: the same two identical passes with no + // previous_outcome (the startup scan's own reporter) report in full twice, so + // the silence above is the suppression and not a dead sink. + std::vector info; + OpcuaPlugin::DiscoveryReporter reporter; + reporter.info = [&info](const std::string & m) { + info.push_back(m); + }; + reporter.warn = kSilent; + + const auto pass = [&]() { + return OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), reporter); + }; + EXPECT_TRUE(pass().has_value()); + const size_t first = info.size(); + EXPECT_GT(first, 0u); + EXPECT_TRUE(pass().has_value()); + EXPECT_EQ(info.size(), 2 * first); +} + +// --------------------------------------------------------------------------- +// Config-less component identity across an adoption +// --------------------------------------------------------------------------- + +TEST(RederivedComponentIdentity, AdoptionReplacesTheProvisionalEndpointDerivedId) { + // The config-less race, end to end over the derivation path: the gateway + // starts before the PLC, its start-up scan finds nothing, and the identity is + // derived from the fallback endpoint plus an empty DeviceInfo. + const auto startup_pass = OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({}), + fake_identify({}), silent_reporter()); + ASSERT_FALSE(startup_pass.has_value()); + const std::string fallback_endpoint = "opc.tcp://localhost:4840"; // OpcuaClientConfig's default + const ComponentIdentity provisional = derive_component_identity(OpcuaClient::DeviceInfo{}, fallback_endpoint); + EXPECT_EQ(provisional.id, "opcua-localhost"); + + // The PLC finishes booting and the rescan adopts it. + const auto adopted = OpcuaPlugin::discover_endpoint( + rescan_cfg(), /*endpoint_configured=*/false, fake_scan({"192.168.1.10:4840"}), + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), silent_reporter()); + ASSERT_TRUE(adopted.has_value()); + + // The session is up, so the device can finally name itself: the component + // must stop being served under the placeholder. + OpcuaClient::DeviceInfo info; + info.di_manufacturer = "Siemens AG"; + info.di_model = "CPU 1505SP F"; + const auto rederived = OpcuaPlugin::rederived_component_identity(provisional.id, info, *adopted); + ASSERT_TRUE(rederived.has_value()) << "an adopted device with a nameplate must replace opcua-localhost"; + EXPECT_EQ(rederived->id, "siemens_ag_cpu_1505sp_f"); + EXPECT_EQ(rederived->name, "Siemens AG CPU 1505SP F"); +} + +TEST(RederivedComponentIdentity, KeepsTheIdentityWhenNothingChanged) { + // Same device on a later reconnect: no rename, so no entity churn and no INFO + // line claiming an identity change that did not happen. + OpcuaClient::DeviceInfo info; + info.di_manufacturer = "Siemens AG"; + info.di_model = "CPU 1505SP F"; + EXPECT_FALSE(OpcuaPlugin::rederived_component_identity("siemens_ag_cpu_1505sp_f", info, "opc.tcp://192.168.1.10:4840") + .has_value()); + + // A nameplate-less server on an adopted endpoint still moves off the + // fallback host it was provisionally named after. + const auto host_derived = OpcuaPlugin::rederived_component_identity("opcua-localhost", OpcuaClient::DeviceInfo{}, + "opc.tcp://192.168.1.10:4840"); + ASSERT_TRUE(host_derived.has_value()); + EXPECT_EQ(host_derived->id, "opcua-192_168_1_10"); +} + +// --------------------------------------------------------------------------- +// ClearFault: a link-state clear does not cascade +// --------------------------------------------------------------------------- + +TEST(MakeClearFaultRequest, LinkStateClearSkipsTheCorrelationCascade) { + // The connect-time PLC_COMMS_LOST clear says the link came back. A + // correlation rule may name PLC_COMMS_LOST as the root cause of every symptom + // the outage produced, and clearing those is an operator's call, not a link + // event's. + const auto link_state = OpcuaPlugin::make_clear_fault_request(kCommsLostFaultCode, /*link_state=*/true); + EXPECT_EQ(link_state.fault_code, kCommsLostFaultCode); + EXPECT_TRUE(link_state.skip_correlation_auto_clear); + + // Positive control on the same request builder: an operator-driven clear (the + // SOVD DELETE route) leaves the cascade alone, so the flag above is the + // link-state rule and not a hardcoded true. + const auto operator_clear = OpcuaPlugin::make_clear_fault_request("PLC_TANK_HIGH", /*link_state=*/false); + EXPECT_EQ(operator_clear.fault_code, "PLC_TANK_HIGH"); + EXPECT_FALSE(operator_clear.skip_correlation_auto_clear); +} + +// --------------------------------------------------------------------------- +// Pending fault dispatch buffer: reports outrank clears +// --------------------------------------------------------------------------- + +namespace { + +OpcuaPlugin::PendingFaultDispatch report_entry(const std::string & code) { + return {OpcuaPlugin::PendingFaultDispatch::Kind::Report, code, []() {}}; +} + +OpcuaPlugin::PendingFaultDispatch clear_entry(const std::string & code) { + return {OpcuaPlugin::PendingFaultDispatch::Kind::Clear, code, []() {}}; +} + +size_t count_kind(const std::vector & buffer, + OpcuaPlugin::PendingFaultDispatch::Kind kind) { + return static_cast( + std::count_if(buffer.begin(), buffer.end(), [kind](const OpcuaPlugin::PendingFaultDispatch & entry) { + return entry.kind == kind; + })); +} + +} // namespace + +TEST(EnqueuePendingDispatch, ReconnectClearsNeverEvictABufferedAlarmReport) { + // A flapping link with no fault_manager: 300 reconnects, each enqueueing a + // connect-time clear, while ten real alarm reports wait to be flushed. The + // reports are one-shot edges from the PLC; the clears are re-derivable. + std::vector buffer; + for (int i = 0; i < 10; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + report_entry("PLC_ALARM_" + std::to_string(i))); + } + for (int i = 0; i < 300; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry(kCommsLostFaultCode)); + } + + EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Report), 10u) + << "connect-time clears evicted buffered alarm reports"; + EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Clear), 1u) + << "at most one clear per fault code may be pending"; + for (int i = 0; i < 10; ++i) { + EXPECT_EQ(buffer[static_cast(i)].fault_code, "PLC_ALARM_" + std::to_string(i)); + } +} + +TEST(EnqueuePendingDispatch, AFullReportBufferRefusesAClearInsteadOfDroppingAReport) { + std::vector buffer; + for (size_t i = 0; i < OpcuaPlugin::kMaxPendingDispatches; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + report_entry("PLC_ALARM_" + std::to_string(i))); + } + ASSERT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); + + EXPECT_EQ(OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + clear_entry(kCommsLostFaultCode)), + OpcuaPlugin::PendingEnqueueOutcome::Refused); + EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Report), OpcuaPlugin::kMaxPendingDispatches); + EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_0") << "the oldest report must survive an incoming clear"; + + // A report arriving at a full buffer still drops the oldest one: reports do + // not outrank each other, so the bound still holds. + EXPECT_EQ( + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_NEW")), + OpcuaPlugin::PendingEnqueueOutcome::EvictedReport); + EXPECT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); + EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_1"); + EXPECT_EQ(buffer.back().fault_code, "PLC_ALARM_NEW"); +} + +TEST(EnqueuePendingDispatch, AFullBufferGivesUpAPendingClearBeforeAReport) { + std::vector buffer; + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_OLD_CLEAR")); + for (size_t i = 1; i < OpcuaPlugin::kMaxPendingDispatches; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + report_entry("PLC_ALARM_" + std::to_string(i))); + } + ASSERT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); + + EXPECT_EQ( + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_NEW")), + OpcuaPlugin::PendingEnqueueOutcome::EvictedClear); + EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Clear), 0u); + EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_1") << "the clear went, not the oldest report"; +} + +TEST(EnqueuePendingDispatch, ARequeuedClearMovesToTheBackSoOrderStillHolds) { + // Report-then-clear for one code must still flush in that order after the + // clear is re-enqueued, or the flush would leave the fault standing. + std::vector buffer; + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_FLAP")); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_FLAP")); + EXPECT_EQ(OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_FLAP")), + OpcuaPlugin::PendingEnqueueOutcome::ReplacedClear); + + ASSERT_EQ(buffer.size(), 2u); + EXPECT_EQ(buffer[0].kind, OpcuaPlugin::PendingFaultDispatch::Kind::Report); + EXPECT_EQ(buffer[1].kind, OpcuaPlugin::PendingFaultDispatch::Kind::Clear) + << "the newest clear must flush after the report it supersedes"; + // Clears for DIFFERENT codes are independent. + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_OTHER")); + EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Clear), 2u); +} + TEST(AdoptRediscoveredEndpoint, AdoptsOnlyADifferentNonEmptyUrl) { const std::string current = "opc.tcp://localhost:4840"; From 48a2632a7f219ce317723e56895fd84a26cfe788 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 16:35:01 +0200 Subject: [PATCH 6/8] test(gateway): assert the freeze-frame capture path from a real capture The two Frame::source constants had no assertion from a capture: only a hand-built frame in the merge helper's test named one, so swapping the DataProvider and route values left the whole suite green. The route and DataProvider loss-of-comms tests now each assert the constant their own path must produce, plus the literal wire value - symbol against symbol stays equal when the two constants are swapped, and that string is what every x-medkit.source consumer reads. The merge test that omits the key keeps its place as a helper contract for a frame a caller built without naming a path - the capture paths always name one - and says so instead of standing in as a control for them. The peer-node count test listed only two of the three helper nodes the gateway creates in its own process, so a lone gateway with a lifecycle reader would have counted a peer and skipped the empty-graph warning. Document the entity-frame source field in the REST fault snapshot reference: an entity frame carries no topic or message type, so source is the only provenance a consumer gets. --- docs/api/rest.rst | 20 ++++++++++------ .../test/test_entity_freeze_frame_capture.cpp | 23 ++++++++++++++++--- .../test/test_gateway_node.cpp | 4 ++++ 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 0321f22fc..0faa882f1 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1501,13 +1501,19 @@ Query and manage faults. - ``freeze_frame``: Data captured at fault confirmation. Entity frames for faults that were already confirmed when the gateway started are captured at gateway start instead and carry ``"capture_origin": "startup"`` in - their ``x-medkit`` block. For a plugin-backed entity that reports its - link down, the values are the plugin's last known ones and may predate - the confirmation by the length of the outage; such entries carry - ``connected`` (the payload's link flag, ``false`` for the loss-of-comms - case) and ``source_timestamp`` (the payload's own timestamp, verbatim) - in ``x-medkit``, both only when the plugin's payload reports them. - ``captured_at`` always dates the capture, not the values. + their ``x-medkit`` block. An entity frame also carries ``source`` in + ``x-medkit``, naming the path that read the values + (``plugin_data_provider`` for the owning plugin's DataProvider, + ``plugin_x_plc_data_route`` for its ``x-plc-data`` route). These values + are not a ROS message, so ``topic`` and ``message_type`` are empty and + ``source`` is the only field saying where the numbers came from. For a + plugin-backed entity that reports its link down, the values are the + plugin's last known ones and may predate the confirmation by the length of + the outage; such entries carry ``connected`` (the payload's link flag, + ``false`` for the loss-of-comms case) and ``source_timestamp`` (the + payload's own timestamp, verbatim) in ``x-medkit``, both only when the + plugin's payload reports them. ``captured_at`` always dates the capture, + not the values. - ``rosbag``: Recording file available via bulk-data endpoint **Response codes:** diff --git a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp index 152297ba0..8ef14dbc4 100644 --- a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp +++ b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp @@ -573,6 +573,15 @@ TEST_F(EntityFreezeFrameCaptureTest, DisconnectedEntityWithLastKnownValuesIsCapt ASSERT_TRUE(frames[0].connected.has_value()); EXPECT_FALSE(*frames[0].connected); EXPECT_EQ(frames[0].source_timestamp, 1234567890); + // Which path read the values. This capture had no DataProvider and went + // through the route fallback, so the frame must name that path; the + // DataProvider flavour of the same case asserts the other constant, which is + // what stops the two from being swapped at their call sites unnoticed. + EXPECT_EQ(frames[0].source, EntityFreezeFrameCapture::kSourceXPlcDataRoute); + // The wire value itself, not just the symbol: swapping what the two constants + // hold is an API break for every consumer of x-medkit.source, and comparing + // symbol against symbol would not see it. + EXPECT_EQ(frames[0].source, "plugin_x_plc_data_route"); } /// @verifies REQ_INTEROP_088 @@ -642,6 +651,11 @@ TEST_F(EntityFreezeFrameCaptureTest, DisconnectedDataProviderWithLastKnownValues ASSERT_TRUE(frames[0].connected.has_value()); EXPECT_FALSE(*frames[0].connected); EXPECT_TRUE(frames[0].source_timestamp.is_null()); // provider content has no timestamp field + // The provider path names itself, and the route path (same case, above) names + // the other constant: the pair is what makes a swap of the two call sites + // visible. The literal pins the wire value the API reference documents. + EXPECT_EQ(frames[0].source, EntityFreezeFrameCapture::kSourceDataProvider); + EXPECT_EQ(frames[0].source, "plugin_data_provider"); } /// @verifies REQ_INTEROP_088 @@ -805,9 +819,12 @@ TEST(MergeEntityFreezeFrames, CarriesCapturePathAsSource) { EXPECT_EQ(snap["message_type"], ""); } -TEST(MergeEntityFreezeFrames, OmitsSourceWhenTheCaptureNamedNoPath) { - // Absence control for the test above, on the same harness: a frame whose - // capture path is unknown must not have one invented for it. +TEST(MergeEntityFreezeFrames, OmitsSourceForAFrameThatNamesNoPath) { + // A merge-helper contract, not a control for the capture tests: both capture + // paths always name themselves (asserted from real captures in + // Disconnected{Entity,DataProvider}WithLastKnownValuesIsCaptured), so this + // frame is one only a caller can build. The helper must then leave the key + // out rather than invent a provenance the wire consumer would trust. json env_data = {{"snapshots", json::array()}}; EntityFreezeFrameCapture::Frame frame; frame.entity_id = "plc_app"; diff --git a/src/ros2_medkit_gateway/test/test_gateway_node.cpp b/src/ros2_medkit_gateway/test/test_gateway_node.cpp index 2f81dbc6c..557aef176 100644 --- a/src/ros2_medkit_gateway/test/test_gateway_node.cpp +++ b/src/ros2_medkit_gateway/test/test_gateway_node.cpp @@ -1058,10 +1058,14 @@ TEST(GatewayStartupSummary, CountPeerNodesExcludesOwnAndHidden) { } TEST(GatewayStartupSummary, CountPeerNodesZeroWhenOnlyOwnNodes) { + // Every helper the gateway creates inside its own process. A gateway alone on + // the graph must report zero peers, so each helper has to be recognized - + // including the lifecycle reader, which the list previously omitted. const std::vector> nodes = { {"ros2_medkit_gateway", "/"}, {"ros2_medkit_gateway_sub", "/"}, {"ros2_medkit_gateway_fault_clients", "/"}, + {"ros2_medkit_gateway_lifecycle_state_reader", "/"}, }; // Zero peers is the condition that triggers the empty-graph warning. EXPECT_EQ(ros2_medkit_gateway::GatewayNode::count_peer_nodes(nodes, "/ros2_medkit_gateway"), 0u); From d1e61d7716718511f22bb953c70ae95c7c010992 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 18:51:20 +0200 Subject: [PATCH 7/8] fix(opcua): close the scoped-clear hole and rank the pending buffer by what is re-derivable The per-entity SOVD route DELETE /{entity}/faults/{code} lands on FaultProvider::clear_fault for a plugin-owned entity, which is the branch the gateway takes instead of its own. The gateway sets skip_correlation_auto_clear there so an operator scoped to one entity cannot cascade-clear correlated symptoms reported by apps in other entities, and the ClearFault contract documents that guarantee, but this plugin sent the flag off and reopened the hole wherever a PLC is involved. It now sets the flag, and the reason each call site sets or clears it travels with the call: a ClearOrigin says whether the device reported the condition inactive (a real resolution, cascade kept), the link came back, or an operator cleared through the scoped route. The pending-dispatch buffer used that same distinction too bluntly. It gave up any clear before any report, but only the link-state clear is re-derivable - the next reconnect sends it again. A device alarm's inactive edge is as one-shot as its raise, so evicting it left the flush replaying the raise with nothing behind it and the fault standing while the device said inactive. Ranking is now by re-derivability: the link-state clear is what a full buffer gives up first, everything else ages out oldest-first as it did before. The start-up discovery sweep claimed to be cancellable through the shutdown flag, but nothing can set that flag while it runs: it happens inside set_context(), during node construction, before the executor the gateway shuts down from ever spins. A SIGTERM during a wide sweep therefore waited the sweep out. Both sweeps now ask one predicate that also reads rclcpp::ok(), which rclcpp's own signal handler turns false, so the start-up sweep ends on the signal and the rescan keeps ending on shutdown() as well. Also: a rescan sweep that throws now stamps the cadence on its way out, or the next poll iteration would immediately start another one; the start-up log line no longer states the wrong reason for reporting in full; and the package changelog records this branch. Measuring that start-up sweep also showed the gateway logging nothing at all while it ran: the discovery report is buffered so a repeated rescan can be reported at DEBUG, and the "scanning [subnets]" announcement had been swept up with it. A minutes-long sweep with no output reads as a hung process, so the announcement is sent before the sweep again, with the same first-pass INFO / rescan DEBUG levelling on its own. --- docs/api/rest.rst | 2 +- .../ros2_medkit_gateway/gateway_node.hpp | 4 +- .../test/test_entity_freeze_frame_capture.cpp | 2 +- .../ros2_medkit_opcua/CHANGELOG.rst | 10 + .../ros2_medkit_opcua/README.md | 13 +- .../docker/scripts/run_discovery_race_test.sh | 8 +- .../ros2_medkit_opcua/opcua_plugin.hpp | 122 +++++-- .../ros2_medkit_opcua/src/opcua_plugin.cpp | 139 +++++--- .../test/test_network_discovery.cpp | 4 +- .../test/test_opcua_plugin.cpp | 333 ++++++++++++++++-- 10 files changed, 507 insertions(+), 130 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 0faa882f1..590414c62 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1509,7 +1509,7 @@ Query and manage faults. ``source`` is the only field saying where the numbers came from. For a plugin-backed entity that reports its link down, the values are the plugin's last known ones and may predate the confirmation by the length of - the outage; such entries carry ``connected`` (the payload's link flag, + the outage. Such entries carry ``connected`` (the payload's link flag, ``false`` for the loss-of-comms case) and ``source_timestamp`` (the payload's own timestamp, verbatim) in ``x-medkit``, both only when the plugin's payload reports them. ``captured_at`` always dates the capture, diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp index d4585d99a..89a153d1b 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp @@ -514,7 +514,7 @@ class GatewayNode : public rclcpp::Node { * `_monitor` or `2`, and dropping a real node is the worse error. * * @param node_fqn Fully qualified node name to test ("/ns/node") - * @param self_fqn The gateway node's own FQN; an empty value matches nothing + * @param self_fqn The gateway node's own FQN. An empty value matches nothing */ bool is_own_gateway_node(const std::string & node_fqn, const std::string & self_fqn); @@ -533,7 +533,7 @@ bool is_own_gateway_node(const std::string & node_fqn, const std::string & self_ * * @param apps App vector to filter in place * @param peer_routing_table Maps entity_id -> peer_name for remote entities - * @param self_fqn The gateway node's own FQN; empty disables the self check + * @param self_fqn The gateway node's own FQN. Empty disables the self check * @return Number of apps removed */ size_t filter_internal_node_apps(std::vector & apps, diff --git a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp index 8ef14dbc4..b97372530 100644 --- a/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp +++ b/src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp @@ -574,7 +574,7 @@ TEST_F(EntityFreezeFrameCaptureTest, DisconnectedEntityWithLastKnownValuesIsCapt EXPECT_FALSE(*frames[0].connected); EXPECT_EQ(frames[0].source_timestamp, 1234567890); // Which path read the values. This capture had no DataProvider and went - // through the route fallback, so the frame must name that path; the + // through the route fallback, so the frame must name that path. The // DataProvider flavour of the same case asserts the other constant, which is // what stops the two from being swapped at their call sites unnoticed. EXPECT_EQ(frames[0].source, EntityFreezeFrameCapture::kSourceXPlcDataRoute); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CHANGELOG.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/CHANGELOG.rst index 9f37ccc7f..1db5561be 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CHANGELOG.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CHANGELOG.rst @@ -2,6 +2,16 @@ Changelog for package ros2_medkit_opcua ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* **Breaking:** ``discovery.interval_s`` now distinguishes unset from an explicit ``0``. Leaving the key out (and leaving ``OPCUA_DISCOVERY_INTERVAL_S`` unset) keeps the built-in 30 s re-scan cadence, while an explicit ``0`` means "discovery on, start-up scan only" instead of selecting that same default. A deployment that wrote ``interval_s: 0`` meaning "use the default" stops re-scanning, and dropping the key restores the previous behaviour. A negative value is refused with a warning and leaves the cadence unset +* While no OPC UA session is established, the reconnect loop re-scans on that cadence and adopts a server that appeared after start-up, so a gateway that booted before its PLC finds it without a restart. The cadence is measured from the end of the previous sweep, the reconnect backoff is capped at the cadence so the documented interval is the real one, and a sweep is cancellable (``SIGINT`` / ``SIGTERM`` during start-up, the plugin's ``shutdown()`` for a re-scan) instead of having to run to completion. A re-scan whose outcome has not changed is reported at DEBUG rather than repeating the whole report every interval +* With no node map, the SOVD component identity is re-derived from the device on the first session after such an adoption, so a component named after the fallback endpoint (because nothing answered at start-up) stops being served under that placeholder once the real PLC is adopted +* ``PLC_COMMS_LOST`` is cleared on every successful connect, including the first, so a fault ``fault_manager`` persisted before a restart does not stand against a healthy link. That clear, and the per-entity ``DELETE /{entity}/faults/{code}`` route when it is served by this plugin, set ``skip_correlation_auto_clear``: neither is an operator resolving a root cause, so neither may cascade-clear correlated symptom faults reported by apps in other entities. A clear reported by the device itself still cascades +* Fault dispatches buffered while ``fault_manager`` is unreachable no longer lose one-shot events. Only the link-state ``PLC_COMMS_LOST`` clear is re-derivable (the next reconnect sends it again), so it is what the bounded buffer gives up first. Alarm reports, a device alarm's inactive edge and an operator's scoped clear age out oldest-first as before, and at most one clear per fault code is pending at a time +* Gateway-side changes that land on this plugin's entities: a freeze-frame captured from a plugin entity now names the path that read the values in ``x-medkit.source`` (``plugin_data_provider`` or ``plugin_x_plc_data_route``, the only provenance an entity frame carries since it has no ROS topic), and the gateway no longer lists its own in-process helper nodes among the discovered apps +* Contributors: @bburda + 0.7.0 (2026-08-27) ------------------ * Config-less discovery. A read-only network scan finds the OPC UA server instead of requiring its endpoint up front (`#509 `_), ``auto_browse`` walks the address space recursively and builds the SOVD tree from it (`#510 `_), and identity, writability and fault triggers are read from the device itself rather than declared in a node map (`#544 `_) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 65c3d2225..7b6abdde0 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -604,8 +604,8 @@ plugins.opcua.discovery: connect_timeout_ms: 600 # per-port TCP connect timeout scan_concurrency: 100 # bounded, polite concurrent connect count identify_timeout_ms: 6000 # per GetEndpoints identify - # re-scan cadence while disconnected. Omit the key for the built-in 30 s; - # set it to 0 to keep discovery on but never re-scan (start-up scan only). + # re-scan cadence while disconnected. Omit the key for the built-in 30 s, + # or set it to 0 to keep discovery on but never re-scan (start-up scan only). interval_s: 30 anonymous_none_only: true # only auto-connect None/Anonymous servers ``` @@ -613,7 +613,7 @@ plugins.opcua.discovery: Environment overrides (Docker / appliance): `OPCUA_DISCOVERY_ENABLED`, `OPCUA_DISCOVERY_SUBNETS` (comma-separated CIDRs), `OPCUA_DISCOVERY_INTERVAL_S`. Leaving `interval_s` (and `OPCUA_DISCOVERY_INTERVAL_S`) unset means "no cadence -stated" and takes the 30 s default; an explicit `0` is honoured as written and +stated" and takes the 30 s default. An explicit `0` is honoured as written and turns the recurring sweep off. A negative value is refused with a warning and leaves the cadence unset. @@ -656,8 +656,11 @@ Safety / OT posture: `interval_s` (default 30 s) for as long as it stays disconnected. Set `interval_s: 0` (or `OPCUA_DISCOVERY_INTERVAL_S=0`) to keep discovery on with the start-up scan only, or `enabled: false` to switch it off entirely. -- A sweep is cancelled when the plugin shuts down, so a stop does not have to - wait out a subnet the size of a /16. +- A sweep is cancellable, so a stop does not have to wait out a subnet the size + of a /16. The start-up sweep runs while the gateway node is still being + constructed, so what ends it is `SIGINT` / `SIGTERM`, which the plugin sees + through `rclcpp::ok()`. A re-scan sweep runs on the poll thread and is ended + by either that or the plugin's own `shutdown()`. - An explicitly configured `endpoint_url` (or `OPCUA_ENDPOINT_URL`) always wins; discovery then does nothing, so it never opens a second session on a PLC the plugin already polls. diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh index dc52d3823..5b807472b 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_discovery_race_test.sh @@ -37,8 +37,8 @@ CONFIG_DIR=/tmp/discovery_race_config # The fallback the plugin keeps when a scan selects nothing (OpcuaClientConfig). FALLBACK_ENDPOINT="opc.tcp://localhost:4840" # Config-less naming: with no node map the component id is derived from the -# device. Before any server exists that can only be the fallback endpoint's host; -# after adoption it is the test server's DI nameplate (Manufacturer "SelfPatch +# device. Before any server exists that can only be the fallback endpoint's +# host. After adoption it is the test server's DI nameplate (Manufacturer "SelfPatch # Devices" + Model "SPX-1000"), slugified. FALLBACK_COMPONENT_ID="opcua-localhost" DEVICE_COMPONENT_ID="selfpatch_devices_spx_1000" @@ -62,8 +62,8 @@ fail() { exit 1 } -# x-plc-status of a named component (the node-map pass pins the id; the -# config-less pass has to look it up first). +# x-plc-status of a named component. The node-map pass pins the id, the +# config-less pass has to look it up first. status_json_for() { curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/components/$1/x-plc-status" || echo '{}' } diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index a646bfccd..702c6cd8d 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -148,14 +148,19 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // Where one discovery pass reports to, plus the memory that keeps a repeated // identical pass quiet. A rescan runs every ``interval_s`` for the life of a - // disconnected process, so re-emitting the same scan line, per-server lines, - // summary and "no auto-connectable server" WARN each time buries every other - // message in the log. ``previous_outcome`` is owned by the caller (the plugin - // keeps one across rescans): when it is non-null and the pass reaches the same - // outcome as the pass before it, the whole report goes to ``debug`` instead. - // The first pass, and every pass whose outcome changed, is always reported at - // info/warn. A null ``previous_outcome`` (the startup scan, and tests that do - // not care) reports every pass. + // disconnected process, so re-emitting the same per-server lines, summary and + // "no auto-connectable server" WARN each time buries every other message in + // the log. ``previous_outcome`` is owned by the caller (the plugin keeps one + // across rescans): when it is non-null and the pass reaches the same outcome + // as the pass before it, the whole report goes to ``debug`` instead. The first + // pass, and every pass whose outcome changed, is always reported at info/warn. + // A null ``previous_outcome`` (tests that do not care) reports every pass. + // + // The "scanning [subnets]" announcement is NOT part of that report. It is sent + // before the sweep runs, because a wide subnet takes minutes and an operator + // watching start-up has to see the gateway working rather than hung. It says + // what the pass is about to do rather than what it found, so it carries the + // same first-pass / rescan levelling on its own. struct DiscoveryReporter { std::function info; std::function warn; @@ -243,15 +248,47 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, const OpcuaClient::DeviceInfo & info, const std::string & endpoint_url); - // Build the ClearFault request for one fault code. ``link_state`` marks a - // clear that only reports the OPC-UA link came back (the connect-time - // ``PLC_COMMS_LOST`` clear). Such a clear must not cascade: a correlation rule - // may name PLC_COMMS_LOST as the root cause of every symptom the outage - // produced, and the link returning is not an operator resolving those. An - // operator-driven clear (the SOVD DELETE route) leaves the flag off and keeps - // the cascade. Static so the wire field is assertable without a fault manager. + // Why a ClearFault is being sent. Two properties follow from it and nothing + // else does, so the origin travels instead of a pair of loose booleans: + // - whether the correlation cascade must be skipped + // (``clear_skips_correlation``), which goes on the wire, and + // - whether the clear is re-derivable (``clear_is_link_state``), which is + // what the pending buffer may give up first under pressure. + enum class ClearOrigin { + /// The device reported the condition inactive (an ``event_alarms`` / + /// ``auto_alarms`` condition, or a threshold rule going false). A one-shot + /// edge nothing will re-send, and a real resolution, so the cascade stands. + DeviceAlarm, + /// The OPC-UA session came back, so ``PLC_COMMS_LOST`` no longer holds. + /// Re-derived on the next reconnect if it is lost, and not an operator + /// resolving a root cause, so it must not cascade. + LinkState, + /// The SOVD per-entity ``DELETE /{entity}/faults/{code}`` route reached + /// FaultProvider::clear_fault. An operator scoped to one entity must not + /// cascade-clear symptoms reported by apps in other entities, which is the + /// same rule the gateway applies on its own (non-plugin) branch of that + /// route. One-shot: nothing re-derives an operator's decision. + ScopedOperator + }; + + // Whether this clear must leave the correlation engine's auto_clear_with_root + // cascade alone. True for everything except a device-reported clear. + static bool clear_skips_correlation(ClearOrigin origin) { + return origin != ClearOrigin::DeviceAlarm; + } + + // Whether this clear will be re-derived if it is dropped. Only the link-state + // clear will: the next reconnect sends it again. + static bool clear_is_link_state(ClearOrigin origin) { + return origin == ClearOrigin::LinkState; + } + + // Build the ClearFault request for one fault code. + // ``skip_correlation_auto_clear`` goes on the wire verbatim (see ClearOrigin + // for who sets it and why). Static so the wire field is assertable without a + // fault manager. static ros2_medkit_msgs::srv::ClearFault::Request make_clear_fault_request(const std::string & fault_code, - bool link_state); + bool skip_correlation_auto_clear); // One entry in the bounded buffer of fault dispatches held while the // fault_manager service is unmatched. @@ -259,28 +296,37 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, enum class Kind { Report, Clear }; Kind kind{Kind::Report}; std::string fault_code; ///< dedup key for a Clear; diagnostic for a Report + /// Clear only: this dispatch is re-derivable (ClearOrigin::LinkState), so + /// the buffer may drop it before anything that is not. + bool link_state{false}; std::function dispatch; }; // What ``enqueue_pending_dispatch`` did, so the caller can log it. enum class PendingEnqueueOutcome { - Buffered, ///< appended, nothing lost - ReplacedClear, ///< superseded the pending clear for the same fault code - EvictedClear, ///< buffer was full: dropped a pending clear to make room - EvictedReport, ///< buffer was full of reports and a report arrived - Refused ///< buffer was full of reports and a clear arrived + Buffered, ///< appended, nothing lost + ReplacedClear, ///< superseded the pending clear for the same fault code + EvictedLinkStateClear, ///< buffer was full: dropped a re-derivable clear to make room + EvictedOldest, ///< buffer was full with nothing re-derivable in it: dropped the oldest entry + Refused ///< buffer was full with nothing re-derivable and the incoming clear was }; // Enqueue policy for the bounded pending-dispatch buffer. // - // Reports outrank clears. A report is a one-shot edge from the PLC that - // nothing will re-send, while a clear is re-derivable: the link state is - // re-observed on the next reconnect. So at most ONE clear per fault code is - // ever pending (a newer one moves to the back, keeping report-then-clear - // order), a full buffer gives up its oldest pending clear first, and a clear - // arriving at a buffer full of reports is refused rather than evicting one. - // Without this a flapping link enqueued one connect-time clear per reconnect - // attempt and pushed real alarm reports out of the buffer. + // Only a link-state clear is re-derivable: the next reconnect sends it again. + // Everything else in the buffer is a one-shot edge nothing will re-send - a + // report, a device alarm going inactive, an operator's scoped clear - so those + // rank together and age out oldest-first, exactly as the buffer behaved before + // any of this. A link-state clear is what a full buffer gives up first, and an + // incoming one is refused rather than pushing a one-shot dispatch out. At most + // ONE clear per fault code is pending at a time (a newer one moves to the + // back, so an interleaved report-then-clear still flushes in that order). + // + // Without the link-state ranking a flapping link enqueued one connect-time + // clear per reconnect attempt and pushed real alarm reports out of the buffer. + // Without the "only link-state" part, a device alarm's inactive edge was + // evicted ahead of an older report and the flush replayed the raise with no + // clear behind it, leaving the fault standing while the device said inactive. static PendingEnqueueOutcome enqueue_pending_dispatch(std::vector & buffer, size_t max_size, PendingFaultDispatch entry); @@ -305,9 +351,9 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // Report/clear fault via ROS 2 service (private helpers, not the FaultProvider overrides) void send_report_fault(const std::string & entity_id, const std::string & fault_code, const std::string & severity_str, const std::string & message); - // ``link_state`` marks a clear that reports the OPC-UA link came back rather - // than an operator resolving a root cause; see make_clear_fault_request. - void send_clear_fault(const std::string & fault_code, bool link_state = false); + // ``origin`` says why the clear is being sent, which decides both the wire + // flag and how the pending buffer ranks it. See ClearOrigin. + void send_clear_fault(const std::string & fault_code, ClearOrigin origin = ClearOrigin::DeviceAlarm); // Clear PLC_COMMS_LOST after the initial connect in set_context() succeeded. // Unconditional on purpose: the fault manager keys faults by fault_code and @@ -379,6 +425,18 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // (null to report every pass in full). DiscoveryReporter discovery_reporter(std::string * previous_outcome) const; + // Abort predicate handed to a discovery sweep. Two independent stop signals, + // because the two sweeps run at different points in the process lifetime: + // - ``shutdown_requested_`` is set by shutdown(), which the gateway calls + // after its executor returns. That ends a RESCAN sweep, which runs on the + // poll thread long after start-up. + // - ``rclcpp::ok()`` turns false as soon as rclcpp's own SIGINT / SIGTERM + // handler runs. The STARTUP sweep runs inside set_context(), i.e. during + // node construction and before the executor spins, so shutdown() cannot + // be reached while it is in progress and the signal is the only thing + // that can end it. + bool discovery_cancelled() const; + // Poll-thread hook bound into PollerConfig::rediscover_endpoint whenever // discovery runs without a configured endpoint. Called from the poller's // reconnect arm, so only while no session is up, and rate-limited to one scan diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index 355570fa4..158523936 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -1049,10 +1049,11 @@ void OpcuaPlugin::on_alarm_change(const std::string & entity_id, } else { log_info("Alarm cleared: " + signal.fault_code + " on " + entity_id); // The poller's own comms-lost clear on a successful reconnect is the same - // link-state clear as the connect-time one, so it does not cascade either. - // Every other code is a real alarm going inactive on the device and keeps - // the default correlation behaviour. - send_clear_fault(signal.fault_code, /*link_state=*/signal.fault_code == kCommsLostFaultCode); + // link-state event as the connect-time one. Every other code here is the + // device reporting its condition inactive, which is a real resolution and a + // one-shot edge, so it keeps the cascade and the buffer treats it as such. + send_clear_fault(signal.fault_code, + signal.fault_code == kCommsLostFaultCode ? ClearOrigin::LinkState : ClearOrigin::DeviceAlarm); } } @@ -1231,7 +1232,11 @@ void OpcuaPlugin::on_event_alarm(const AlarmEventDelivery & delivery) { break; case AlarmAction::ClearFault: log_info("AlarmCondition CLEARED: " + delivery.fault_code); - send_clear_fault(delivery.fault_code); + // The device itself reported the condition cleared (Part 9 lifecycle), so + // this IS a resolution at the source and the correlation engine may act on + // it. DeviceAlarm is also the default, spelled out here because this is + // the one call site where the cascade is deliberately kept. + send_clear_fault(delivery.fault_code, ClearOrigin::DeviceAlarm); break; case AlarmAction::NoOp: break; @@ -1266,34 +1271,29 @@ void OpcuaPlugin::send_report_fault(const std::string & entity_id, const std::st request->severity = ros2_medkit_msgs::msg::Fault::SEVERITY_INFO; } - send_or_buffer({PendingFaultDispatch::Kind::Report, fault_code, [this, request]() { + send_or_buffer({PendingFaultDispatch::Kind::Report, fault_code, /*link_state=*/false, [this, request]() { fault_clients_->report->async_send_request(request); }}); } ros2_medkit_msgs::srv::ClearFault::Request OpcuaPlugin::make_clear_fault_request(const std::string & fault_code, - bool link_state) { + bool skip_correlation_auto_clear) { ros2_medkit_msgs::srv::ClearFault::Request request; request.fault_code = fault_code; - // A link-state clear reports that the OPC-UA session came back. It is not an - // operator resolving a root cause, so it must not trip the correlation - // engine's auto_clear_with_root cascade: a rule naming PLC_COMMS_LOST as the - // root cause would otherwise clear every symptom fault the outage produced, - // none of which this plugin has any evidence about. - request.skip_correlation_auto_clear = link_state; + request.skip_correlation_auto_clear = skip_correlation_auto_clear; return request; } -void OpcuaPlugin::send_clear_fault(const std::string & fault_code, bool link_state) { +void OpcuaPlugin::send_clear_fault(const std::string & fault_code, ClearOrigin origin) { if (!fault_clients_->clear) { log_warn("ClearFault service client not available"); return; } - auto request = - std::make_shared(make_clear_fault_request(fault_code, link_state)); + auto request = std::make_shared( + make_clear_fault_request(fault_code, clear_skips_correlation(origin))); - send_or_buffer({PendingFaultDispatch::Kind::Clear, fault_code, [this, request]() { + send_or_buffer({PendingFaultDispatch::Kind::Clear, fault_code, clear_is_link_state(origin), [this, request]() { fault_clients_->clear->async_send_request(request); }}); } @@ -1305,8 +1305,14 @@ void OpcuaPlugin::clear_comms_lost_on_connect() { // ClearFault is idempotent from this side: send_clear_fault is // fire-and-forget, so a "Fault not found" answer for a code that was never // raised costs nothing here and is the normal case on a healthy start. + // + // LinkState: this says the session came back, not that an operator resolved + // anything, so a correlation rule naming PLC_COMMS_LOST as a root cause must + // not cascade-clear the symptoms the outage produced. It is also the one clear + // the next reconnect re-derives, so the pending buffer may drop it before + // anything one-shot. log_info(std::string("OPC-UA connection established; clearing any standing ") + kCommsLostFaultCode); - send_clear_fault(kCommsLostFaultCode, /*link_state=*/true); + send_clear_fault(kCommsLostFaultCode, ClearOrigin::LinkState); } OpcuaPlugin::PendingEnqueueOutcome OpcuaPlugin::enqueue_pending_dispatch(std::vector & buffer, @@ -1329,21 +1335,22 @@ OpcuaPlugin::PendingEnqueueOutcome OpcuaPlugin::enqueue_pending_dispatch(std::ve PendingEnqueueOutcome outcome = replaced ? PendingEnqueueOutcome::ReplacedClear : PendingEnqueueOutcome::Buffered; if (buffer.size() >= max_size) { - // A report is a one-shot edge from the PLC that nothing will re-send; a - // clear is re-derivable from the next reconnect. So a full buffer gives up a - // pending clear first, and refuses an incoming clear rather than evicting a - // report for it. - const auto oldest_clear = std::find_if(buffer.begin(), buffer.end(), [](const PendingFaultDispatch & pending) { - return pending.kind == PendingFaultDispatch::Kind::Clear; + // Only a link-state clear is re-derivable: the next reconnect sends it + // again. A full buffer gives that up first, and refuses an incoming one + // rather than pushing out a dispatch nothing will re-send. Everything else - + // reports, a device alarm's inactive edge, an operator's scoped clear - is + // one-shot and ages out oldest-first. + const auto oldest_link_state = std::find_if(buffer.begin(), buffer.end(), [](const PendingFaultDispatch & pending) { + return pending.kind == PendingFaultDispatch::Kind::Clear && pending.link_state; }); - if (oldest_clear != buffer.end()) { - buffer.erase(oldest_clear); - outcome = PendingEnqueueOutcome::EvictedClear; - } else if (is_clear) { + if (oldest_link_state != buffer.end()) { + buffer.erase(oldest_link_state); + outcome = PendingEnqueueOutcome::EvictedLinkStateClear; + } else if (is_clear && entry.link_state) { return PendingEnqueueOutcome::Refused; } else { buffer.erase(buffer.begin()); - outcome = PendingEnqueueOutcome::EvictedReport; + outcome = PendingEnqueueOutcome::EvictedOldest; } } @@ -1360,15 +1367,15 @@ void OpcuaPlugin::send_or_buffer(PendingFaultDispatch entry) { std::lock_guard lock(pending_reports_mutex_); outcome = enqueue_pending_dispatch(pending_reports_, kMaxPendingDispatches, std::move(entry)); } - if (outcome == PendingEnqueueOutcome::EvictedReport) { + if (outcome == PendingEnqueueOutcome::EvictedOldest) { log_warn("pending fault dispatch buffer full (" + std::to_string(kMaxPendingDispatches) + - "), dropping the oldest report"); - } else if (outcome == PendingEnqueueOutcome::EvictedClear) { + "), dropping the oldest dispatch"); + } else if (outcome == PendingEnqueueOutcome::EvictedLinkStateClear) { log_warn("pending fault dispatch buffer full (" + std::to_string(kMaxPendingDispatches) + - "), dropping the oldest pending clear"); + "), dropping a link-state clear the next reconnect re-derives"); } else if (outcome == PendingEnqueueOutcome::Refused) { - log_warn("pending fault dispatch buffer full of reports (" + std::to_string(kMaxPendingDispatches) + - "), dropping this clear instead of a report"); + log_warn("pending fault dispatch buffer full of one-shot dispatches (" + std::to_string(kMaxPendingDispatches) + + "), dropping this link-state clear instead"); } // Drains immediately (in order) if the sink is already matched. flush_pending_reports(); @@ -1464,7 +1471,7 @@ std::optional OpcuaPlugin::rederived_component_identity(const } void OpcuaPlugin::maybe_rederive_component_identity() { - // An explicit node map owns the component name; only the config-less path + // An explicit node map owns the component name. Only the config-less path // derives it from the device. if (!node_map_path_.empty() || !client_ || !client_->is_connected()) { return; @@ -1650,12 +1657,19 @@ std::optional OpcuaPlugin::rescan_step(int interval_s, if (now() - *last_scan_end < std::chrono::seconds(interval_s)) { return std::nullopt; } - const auto result = sweep(); // Stamp the END of the sweep: a legal /16 runs for minutes, and stamping its // start would make the next one due the moment this one returned - the poll // thread would sweep back to back and only attempt a reconnect once a sweep. - *last_scan_end = now(); - return result; + // A sweep that threw still consumed that time, so the stamp is owed either + // way, or the next poll iteration would start another one immediately. + try { + const auto result = sweep(); + *last_scan_end = now(); + return result; + } catch (...) { + *last_scan_end = now(); + throw; + } } std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryConfig & config, bool endpoint_configured, @@ -1717,8 +1731,18 @@ std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryCo for (const auto & s : subnets) { subnet_list += (subnet_list.empty() ? "" : ", ") + s; } - info_line("OPC-UA discovery: read-only active scan of [" + subnet_list + "] on " + - std::to_string(config.ports.size()) + " port(s)..."); + // The announcement goes out NOW, not through the buffered report: a sweep of a + // wide subnet runs for minutes, and an operator watching start-up has to see + // that the gateway is scanning rather than hung. It says what the pass is + // about to do, not what it found, so it stays out of the outcome digest and is + // levelled on its own - the first pass announces at INFO, a rescan at DEBUG so + // the recurring sweep does not repeat it every interval. + const bool first_pass = reporter.previous_outcome == nullptr || reporter.previous_outcome->empty(); + const auto & announce_sink = first_pass ? reporter.info : reporter.debug; + if (announce_sink) { + announce_sink("OPC-UA discovery: read-only active scan of [" + subnet_list + "] on " + + std::to_string(config.ports.size()) + " port(s)..."); + } const std::vector found = discovery.run(cancelled); @@ -1776,12 +1800,15 @@ void OpcuaPlugin::run_startup_discovery() { return; } - // The startup scan is always reported in full (no previous outcome to compare - // against) and always cancellable, so a shutdown during set_context does not - // wait out a whole sweep. + // The startup scan is the first pass, so its outcome digest is still empty and + // the report comes out in full. It is cancellable too, but not by shutdown(): + // this runs inside set_context(), i.e. during node construction and before the + // executor spins, so nothing can call shutdown() until this returns. What ends + // it is the SIGINT / SIGTERM that rclcpp's own handler turns into + // !rclcpp::ok() - see discovery_cancelled(). const auto chosen = discover_endpoint(discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, discovery_reporter(&last_discovery_outcome_), [this]() { - return shutdown_requested_.load(); + return discovery_cancelled(); }); // Stamp when the sweep FINISHED: the rescan cadence is measured from the end // of the previous sweep, so a long sweep is not immediately followed by @@ -1812,6 +1839,16 @@ void OpcuaPlugin::run_startup_discovery() { log_info("OPC-UA discovery: auto-selected endpoint " + *chosen + " - handing to the connect + introspect path."); } +bool OpcuaPlugin::discovery_cancelled() const { + // Either stop signal ends a sweep. shutdown() is what ends a RESCAN (it runs + // on the poll thread, long after start-up). rclcpp::ok() going false is what + // ends the STARTUP sweep, which runs during node construction where shutdown() + // is not reachable yet. Checking both in one predicate keeps the two sweeps + // from drifting apart, and rclcpp::ok() only reads the default context's + // atomic shutdown flag, so it is safe to call from either thread. + return shutdown_requested_.load() || !rclcpp::ok(); +} + OpcuaPlugin::DiscoveryReporter OpcuaPlugin::discovery_reporter(std::string * previous_outcome) const { DiscoveryReporter reporter; reporter.info = [this](const std::string & m) { @@ -1831,7 +1868,7 @@ std::optional OpcuaPlugin::rescan_endpoint_for_reconnect() { // A sweep is a bounded but multi-second blocking call on the poll thread, and // stop() has to wait for whatever it is in the middle of. Do not start one the // shutdown is going to throw away. - if (shutdown_requested_.load()) { + if (discovery_cancelled()) { return std::nullopt; } const int interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); @@ -1845,7 +1882,7 @@ std::optional OpcuaPlugin::rescan_endpoint_for_reconnect() { [this]() { return discover_endpoint(discovery_config_, endpoint_configured_, discovery_scan_fn_, discovery_identify_fn_, discovery_reporter(&last_discovery_outcome_), [this]() { - return shutdown_requested_.load(); + return discovery_cancelled(); }); }); // The live client config, not client_config_: this runs on the poll thread @@ -2358,7 +2395,13 @@ tl::expected OpcuaPlugin::clear_f return tl::make_unexpected(FaultProviderErrorInfo{FaultProviderError::Internal, "plugin not initialized", 503}); } - send_clear_fault(fault_code); + // This is the per-entity SOVD route DELETE /{entity}/faults/{code}. The + // gateway sets skip_correlation_auto_clear on its own branch of that route so + // an operator scoped to one entity cannot cascade-clear symptoms reported by + // apps in other entities, and a plugin-owned entity must not be the hole in + // that rule: the request goes through this provider instead, so the same flag + // has to be set here. + send_clear_fault(fault_code, ClearOrigin::ScopedOperator); return dto::FaultClearResult{ nlohmann::json{{"status", "cleared"}, {"fault_code", fault_code}, {"entity_id", entity_id}}}; } diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp index 96c32e339..7ef7c0e4f 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_network_discovery.cpp @@ -408,8 +408,8 @@ TEST(NetworkDiscoveryRun, IdentifyFailureRecordedAsLead) { // run(cancelled): a shutdown must not wait out a whole sweep // --------------------------------------------------------------------------- // TEST(NetworkDiscoveryRun, CancelStopsTheSweepInsteadOfProbingEveryHost) { - // A /24 is 254 probes and a legal /16 is 65k; the caller runs them on the poll - // thread a shutdown has to join. With scan_concurrency 1 the sweep is + // A /24 is 254 probes and a legal /16 is 65k, and the caller runs them on the + // poll thread a shutdown has to join. With scan_concurrency 1 the sweep is // sequential, so the probe count is exactly what the cancel predicate allowed. std::atomic probes{0}; std::atomic stop{false}; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index 9984cef85..9d5a0af9a 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -822,6 +822,39 @@ TEST(RescanStep, SpacesSweepsFromTheEndOfThePreviousOne) { EXPECT_EQ(sweeps, 2); } +TEST(RescanStep, AThrowingSweepStillStampsTheCadence) { + // A sweep that throws still consumed its minutes. If the stamp were owed only + // on the normal path, the next poll iteration would find the cadence due and + // start another sweep immediately, so a server that makes the identify throw + // would turn the poll thread into a continuous scanner. + const auto t0 = std::chrono::steady_clock::time_point{}; + auto clock_now = t0 + std::chrono::seconds(30); + const auto now = [&clock_now]() { + return clock_now; + }; + int sweeps = 0; + const auto throwing_sweep = [&sweeps, &clock_now]() -> std::optional { + ++sweeps; + clock_now += std::chrono::seconds(120); + throw std::runtime_error("identify blew up mid-sweep"); + }; + + auto last_end = t0; + EXPECT_THROW(OpcuaPlugin::rescan_step(30, now, &last_end, throwing_sweep), std::runtime_error); + EXPECT_EQ(sweeps, 1); + EXPECT_EQ(last_end, clock_now) << "a sweep that threw still has to stamp the cadence"; + + // Inside the interval after that failed sweep: not due, so no second sweep. + clock_now += std::chrono::seconds(29); + EXPECT_NO_THROW(OpcuaPlugin::rescan_step(30, now, &last_end, throwing_sweep)); + EXPECT_EQ(sweeps, 1) << "a failed sweep let the next one start inside the interval"; + + // Positive control: one full interval later it is due again (and throws again). + clock_now += std::chrono::seconds(1); + EXPECT_THROW(OpcuaPlugin::rescan_step(30, now, &last_end, throwing_sweep), std::runtime_error); + EXPECT_EQ(sweeps, 2); +} + TEST(RescanStep, DoesNothingWithoutACadence) { const auto t0 = std::chrono::steady_clock::time_point{}; auto last_end = t0; @@ -929,6 +962,86 @@ TEST(DiscoverEndpoint, AnUnchangedRescanReportsAtDebugInsteadOfRepeatingItself) EXPECT_GT(info.size(), first_info) << "a changed outcome must be reported at INFO"; } +TEST(DiscoverEndpoint, APredicateThatFlipsMidSweepEndsThePass) { + // What a stop signal does to a sweep in progress. The plugin hands + // discover_endpoint a predicate that answers for both stop signals (the + // shutdown flag and rclcpp::ok()). Here it flips after a handful of probes, + // as either would mid-sweep. + std::atomic probes{0}; + std::atomic stop{false}; + auto stopping_scan = [&probes, &stop](const std::string & ip, uint16_t port, int) { + if (probes.fetch_add(1) + 1 >= 5) { + stop.store(true); + } + return ip == "192.168.1.10" && port == 4840; // the PLC IS there to be found + }; + + OpcuaDiscoveryConfig cfg = rescan_cfg(); + cfg.scan_concurrency = 1; // sequential, so the probe count is the predicate's doing + const auto chosen = OpcuaPlugin::discover_endpoint(cfg, /*endpoint_configured=*/false, stopping_scan, + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), + silent_reporter(), [&stop]() { + return stop.load(); + }); + + EXPECT_FALSE(chosen.has_value()) << "a cancelled pass must not hand back a partial result"; + EXPECT_LE(probes.load(), 6) << "the sweep ran on after the stop signal"; + + // Positive control on the same fakes: without the predicate the very same + // sweep visits all 254 hosts and selects the PLC. + probes.store(0); + stop.store(false); + const auto uncancelled = OpcuaPlugin::discover_endpoint( + cfg, /*endpoint_configured=*/false, + [&probes](const std::string & ip, uint16_t port, int) { + probes.fetch_add(1); + return ip == "192.168.1.10" && port == 4840; + }, + fake_identify({{"opc.tcp://192.168.1.10:4840", plc_identity()}}), silent_reporter()); + ASSERT_TRUE(uncancelled.has_value()); + EXPECT_EQ(*uncancelled, "opc.tcp://192.168.1.10:4840"); + EXPECT_EQ(probes.load(), 254); +} + +TEST(DiscoverEndpoint, TheScanIsAnnouncedBeforeTheSweepRuns) { + // A /16 sweep runs for minutes. If the announcement waited for the report at + // the end of the pass, start-up would log nothing while it swept and an + // operator would read that as a hung gateway. + std::vector info; + std::vector debug; + std::string announced_before_first_probe; + std::string outcome; + OpcuaPlugin::DiscoveryReporter reporter; + reporter.info = [&info](const std::string & m) { + info.push_back(m); + }; + reporter.warn = kSilent; + reporter.debug = [&debug](const std::string & m) { + debug.push_back(m); + }; + reporter.previous_outcome = &outcome; + + auto scan_recording_the_log = [&info, &announced_before_first_probe](const std::string &, uint16_t, int) { + if (announced_before_first_probe.empty() && !info.empty()) { + announced_before_first_probe = info.front(); + } + return false; + }; + OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, scan_recording_the_log, fake_identify({}), + reporter); + EXPECT_NE(announced_before_first_probe.find("read-only active scan of"), std::string::npos) + << "the sweep started before the operator was told anything (first INFO line: '" + << (info.empty() ? std::string("") : info.front()) << "')"; + + // On a rescan the announcement drops to DEBUG: the sweep repeats every + // interval_s for the life of the outage and must not narrate every pass. + const size_t info_after_first = info.size(); + OpcuaPlugin::discover_endpoint(rescan_cfg(), /*endpoint_configured=*/false, fake_scan({}), fake_identify({}), + reporter); + EXPECT_EQ(info.size(), info_after_first) << "the rescan announced itself at INFO again"; + EXPECT_FALSE(debug.empty()); +} + TEST(DiscoverEndpoint, WithNoRepeatMemoryEveryPassIsReported) { // Positive control for the test above: the same two identical passes with no // previous_outcome (the startup scan's own reporter) report in full twice, so @@ -1001,38 +1114,61 @@ TEST(RederivedComponentIdentity, KeepsTheIdentityWhenNothingChanged) { } // --------------------------------------------------------------------------- -// ClearFault: a link-state clear does not cascade +// ClearFault: only a clear the device itself reported may cascade // --------------------------------------------------------------------------- -TEST(MakeClearFaultRequest, LinkStateClearSkipsTheCorrelationCascade) { - // The connect-time PLC_COMMS_LOST clear says the link came back. A - // correlation rule may name PLC_COMMS_LOST as the root cause of every symptom - // the outage produced, and clearing those is an operator's call, not a link - // event's. - const auto link_state = OpcuaPlugin::make_clear_fault_request(kCommsLostFaultCode, /*link_state=*/true); - EXPECT_EQ(link_state.fault_code, kCommsLostFaultCode); - EXPECT_TRUE(link_state.skip_correlation_auto_clear); - - // Positive control on the same request builder: an operator-driven clear (the - // SOVD DELETE route) leaves the cascade alone, so the flag above is the - // link-state rule and not a hardcoded true. - const auto operator_clear = OpcuaPlugin::make_clear_fault_request("PLC_TANK_HIGH", /*link_state=*/false); - EXPECT_EQ(operator_clear.fault_code, "PLC_TANK_HIGH"); - EXPECT_FALSE(operator_clear.skip_correlation_auto_clear); +TEST(ClearOrigin, OnlyADeviceReportedClearKeepsTheCorrelationCascade) { + using Origin = OpcuaPlugin::ClearOrigin; + // The link coming back is not an operator resolving a root cause, and neither + // is an operator scoped to ONE entity: a correlation rule naming + // PLC_COMMS_LOST as the root cause would otherwise clear symptom faults + // reported by apps in entities that operator cannot even see. The gateway + // applies exactly this rule on its own branch of the same DELETE route. + EXPECT_TRUE(OpcuaPlugin::clear_skips_correlation(Origin::LinkState)); + EXPECT_TRUE(OpcuaPlugin::clear_skips_correlation(Origin::ScopedOperator)); + // Positive control on the same predicate: the device reporting its own + // condition inactive IS a resolution at the source, so that clear cascades. + // Without this case the rule above would be indistinguishable from a + // hardcoded true. + EXPECT_FALSE(OpcuaPlugin::clear_skips_correlation(Origin::DeviceAlarm)); + + // The buffer's ranking is a different question from the wire flag: only the + // link-state clear is re-derivable, the operator's scoped clear is as + // one-shot as an alarm report. + EXPECT_TRUE(OpcuaPlugin::clear_is_link_state(Origin::LinkState)); + EXPECT_FALSE(OpcuaPlugin::clear_is_link_state(Origin::ScopedOperator)); + EXPECT_FALSE(OpcuaPlugin::clear_is_link_state(Origin::DeviceAlarm)); +} + +TEST(MakeClearFaultRequest, CarriesTheSkipFlagAndCodeVerbatim) { + const auto skipping = OpcuaPlugin::make_clear_fault_request(kCommsLostFaultCode, true); + EXPECT_EQ(skipping.fault_code, kCommsLostFaultCode); + EXPECT_TRUE(skipping.skip_correlation_auto_clear); + + const auto cascading = OpcuaPlugin::make_clear_fault_request("PLC_TANK_HIGH", false); + EXPECT_EQ(cascading.fault_code, "PLC_TANK_HIGH"); + EXPECT_FALSE(cascading.skip_correlation_auto_clear); } // --------------------------------------------------------------------------- -// Pending fault dispatch buffer: reports outrank clears +// Pending fault dispatch buffer: only what is re-derivable may be dropped first // --------------------------------------------------------------------------- namespace { OpcuaPlugin::PendingFaultDispatch report_entry(const std::string & code) { - return {OpcuaPlugin::PendingFaultDispatch::Kind::Report, code, []() {}}; + return {OpcuaPlugin::PendingFaultDispatch::Kind::Report, code, /*link_state=*/false, []() {}}; +} + +// A clear the next reconnect will send again (PLC_COMMS_LOST). +OpcuaPlugin::PendingFaultDispatch link_state_clear_entry(const std::string & code) { + return {OpcuaPlugin::PendingFaultDispatch::Kind::Clear, code, /*link_state=*/true, []() {}}; } -OpcuaPlugin::PendingFaultDispatch clear_entry(const std::string & code) { - return {OpcuaPlugin::PendingFaultDispatch::Kind::Clear, code, []() {}}; +// A clear nothing will re-send: the device reported its condition inactive, or +// an operator cleared through the scoped SOVD route. +OpcuaPlugin::PendingFaultDispatch device_clear_entry(const std::string & code) { + return {OpcuaPlugin::PendingFaultDispatch::Kind::Clear, code, /*link_state=*/false, []() {}}; } size_t count_kind(const std::vector & buffer, @@ -1048,14 +1184,15 @@ size_t count_kind(const std::vector & buffer, TEST(EnqueuePendingDispatch, ReconnectClearsNeverEvictABufferedAlarmReport) { // A flapping link with no fault_manager: 300 reconnects, each enqueueing a // connect-time clear, while ten real alarm reports wait to be flushed. The - // reports are one-shot edges from the PLC; the clears are re-derivable. + // reports are one-shot edges from the PLC, the clears are re-derivable. std::vector buffer; for (int i = 0; i < 10; ++i) { OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_" + std::to_string(i))); } for (int i = 0; i < 300; ++i) { - OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry(kCommsLostFaultCode)); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + link_state_clear_entry(kCommsLostFaultCode)); } EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Report), 10u) @@ -1067,7 +1204,7 @@ TEST(EnqueuePendingDispatch, ReconnectClearsNeverEvictABufferedAlarmReport) { } } -TEST(EnqueuePendingDispatch, AFullReportBufferRefusesAClearInsteadOfDroppingAReport) { +TEST(EnqueuePendingDispatch, AFullOneShotBufferRefusesALinkStateClearInsteadOfDroppingOne) { std::vector buffer; for (size_t i = 0; i < OpcuaPlugin::kMaxPendingDispatches; ++i) { OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, @@ -1076,24 +1213,25 @@ TEST(EnqueuePendingDispatch, AFullReportBufferRefusesAClearInsteadOfDroppingARep ASSERT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); EXPECT_EQ(OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, - clear_entry(kCommsLostFaultCode)), + link_state_clear_entry(kCommsLostFaultCode)), OpcuaPlugin::PendingEnqueueOutcome::Refused); EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Report), OpcuaPlugin::kMaxPendingDispatches); - EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_0") << "the oldest report must survive an incoming clear"; + EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_0") << "the oldest report must survive an incoming link-state clear"; - // A report arriving at a full buffer still drops the oldest one: reports do - // not outrank each other, so the bound still holds. + // A report arriving at the same full buffer still drops the oldest entry: + // one-shot dispatches do not outrank each other, so the bound still holds. EXPECT_EQ( OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_NEW")), - OpcuaPlugin::PendingEnqueueOutcome::EvictedReport); + OpcuaPlugin::PendingEnqueueOutcome::EvictedOldest); EXPECT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_1"); EXPECT_EQ(buffer.back().fault_code, "PLC_ALARM_NEW"); } -TEST(EnqueuePendingDispatch, AFullBufferGivesUpAPendingClearBeforeAReport) { +TEST(EnqueuePendingDispatch, AFullBufferGivesUpALinkStateClearBeforeAReport) { std::vector buffer; - OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_OLD_CLEAR")); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + link_state_clear_entry(kCommsLostFaultCode)); for (size_t i = 1; i < OpcuaPlugin::kMaxPendingDispatches; ++i) { OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_" + std::to_string(i))); @@ -1102,18 +1240,55 @@ TEST(EnqueuePendingDispatch, AFullBufferGivesUpAPendingClearBeforeAReport) { EXPECT_EQ( OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_NEW")), - OpcuaPlugin::PendingEnqueueOutcome::EvictedClear); + OpcuaPlugin::PendingEnqueueOutcome::EvictedLinkStateClear); EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Clear), 0u); - EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_1") << "the clear went, not the oldest report"; + EXPECT_EQ(buffer.front().fault_code, "PLC_ALARM_1") << "the re-derivable clear went, not the oldest report"; +} + +TEST(EnqueuePendingDispatch, ADeviceAlarmClearIsNotEvictedAheadOfAnOlderReport) { + // The device says an alarm went inactive while the fault_manager is + // unreachable. That edge is as one-shot as the raise: drop it and the flush + // replays the raise with nothing behind it, so the fault stands while the + // device reports it clear. Only the link-state clear is re-derivable. + std::vector buffer; + for (int i = 0; i < 100; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + report_entry("PLC_ALARM_" + std::to_string(i))); + } + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_TANK_HIGH")); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + device_clear_entry("PLC_TANK_HIGH")); + for (size_t i = buffer.size(); i < OpcuaPlugin::kMaxPendingDispatches; ++i) { + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + report_entry("PLC_FILLER_" + std::to_string(i))); + } + ASSERT_EQ(buffer.size(), OpcuaPlugin::kMaxPendingDispatches); + + EXPECT_EQ( + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_ALARM_NEW")), + OpcuaPlugin::PendingEnqueueOutcome::EvictedOldest); + EXPECT_NE(buffer.front().fault_code, "PLC_ALARM_0") << "the oldest entry is what ages out"; + const auto device_clear = + std::find_if(buffer.begin(), buffer.end(), [](const OpcuaPlugin::PendingFaultDispatch & entry) { + return entry.kind == OpcuaPlugin::PendingFaultDispatch::Kind::Clear && entry.fault_code == "PLC_TANK_HIGH"; + }); + ASSERT_NE(device_clear, buffer.end()) << "a device alarm's inactive edge was evicted ahead of an older report"; + // ... and it still flushes after the raise it supersedes. + const auto raise = std::find_if(buffer.begin(), buffer.end(), [](const OpcuaPlugin::PendingFaultDispatch & entry) { + return entry.kind == OpcuaPlugin::PendingFaultDispatch::Kind::Report && entry.fault_code == "PLC_TANK_HIGH"; + }); + ASSERT_NE(raise, buffer.end()); + EXPECT_LT(raise - buffer.begin(), device_clear - buffer.begin()); } TEST(EnqueuePendingDispatch, ARequeuedClearMovesToTheBackSoOrderStillHolds) { // Report-then-clear for one code must still flush in that order after the // clear is re-enqueued, or the flush would leave the fault standing. std::vector buffer; - OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_FLAP")); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, link_state_clear_entry("PLC_FLAP")); OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, report_entry("PLC_FLAP")); - EXPECT_EQ(OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_FLAP")), + EXPECT_EQ(OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, + link_state_clear_entry("PLC_FLAP")), OpcuaPlugin::PendingEnqueueOutcome::ReplacedClear); ASSERT_EQ(buffer.size(), 2u); @@ -1121,7 +1296,7 @@ TEST(EnqueuePendingDispatch, ARequeuedClearMovesToTheBackSoOrderStillHolds) { EXPECT_EQ(buffer[1].kind, OpcuaPlugin::PendingFaultDispatch::Kind::Clear) << "the newest clear must flush after the report it supersedes"; // Clears for DIFFERENT codes are independent. - OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, clear_entry("PLC_OTHER")); + OpcuaPlugin::enqueue_pending_dispatch(buffer, OpcuaPlugin::kMaxPendingDispatches, device_clear_entry("PLC_OTHER")); EXPECT_EQ(count_kind(buffer, OpcuaPlugin::PendingFaultDispatch::Kind::Clear), 2u); } @@ -1629,4 +1804,92 @@ component_id: race_runtime << "flush_pending_reports never dispatched - swap-vs-push path not covered"; } +// The SOVD per-entity route DELETE /{entity}/faults/{code} lands on +// FaultProvider::clear_fault for a plugin-owned entity, which is the branch the +// gateway takes INSTEAD of its own (where it sets skip_correlation_auto_clear +// itself). So the flag has to be set here or the documented guarantee - an +// operator scoped to one entity cannot cascade-clear symptoms reported by apps +// in other entities - has a hole exactly where a PLC is involved. This drives +// the real route entry point and reads the field off the wire. +TEST(OpcuaPluginScopedClear, SovdDeleteSkipsTheCorrelationCascade) { + ScopedRclcpp rclcpp_scope; + auto node = std::make_shared("opcua_scoped_clear_flag"); + auto fault_manager = std::make_shared("opcua_scoped_clear_faultmgr"); + + std::mutex received_mutex; + std::vector received; + auto report_srv = fault_manager->create_service( + "/fault_manager/report_fault", [](const std::shared_ptr, + std::shared_ptr res) { + res->accepted = true; + }); + auto clear_srv = fault_manager->create_service( + "/fault_manager/clear_fault", + [&received, &received_mutex](const std::shared_ptr req, + std::shared_ptr res) { + { + std::lock_guard lock(received_mutex); + received.push_back(*req); + } + res->success = true; + }); + + const std::string yaml_path = "/tmp/test_opcua_scoped_clear_nodemap.yaml"; + { + std::ofstream f(yaml_path); + f << R"( +area_id: scoped_plc +component_id: scoped_runtime +nodes: + - node_id: "ns=2;i=1" + entity_id: tank + data_name: level + data_type: float +)"; + } + + OpcuaPlugin plugin; + nlohmann::json config; + config["node_map_path"] = yaml_path; + config["endpoint_url"] = "opc.tcp://127.0.0.1:1"; // nothing listening; the fault sink is the subject + config["poll_interval_ms"] = 100; + plugin.configure(config); + + RealNodePluginContext ctx(node.get()); + ctx.entities["tank"] = {SovdEntityType::APP, "tank", "/scoped_plc", "/scoped_plc/scoped_runtime/tank"}; + plugin.set_context(ctx); + + ScopedExecutorSpin spinner({node, fault_manager}); + auto probe = node->create_client("/fault_manager/clear_fault"); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + while (!probe->service_is_ready() && std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + ASSERT_TRUE(probe->service_is_ready()) << "stub ClearFault server never became discoverable"; + + // The route's own entry point, not a helper it happens to call. + const auto result = plugin.clear_fault("tank", "PLC_TANK_HIGH"); + ASSERT_TRUE(result.has_value()); + + const auto flush_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + bool delivered = false; + while (!delivered && std::chrono::steady_clock::now() < flush_deadline) { + { + std::lock_guard lock(received_mutex); + delivered = !received.empty(); + } + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + + spinner.stop(); + plugin.shutdown(); + std::remove(yaml_path.c_str()); + + std::lock_guard lock(received_mutex); + ASSERT_FALSE(received.empty()) << "the scoped DELETE never reached the fault manager"; + EXPECT_EQ(received.front().fault_code, "PLC_TANK_HIGH"); + EXPECT_TRUE(received.front().skip_correlation_auto_clear) + << "a per-entity DELETE served by the plugin cascade-cleared correlated symptoms"; +} + } // namespace ros2_medkit_gateway From bfecdd4b1ab33587a6625684dc76559d17531689 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 20:24:12 +0200 Subject: [PATCH 8/8] test(opcua): pin the clear-origin sites and the sweep predicate on the wire Four call sites decide whether a ClearFault may cascade, and only the scoped SOVD DELETE was pinned: swapping the origin at any of the other three left the suite green. The two that need a live session are now driven against the test_alarm_server fixture with a real fault manager on the other end, so the flag is read off the wire. A successful connect must clear PLC_COMMS_LOST without cascading, and the same test drives the fixture's own CLI to fire and clear a condition, because a clear the device itself reported is a resolution at the source and must keep the cascade. Having both cases in one test is what makes each flag a decision rather than a constant. The fixture harness gained a stdin pipe to send those commands, the way the docker scenario already drives it through a FIFO. The poller's own clear travels the same callback as every device alarm, so the rule that tells them apart moved into clear_origin_for_signal and is tested on both branches, exact code match included. The sweep's cancel predicate had the same shape of hole: it is private and no test reached it, so reverting it to the shutdown flag alone left everything green while a SIGTERM during the start-up sweep would again have to wait the sweep out. The rule is now the static discovery_cancelled_for, tested on both inputs, with the member reduced to reading the two values off the process. A second test shows the input is real by shutting a private rclcpp context down and reading rclcpp::ok() back. The comment on the cancellation test no longer claims it exercises the plugin's own predicate, which it never did. Also: the remaining prose semicolons on this branch (two comments and five operator-visible log strings) are periods and commas now, and the Refused outcome's doc comment says what it means. --- .../ros2_medkit_opcua/opcua_plugin.hpp | 42 ++- .../ros2_medkit_opcua/src/opcua_plugin.cpp | 26 +- .../test/test_opcua_identity.cpp | 253 ++++++++++++++++++ .../test/test_opcua_plugin.cpp | 54 +++- 4 files changed, 343 insertions(+), 32 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index 702c6cd8d..cfe7a5bed 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -283,6 +283,30 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, return origin == ClearOrigin::LinkState; } + // Whether a discovery sweep must stop now, given the two independent stop + // signals. Static and pure so both inputs are testable: the member + // ``discovery_cancelled()`` only reads them off the process and hands them + // here, so this is the whole rule. + // + // - ``shutdown_requested`` is set by shutdown(), which the gateway calls + // after its executor returns. That ends a RESCAN sweep, which runs on the + // poll thread long after start-up. + // - ``rclcpp_ok`` is false once rclcpp's own SIGINT / SIGTERM handler has + // run. The START-UP sweep runs inside set_context(), during node + // construction and before the executor spins, so shutdown() cannot be + // reached while it is in progress and the signal is the only thing that + // can end it. + static bool discovery_cancelled_for(bool shutdown_requested, bool rclcpp_ok) { + return shutdown_requested || !rclcpp_ok; + } + + // Which kind of clear a fault-detection signal going inactive is. The poller + // emits the component-scoped ``PLC_COMMS_LOST`` clear through the same + // callback as every device alarm, and only that one is a link-state event. + static ClearOrigin clear_origin_for_signal(const std::string & fault_code) { + return fault_code == kCommsLostFaultCode ? ClearOrigin::LinkState : ClearOrigin::DeviceAlarm; + } + // Build the ClearFault request for one fault code. // ``skip_correlation_auto_clear`` goes on the wire verbatim (see ClearOrigin // for who sets it and why). Static so the wire field is assertable without a @@ -295,7 +319,7 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, struct PendingFaultDispatch { enum class Kind { Report, Clear }; Kind kind{Kind::Report}; - std::string fault_code; ///< dedup key for a Clear; diagnostic for a Report + std::string fault_code; ///< dedup key for a Clear, diagnostic for a Report /// Clear only: this dispatch is re-derivable (ClearOrigin::LinkState), so /// the buffer may drop it before anything that is not. bool link_state{false}; @@ -308,7 +332,8 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, ReplacedClear, ///< superseded the pending clear for the same fault code EvictedLinkStateClear, ///< buffer was full: dropped a re-derivable clear to make room EvictedOldest, ///< buffer was full with nothing re-derivable in it: dropped the oldest entry - Refused ///< buffer was full with nothing re-derivable and the incoming clear was + Refused ///< buffer was full with nothing re-derivable in it and the incoming + ///< dispatch was itself a re-derivable clear, so it was dropped instead }; // Enqueue policy for the bounded pending-dispatch buffer. @@ -425,16 +450,9 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // (null to report every pass in full). DiscoveryReporter discovery_reporter(std::string * previous_outcome) const; - // Abort predicate handed to a discovery sweep. Two independent stop signals, - // because the two sweeps run at different points in the process lifetime: - // - ``shutdown_requested_`` is set by shutdown(), which the gateway calls - // after its executor returns. That ends a RESCAN sweep, which runs on the - // poll thread long after start-up. - // - ``rclcpp::ok()`` turns false as soon as rclcpp's own SIGINT / SIGTERM - // handler runs. The STARTUP sweep runs inside set_context(), i.e. during - // node construction and before the executor spins, so shutdown() cannot - // be reached while it is in progress and the signal is the only thing - // that can end it. + // Abort predicate handed to a discovery sweep: reads the two stop signals off + // the process and applies ``discovery_cancelled_for``, which holds the rule + // and the reasoning behind it. bool discovery_cancelled() const; // Poll-thread hook bound into PollerConfig::rediscover_endpoint whenever diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index 158523936..bbdfc11d8 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -1052,8 +1052,7 @@ void OpcuaPlugin::on_alarm_change(const std::string & entity_id, // link-state event as the connect-time one. Every other code here is the // device reporting its condition inactive, which is a real resolution and a // one-shot edge, so it keeps the cascade and the buffer treats it as such. - send_clear_fault(signal.fault_code, - signal.fault_code == kCommsLostFaultCode ? ClearOrigin::LinkState : ClearOrigin::DeviceAlarm); + send_clear_fault(signal.fault_code, clear_origin_for_signal(signal.fault_code)); } } @@ -1311,7 +1310,7 @@ void OpcuaPlugin::clear_comms_lost_on_connect() { // not cascade-clear the symptoms the outage produced. It is also the one clear // the next reconnect re-derives, so the pending buffer may drop it before // anything one-shot. - log_info(std::string("OPC-UA connection established; clearing any standing ") + kCommsLostFaultCode); + log_info(std::string("OPC-UA connection established, clearing any standing ") + kCommsLostFaultCode); send_clear_fault(kCommsLostFaultCode, ClearOrigin::LinkState); } @@ -1723,7 +1722,7 @@ std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryCo const auto subnets = discovery.resolve_subnets(); if (subnets.empty()) { - warn_line("OPC-UA discovery: no subnet configured and could not derive a local /24; nothing to scan."); + warn_line("OPC-UA discovery: no subnet configured and could not derive a local /24, nothing to scan."); emit(); return std::nullopt; } @@ -1779,7 +1778,7 @@ std::optional OpcuaPlugin::discover_endpoint(const OpcuaDiscoveryCo const DiscoveredEndpoint * chosen = NetworkDiscovery::select_auto_endpoint(found, config.anonymous_none_only); if (chosen == nullptr) { warn_line( - "OPC-UA discovery: no auto-connectable None/Anonymous data server found; leaving the endpoint unchanged. " + "OPC-UA discovery: no auto-connectable None/Anonymous data server found, leaving the endpoint unchanged. " "Secured-only servers require operator credentials."); emit(); return std::nullopt; @@ -1796,7 +1795,7 @@ void OpcuaPlugin::run_startup_discovery() { } if (endpoint_configured_) { log_info("OPC-UA discovery enabled but endpoint_url is explicitly configured (" + client_config_.endpoint_url + - "); skipping auto-discovery to avoid a second session."); + "). Skipping auto-discovery to avoid a second session."); return; } @@ -1824,11 +1823,11 @@ void OpcuaPlugin::run_startup_discovery() { // which of the two they configured. const int startup_interval_s = effective_rescan_interval_s(discovery_config_, endpoint_configured_); if (startup_interval_s > 0) { - log_info("OPC-UA discovery: startup scan selected no endpoint; the reconnect loop rescans every " + + log_info("OPC-UA discovery: startup scan selected no endpoint. The reconnect loop rescans every " + std::to_string(startup_interval_s) + "s while down."); } else { log_warn( - "OPC-UA discovery: startup scan selected no endpoint and re-scanning is off (interval_s: 0); the endpoint " + "OPC-UA discovery: startup scan selected no endpoint and re-scanning is off (interval_s: 0). The endpoint " "stays at " + client_config_.endpoint_url + " until the plugin is restarted."); } @@ -1840,13 +1839,10 @@ void OpcuaPlugin::run_startup_discovery() { } bool OpcuaPlugin::discovery_cancelled() const { - // Either stop signal ends a sweep. shutdown() is what ends a RESCAN (it runs - // on the poll thread, long after start-up). rclcpp::ok() going false is what - // ends the STARTUP sweep, which runs during node construction where shutdown() - // is not reachable yet. Checking both in one predicate keeps the two sweeps - // from drifting apart, and rclcpp::ok() only reads the default context's - // atomic shutdown flag, so it is safe to call from either thread. - return shutdown_requested_.load() || !rclcpp::ok(); + // rclcpp::ok() only reads the default context's atomic shutdown flag, so it is + // safe to call from the set_context thread and the poll thread alike. The rule + // itself, and why both signals are needed, lives in discovery_cancelled_for. + return discovery_cancelled_for(shutdown_requested_.load(), rclcpp::ok()); } OpcuaPlugin::DiscoveryReporter OpcuaPlugin::discovery_reporter(std::string * previous_outcome) const { diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp index 93b84ee1f..f96daf0e3 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_identity.cpp @@ -45,7 +45,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -53,6 +55,8 @@ #include #include +#include +#include #include "ros2_medkit_gateway/plugins/ros_plugin_context.hpp" @@ -193,17 +197,28 @@ class AlarmServer { if (pipe(pipefd) != 0) { return false; } + int stdin_pipe[2]; + if (pipe(stdin_pipe) != 0) { + close(pipefd[0]); + close(pipefd[1]); + return false; + } pid_ = fork(); if (pid_ < 0) { close(pipefd[0]); close(pipefd[1]); + close(stdin_pipe[0]); + close(stdin_pipe[1]); return false; } if (pid_ == 0) { dup2(pipefd[1], STDOUT_FILENO); dup2(pipefd[1], STDERR_FILENO); + dup2(stdin_pipe[0], STDIN_FILENO); close(pipefd[0]); close(pipefd[1]); + close(stdin_pipe[0]); + close(stdin_pipe[1]); std::string port_str = std::to_string(port); std::vector argv_vec{binary.c_str(), "--port", port_str.c_str()}; for (const auto & arg : extra_args) { @@ -214,11 +229,27 @@ class AlarmServer { _exit(127); } close(pipefd[1]); + close(stdin_pipe[0]); read_fd_ = pipefd[0]; + write_fd_ = stdin_pipe[1]; return wait_for_ready(15000); } + // One CLI command ("fire Overpressure 750", "clear Overpressure", ...). The + // fixture reads them line by line off stdin. + bool send(const std::string & command) { + if (write_fd_ < 0) { + return false; + } + const std::string line = command + "\n"; + return write(write_fd_, line.c_str(), line.size()) == static_cast(line.size()); + } + void stop() { + if (write_fd_ >= 0) { + close(write_fd_); + write_fd_ = -1; + } if (pid_ > 0) { kill(pid_, SIGTERM); int status = 0; @@ -258,6 +289,7 @@ class AlarmServer { pid_t pid_{-1}; int read_fd_{-1}; + int write_fd_{-1}; }; std::string fixture_binary() { @@ -631,4 +663,225 @@ TEST_F(OpcuaIdentityE2ETest, SuccessfulConnectClearsCommsLostNeverRaisedHere) { << "comms-lost must not be raised while the connection is up"; } +namespace { + +// RAII rclcpp init/shutdown, tearing down only what it started. +struct ScopedRclcpp { + const bool owned_; + ScopedRclcpp() : owned_(!rclcpp::ok()) { + if (owned_) { + rclcpp::init(0, nullptr); + } + } + ~ScopedRclcpp() { + if (owned_ && rclcpp::ok()) { + rclcpp::shutdown(); + } + } + ScopedRclcpp(const ScopedRclcpp &) = delete; + ScopedRclcpp & operator=(const ScopedRclcpp &) = delete; +}; + +// The plugin only builds its fault-service clients when the context hands it a +// real node, which is what makes the ClearFault request observable on the wire. +class RealNodePluginContext : public FakePluginContext { + public: + explicit RealNodePluginContext(rclcpp::Node * node) : node_(node) { + } + rclcpp::Node * node() const override { + return node_; + } + + private: + rclcpp::Node * node_; +}; + +} // namespace + +// The connect-time clear, read off the wire. clear_comms_lost_on_connect() is +// only reachable through a connect that SUCCEEDS, so it needs the live fixture, +// and the flag it sets is only observable with a real fault-manager service on +// the other end. A correlation rule may name PLC_COMMS_LOST as the root cause of +// every symptom an outage produced, and the link coming back is not an operator +// resolving those, so this clear must not cascade. +TEST_F(OpcuaIdentityE2ETest, ConnectTimeCommsLostClearSkipsTheCorrelationCascade) { + ScopedRclcpp rclcpp_scope; + auto node = std::make_shared("opcua_identity_connect_clear"); + auto fault_manager = std::make_shared("opcua_identity_connect_clear_faultmgr"); + + std::mutex received_mutex; + std::vector cleared_requests; + auto report_srv = fault_manager->create_service( + "/fault_manager/report_fault", [](const std::shared_ptr, + std::shared_ptr res) { + res->accepted = true; + }); + auto clear_srv = fault_manager->create_service( + "/fault_manager/clear_fault", + [&cleared_requests, &received_mutex](const std::shared_ptr req, + std::shared_ptr res) { + { + std::lock_guard lock(received_mutex); + cleared_requests.push_back(*req); + } + res->success = true; + }); + + rclcpp::executors::MultiThreadedExecutor executor; + executor.add_node(node); + executor.add_node(fault_manager); + std::thread spin_thread([&executor]() { + executor.spin(); + }); + + const std::string yaml_path = write_minimal_node_map(); + OpcuaPlugin plugin; + nlohmann::json config; + config["endpoint_url"] = endpoint_; + config["node_map_path"] = yaml_path; + config["poll_interval_ms"] = 100; + plugin.configure(config); + + RealNodePluginContext ctx(node.get()); + ctx.entities["tank"] = {SovdEntityType::APP, "tank", "/test_plc", "/test_plc/test_runtime/tank"}; + // The connect inside set_context() succeeds against the fixture, which is the + // only way to reach the connect-time clear. + plugin.set_context(ctx); + + // The clear may be buffered until the stub service is DDS-matched. The poll + // thread drains the buffer on its next cycle. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(20); + bool delivered = false; + while (!delivered && std::chrono::steady_clock::now() < deadline) { + { + std::lock_guard lock(received_mutex); + delivered = !cleared_requests.empty(); + } + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + + executor.cancel(); + if (spin_thread.joinable()) { + spin_thread.join(); + } + plugin.shutdown(); + std::remove(yaml_path.c_str()); + + std::lock_guard lock(received_mutex); + ASSERT_FALSE(cleared_requests.empty()) << "a successful connect sent no ClearFault at all"; + EXPECT_EQ(cleared_requests.front().fault_code, std::string(kCommsLostFaultCode)); + EXPECT_TRUE(cleared_requests.front().skip_correlation_auto_clear) + << "the connect-time clear cascade-cleared the symptoms of the outage it ended"; +} + +// The other side of the same rule, also on the wire: when the DEVICE reports its +// condition inactive, that IS a resolution at the source, so the correlation +// engine may act on it and the flag stays off. Only a live AlarmCondition +// lifecycle reaches on_event_alarm's ClearFault arm, so this drives the +// fixture's own CLI to fire and then clear a condition. +TEST_F(OpcuaIdentityE2ETest, DeviceReportedAlarmClearKeepsTheCorrelationCascade) { + ScopedRclcpp rclcpp_scope; + auto node = std::make_shared("opcua_identity_device_clear"); + auto fault_manager = std::make_shared("opcua_identity_device_clear_faultmgr"); + + std::mutex received_mutex; + std::vector reported; + std::vector cleared_requests; + auto report_srv = fault_manager->create_service( + "/fault_manager/report_fault", + [&reported, &received_mutex](const std::shared_ptr req, + std::shared_ptr res) { + { + std::lock_guard lock(received_mutex); + reported.push_back(req->fault_code); + } + res->accepted = true; + }); + auto clear_srv = fault_manager->create_service( + "/fault_manager/clear_fault", + [&cleared_requests, &received_mutex](const std::shared_ptr req, + std::shared_ptr res) { + { + std::lock_guard lock(received_mutex); + cleared_requests.push_back(*req); + } + res->success = true; + }); + + rclcpp::executors::MultiThreadedExecutor executor; + executor.add_node(node); + executor.add_node(fault_manager); + std::thread spin_thread([&executor]() { + executor.spin(); + }); + + OpcuaPlugin plugin; + nlohmann::json config; + config["endpoint_url"] = endpoint_; + config["poll_interval_ms"] = 100; + // Zero-config native A&C on the Server EventNotifier, with auto_clear so the + // condition going inactive clears the fault without an operator ack/confirm. + config["auto_alarms"] = nlohmann::json{{"enabled", true}, {"auto_clear", true}}; + plugin.configure(config); + + RealNodePluginContext ctx(node.get()); + plugin.set_context(ctx); + + const auto reported_count = [&received_mutex, &reported]() { + std::lock_guard lock(received_mutex); + return reported.size(); + }; + // The connect-time PLC_COMMS_LOST clear also lands here (this connect + // succeeded), so a clear is looked up by the code it names. + const auto clear_for = [&received_mutex, &cleared_requests](const std::string & code) -> std::optional { + std::lock_guard lock(received_mutex); + for (const auto & req : cleared_requests) { + if (req.fault_code == code) { + return req.skip_correlation_auto_clear; + } + } + return std::nullopt; + }; + + // Fire until the event subscription is up and a report lands. The retry is the + // subscription handshake, not flakiness in the assertion: an event fired + // before the subscribe simply is not delivered. + const auto fire_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); + while (reported_count() == 0 && std::chrono::steady_clock::now() < fire_deadline) { + ASSERT_TRUE(server_.send("fire Overpressure 750")); + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + } + ASSERT_GT(reported_count(), 0u) << "the fixture's AlarmCondition never reached the fault manager"; + + std::string alarm_code; + { + std::lock_guard lock(received_mutex); + alarm_code = reported.front(); + } + ASSERT_TRUE(server_.send("clear Overpressure")); + const auto clear_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); + while (!clear_for(alarm_code).has_value() && std::chrono::steady_clock::now() < clear_deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + + executor.cancel(); + if (spin_thread.joinable()) { + spin_thread.join(); + } + plugin.shutdown(); + + const auto device_clear_skips = clear_for(alarm_code); + ASSERT_TRUE(device_clear_skips.has_value()) + << "the device reporting condition " << alarm_code << " inactive sent no ClearFault"; + EXPECT_FALSE(*device_clear_skips) << "a clear the device itself reported must keep the correlation cascade"; + + // The connect-time clear travelled the same wire in the same test, and it is + // the opposite case: not an operator resolving anything, so it does not + // cascade. Having both here is what makes the flag above a decision rather + // than a constant. + const auto link_state_clear_skips = clear_for(kCommsLostFaultCode); + ASSERT_TRUE(link_state_clear_skips.has_value()) << "the connect-time clear never arrived"; + EXPECT_TRUE(*link_state_clear_skips); +} + } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index 9d5a0af9a..0f53ddb05 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -963,10 +963,11 @@ TEST(DiscoverEndpoint, AnUnchangedRescanReportsAtDebugInsteadOfRepeatingItself) } TEST(DiscoverEndpoint, APredicateThatFlipsMidSweepEndsThePass) { - // What a stop signal does to a sweep in progress. The plugin hands - // discover_endpoint a predicate that answers for both stop signals (the - // shutdown flag and rclcpp::ok()). Here it flips after a handful of probes, - // as either would mid-sweep. + // What a stop signal does to a sweep in progress. This predicate is the + // test's own, standing in for the one the plugin passes: it flips after a + // handful of probes, as either of the plugin's two stop signals would + // mid-sweep. The plugin's own predicate is pinned separately, by + // DiscoveryCancelledFor. std::atomic probes{0}; std::atomic stop{false}; auto stopping_scan = [&probes, &stop](const std::string & ip, uint16_t port, int) { @@ -1113,10 +1114,53 @@ TEST(RederivedComponentIdentity, KeepsTheIdentityWhenNothingChanged) { EXPECT_EQ(host_derived->id, "opcua-192_168_1_10"); } +// --------------------------------------------------------------------------- +// The stop signals a discovery sweep watches +// --------------------------------------------------------------------------- + +TEST(DiscoveryCancelledFor, EitherStopSignalEndsASweep) { + // The plugin's own predicate is this rule applied to two values it reads off + // the process, so this is the whole of it. + EXPECT_FALSE(OpcuaPlugin::discovery_cancelled_for(/*shutdown_requested=*/false, /*rclcpp_ok=*/true)) + << "a running process must not cancel its own sweep"; + // shutdown() ends a rescan sweep on the poll thread. + EXPECT_TRUE(OpcuaPlugin::discovery_cancelled_for(/*shutdown_requested=*/true, /*rclcpp_ok=*/true)); + // SIGINT / SIGTERM ends the start-up sweep, which runs during node + // construction where shutdown() cannot be reached at all. + EXPECT_TRUE(OpcuaPlugin::discovery_cancelled_for(/*shutdown_requested=*/false, /*rclcpp_ok=*/false)) + << "a signal during the start-up sweep left it running"; + EXPECT_TRUE(OpcuaPlugin::discovery_cancelled_for(true, false)); +} + +TEST(DiscoveryCancelledFor, RclcppOkIsTheSignalTheStartUpSweepWatches) { + // The second input is not hypothetical: rclcpp's shutdown is what a SIGTERM + // turns into, and it is observable exactly this way. A private context keeps + // the process-wide default one (which other tests here initialise) untouched. + auto context = std::make_shared(); + context->init(0, nullptr); + ASSERT_TRUE(rclcpp::ok(context)); + EXPECT_FALSE(OpcuaPlugin::discovery_cancelled_for(/*shutdown_requested=*/false, rclcpp::ok(context))); + + context->shutdown("simulated SIGTERM"); + ASSERT_FALSE(rclcpp::ok(context)) << "rclcpp::ok did not follow the shutdown a signal performs"; + EXPECT_TRUE(OpcuaPlugin::discovery_cancelled_for(/*shutdown_requested=*/false, rclcpp::ok(context))); +} + // --------------------------------------------------------------------------- // ClearFault: only a clear the device itself reported may cascade // --------------------------------------------------------------------------- +TEST(ClearOriginForSignal, OnlyTheCommsLostCodeIsALinkStateClear) { + // The poller emits its component-scoped comms-lost clear through the same + // callback as every device alarm going inactive, so the fault code is the only + // thing that tells the two apart on that path. + EXPECT_EQ(OpcuaPlugin::clear_origin_for_signal(kCommsLostFaultCode), OpcuaPlugin::ClearOrigin::LinkState); + EXPECT_EQ(OpcuaPlugin::clear_origin_for_signal("PLC_TANK_HIGH"), OpcuaPlugin::ClearOrigin::DeviceAlarm); + EXPECT_EQ(OpcuaPlugin::clear_origin_for_signal(std::string(kCommsLostFaultCode) + "_UPSTREAM"), + OpcuaPlugin::ClearOrigin::DeviceAlarm) + << "the match must be the exact code, not a prefix"; +} + TEST(ClearOrigin, OnlyADeviceReportedClearKeepsTheCorrelationCascade) { using Origin = OpcuaPlugin::ClearOrigin; // The link coming back is not an operator resolving a root cause, and neither @@ -1851,7 +1895,7 @@ component_id: scoped_runtime OpcuaPlugin plugin; nlohmann::json config; config["node_map_path"] = yaml_path; - config["endpoint_url"] = "opc.tcp://127.0.0.1:1"; // nothing listening; the fault sink is the subject + config["endpoint_url"] = "opc.tcp://127.0.0.1:1"; // nothing listening, the fault sink is the subject config["poll_interval_ms"] = 100; plugin.configure(config);