Lyrical: try new executor - #2249
Merged
Merged
Conversation
Three CI improvements: 1. Pin ros-industrial/industrial_ci from @master to @v0.10.0 (the latest release). @master is mutable; a breaking upstream change can break CI overnight. Bumps can be handled by dependabot (already watches github-actions ecosystem). 2. Add ccache to the Coverage workflow. Coverage builds with Debug + --coverage are slow (~5-9 min for mavros); ccache keyed separately from the CI cache (ccache-cov- prefix, per-component) so the two build types don't thrash each other. Install ccache via apt, set CCACHE_DIR, add a cache step, and pass -DCMAKE_{C,CXX}_COMPILER_LAUNCHER=ccache to colcon. 3. Add paths-ignore to both CI and Coverage triggers so that docs-only, README, LICENSE, and dependabot.yml changes no longer trigger a full multi-distro build + coverage run. This eliminates the churn from dependabot pip bumps in docs/ (recently produced several cancelled CI runs). Note: TEST_ENABLE_E2E and PIP_BREAK_SYSTEM_PACKAGES in coverage.yml are intentionally kept — they gate the conditional <test_depend condition="$TEST_ENABLE_E2E == 1">python3-pymavlink-pip</test_depend> in libmavconn/package.xml, which rosdep evaluates during the coverage dependency install step. Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Introduce make_executor() factory honoring the MAVROS_EXECUTOR_TYPE env var (default 'mt', 'events'/'cbg' selects rclcpp's new Callback Group Events executor on Lyrical+). The UASExecutor wrapper and mavros_node container now build their executor via the factory, so the new EventsCBGExecutor can be tried without a rebuild. EventsCBGExecutor only exists since Lyrical (rclcpp >= 30.0.0); on older distros the events request warns and falls back to MultiThreadedExecutor. Also drop the now-dead UASExecutor::set_ids()/run() overrides (set_ids had no callers; run() is non-virtual in Lyrical's MultiThreadedExecutor). Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Two fixes so mavros::router::Router and mavros::uas::UAS work cleanly as composable nodes in a component container: 1. Break the plugin <-> UAS ownership cycle. Plugin held a shared_ptr to the UAS node while the UAS node owned the plugins, so on component unload the nodes were never destroyed (they lingered and got duplicated on reload). Plugin now stores a non-owning UAS*; the UAS still owns the plugins. Verified: unload now removes UAS + all plugin nodes, reload does not duplicate them. 2. Dynamically created plugin nodes no longer inherit process-global __node/__ns remap rules. A component container remaps itself with e.g. '-r __node:=<container>', which renamed every plugin node to the container name and caused topic type conflicts. Plugin nodes are now created with use_global_arguments(false), keeping their subnode name and the UAS namespace. Also move the Plugin constructors to plugin.cpp (UAS is an incomplete type at the point plugin.hpp is included) and update the composable test launch: fcu_url/gcs_url as launch arguments (no env-specific URL hardcoded) and the container selected by distro (component_container --executor-type events-cbg on Lyrical+, component_container_mt otherwise). Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
Enable use_intra_process_comms(true) for the mavros_node container and the dynamically created plugin nodes, so the internal MAVLink bus and co-located subscribers go zero-copy (cross-process consumers fall back to DDS, so this is safe either way). test_compose.launch.py: - fcu_url/gcs_url as launch arguments instead of hardcoded URLs - add an 'executor' argument (mt by default, events/auto opt-in). The Callback Group Events executor's component-container support is still immature upstream (ros2/rclcpp#3186: shutdown double-remove; ros2/rclcpp#3123: container support), so default to the reliable component_container_mt and only use component_container --executor-type events-cbg when asked - resolve URLs in an OpaqueFunction so list parameters keep their type - tone down container log level from DEBUG to INFO Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
… launch Clarify the plugin<->UAS ownership comment (UAS owns the plugins, so the plugins must not own the UAS). Document MAVROS_EXECUTOR_TYPE and MAVROS_UAS_EXECUTOR_THREADS env vars and the test_compose.launch.py arguments in mavros/README.md. Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
The Scan (semgrep) check fails on github-actions-mutable-action-tag for actions/cache@v6 and friends. GitHub's own actions are trusted and keep their v-tags stable, so annotate them with # nosemgrep (consistent with the existing industrial_ci@master annotation). Pin the third-party ros-tooling/setup-ros@v0.7 to its commit SHA. Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
UAS owns an executor thread, TF listeners, subscriptions and plugin nodes, so copying it (still implicitly generated because of the user-declared destructor) would be a bug. Delete copy and move (as in PR #1961). Nothing in the codebase copies or moves a UAS. Signed-off-by: Vladimir Ermakov <vooon341@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Try the new Callback Group Events executor (
EventsCBGExecutor) introduced in ROS 2 Lyrical Luth (rclcpp >= 30.0.0), and fix the resource/ownership problems that preventedmavros::router::Routerandmavros::uas::UASfrom working cleanly as composable nodes in a component container.Replaces / fixes
shared_ptrto UAS) meant the nodes were never destroyed on component unload and got duplicated on reload. This PR breaks the cycle with a raw, non-owningUAS*in plugins (as in On the way to fixing #1919 #1920's approach, but withoutweak_ptr).use_intra_process_comms(true)for the internal MAVLink bus and co-located subscribers (zero-copy when router/UAS/plugins share a process; DDS fallback otherwise).sys_time) — considered but not adopted: theWallRate→std::chronochange there is unlikely to address the CPU usage issue.Changes
mavros::uas::make_executor()honoring theMAVROS_EXECUTOR_TYPEenv var (mtdefault,events). Used by bothmavros_nodeand the UAS plugin executor;EventsCBGExecutoris gated to Lyrical+ (rclcpp >= 30.0.0) with aMultiThreadedExecutorfallback on older distros. Thread count remains configurable viaMAVROS_UAS_EXECUTOR_THREADS.use_global_arguments(false)so a component container's process-global-r __node:=<container>remaps no longer rename every plugin node (previously caused topic type conflicts).use_intra_process_comms(true)onmavros_nodeoptions and plugin nodes.test_compose.launch.py:fcu_url/gcs_urlas launch arguments (no env-specific URL hardcoded), anexecutorargument (mtdefault /events/auto), typed parameter resolution viaOpaqueFunction, INFO log level.mavros/README.mddocumentsMAVROS_EXECUTOR_TYPE,MAVROS_UAS_EXECUTOR_THREADS, andtest_compose.launch.pyusage.Scancheck (github-actions-mutable-action-tag):# nosemgrepon GitHub first-party actions, SHA-pinros-tooling/setup-ros.Verification
ctest13/13 green on Lyrical (uncrustify, flake8, pep257, xmllint, cppcheck, cpplint, gmock/gtest, pytest).test_compose.launch.pyloads Router + UAS as composable nodes;ros2 component unloadremoves all nodes; reload shows no duplication.MAVROS_EXECUTOR_TYPE=eventsruns standalonemavros_node(andexecutor:=eventsthe composable container) with clean shutdown.Caveats
EventsCBGExecutorin a component container is upstream-immature on Lyrical (EventsCBGExecutor shutdown + ComponentManager destructor cause double remove and runtime_error on shutdown ros2/rclcpp#3186 shutdown double-remove, #3123 container support, #3129 container refactor):load_nodecan intermittently time out andlist_nodesisn't graph-discoverable. The composable launch therefore defaults to the reliablecomponent_container_mt;executor:=eventsis opt-in. Standalonemavros_nodewith the events executor works well.MAVROS_EXECUTOR_TYPEdefaults tomt, so existing behavior is unchanged.mavros-uas-testwas stale fastdds/dev/shmstate (gone after reboot, 0/30 runs; also happens on baseline) — not a regression from this PR.