Data triggers on plugin entities: resolve from declared topics, fail loud on bad names - #592
Conversation
PLC symbols register under sanitized leaf names (MAIN.counter -> counter), so the fault-trigger 400 now resolves that mapping outright and ranks the truncated available-list by edit distance instead of cutting it alphabetically before the entry the operator needed (#584).
A trigger on a plugin entity resolved against the ROS-graph topic list, which never carries plugin data points, so it returned 201 ACTIVE and stayed silent forever. Reject at create with a did-you-mean when the name is not a data point the plugin can read; an existing point that did not resolve keeps the deferred path (the entity cache lags the live node map by up to one refresh cycle), and deferred resolution now warns when it gives up instead of dropping the retry entry silently.
The PLC bridges have no DataProvider, so GET /apps/<id>/data 404ed for exactly the entities whose values freeze-frame already reads in-process. Fall back to the same route dispatch, list and single-item (#584).
Graph discovery attributes /plc/* topics to the gateway node that publishes them, so entity-scoped lookups on plugin entities were empty and data triggers on PLC points never fired. Declare each non-string entry's topic on its entity in introspect() and document the field.
There was a problem hiding this comment.
Pull request overview
This PR fixes a class of “ACTIVE but dead” triggers for plugin-provided entities by ensuring topic resolution can succeed (via plugin-declared topics) and by making bad data-point names fail loudly with actionable suggestions, while also improving operator visibility when deferred resolution gives up.
Changes:
- OPC-UA plugin now declares value-bridge
/plc/...topics on the owning entity during introspection so entity-scoped topic lookup (trigger resolution) can work. - Data triggers and fault triggers improve “unknown data point” handling with deterministic leaf-name suggestions and edit-distance-ranked “closest” lists.
- Plugin entities’
/dataendpoints now fall back to in-process dispatch of the plugin’sx-plc-dataroute when noDataProviderexists; deferred trigger resolution expiry now emits a warning.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp | Declares value-bridge topics on plugin-owned entities during introspection. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp | Adds coverage asserting topic declaration behavior for non-string entries. |
| src/ros2_medkit_gateway/src/http/handlers/trigger_handlers.cpp | Rejects unknown plugin data points early (400) with suggestions/closest list. |
| src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp | Falls back to plugin x-plc-data route when no DataProvider exists. |
| src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp | Extends in-process route fetch to support list vs single-item dispatch. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/plugins/plugin_manager.hpp | Updates API docs/signature for optional single-item dispatch. |
| src/ros2_medkit_gateway/src/core/managers/trigger_manager.cpp | Adds warning sink + configurable unresolved timeout; warns on expiry. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/trigger_manager.hpp | Exposes warn-log hook and unresolved-timeout setter. |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Wires TriggerManager warning sink to ROS logger. |
| src/ros2_medkit_gateway/src/core/fault_trigger_engine.cpp | Uses shared suggestion helpers + distance-ranked truncated lists. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/data_point_suggest.hpp | Introduces header-only helpers for leaf sanitization + suggestions/ranking. |
| src/ros2_medkit_gateway/test/test_data_point_suggest.cpp | Adds unit tests for suggestion helpers and ranking behavior. |
| src/ros2_medkit_gateway/test/test_fault_trigger_engine.cpp | Adds regression coverage for “did you mean” + ranked truncation. |
| src/ros2_medkit_gateway/test/test_plugin_manager.cpp | Adds coverage for single-item x-plc-data/<item> dispatch. |
| src/ros2_medkit_gateway/test/test_trigger_manager_routing.cpp | Adds coverage ensuring unresolved-trigger expiry warns and stops retrying. |
| src/ros2_medkit_gateway/CMakeLists.txt | Registers the new suggestion-helper unit test. |
| docs/tutorials/plugin-system.rst | Documents declaring App::topics for plugin topic-backed data points. |
Suppressed comments (1)
src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/data_point_suggest.hpp:116
- After switching this header to
#pragma once, this trailing#endifbecomes unmatched and will break compilation. Remove it as part of the conversion away from the include guard.
#endif // ROS2_MEDKIT_GATEWAY__CORE__DATA_POINT_SUGGEST_HPP_
bburda
left a comment
There was a problem hiding this comment.
Two things outside this diff that I think also need a change:
- There is no integration test for the three behaviours here: that a data trigger on a plugin entity now resolves and fires, that the 400 really comes back from
POST /apps/<id>/triggers, and thatGET /apps/<id>/dataserves the route fallback. The reported failure was "passed validation, returned 201, never fired", and unit tests cannot catch that kind of failure.test/demo_nodes/test_route_data_plugin.cppalready exists as anx-plc-datafixture, andtest_entity_freeze_frame.test.pyalready loads it, so the setup is there. docs/api/rest.rstlists the 400 conditions forPOST /{entity}/triggers, but the new case "data point does not exist on a plugin entity" is missing there. The/datafallback is not documented there either.
Create-time enumeration guards on content_has_live_data (a dead-link bridge answers 200 with empty items - that is unreadable, not empty), tries DataProvider before the apps-scoped route so plugin components and provider-only plugins get the same reject-or-park split, and the retry budget scales with refresh_interval_ms so the two cannot be configured into a contradiction. Suggestion distances get the budget of the string they were measured against. The /data fallback normalizes items to carry id and category. The data capability survives when the route can serve it. opcua declaration and publisher creation share one predicate. Warnings emit outside triggers_mutex_; the suggest header uses pragma once.
A minimal topic-backed test plugin (declares its value topic on the entity, publishes it, serves x-plc-data) drives the three reported behaviours through a real gateway: a data trigger on a plugin entity resolves and delivers SSE events, a wrong data point name 400s with a suggestion at create time, and GET /data serves the normalized route fallback.
|
On the integration-test ask: e003512 adds a minimal topic-backed test plugin (declares its value topic on the owning entity, publishes it at 5 Hz, serves x-plc-data) and drives the three reported behaviours through a real gateway in test_triggers_plugin_entity: the data trigger resolves and delivers SSE events, a wrong data point name comes back 400 with did_you_mean at create time, and GET /apps//data serves the normalized route fallback. All three ran green locally (Ran 3 tests, OK) along with the full gateway and opcua unit suites. |
Fixes #584.
A data trigger on a plugin-provided entity passed validation, returned 201 ACTIVE, and never fired. Topic resolution reads the entity's topic list from the discovery cache, and that list is built from the ROS graph, which attributes plugin-published
/plc/*topics to the gateway node - so plugin entities always resolved to nothing. The deferred-resolution retry ran the same empty lookup every 5 s and silently dropped the entry after 60 s, leaving the trigger permanently active and permanently dead.What changed:
App::topicswas already part of the plugin introspection API and flows through both entity-injection paths; the opcua plugin now declares each non-string entry's value-bridge topic on the entity that owns it, which makes the existing resolution and firing path just work. Documented in the plugin tutorial as the pattern for topic-backed data points.MAIN.counterresolves deterministically), and the closest names inparams. An existing point that merely failed to resolve keeps the deferred path - the cache lags the live node map by at most one refresh cycle, which is inside the retry budget.GET /apps/<id>/data(list and single item) falls back to the plugin's ownx-plc-dataroute when there is no DataProvider - the same in-process dispatch freeze-frame already uses - instead of 404ing on entities whose values other parts of the gateway happily read.Verified: full gateway + opcua unit suites in the jazzy container (129 + 16 green;
test_opcua_securedfails in that local container on unmodified main as well, so CI here is the arbiter), and end to end against a live Beckhoff target through a protocol plugin that declares its topics - trigger create resolves and subscribes to the declared/plc/...topic, and OnChange stays correctly silent for a constant value.