Extract GraphProviderPlugin to loadable .so plugin#289
Conversation
Prepares for PluginContext vtable changes (new virtual methods for entity snapshot, fault listing, and sampler registration). Refs #277
…ng, sampler registration Add three new virtual methods to PluginContext: - get_entity_snapshot(): bulk entity access via IntrospectionInput - list_all_faults(): query all faults without direct FaultManager access - register_sampler(): register cyclic subscription samplers These replace the dynamic_cast<GatewayNode*> pattern used by plugins that need entity cache, fault data, or sampler registration. Refs #277
Replace direct GatewayNode access with PluginContext methods: - get_entity_snapshot() instead of get_thread_safe_cache() - list_all_faults() instead of get_fault_manager()->list_faults() - register_sampler() instead of get_sampler_registry()->register_sampler() Remove dynamic_cast<GatewayNode*> and GatewayNode/FaultManager includes. Update tests to set FakePluginContext::entity_snapshot_ so the HTTP-path rebuild in get_cached_or_built_graph() uses the correct entity state. Refs #277
New colcon package for the graph provider plugin as a loadable .so. Extracted from ros2_medkit_gateway - the plugin is now independently buildable and optional. Scopes diagnostic_msgs dependency to this package only. Refs #277
Move FakePluginContext-based unit tests from ros2_medkit_gateway. GatewayNode-based tests will be migrated to integration tests separately. Make PluginContext::send_error/send_json inline so the new package can link the test without depending on gateway_lib at link time. Refs #277
Remove graph_provider_plugin sources, header, tests, and hardcoded add_plugin() call. Remove diagnostic_msgs dependency (only used by graph provider). The plugin now lives in ros2_medkit_graph_provider. Refs #277
Launch file computes absolute path to graph provider .so using ament_index. Plugin path resolution uses explicit absolute paths only - no magic auto-resolution in the loader. Gateway starts gracefully even if the graph provider package is not installed (launch file catches PackageNotFoundError). Refs #277
Unit tests must not depend on specific plugins being loaded. Graph provider assertions moved to integration tests. Refs #277
… linking Revert the inlining of PluginContext::send_error/send_json that broke beacon plugin tests (redefinition errors). Restore out-of-line definitions in plugin_context.cpp delegating to HandlerContext. For the graph provider MODULE target, use --unresolved-symbols=ignore-all so send_error/send_json resolve at runtime from the host process. Add stub implementations in graph provider unit tests (matching beacon plugin test pattern). Refs #277
Launch gateway with graph provider .so loaded dynamically via hybrid
discovery mode. Verify:
- Plugin loads and registers x-medkit-graph capability on functions
- GET /functions/{id}/x-medkit-graph returns valid graph document
- Nonexistent function returns 404
- Graph nodes reflect apps hosted by the function
Refs #277
Add ros2_medkit_graph_provider package docs, configuration examples, and new PluginContext API methods. Refs #277
- Add missing test_depend for ros2_medkit_graph_provider in integration tests package.xml (CI blocker - colcon didn't guarantee build order) - Restore default plugins: [""] in gateway_params.yaml (was causing ERROR logs on standalone launches without gateway.launch.py) - Launch file now injects both plugins list and path when graph provider is installed, with visible print() when package is not found - Fix stale "1 built-in plugin" log message (no built-in plugins remain) - Fix API version in plugin-system.rst docs (was "1", now "4") - Clarify ABI compat note: rebuild required for version check even though new PluginContext methods have default no-op implementations - Declare plugin path parameters as read_only to prevent runtime changes Refs #277
The default no-op register_sampler() took std::function by value, triggering performance-unnecessary-value-param on every TU that includes plugin_context.hpp. Take by const ref instead. Refs #277
|
Hi @eclipse0922 could you check this? I moved your plugin to a separate package. |
There was a problem hiding this comment.
Pull request overview
This PR extracts the GraphProviderPlugin from the gateway binary into an optional, dynamically loaded MODULE .so shipped as a new colcon package (ros2_medkit_graph_provider). It extends the plugin context API so plugins can access entity snapshots, faults, and cyclic sampler registration without dynamic_cast<GatewayNode*>, and updates docs/tests accordingly.
Changes:
- Introduces
ros2_medkit_graph_providerpackage that builds and installslibros2_medkit_graph_provider_plugin.sowith properextern "C"exports. - Extends
PluginContextwithget_entity_snapshot(),list_all_faults(), andregister_sampler()and wires these into the gateway’s concrete plugin context. - Updates gateway launch/config/docs/tests to load the graph provider plugin by path and validates functionality via a new integration test.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt | Builds/installs graph provider MODULE plugin + unit test target. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/package.xml | New plugin package manifest + dependencies. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/include/ros2_medkit_graph_provider/graph_provider_plugin.hpp | Public header for extracted plugin. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin.cpp | Plugin implementation updated to use PluginContext accessors instead of GatewayNode casts. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/src/graph_provider_plugin_exports.cpp | dlopen export surface (plugin_api_version, create_plugin, provider getter). |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/test/test_graph_provider_plugin.cpp | Plugin tests updated/migrated for new context API and package layout. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_context.hpp | Adds new plugin context methods (snapshot/faults/sampler registration). |
| src/ros2_medkit_gateway/src/plugins/plugin_context.cpp | Implements new context methods for the gateway runtime. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_types.hpp | Bumps PLUGIN_API_VERSION to 4. |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Removes built-in graph plugin injection; adds read-only plugin path parameter descriptor; passes sampler registry into plugin context. |
| src/ros2_medkit_gateway/CMakeLists.txt | Drops graph provider sources and diagnostic_msgs dependency from core gateway build/tests. |
| src/ros2_medkit_gateway/package.xml | Removes diagnostic_msgs dependency from gateway package. |
| src/ros2_medkit_gateway/launch/gateway.launch.py | Resolves graph provider .so path via ament index and injects parameters when installed. |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Adds graph provider plugin parameters and notes optional loading via launch. |
| src/ros2_medkit_gateway/test/test_gateway_node.cpp | Removes assertions that depended on built-in graph provider presence. |
| src/ros2_medkit_integration_tests/test/features/test_graph_provider_plugin.test.py | New end-to-end test verifying dynamic plugin loading and endpoint behavior. |
| src/ros2_medkit_integration_tests/package.xml | Adds ros2_medkit_graph_provider as a test dependency. |
| docs/tutorials/plugin-system.rst | Documents new PluginContext APIs and the optional graph provider plugin. |
src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt
Outdated
Show resolved
Hide resolved
| # Allow unresolved symbols - they resolve from the host process at runtime | ||
| target_link_options(ros2_medkit_graph_provider_plugin PRIVATE | ||
| -Wl,--unresolved-symbols=ignore-all | ||
| ) | ||
|
|
There was a problem hiding this comment.
Intentional. MODULE targets with --unresolved-symbols=ignore-in-shared-libs still fail because the unresolved symbols are in the MODULE's own object files, not in linked shared libs. ignore-all is required here because gateway symbols (PluginContext::send_error, send_json, GatewayPlugin::log_info etc.) resolve at runtime from the host gateway_node executable via ENABLE_EXPORTS ON + RTLD_LOCAL. This is the same pattern used by all plugin .so targets in this workspace.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_context.hpp
Show resolved
Hide resolved
Thanks for the changes, I’ll review it. |
|
Thanks for taking this on. I think extracting I have one structural concern. After the extraction, the configuration boundary feels a bit inconsistent. The gateway plugin system models plugin config as Because of that, the package split looks right conceptually, but the runtime/config boundary does not feel fully aligned yet. I think it would be better if the extracted plugin followed the same config path as the rest of the plugin system, so the docs, YAML shape, and runtime behavior all match. Other than that, the overall extraction looks solid to me. My only non-blocking note is that |
- Fix pre-existing bug: load_parameters() now reads from JSON config passed to configure() instead of mismatched ROS parameter names (graph_provider.* vs plugins.graph_provider.*) - Add unit test for cyclic subscription sampler registration - Add config override test via configure() JSON - Add diagnostics subscription test with rclcpp::Node - Add @verifies REQ_INTEROP_003 tags to key route tests - Launch file verifies .so exists on disk before injecting path Refs #277
ROS_DOMAIN_ID=70 was already used by ros2_medkit_param_beacon, causing potential DDS cross-talk when CTest runs in parallel. Refs #277
|
@eclipse0922 Thanks for the review! Good catch on the config boundary - you're right that Regarding PluginContext growing around graph-provider needs - agreed. The three new methods ( |
Resolve conflicts between graph provider extraction and locking feature: - plugin_context.hpp: keep both entity snapshot/faults/sampler and lock methods - plugin_context.cpp: keep both includes and implementations - test_graph_provider_plugin.cpp: add lock method stubs to FakePluginContext Adapt graph provider CMakeLists.txt to use ros2_medkit_cmake package (replaces removed cmake/ directory path from PR #294).
Pull Request
Summary
Extract
GraphProviderPluginfrom a hardcoded built-in plugin into a dynamically loadable.soin a separate colcon package (ros2_medkit_graph_provider). Users can now opt-in/out of the graph provider without recompiling the gateway.Key changes:
PluginContextwithget_entity_snapshot(),list_all_faults(),register_sampler()- replacing thedynamic_cast<GatewayNode*>patternsrc/ros2_medkit_plugins/ros2_medkit_graph_provider/following the beacon plugin MODULE patterndiagnostic_msgsdependency to the plugin package onlyPLUGIN_API_VERSIONto 4 (PluginContext vtable changes)Issue
Type
Testing
test_graph_provider_plugin.test.pyvalidates plugin loading, capability registration, and graph endpoint responsestest_gateway_node.cppBreaking changes
PLUGIN_API_VERSIONbumped from 3 to 4 - external plugins must be recompileddiagnostic_msgsremoved fromros2_medkit_gatewaydependenciesChecklist