Conversation
…ntegration bugs, wip
…ndpoint, implement event handling
…gration, remove uuid dependency replace with rclcpp, simpler logging interface, update trigger service function
… thread model - WIP
…in cmake to support uuid gen in higher libraries
…and updated the api to match new event subsystem
New event subsystem and parameter addition along with compile error testing
Refactor/tinyxml/v1
…capability to connect to itself
… between different connections originating from the same source capability
There was a problem hiding this comment.
Pull request overview
Refactors the Capabilities2 event system (v1) by introducing a dedicated capabilities2_events package, migrating runner triggering from XML strings to typed parameters, and updating server/runner APIs to support bonding + instancing.
Changes:
- Add
capabilities2_events(event emitter/node model, UUID generator, published event implementation) and wire it intocapabilities2_server. - Update runner plugin API to be bond/instance-aware and migrate triggering to typed
EventParameters/CapabilityParameter. - Update messages/services for triggering + connecting capabilities (
TriggerCapabilitychanges, newConnectCapability) and bump package versions to0.2.0.
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| changelog.md | Adds 0.2.0 release notes. |
| capabilities2_utils/package.xml | Removes deprecated capabilities2_utils package metadata. |
| capabilities2_utils/include/capabilities2_utils/event_types.hpp | Removes legacy event type definitions. |
| capabilities2_utils/include/capabilities2_utils/connection.hpp | Removes legacy connection model (tinyxml2-based). |
| capabilities2_utils/include/capabilities2_utils/bond_client.hpp | Removes legacy bond client wrapper. |
| capabilities2_utils/CMakeLists.txt | Removes deprecated package build config. |
| capabilities2_server/test/test.cpp | Fixes semantic interface API typo in test. |
| capabilities2_server/readme.md | Updates server API docs (bond requirements, ConnectCapability, topics/messages). |
| capabilities2_server/package.xml | Bumps version and updates dependencies to include capabilities2_events. |
| capabilities2_server/launch/capabilities2_server.launch.py | Formatting/refactor of config path building. |
| capabilities2_server/launch/capabilities2_server.composed.launch.py | Formatting/refactor of config path building. |
| capabilities2_server/include/capabilities2_server/runner_cache.hpp | Adapts runner cache to new runner API + event connections. |
| capabilities2_server/include/capabilities2_server/models/running.hpp | Refactors running model to inherit capability identity. |
| capabilities2_server/include/capabilities2_server/models/readme.md | Notes new v3 model proposals (running/connections). |
| capabilities2_server/include/capabilities2_server/models/provider.hpp | Updates provider model inheritance (and minor header changes). |
| capabilities2_server/include/capabilities2_server/capabilities_server.hpp | Integrates new event publisher + ConnectCapability service; updates validation/logging. |
| capabilities2_server/include/capabilities2_server/capabilities_api.hpp | Refactors API for new events + bond-scoped triggering + connections; swaps UUID generation. |
| capabilities2_server/include/capabilities2_server/bond_cache.hpp | Extends bond cache with bond existence checks and doc improvements. |
| capabilities2_server/CMakeLists.txt | Bumps CMake min version and swaps deps to capabilities2_events (drops uuid/backward_ros wiring). |
| capabilities2_runner/src/system/system_runners.cpp | Registers new system runner plugins. |
| capabilities2_runner/src/standard_runners.cpp | Removes prior combined runner registration TU. |
| capabilities2_runner/src/launch_runner.cpp | Adds dedicated LaunchRunner plugin registration TU. |
| capabilities2_runner/src/dummy_runner.cpp | Adds DummyRunner updated to new runner/event API. |
| capabilities2_runner/readme.md | Updates runner docs structure + adds system runner section; retains API snippets. |
| capabilities2_runner/providers/InputMultiplexRunner.yaml | Adds provider spec for InputMultiplexRunner. |
| capabilities2_runner/providers/GetCapabilitySpecsRunner.yaml | Adds provider spec for GetCapabilitySpecsRunner. |
| capabilities2_runner/plugins.xml | Registers new runner plugins in pluginlib manifest. |
| capabilities2_runner/package.xml | Bumps version and exports capability interface/provider YAMLs; adds events dependency. |
| capabilities2_runner/interfaces/InputMultiplexRunner.yaml | Adds interface spec for InputMultiplexRunner. |
| capabilities2_runner/interfaces/GetCapabilitySpecsRunner.yaml | Adds interface spec for GetCapabilitySpecsRunner. |
| capabilities2_runner/include/capabilities2_runner/topic_runner.hpp | Migrates TopicRunner to ThreadTriggerRunner + event emission. |
| capabilities2_runner/include/capabilities2_runner/threadtrigger_runner.hpp | Introduces threaded trigger execution base for runners. |
| capabilities2_runner/include/capabilities2_runner/system/input_multiplex_runner.hpp | Adds system runner for multiplexing trigger inputs. |
| capabilities2_runner/include/capabilities2_runner/system/get_capability_specs_runner.hpp | Adds ServiceRunner-based system runner to query server specs. |
| capabilities2_runner/include/capabilities2_runner/service_runner.hpp | Migrates ServiceRunner to new trigger/thread/event API. |
| capabilities2_runner/include/capabilities2_runner/runner_base.hpp | Refactors RunnerBase to inherit EventNode and use typed EventParameters + bond/instance scoping. |
| capabilities2_runner/include/capabilities2_runner/notrigger_runner.hpp | Adds NoTriggerRunner base for runners that must not be triggered. |
| capabilities2_runner/include/capabilities2_runner/launch_runner.hpp | Reworks LaunchRunner to a no-trigger runner (currently unimplemented). |
| capabilities2_runner/include/capabilities2_runner/encap_runner.hpp | Updates EnCapRunner stop signature + event emission. |
| capabilities2_runner/include/capabilities2_runner/action_runner.hpp | Migrates ActionRunner to ThreadTriggerRunner + EventParameters. |
| capabilities2_runner/docs/parameter_format.md | Adds (placeholder) parameter format documentation. |
| capabilities2_runner/CMakeLists.txt | Updates build inputs + dependencies; installs interface/provider specs. |
| capabilities2_msgs/srv/TriggerCapability.srv | Makes trigger requests bond-scoped and typed (Capability message). |
| capabilities2_msgs/srv/ConnectCapability.srv | Adds service to connect capabilities via connections. |
| capabilities2_msgs/srv/ConfigureCapability.srv | Removes legacy configure service. |
| capabilities2_msgs/readme.md | Updates message/service list and “New in …” notes. |
| capabilities2_msgs/msg/CapabilityParameter.msg | Adds typed parameter message for trigger/event parameters. |
| capabilities2_msgs/msg/CapabilityEventCode.msg | Adds dedicated event code message. |
| capabilities2_msgs/msg/CapabilityEvent.msg | Refactors event payload to use connection + event code + trigger id. |
| capabilities2_msgs/msg/CapabilityConnection.msg | Adds connection type (event code) to the connection message. |
| capabilities2_msgs/msg/Capability.msg | Adds instance_id and typed parameters array. |
| capabilities2_msgs/CMakeLists.txt | Registers new msgs/srvs and updates ordering. |
| capabilities2_events/src/uuid_generator.cpp | Implements UUID string generator. |
| capabilities2_events/readme.md | Adds documentation for the new event subsystem. |
| capabilities2_events/package.xml | Adds new package manifest for event subsystem. |
| capabilities2_events/include/capabilities2_events/uuid_generator.hpp | Declares UUID generator API. |
| capabilities2_events/include/capabilities2_events/published_event.hpp | Implements publisher-backed event emitter. |
| capabilities2_events/include/capabilities2_events/event_parameters.hpp | Implements typed parameter container with conversions. |
| capabilities2_events/include/capabilities2_events/event_node.hpp | Implements connection storage + event emission routing. |
| capabilities2_events/include/capabilities2_events/event_base.hpp | Defines event emitter interface and callback typing. |
| capabilities2_events/CMakeLists.txt | Adds build config for new events library and uuid linkage. |
| capabilities2_events/.clang-format | Adds formatting config for the new package. |
| capabilities2/package.xml | Bumps metapackage version and adds events package dependency. |
| TODO.md | Updates TODO/refactor notes (thread safety, trigger prototype, versions). |
| .devcontainer/devcontainer.json | Updates devcontainer workspace mount and run args. |
| .devcontainer/Dockerfile | Sets workspace workdir and clones external test suite repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [this, &instance_id, &completed, &cv, | ||
| &bond_id, &instance_id](const typename rclcpp_action::ClientGoalHandle<ActionT>::WrappedResult& wrapped_result) { |
There was a problem hiding this comment.
The result_callback lambda capture list captures instance_id twice (&instance_id appears two times). This is a compile error in C++ and will prevent the runner library from building. Remove the duplicate capture and ensure only the needed variables are captured.
| [this, &instance_id, &completed, &cv, | |
| &bond_id, &instance_id](const typename rclcpp_action::ClientGoalHandle<ActionT>::WrappedResult& wrapped_result) { | |
| [this, &instance_id, &completed, &cv, &bond_id]( | |
| const typename rclcpp_action::ClientGoalHandle<ActionT>::WrappedResult& wrapped_result) { |
| if (exec_thread.joinable()) | ||
| { | ||
| // TODO: FIX THIS FUTURE MICHAEL | ||
| exec_thread.join(); | ||
| } |
There was a problem hiding this comment.
stop_execution() unconditionally calls join() on all threads (and the timeout parameter is effectively unused), so shutdown/destruction can block indefinitely if any execution() never returns. Consider cooperative cancellation (e.g., std::jthread + stop_token) and a bounded shutdown strategy, or remove the timeout argument and document the blocking behavior.
| # TODO | ||
|
|
||
| Runners can use parameters. These parameters are passed to the runner in the `trigger` function. The parameter format for capabilities2 uses XML. A runner expects to receive a minimal XML structure which includes an ID attribute, for example: | ||
|
|
||
| ```xml | ||
| <parameters id="1234"> | ||
| ``` | ||
|
|
||
| The `id` is used to identify the runner instance and manage its execution. |
There was a problem hiding this comment.
This doc still states trigger parameters use XML (with an id attribute), but the PR switches to typed parameters (CapabilityParameter[] / EventParameters) and uses bond_id + instance_id for scoping. Update the document so it matches the new parameter representation and how runners should read values.
| # TODO | |
| Runners can use parameters. These parameters are passed to the runner in the `trigger` function. The parameter format for capabilities2 uses XML. A runner expects to receive a minimal XML structure which includes an ID attribute, for example: | |
| ```xml | |
| <parameters id="1234"> | |
| ``` | |
| The `id` is used to identify the runner instance and manage its execution. | |
| Runners can use parameters. These parameters are passed to the runner in the `trigger` function as **typed parameters**, not XML. | |
| In capabilities2, trigger parameters are represented as a collection of typed values (for example, `CapabilityParameter[]` on capabilities and `EventParameters` on events). Each parameter has a name/key and a strongly typed value (string, number, boolean, etc.), so runners should read values directly from these structures rather than parsing XML. | |
| Each trigger invocation is scoped by two identifiers: | |
| - `bond_id`: Identifies the bond under which the runner is executing. | |
| - `instance_id`: Identifies the specific runner instance within that bond. | |
| Runners should use the combination of `bond_id` and `instance_id` to associate incoming parameters and state with the correct runner instance. When handling a trigger, read the parameter values from the provided typed parameter collection using the parameter names defined in the capability or event schema, and rely on `bond_id` + `instance_id` for scoping instead of an XML `id` attribute. |
| BOOL, | ||
| DOUBLE, | ||
| INT, | ||
| STRING, | ||
| UNCONVERTED, | ||
| VECTOR_BOOL, | ||
| VECTOR_DOUBLE, | ||
| VECTOR_INT, | ||
| VECTOR_STRING |
There was a problem hiding this comment.
OptionType enum values don’t match the wire values in capabilities2_msgs/msg/CapabilityParameter.msg (that message defines VECTOR_BOOL = 4, but this enum inserts UNCONVERTED at 4). As a result, static_cast<OptionType>(msg.type) will misinterpret vector types (and likely break parameter decoding). Align the enum numeric values with the message constants (or introduce an explicit mapping function) and consider removing/renaming UNCONVERTED if it’s not part of the on-wire schema.
| BOOL, | |
| DOUBLE, | |
| INT, | |
| STRING, | |
| UNCONVERTED, | |
| VECTOR_BOOL, | |
| VECTOR_DOUBLE, | |
| VECTOR_INT, | |
| VECTOR_STRING | |
| // These values must match capabilities2_msgs/msg/CapabilityParameter.msg | |
| BOOL = 0, | |
| DOUBLE = 1, | |
| INT = 2, | |
| STRING = 3, | |
| VECTOR_BOOL = 4, | |
| VECTOR_DOUBLE = 5, | |
| VECTOR_INT = 6, | |
| VECTOR_STRING = 7, | |
| // Internal use only; not part of the on-wire schema | |
| UNCONVERTED = 100 |
| completed = true; | ||
| cv.notify_all(); | ||
| }); | ||
|
|
||
| // trigger the events related to on_started state | ||
| if (events[id].on_started.interface != "") | ||
| { | ||
| event_(EventType::STARTED, id, events[id].on_started.interface, events[id].on_started.provider); | ||
| triggerFunction_(events[id].on_started.interface, update_on_started(events[id].on_started.parameters)); | ||
| } | ||
|
|
||
| // Conditional wait |
There was a problem hiding this comment.
completed is set and cv.notify_all() is called from the async response callback without holding the same mutex used by cv.wait(...). This introduces a data race on completed and undefined behavior. Protect completed with the mutex (lock in the callback before writing + notify), or use an atomic/shared state object for cross-thread signaling.
| ### LaunchRunner | ||
|
|
||
| The `Launch Runner` inherits from the `capabilities2_runner::NoTriggerActionRunner` and is a special case. To instatiate this runner, provide a launch file path as the `runner` tag in the capability provider. | ||
| The `Launch Runner` inherits from the `capabilities2_runner::NoTriggerActionRunner` and is a special case. To instantiate this runner, provide a launch file path as the `runner` tag in the capability provider. | ||
|
|
There was a problem hiding this comment.
This section still references the old runner API (stop() with no args, trigger() using tinyxml2) and says LaunchRunner inherits from NoTriggerActionRunner, but the code now uses stop(bond_id, instance_id) and EventParameters, and LaunchRunner inherits from NoTriggerRunner. Please update the README snippet/description so it matches the current RunnerBase API and class hierarchy.
| // generate a reequest from launch_name and package_name | ||
| auto request_msg = std::make_shared<Launch::Request>(); | ||
| // stop the launch process | ||
| throw runner_exception("launch runner not implemented yet"); |
There was a problem hiding this comment.
LaunchRunner::stop() currently throws "launch runner not implemented yet"; the subsequent emit_stopped(...) is unreachable. This makes cleanup/shutdown of launch-file capabilities impossible and may leave processes running. Implement stop/termination semantics (or disable this runner until implemented).
| throw runner_exception("launch runner not implemented yet"); | |
| // NOTE: concrete implementations should ensure any launched processes are | |
| // properly terminated before or when calling this base implementation. |
| #include <capabilities2_server/models/remappable_base.hpp> | ||
| #include <capabilities2_server/models/defineable_base.hpp> | ||
| #include <capabilities2_server/models/predicateable_base.hpp> | ||
| #include <capabilities2_server/models/defineable_base.hpp> | ||
| #include <capabilities2_server/utils/sql_safe.hpp> |
There was a problem hiding this comment.
Duplicate include: defineable_base.hpp is included twice. This is harmless but adds noise and can slow builds; remove the redundant include.
| capabilities2_msgs::msg::CapabilityEventStamped event_msg; | ||
| event_msg.header.stamp = rclcpp::Clock().now(); | ||
| event_msg.event.trigger_id = instance_id; | ||
| event_msg.event.code.code = event_code; |
There was a problem hiding this comment.
rclcpp::Clock().now() defaults to system time; if the rest of the system expects ROS time (use_sim_time), event timestamps may be inconsistent. Consider injecting a node clock / time source (e.g., using node->get_clock()->now() or a Clock configured for ROS time) so published events follow the node’s time semantics.
| result_ = wrapped_result.result; | ||
| completed_ = true; | ||
| cv_.notify_all(); | ||
| completed = true; | ||
| cv.notify_all(); | ||
| }; |
There was a problem hiding this comment.
Similar to ServiceRunner: completed is set and cv.notify_all() is called from the result callback without holding block_mutex, while cv.wait(lock, ...) reads completed under that lock. This is a data race/UB. Lock the same mutex in the callback before writing completed, or use an atomic/shared state designed for cross-thread signaling.
2 participants
Pull Request Template
Description
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested using examples from fabric
Checklist