Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified shared-library plugin framework for ros2_medkit_gateway, replacing the previous update-backend-only plugin mechanism and enabling plugins to provide update backends, introspection (preview), and custom REST routes.
Changes:
- Added core plugin ABI + loader/manager (
GatewayPlugin,PluginLoader,PluginManager) with lifecycle orchestration and error isolation. - Migrated software updates from
UpdateBackend/UpdatePluginLoadertoUpdateProviderintegrated via the plugin framework. - Added extensive unit + integration test coverage and updated docs/config/changelog for the new
pluginsparameter model.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/test/features/test_updates.test.py | Updates integration test configuration to use plugins parameters. |
| src/ros2_medkit_gateway/test/test_update_manager.cpp | Adapts UpdateManager unit tests to non-owning UpdateProvider* wiring. |
| src/ros2_medkit_gateway/test/test_plugin_manager.cpp | New unit tests for PluginManager lifecycle, dispatch, and isolation. |
| src/ros2_medkit_gateway/test/test_plugin_loader.cpp | New unit tests for dlopen loader, ABI checks, and error paths. |
| src/ros2_medkit_gateway/test/demo_nodes/test_version_only_plugin.cpp | Test fixture plugin exporting only plugin_api_version(). |
| src/ros2_medkit_gateway/test/demo_nodes/test_update_backend.cpp | Converts integration-test backend to a plugin (GatewayPlugin + UpdateProvider). |
| src/ros2_medkit_gateway/test/demo_nodes/test_null_factory_plugin.cpp | Test fixture plugin with null factory return. |
| src/ros2_medkit_gateway/test/demo_nodes/test_no_symbols_plugin.cpp | Test fixture shared library with no required plugin symbols. |
| src/ros2_medkit_gateway/test/demo_nodes/test_minimal_plugin.cpp | Test fixture minimal valid plugin (required exports only). |
| src/ros2_medkit_gateway/test/demo_nodes/test_gateway_plugin.cpp | Test fixture full-feature plugin (providers + custom route). |
| src/ros2_medkit_gateway/test/demo_nodes/test_bad_version_plugin.cpp | Test fixture plugin with API version mismatch. |
| src/ros2_medkit_gateway/src/updates/update_manager.cpp | Removes dlopen ownership; adds set_backend(UpdateProvider*). |
| src/ros2_medkit_gateway/src/updates/plugin_loader.cpp | Removes legacy updates-only plugin loader. |
| src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp | New plugin lifecycle orchestration, dispatch, and isolation logic. |
| src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp | New dlopen/dlsym loader with path validation + API versioning. |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Registers plugin-provided routes during REST server setup. |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Adds plugin parameter handling + loads plugins; wires updates via PluginManager. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp | New header containing update-related shared types. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_manager.hpp | Migrates UpdateManager to UpdateProvider interface. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_backend.hpp | Removes legacy UpdateBackend interface header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/plugin_loader.hpp | Removes legacy updates-only plugin loader header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/update_provider.hpp | New provider interface replacing UpdateBackend. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/introspection_provider.hpp | New provider interface for platform introspection (preview). |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_types.hpp | Defines plugin ABI constants/types and export visibility macro. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp | Declares PluginManager API and dispatch semantics. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp | Declares plugin loader + RAII load result contract. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/gateway_plugin.hpp | Defines the base plugin lifecycle interface + logging callback. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Exposes PluginManager accessor; enforces member destruction order. |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Updates configuration to plugins array and removes updates backend params. |
| src/ros2_medkit_gateway/CMakeLists.txt | Wires new plugin sources; adds plugin .so fixtures and new tests. |
| docs/tutorials/plugin-system.rst | New tutorial documenting the plugin system and ABI. |
| docs/tutorials/index.rst | Adds plugin tutorial to Sphinx toctree. |
| docs/troubleshooting.rst | Updates guidance to reflect plugin-based endpoint extension. |
| docs/config/server.rst | Documents plugin framework parameters and lifecycle. |
| docs/changelog.rst | Documents new plugin framework and breaking config changes. |
| docs/api/rest.rst | Updates updates API docs to reference plugin framework + UpdateProvider. |
| .gitignore | Adds .worktrees/ ignore entry. |
| return false; | ||
| } | ||
| return std::all_of(name.begin(), name.end(), [](char c) { | ||
| return std::isalnum(c) || c == '_' || c == '-'; |
There was a problem hiding this comment.
std::isalnum(c) is undefined behavior when char is signed and c is negative (non-ASCII bytes). Cast to unsigned char (and typically to int) before calling std::isalnum to make the validation safe and locale-correct.
| return std::isalnum(c) || c == '_' || c == '-'; | |
| return std::isalnum(static_cast<unsigned char>(c)) || c == '_' || c == '-'; |
There was a problem hiding this comment.
Fixed in e51e8d7 - added static_cast<unsigned char>(c) before std::isalnum.
| for (const auto & pname : plugin_names) { | ||
| if (!is_valid_plugin_name(pname)) { | ||
| RCLCPP_ERROR(get_logger(), | ||
| "Invalid plugin name '%s': must be alphanumeric, underscore, or hyphen (max 256 chars)", | ||
| pname.c_str()); | ||
| continue; | ||
| } | ||
| auto path_param = "plugins." + pname + ".path"; | ||
| declare_parameter(path_param, std::string("")); | ||
| auto path = get_parameter(path_param).as_string(); |
There was a problem hiding this comment.
plugins can contain duplicate names; in that case declare_parameter("plugins.<name>.path", ...) will throw on the second occurrence and crash node startup. Consider deduplicating plugin_names (e.g., track a std::unordered_set) or checking has_parameter(path_param) before declaring.
There was a problem hiding this comment.
Fixed in e51e8d7 - added std::unordered_set to skip duplicates with a warning.
| void PluginManager::disable_plugin(LoadedPlugin & lp) { | ||
| lp.update_provider = nullptr; | ||
| lp.introspection_provider = nullptr; | ||
| lp.load_result.update_provider = nullptr; | ||
| lp.load_result.introspection_provider = nullptr; | ||
| lp.load_result.plugin.reset(); | ||
| } |
There was a problem hiding this comment.
disable_plugin() destroys the plugin instance via lp.load_result.plugin.reset() without calling GatewayPlugin::shutdown(), but GatewayPlugin’s contract says shutdown() is called before destruction. Consider invoking shutdown() (wrapped in try/catch like shutdown_all()) before resetting the plugin, or update the documented lifecycle if this is intentional.
There was a problem hiding this comment.
Fixed in e51e8d7 - disable_plugin() now calls shutdown() (with try/catch) before resetting the plugin, matching the shutdown_all() pattern. The plugin pointer null check guards against double-shutdown.
| extern "C" GATEWAY_PLUGIN_EXPORT void * create_plugin() { | ||
| return nullptr; |
There was a problem hiding this comment.
This test plugin exports create_plugin() with return type void*, but the plugin ABI contract expects GatewayPlugin*. Even though it currently returns nullptr, keeping the signature correct avoids UB if the implementation changes and documents the contract more clearly.
There was a problem hiding this comment.
Fixed in e51e8d7 - changed return type to GatewayPlugin* and added the gateway_plugin.hpp include.
| return ros2_medkit_gateway::PLUGIN_API_VERSION + 1; | ||
| } | ||
|
|
||
| extern "C" GATEWAY_PLUGIN_EXPORT void * create_plugin() { |
There was a problem hiding this comment.
This test plugin exports create_plugin() with return type void*, but the plugin ABI contract expects GatewayPlugin*. Even if the gateway rejects this plugin on version mismatch before calling create_plugin(), keeping the signature correct helps prevent accidental UB and better reflects the intended interface.
| extern "C" GATEWAY_PLUGIN_EXPORT void * create_plugin() { | |
| extern "C" GATEWAY_PLUGIN_EXPORT ros2_medkit_gateway::GatewayPlugin * create_plugin() { |
There was a problem hiding this comment.
Fixed in e51e8d7 - same as above, changed to GatewayPlugin* with the proper include.
docs/config/server.rst
Outdated
| 3. ``create_plugin()`` factory is called to instantiate the plugin | ||
| 4. Provider interfaces are queried via ``extern "C"`` functions | ||
| 5. ``configure()`` is called with per-plugin config | ||
| 6. ``set_node()`` provides access to the ROS 2 node |
There was a problem hiding this comment.
The documentation mentions that set_node() is replaced by set_context() in the plugin lifecycle, but the comment on line 289 still refers to the old method name. The text should read "Passes the gateway context..." and mention set_context() instead of set_node().
| 6. ``set_node()`` provides access to the ROS 2 node | |
| 6. ``set_context()`` passes the gateway context to the plugin |
There was a problem hiding this comment.
Fixed in e51e8d7 - updated to set_context().
| auto update_fn = reinterpret_cast<UpdateProviderFn>(dlsym(handle, "get_update_provider")); | ||
| if (update_fn) { | ||
| result.update_provider = update_fn(raw_plugin); | ||
| } | ||
|
|
||
| using IntrospectionProviderFn = IntrospectionProvider * (*)(GatewayPlugin *); | ||
| auto introspection_fn = reinterpret_cast<IntrospectionProviderFn>(dlsym(handle, "get_introspection_provider")); | ||
| if (introspection_fn) { | ||
| result.introspection_provider = introspection_fn(raw_plugin); | ||
| } |
There was a problem hiding this comment.
The provider query functions (get_update_provider, get_introspection_provider) at lines 159-168 lack exception handling, unlike plugin_api_version() and create_plugin() which are wrapped in try-catch blocks. If a buggy plugin's get_*_provider function throws an exception, it will propagate up and potentially crash the gateway during plugin loading. Consider adding try-catch blocks around these calls for consistency with the other plugin lifecycle calls, or document why exceptions here are acceptable.
There was a problem hiding this comment.
Fixed in e51e8d7 - wrapped both provider query calls in try/catch (matching the create_plugin() pattern). On exception the provider pointer stays nullptr and the plugin continues loading with that provider unavailable - provider interfaces are optional, so a broken provider should not prevent the core plugin from functioning.
| // Determine expected type from route path | ||
| auto expected_type = SovdEntityType::UNKNOWN; | ||
| auto path = req.path; | ||
| if (path.find("/components/") != std::string::npos) { |
There was a problem hiding this comment.
path.find("/components/") matches substrings anywhere - a vendor endpoint like /api/v1/x-vendor/components/foo would incorrectly match as COMPONENT. Consider reusing extract_entity_type_from_path() from http_utils.hpp which does proper boundary-checked matching.
There was a problem hiding this comment.
Fixed in e51e8d7 - replaced the find() chain with extract_entity_type_from_path() from http_utils.hpp, which does proper segment-boundary matching.
| namespace ros2_medkit_gateway { | ||
| namespace handlers { | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
If a plugin registers "x-foo" via both register_capability(APP, "x-foo") and register_entity_capability("sensor1", "x-foo"), the capability appears twice in the response. Dedup only works within each scope, not across both.
There was a problem hiding this comment.
You're right - the two scopes are concatenated in append_plugin_capabilities() without cross-dedup. Per-scope dedup prevents accidental double-registration within each map, but a plugin deliberately registering the same capability at both type and entity level would produce duplicates in the response. That said, the scenario requires intentional misuse of two separate registration APIs for the same string. Adding a dedup pass in append_plugin_capabilities() (e.g. collecting into an unordered_set first) is cheap - I can add it if you think it's worth the extra safety. Otherwise happy to leave it as a known edge case.
| return tl::make_unexpected("Plugin path must be absolute: " + plugin_path); | ||
| } | ||
|
|
||
| if (fs_path.extension() != ".so") { |
There was a problem hiding this comment.
fs_path.extension() != ".so" rejects versioned libs like .so.1. Not a real issue for custom plugins, but worth a comment explaining this is intentional.
There was a problem hiding this comment.
Intentional - plugins are loaded by explicit absolute path from config, not via ldconfig. Versioned sonames (libfoo.so.1) are a system packaging concern; gateway plugins are always unversioned .so files. Added a comment in e51e8d7 to document this intent.
| if (!first) { | ||
| first = lp.update_provider; | ||
| } else { | ||
| RCLCPP_WARN(logger(), "Multiple UpdateProvider plugins loaded - ignoring '%s'", |
There was a problem hiding this comment.
The "Multiple UpdateProvider" warning fires every time get_update_provider() is called. Currently once at init (fine), but if called again it spams. Consider logging it once during load_plugins() instead.
There was a problem hiding this comment.
Fixed in e51e8d7 - moved the multi-provider detection and warning into load_plugins(), where providers are first discovered. get_update_provider() is now a pure accessor returning a cached first_update_provider_ member. Cache is also properly invalidated (with re-scan) in disable_plugin() if the disabled plugin was the cached provider.
|
|
||
| // Prevent copy/move (owns async tasks) | ||
| UpdateManager(const UpdateManager &) = delete; | ||
| UpdateManager & operator=(const UpdateManager &) = delete; |
There was a problem hiding this comment.
set_backend() is safe (called once at init) but a doc comment like "Must be called before any update operations" would prevent future misuse.
There was a problem hiding this comment.
Fixed in e51e8d7 - added doc comment with both ownership and temporal ordering: "Must be called before any update operations; operations return NoBackend error until set."
docs/tutorials/plugin-system.rst
Outdated
| ------------------ | ||
|
|
||
| - **Same compiler and ABI** as the gateway executable | ||
| - **RTTI must be enabled** - do NOT compile plugins with ``-fno-rtti`` |
There was a problem hiding this comment.
"RTTI must be enabled" - this is only needed for in-process add_plugin() which uses dynamic_cast. PluginLoader::load() avoids RTTI via extern "C". Worth clarifying which scenario needs it.
There was a problem hiding this comment.
Fixed in e51e8d7 - clarified that RTTI is only needed for in-process add_plugin() (which uses dynamic_cast), and that shared-library plugins loaded via PluginLoader::load() use extern "C" query functions and do not require RTTI.
Define the base class for all gateway plugins with lifecycle methods (configure, set_node, register_routes, shutdown) and callback-based logging. Plugins implement this plus typed provider interfaces. Refs #235
Typed provider interfaces that plugins implement alongside GatewayPlugin. UpdateProvider mirrors existing UpdateBackend. IntrospectionProvider defines the enrichment/discovery interface. Refs #235
Loads GatewayPlugin instances from .so via dlopen/dlsym. Checks plugin_api_version() before calling create_plugin() factory. Test plugin implements both UpdateProvider and IntrospectionProvider. Refs #235
Replace dynamic_cast with extern "C" provider query functions to avoid RTTI failures across dlopen boundaries. Add RAII to GatewayPluginLoadResult with correct destruction order (providers, plugin, dlclose) and move-only semantics. Add path validation (absolute, .so extension, canonical). Switch RTLD_LAZY to RTLD_NOW. Add GATEWAY_PLUGIN_EXPORT visibility macro. Replace non-portable libc.so.6 test with dedicated test plugins. Expand test coverage from 5 to 10 cases (version mismatch, null factory, missing factory symbol, path validation). Refs #235
Orchestrates plugin loading, configuration, route registration, and shutdown. Uses extern "C" provider pointers from PluginLoader for .so plugins (safe across dlopen boundary) and dynamic_cast for compile-time test plugins (safe within same binary). Error isolation: throwing plugins are disabled, not crashed. 10 test cases covering empty state, dispatch, configure, shutdown, multi-provider, single provider, duplicate UpdateProvider, throwing plugin, and load failure. Refs #235
Rename update_backend.hpp to update_types.hpp (types-only header). Remove the UpdateBackend abstract class - UpdateProvider replaces it. UpdateManager now takes non-owning UpdateProvider* via set_backend() instead of owning unique_ptr<UpdateBackend>. PluginManager owns the plugin lifecycle and dlopen handles. Old UpdatePluginLoader removed. test_update_backend.cpp migrated to new plugin framework exports. All 27 update manager tests pass with the new interface. Refs #235
Wire plugin loading, configuration, and lifecycle into the gateway. Plugins are declared via the 'plugins' parameter and loaded from per-plugin path config. UpdateProvider from plugins is automatically wired to UpdateManager when updates are enabled. Plugin routes are registered on the REST server after core routes.
- Fix integration test to use new plugin framework parameters (#235) - Update docs: remove stale updates.backend/plugin_path, add plugin framework section to server.rst, update changelog, rest.rst, FAQ - Add plugins section to gateway_params.yaml with documented examples - Add RTLD_LOCAL flag to dlopen for explicit symbol isolation - Use API_BASE_PATH constant instead of hardcoded "/api/v1" - Disable plugins on set_node()/register_routes() failure - Add logging to shutdown_all() catch block - Add doxygen to get_plugin_manager()
Documents plugin interface, configuration, extern "C" exports, lifecycle, custom REST endpoints, multi-plugin dispatch, error handling, and API versioning. Refs #235
Add shutdown idempotency guard preventing double-shutdown UB. Extract disable_plugin() helper to DRY lifecycle error handling and clear dangling provider pointers in GatewayPluginLoadResult. Re-check .so extension after symlink resolution. Validate plugin names from YAML config. Port thread safety docs from deleted UpdateBackend. Add 8 new tests covering set_node/register_routes error isolation, shutdown idempotency, move semantics, minimal plugin, and load_plugins success path. Add complete minimal plugin example to tutorial and fix misleading "Optional" labels on required provider query functions.
…ty registration Replace set_node(rclcpp::Node*) with set_context(PluginContext&) to give plugins access to entity cache, fault data, HTTP utilities, and capability registration. Plugins can now register custom capabilities per entity type or per specific entity, which appear in discovery responses with auto-generated hrefs.
Scan parameter overrides for plugins.<name>.* keys (excluding .path), convert ROS 2 parameter types to JSON, and pass the resulting object to each plugin's configure() method. Supports all ROS 2 parameter types.
- Cast to unsigned char before std::isalnum to prevent UB on signed char - Deduplicate plugin names to avoid ParameterAlreadyDeclaredException crash - Call shutdown() in disable_plugin() to honor lifecycle contract - Add try/catch around provider query functions in plugin_loader - Use extract_entity_type_from_path() for proper segment-boundary matching - Cache first UpdateProvider in load_plugins(), log warning once on duplicates - Fix test plugin return types from void* to GatewayPlugin* - Add temporal ordering doc to set_backend() - Fix stale set_node() reference in server.rst - Clarify RTTI scope in plugin-system.rst - Document .so extension intent in plugin_loader
e51e8d7 to
21c9663
Compare
Pull Request
Summary
Add a plugin framework for extending the gateway with custom functionality via shared libraries (
.sofiles). Plugins can provide software update backends, platform-specific introspection (preview), and custom REST endpoints.Key design decisions:
extern "C"provider query functions instead ofdynamic_castacross dlopen boundary (RTLD_LOCALmakes RTTI unreliable)GatewayPluginLoadResultwith correct destruction ordering (providers -> plugin -> dlclose)UpdateProvider*inUpdateManager(ownership centralized inPluginManager)What changed:
GatewayPluginbase class with lifecycle methods + logging APIUpdateProviderandIntrospectionProvidertyped provider interfacesPluginLoaderwith path validation, API version checking, andRTLD_NOW | RTLD_LOCALPluginManagerorchestrating loading, lifecycle, error isolation, and dispatchUpdateBackendreplaced byUpdateProvider(non-owning pointer, dependency inversion)GatewayNodeintegration with config-driven plugin loadingIssue
Type
Testing
Unit tests (28 new):
test_plugin_loader(14 tests): happy path, path validation, symbol validation, minimal plugin (no providers), move semantics (ctor + assignment),load_pluginssuccess pathtest_plugin_manager(14 tests): lifecycle, dispatch, multi-capability, error isolation for configure/set_node/register_routes, shutdown idempotency, shutdown exception swallowingTest fixtures (6
.soplugins):test_gateway_plugin- full-featured plugin (both providers + custom route)test_minimal_plugin- required exports only, no provider query functionstest_bad_version_plugin,test_no_symbols_plugin,test_null_factory_plugin,test_version_only_plugin- error path coverageIntegration tests:
test_updates.test.pyupdated to use newpluginsconfig array instead ofupdates.backend/updates.plugin_pathFull suite: 1623 tests, 0 failures
Checklist
Breaking Changes
updates.backendandupdates.plugin_pathparameters removed - usepluginsarray withplugins.<name>.pathentriesUpdateBackendclass replaced byUpdateProviderinterface (same method signatures, different base class)