Conversation
…_runner_audio, and capabilities2_runner_nav2 by fixing issues and adding new std::optinals functionality from action_runner
…searchLab/capabilities2 into capabilities2-server-fabric
…unchfile, config file and default xl file addition
…he same name(old capabilities_executor package) from capabilities_server package for clarity. removed a redundant ros2 action server as a result
…aths in config yaml
…ltiple interdependent ros2 service clients.fixed issue with dynamically linking tinyXML2 and solved with statically linking that
…in recovery and parallel routiens
…recovery-prallel Fix/fix control recovery prallel
Fix/restructuting
There was a problem hiding this comment.
Pull request overview
This PR refactors the Capabilities2 stack to support FSM-style execution with an expanded event/trigger system, updates the server launch/runtime model, and reorganizes documentation and ancillary packages.
Changes:
- Adds new event/trigger APIs across
capabilities2_server,capabilities2_runner, andcapabilities2_msgs. - Introduces a new
capabilities2_utilsheader-only utilities package and updates package dependencies. - Reworks docs/README structure and removes several older runner/proxy/executor packages and artifacts.
Reviewed changes
Copilot reviewed 109 out of 117 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| readme.md | Restructures top-level README and links to new/updated docs. |
| docs/setup_with_dev.md | Adds devcontainer-based setup guide. |
| docs/setup.md | Adds non-devcontainer setup guide. |
| docs/run_test_scripts.md | Updates test-script running instructions. |
| docs/foxglove_studio.md | Adds Foxglove Studio dependency/setup note. |
| docs/design.md | Moves design content into a dedicated doc. |
| capabilities2_utils/package.xml | Renames/repurposes utils package metadata. |
| capabilities2_utils/include/capabilities2_utils/event_types.hpp | Adds event type + options structures. |
| capabilities2_utils/include/capabilities2_utils/connection.hpp | Adds connection/node structs for event wiring. |
| capabilities2_utils/include/capabilities2_utils/bond_client.hpp | Adds a bond client helper. |
| capabilities2_utils/CMakeLists.txt | Adds install/export rules for utils headers. |
| capabilities2_server/src/capabilities_server_node.cpp | Adds standalone node entrypoint with multithreaded executor. |
| capabilities2_server/src/capabilities_server_component.cpp | Keeps component registration. |
| capabilities2_server/readme.md | Updates server docs and adds launch instructions/links. |
| capabilities2_server/package.xml | Adds new dependencies (utils, event logger, tinyxml2 vendor, etc.). |
| capabilities2_server/launch/capabilities2_server.launch.py | Switches default launch to standalone node executable. |
| capabilities2_server/launch/capabilities2_server.composed.launch.py | Adds composed/component launch option. |
| capabilities2_server/include/capabilities2_server/utils/sql_safe.hpp | Adds SQL escaping helpers used by models/DB. |
| capabilities2_server/include/capabilities2_server/runner_cache.hpp | Refactors runner cache API (triggering + event attachments). |
| capabilities2_server/include/capabilities2_server/models/semantic_interface.hpp | Applies SQL escaping to stored YAML. |
| capabilities2_server/include/capabilities2_server/models/readme.md | Adds model/trait documentation. |
| capabilities2_server/include/capabilities2_server/models/provider.hpp | Applies SQL escaping to stored YAML fields. |
| capabilities2_server/include/capabilities2_server/models/interface.hpp | Applies SQL escaping to stored YAML fields. |
| capabilities2_server/include/capabilities2_server/models/header.hpp | Applies SQL escaping to description. |
| capabilities2_server/include/capabilities2_server/capabilities_server.hpp | Major server refactor: new services (trigger/configure) + event logging. |
| capabilities2_server/include/capabilities2_server/capabilities_db.hpp | Applies unescaping when reading YAML/strings from SQLite. |
| capabilities2_server/include/capabilities2_server/capabilities_api.hpp | Refactors API to use event logger + adds triggers/trigger_capability. |
| capabilities2_server/include/capabilities2_server/bond_cache.hpp | Changes bond topic defaults and heartbeat settings. |
| capabilities2_server/docs/terminal_usage.md | Splits terminal usage instructions into dedicated doc. |
| capabilities2_server/docs/register.md | Splits capability registration instructions into dedicated doc. |
| capabilities2_server/config/capabilities.yaml | Adjusts defaults (notably rebuild + package_paths). |
| capabilities2_server/CMakeLists.txt | Splits component library vs standalone node executable build. |
| capabilities2_runner/src/standard_runners.cpp | Updates runner plugin exports + adapts DummyRunner to new API. |
| capabilities2_runner/package.xml | Adds new deps (utils/event logger/tinyxml2 vendor). |
| capabilities2_runner/include/capabilities2_runner/topic_runner.hpp | Adds a generic topic-based runner base. |
| capabilities2_runner/include/capabilities2_runner/service_runner.hpp | Adds a generic service-based runner base. |
| capabilities2_runner/include/capabilities2_runner/runner_base.hpp | Major runner API rewrite: trigger threading + event attachment + event logging. |
| capabilities2_runner/include/capabilities2_runner/notrigger_action_runner.hpp | Attempts to adapt “no trigger” runner wrapper to new runner API. |
| capabilities2_runner/include/capabilities2_runner/multiplex_base_runner.hpp | Adds a base for multiplexing-style runners. |
| capabilities2_runner/include/capabilities2_runner/launch_runner.hpp | Reimplements launch runner using services instead of action proxy. |
| capabilities2_runner/include/capabilities2_runner/encap_runner.hpp | Updates encapsulated runner scaffolding (API changes). |
| capabilities2_runner/include/capabilities2_runner/action_runner.hpp | Refactors action runner to new trigger/execution/event model. |
| capabilities2_runner/CMakeLists.txt | Updates runner build deps and TinyXML2 handling. |
| capabilities2_msgs/srv/TriggerCapability.srv | Adds trigger service definition. |
| capabilities2_msgs/srv/Launch.srv | Adds launch service definition. |
| capabilities2_msgs/srv/ConfigureCapability.srv | Adds configure service definition for event wiring. |
| capabilities2_msgs/readme.md | Updates msgs docs + notes new services. |
| capabilities2_msgs/package.xml | Bumps version + adds maintainer/author. |
| capabilities2_msgs/msg/CapabilityEventStamped.msg | Adds stamped event wrapper message. |
| capabilities2_msgs/msg/CapabilityEvent.msg | Redefines event message schema and enums. |
| capabilities2_msgs/msg/CapabilityConnection.msg | Simplifies connection message to source/target. |
| capabilities2_msgs/msg/Capability.msg | Adds parameters for trigger payloads. |
| capabilities2_msgs/action/Launch.action | Removes event feedback field from action. |
| capabilities2_msgs/CMakeLists.txt | Updates ROSIDL generation inputs and minimum CMake version. |
| capabilities2_capabilites/src/capabilities2_runner.cpp | Registers new runner plugins for “capabilites” package. |
| capabilities2_capabilites/providers/* | Adds provider specs for new runner plugins. |
| capabilities2_capabilites/interfaces/* | Adds interface specs for new runner plugins. |
| capabilities2_capabilites/plugins.xml | Declares pluginlib classes for new runner plugins. |
| capabilities2_capabilites/package.xml | Adds new package exporting capability specs. |
| capabilities2_capabilites/include/* | Adds implementations for get + multiplex runners. |
| capabilities2_capabilites/CMakeLists.txt | Builds and exports the new “capabilites” plugin library. |
| capabilities2_capabilites/.clang-format | Adds formatting config for the new package. |
| capabilities2/package.xml | Adds maintainer entry. |
| capabilities2/CMakeLists.txt | Updates minimum CMake version. |
| TODO.md | Updates TODO list to mark SQL escaping + events work as done and adds new TODOs. |
| AUTHORS | Formatting/alignment tweak. |
| .devcontainer/humble/devcontainer.json | Adds Humble devcontainer config. |
| .devcontainer/humble/Dockerfile | Adds Humble devcontainer Dockerfile. |
| .devcontainer/devcontainer.json | Updates Jazzy devcontainer name/mount path. |
| capabilities2_runner_nav2/* | Removes the nav2 runner package contents. |
| capabilities2_runner_bt/* | Removes the behavior-tree runner package contents. |
| capabilities2_runner_audio/* | Removes the audio runner package contents. |
| capabilities2_launch_proxy/* | Removes the Python launch proxy implementation. |
| capabilities2_executor/* | Removes the executor implementation and related files. |
| capabilities2_msgs/action/Plan.action | Removes plan action. |
| capabilities2_msgs/action/Connections.action | Removes connections action. |
| capabilities2_launch_proxy/test/call_use_launch_runner.py | Removes outdated test script. |
Comments suppressed due to low confidence (1)
capabilities2_server/include/capabilities2_server/capabilities_server.hpp:93
initialize()declares a localdouble loop_hz_which shadows theloop_hz_member. If the member is expected to be used later, it will remain uninitialized. Assign to the member (this->loop_hz_ = ...) or remove the unused member/local variable to avoid confusion.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tinyxml2::XMLElement* convert_to_xml(const std::string& parameters) | ||
| { | ||
| if (parameters != "") | ||
| { | ||
| doc.Parse(parameters.c_str()); | ||
| tinyxml2::XMLElement* element = doc.FirstChildElement(); | ||
| return element; | ||
| } | ||
| else | ||
| { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
convert_to_xml() parses into a single member tinyxml2::XMLDocument doc and returns pointers into it, but trigger() stores those raw XMLElement* pointers in parameters_ and starts threads. Any subsequent parse will invalidate earlier pointers (and concurrent triggers will race), causing use-after-free/dangling pointers. Use per-trigger/per-id document ownership (e.g., store the XML string, or store a shared_ptr<XMLDocument> per id) and make access thread-safe.
| #include <stdexcept> | ||
| #include <string> | ||
| #include <functional> | ||
| #include <optional> | ||
| #include <mutex> | ||
| #include <thread> | ||
| #include <tinyxml2.h> | ||
| #include <rclcpp/rclcpp.hpp> | ||
|
|
||
| #include <capabilities2_utils/event_types.hpp> | ||
|
|
||
| #include <event_logger_msgs/msg/event.hpp> | ||
| #include <event_logger_msgs/msg/event_capability.hpp> | ||
| #include <event_logger/event_client.hpp> | ||
|
|
There was a problem hiding this comment.
RunnerBase uses std::vector, std::map, and std::condition_variable, but the header doesn’t include <vector>, <map>, or <condition_variable>. Relying on transitive includes is brittle and can break builds depending on include order. Add the missing standard headers here.
| event_->info("[BondClient] creating bond to capabilities server"); | ||
|
|
||
| bond_ = std::make_unique<bond::Bond>(topic_, bond_id_, node_, std::bind(&BondClient::on_broken, this), std::bind(&BondClient::on_formed, this)); | ||
|
|
||
| bond_->setHeartbeatPeriod(0.10); | ||
| bond_->setHeartbeatTimeout(10.0); | ||
| bond_->start(); |
There was a problem hiding this comment.
BondClient::start() assigns std::make_unique<bond::Bond>(...) to bond_, but bond_ is declared as std::shared_ptr<bond::Bond>. This won’t compile. Use std::make_shared (or change the member type to std::unique_ptr).
| CapabilitiesServer(const rclcpp::NodeOptions& options = rclcpp::NodeOptions()) | ||
| : Node("capabilities", options), CapabilitiesAPI() | ||
| : Node("capabilities2", options), CapabilitiesAPI() | ||
| { | ||
| try | ||
| { | ||
| // Only call setup if this object is already owned by a shared_ptr | ||
| if (shared_from_this()) | ||
| { | ||
| initialize(); | ||
| } | ||
| } | ||
| catch (const std::bad_weak_ptr&) | ||
| { | ||
| // Not yet safe — probably standalone without make_shared | ||
| } | ||
| } |
There was a problem hiding this comment.
This constructor tries to call shared_from_this() and initialize() during construction. shared_from_this() is not safe in constructors and will always throw std::bad_weak_ptr, so component instantiation (via RCLCPP_COMPONENTS_REGISTER_NODE) will never call initialize(), leaving the node uninitialized. Please move initialization to a place guaranteed to run for both component + standalone node (e.g., do not require shared_from_this(), or provide a post-construction init hook in the component entrypoint).
| // reset the runner pointer | ||
| runner_cache_[capability].reset(); | ||
| // runner_cache_[capability].reset(); | ||
|
|
||
| // remove the runner from map | ||
| runner_cache_.erase(capability); | ||
| // runner_cache_.erase(capability); | ||
| } |
There was a problem hiding this comment.
remove_runner() no longer removes the runner from runner_cache_ (both reset() and erase() are commented out). This means running(capability) stays true and the capability can never be restarted, and the cache grows unbounded. Please restore the erase/reset (or otherwise implement a correct removal strategy).
| class NoTriggerActionRunner : public ActionRunner<ActionT> | ||
| { | ||
| public: | ||
| // throw on trigger function | ||
| std::optional<std::function<void(std::shared_ptr<tinyxml2::XMLElement>)>> | ||
| trigger(std::shared_ptr<tinyxml2::XMLElement> parameters) override | ||
| void trigger(tinyxml2::XMLElement* parameters) override | ||
| { | ||
| throw runner_exception("cannot trigger this is a no-trigger action runner"); | ||
| } | ||
|
|
||
| protected: | ||
| // throw on xml conversion functions | ||
| typename ActionT::Goal generate_goal(std::shared_ptr<tinyxml2::XMLElement>) override | ||
|
|
||
| // throw on triggerExecution function | ||
| void execution() override | ||
| { | ||
| throw runner_exception("cannot generate goal this is a no-trigger action runner"); | ||
| throw runner_exception("no triggerExecution() this is a no-trigger action runner"); | ||
| } | ||
|
|
||
| std::shared_ptr<tinyxml2::XMLElement> generate_result(const typename ActionT::Result::SharedPtr& result) override | ||
| // throw on xml conversion functions | ||
| typename ActionT::Goal generate_goal(tinyxml2::XMLElement*) override | ||
| { | ||
| throw runner_exception("cannot generate result this is a no-trigger action runner"); | ||
| throw runner_exception("cannot generate goal this is a no-trigger action runner"); |
There was a problem hiding this comment.
NoTriggerActionRunner does not compile as written: methods are marked override but don’t match the base class signatures (RunnerBase::trigger(const std::string&) and RunnerBase::execution(int), plus ActionRunner::generate_goal(XMLElement*, int)). This will produce immediate compile errors. Update the overrides to match the new RunnerBase/ActionRunner API (or remove the override specifiers).
| std::string to_sql_safe(const std::string& input) | ||
| { | ||
| std::string result = input; | ||
|
|
||
| // Escape backslashes (if necessary for your SQL dialect) | ||
| result = std::regex_replace(result, std::regex(R"(\\)"), R"(\\\\)"); | ||
|
|
||
| // Escape single quotes by replacing them with two single quotes | ||
| result = std::regex_replace(result, std::regex(R"(')"), R"('')"); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * @brief convert a sql safe std::string to a generic std::string | ||
| * | ||
| * @param input sql safe std::string | ||
| * @return generic std::string | ||
| * | ||
| */ | ||
| std::string from_sql_safe(const std::string& input) | ||
| { | ||
| std::string result = input; | ||
|
|
||
| // Escape backslashes (if necessary for your SQL dialect) | ||
| result = std::regex_replace(result, std::regex(R"(\\\\)"), R"(\\)"); | ||
|
|
||
| // Escape single quotes by replacing them with two single quotes | ||
| result = std::regex_replace(result, std::regex(R"('')"), R"(')"); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
to_sql_safe/from_sql_safe are defined in a header without inline (or static). Because this header is included from multiple translation units, this will cause multiple-definition linker errors. Mark these functions inline (or move the definitions to a .cpp and keep only declarations in the header).
| void set_runner_triggers(const std::string& capability, capabilities2::event_opts& event_options) | ||
| { | ||
| runner_cache_[capability]->attach_events(event_options, | ||
| std::bind(&capabilities2_server::RunnerCache::trigger_runner, this, | ||
| std::placeholders::_1, std::placeholders::_2)); | ||
| } |
There was a problem hiding this comment.
set_runner_triggers() uses runner_cache_[capability] without checking running(capability). If the capability is missing, operator[] inserts a null entry and the subsequent ->attach_events(...) will crash. Add an existence check (and avoid operator[] here) before dereferencing.
| virtual void trigger(const std::string& parameters) | ||
| { | ||
| // extract the unique id for the runner and use that as the thread id | ||
| tinyxml2::XMLElement * element = nullptr; | ||
| element = convert_to_xml(parameters); | ||
| element->QueryIntAttribute("id", &runner_id); | ||
|
|
||
| parameters_[runner_id] = element; | ||
|
|
||
| info_("received new parameters with event id : " + std::to_string(runner_id), runner_id); | ||
|
|
||
| executionThreadPool[runner_id] = std::thread(&RunnerBase::execution, this, runner_id); | ||
|
|
||
| info_("started execution", runner_id); | ||
| } |
There was a problem hiding this comment.
RunnerBase::trigger() assumes convert_to_xml(parameters) returns a non-null element and that an id attribute exists. If the service passes an empty/invalid XML string (or missing id), this will dereference null and crash. Add validation for parse success + element existence, and handle missing/invalid id gracefully.
2 participants
Pull Request Template
Description
Modifications to support functionality as an FSM. Added event functionality.
Fixes #[issue]
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide details of your testing environment, and the tests you ran to see how your change affects other areas of the code.
Checklist