Fix: Select hangar_sim nav2 params by ROS distro so Jazzy navigation starts - #863
Fix: Select hangar_sim nav2 params by ROS distro so Jazzy navigation starts#863dv-picknik wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
724ce97 to
335086b
Compare
|
335086b to
c4ce908
Compare
|
|
Nav2's pluginlib lookup names are mutually exclusive between Humble (pkg/Class) and Jazzy (pkg::Class), and Jazzy's bt_navigator loads its default BT plugin libraries implicitly. Ship a Jazzy parameter file alongside the Humble one and pick between them at launch, so hangar_sim navigation starts on both distros the 9.4 line publishes. Closes #21729 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c4ce908 to
d54824e
Compare
|
[written by AI] Review gateAgents run sequentially on this branch, findings applied between each. Final tree is
Findings worth surfacing rather than burying:
Deferred with cause: fail-closed |
|
[written by AI]
needs: moveit_pro/#21807
Problem
hangar_sim navigation cannot start on Jazzy on the
v9.4line. Two pre-Jazzy patterns innav2_params.yaml::370,:392-400). Jazzy's nav2 exports these plugins with nonameattribute, so the lookup name is thepkg::Classtype.planner_serveraborts bringup withclass nav2_navfn_planner/NavfnPlanner ... does not exist, beforebt_navigatoris reached.plugin_lib_nameslist (:56-104) duplicating what Jazzy'sbt_navigatorloads implicitly. The parameter is additive since Jazzy, so it double-registers and configure fails withID [ComputePathToPose] already registered, killingnav2_container.And one more, found during review — outside those files but squarely in "navigation doesn't work on Jazzy":
objectives/navigate_to_clicked_point_with_replanning.xmlhardcoded/opt/ros/humble/share/nav2_bt_navigator/.../navigate_to_pose_w_replanning_and_recovery.xml./opt/ros/humbledoes not exist on the Jazzy image at all, and this is the only hangar_sim objective that drivesNavigateToPoseAction. Without this, nav2 would come up and the one navigation objective would still fail.#809fixed the first two onmainand explicitly says "Do not backport" — correctly, since the slash form is right on Humble.mainis Jazzy-only (ARG ROS_DISTRO=jazzy); 9.4 publishes bothv9.4-humble-amd64andv9.4-jazzy-amd64. Upstream confirms the forms are mutually exclusive: Humble'sglobal_planner_plugin.xmldeclaresname="nav2_navfn_planner/NavfnPlanner"; Jazzy's declares noname. A straight cherry-pick fixes Jazzy and silently regresses Humble — and this repo's v9.4 CI is jazzy-only (ACTIVE_DISTRO = 'jazzy'), so nothing would catch it.Approach
One parameter file per distro, chosen at launch:
params/nav2_params.yaml— byte-identical tov9.4, still serving Humble.params/nav2_params_jazzy.yaml— generated from it, differing only where Jazzy requires it: the 6 plugin names in::form, noplugin_lib_names, and 3 parameters Jazzy renamed —progress_checker_plugins(Jazzy takes a list), andbehavior_server'slocal_costmap_topic/local_footprint_topic(Jazzy split these intolocal_/global_pairs).robot_drivers_to_persist_sim.launch.pypicks by$ROS_DISTRO, defaults to the Humble file, and logs the resolved path — the wrong branch reproduces the very bug this avoids, and its only symptom would be a component container aborting. It logs theparams_fileactually in effect, so an override cannot make the line lie.behavior_tree_pathbecomes"", so nav2 falls back todefault_nav_to_pose_bt_xmland no distro string appears in the path. A comment records that this resolves to each distro's own default tree, which are not identical (Jazzy backs up faster and gates recovery on error codes).Those 3 renamed parameters matter even though they are inert today. Their Humble spellings are undeclared on Jazzy, so they are silently ignored rather than rejected, and the values only coincide with Jazzy's defaults — which is exactly what would hide a divergence the moment anyone renames the progress-checker id or namespaces the costmaps.
costmap_topicalso feeds the collision check guardingSpin/BackUp. Verified on a running stack: before the renameprogress_checker_pluginandcostmap_topicread backParameter not seton Jazzy while nav2's own spellings carried the defaults; after, they read back with the configured values. On Humble the singular/unprefixed spellings are genuinely declared, which is why the split is per-distro rather than a blanket rename.The selection lives in one place: that launch file is the only one resolving the params path against
hangar_sim's share directory, and it passesparams_filetoslam_launch.py,localization_launch.py,navigation_launch.py, and thenav2_containernode. The other launch files' own defaults point atnav2_bringup's stock params and are dead here, so they are untouched.A build-time alternative was weighed — branch in
CMakeLists.txton$ENV{ROS_DISTRO}and install one file under a single name. That removes the runtime env dependency entirely (the image that builds is the image that runs), but it makes the installed share tree differ from the source tree, which is worse to debug on a frozen release branch. The runtime read is reversible and visible in the log; the build-time trick is neither.Two parallel files is the cost, so
test/nav2_params_distro_parity_test.pypins the complete set of allowed differences — the 6 renames, plus the Humble-only and Jazzy-only key sets — and that both filenames the launch file names exist. Any other divergence fails. It rejects thepkg/Classform generally, not just today's six. Verified it fails on injected drift, not just passes.Verification
Full stack on both published v9.4 images, hangar_sim, overlay built per distro, both runs at this branch's final commit. Jazzy runs on top of #21807's image, without which
bt_navigatorcrashes regardless of this change. (The Jazzy column was first produced against #21671 and re-run against #21807 after that PR was narrowed and closed; the results are identical -- 5.88 m in 41.8 s either way.)v9.4-humble-amd64v9.4-jazzy-amd64.../params/nav2_params.yaml.../params/nav2_params_jazzy.yamlbt_navigator/planner_server/controller_serveractive [3]active [3]progress_checker_pluginset (Humble spelling is real here)progress_checker_plugins,local_costmap_topic,local_footprint_topicall setNavigateToPosewith old hardcoded pathNavigateToPosewithbehavior_tree=""On Jazzy a 6 m goal completes:
done=True elapsed=41.8s traveled=5.89 m. Before this change, bringup aborted atplanner_server; with names fixed but the list still present,nav2_containerdied (exit-6).The Humble column is the half CI cannot produce — v9.4 integration is jazzy-only, and the suite does not observe nav2 bringup anyway (both navigate objectives are in
skip_objectives, andwait_for_robot_tfonly waits onworld → grasp_link). It was run by hand for this reason.colcon testonhangar_sim: 17/17 pass, including the new parity test through the ament/ctest harness and thecopyrightandlint_cmaketargets.pre-commitpasses on all changed files.MPPI still commands ~0.176 m/s against
vx_max: 0.5on Jazzy. That is the separate upstream regression tracked by PickNikRobotics/moveit_pro#21730 and is out of scope here.Merge order
Land after PickNikRobotics/moveit_pro#21807 (the narrowed replacement for the closed #21671). The
needs:line above points CI at that PR's image so integration runs against a coherent BT.CPP/nav2 pair.Closes PickNikRobotics/moveit_pro#21729