fix(hangar_sim): make robot_state_publisher the sole owner of the ridgeback_base_link transform (backport to v9.4) - #866
Conversation
Co-authored-by: marty-mcfly-bot[bot] <marty-mcfly-bot[bot]@users.noreply.github.com>
…v9.4 [v9.4] Fix Ubuntu 24.04 user ID collision
The workspace integration test runs MoveIt Pro inside a bridge-networked container on an ephemeral runner -- no TPM, and no interface the licensing fingerprint will accept, since a container veth is rejected by design. Through 9.4.1 that was fine: the fingerprint hashed eth0's MAC and took the veth's. From 9.4.2 (moveit_pro#21596) it resolves a TPM endorsement key, else a permanent hardware NIC, and fails closed with neither -- so this job would stop activating a license the moment it pulls a 9.4.2 image. v0.9.1 is one commit on top of the pinned v0.9.0, adding only the step that builds a synthetic sysfs tree with a fixed MAC and points MOVEIT_HOST_SYSFS at it. No input signature change. The MAC is fixed so the runner fleet presents one identity rather than each ephemeral runner consuming its own activation. v10.0 already pins v0.9.1; this brings the 9.4 line in line ahead of 9.4.2.
….9.1-v9.4 Ci: Bump moveit_pro_ci to v0.9.1 for the license hardware identity
…r-base-link Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim
…_sim architecture doc
|
This is a duplicate opened automatically by the no-mistakes validation pipeline against |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe simulation now keeps odometry messages while assigning live TF ownership to ChangesSimulation transform and odometry flow
Integration CI workflow
Container workspace user setup
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
Intent
Backport moveit_pro_example_ws config changes paired with two MuJoCo fixes being backported to moveit_pro v9.4 (maintainer decision).
base_link_name config for hangar_sim (pairs with moveit_pro PR 20258, backported to v9.4 as moveit_pro#21734, in CI as of this writing): cherry-picked merged main-branch PR Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim #756 (merge commit 59d74ff, branch fix/18174-tf-single-owner-base-link) content-faithfully onto v9.4. Config has NO EFFECT until moveit_pro#21734 lands on v9.4 - stated plainly in the PR body; merge/deploy should wait on that landing first.
Investigated whether an example_ws config change pairs with moveit_pro PR 21443 (site frames via mj_world), whose body references moveit_pro_example_ws#790. Verdict: NO backport needed. Add fuse odometry localization for the hangar_sim mobile base #790 is a large unmerged v10.0-only feature (fuse-odometry + AMCL stack) not present on v9.4; the one relevant commit (removing a mj_world-parented base_gt ground-truth site) touches a site that doesn't exist on v9.4's hangar_sim.
This branch (backport/756-hangar-sim-base-link-name-v9.4) already has an OPEN, CORRECTLY-BASED draft PR: #864 (base v9.4, milestone 9.4.2). This exact content already went through a full no-mistakes pipeline run (review, test, document, lint) on a differently-named branch (fm/exws-backport-baselink-config-v94) that reached identical validated content at this same commit (bb8a034) - that prior run's review/test/document gates were already resolved by captain decision: (a) doc-amcl-fabricated finding fixed to reflect v9.4's actual no-AMCL state, (b) 3 findings reproducing already-merged-upstream PR 756 design accepted as out-of-scope, (c) test failure against the not-yet-backported v9.4 image confirmed as expected/documented dependency behavior, (d) doc divergence note kept but reworded to be self-contained. That prior run's own PR step incorrectly opened a duplicate PR (#865) against main instead of reusing #864, because it ran from a branch with no existing PR - it has been closed with a pointer comment to #864, and this correctly-named branch was fast-forwarded to the same validated commit and pushed to keep #864 up to date.
PIPELINE NOTE: this is a release-branch backport (base v9.4, not main). If the pipeline offers/attempts a rebase-onto-main step, skip/decline it. Since this branch already has an open PR (#864, correct base v9.4), the PR step should recognize and reuse it rather than opening a new one - if it opens another duplicate against main anyway, close it with a pointer comment to #864 and verify #864's branch head/base/milestone via the API, same recovery as before.
What Changed
ridgeback_base_linktransform (hangar_sim). The MuJoCo hardware plugin no longer broadcasts a competing parent for that frame:odom_publish_tf: falseandbase_link_name: ridgeback_base_link(stopping the lidar TF fill-in chain at the base instead of walking to the MJCF worldbody) inpicknik_ur_mujoco_ros2_control.xacro, pluspublish_tf: falsefor fuse's state estimator.robot_state_publishernow owns the live edge via the virtual-rail joint chain, while/odommessages keep flowing for Nav2. The misspelled, silently-ignoredodom_zero_zparam is replaced with the realodom_planar, and theworld -> virtual_rail_link_1origin gains the-0.048 mz offset that matches the MJCFbase_platform_rotationanchor. Note:base_link_nameis not yet a recognized parameter on v9.4 — this config has no effect until the pairedmoveit_probackport (moveit_pro#21734) lands on v9.4, so merge/deploy should wait on that.robot_drivers_to_persist_sim.launch.pyre-anchors the static TF frommj_world -> worldtoodom -> world(REP-105 ordering now thatodomis no longer created by a MuJoCo broadcast) and drops the_check_fuse_publish_odomOpaqueFunctionguard, which enforced the now-obsolete "publish_odommust be false whenuse_fuse" rule. A newtest_base_link_has_single_tf_parentintegration test samples/tfand assertsvirtual_rail_link_2is the sole parent ofridgeback_base_link, and checks/odompose z is zeroed to pinodom_planar's effect. The nav2/whole-body architecture doc is updated to match the new TF ownership and joint-state path.Dockerfileremoves the pre-existingubuntuUID/GID 1000 user on Ubuntu 24.04 before creating the workspace user (tolerating a non-zerouserdelexit), and the workspace integration test workflow is bumped tomoveit_pro_civ0.9.1, which adds the synthetic host NIC the licensing fingerprint requires from MoveIt Pro 9.4.2 onward.Risk Assessment
✅ Low: Config/doc/test-only backport whose code files are byte-identical to the already-merged upstream PR #756 (modulo documented v9.4 adaptations), both prior-round fixes are correctly applied and verified against the real launch code, and the only new observation is a dormant non-default-path comment inaccuracy.
Testing
Ran the hangar_sim TF-ownership integration test against a live, licensed MoveIt Pro backend with MuJoCo on GPU/EGL in three configurations, plus a paired docker build of both Dockerfiles and a paired use_fuse launch. The change does what it claims on a v9.4 image carrying the paired core backport: three competing publishers of ridgeback_base_link collapse to robot_state_publisher alone, /odom pose z goes from 0.048 m to exactly 0, the mobile base still executes its 2 m move objective, and the obsolete fuse launch guard no longer aborts the nav stack. The Dockerfile fix was reproduced as a real build failure before and a clean build after. The only failing run is the expected one on the published v9.4 image, where base_platform remains a second parent because moveit_pro#21734 is not in that core yet — confirming the PR body's dependency rather than contradicting it. Evidence is a rendered before/after TF-ownership graph, the raw TF edge dumps, and the pytest and docker build transcripts; all test containers, images and temp dirs were removed and the worktree is back to a clean state with submodules deinitialized.
/home/griswald/.no-mistakes/evidence/01M0E09QSBRPE4Z4GSNABTF6S9/tf-ownership-before-after.png)Evidence: Integration test transcript — all three image/config scenarios
=== 1) BASE COMMIT config on the v9.4 + moveit_pro#21734 image -> FAILS === E AssertionError: Expected robot_state_publisher's virtual_rail_link_2 as the sole TF parent of ridgeback_base_link, saw parents: ['base_platform', 'odom', 'virtual_rail_link_2'] === 2) THIS BRANCH on the v9.4 + moveit_pro#21734 image -> PASSES === PASSED objectives_integration_test.py::test_base_link_has_single_tf_parent (12.7s) PASSED objectives_integration_test.py::test_all_objectives[.../solution_move_forward_2m.xml] (12.9s) === 2 passed, 66 deselected, 1 warning in 35.91s === === 3) THIS BRANCH on the PUBLISHED v9.4 image (no base_link_name in core) -> FAILS, as documented === E AssertionError: ... saw parents: ['base_platform', 'virtual_rail_link_2']Evidence: Dockerfile Ubuntu 24.04 UID/GID 1000 collision — build before vs after
=== BASE COMMIT Dockerfile (no userdel) -> build FAILS at user creation === #7 0.136 groupadd: GID '1000' already exists #7 ERROR: ... did not complete successfully: exit code: 4 === THIS BRANCH Dockerfile -> ubuntu user removed, user creation succeeds === #6 [base 2/9] RUN if id -u ubuntu > /dev/null 2>&1; then userdel -r ubuntu || true; fi #6 DONE 0.2s #8 DONE 11.9sEvidence: use_fuse:=true launch guard removal, before vs after
Evidence: Raw live TF edge dump — base commit (3 parents, /odom |z| = 0.048)
/home/griswald/.no-mistakes/evidence/01M0E09QSBRPE4Z4GSNABTF6S9/tf-edges-after.json) - Outcome:Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
.github/workflows/ci.yaml- merge conflict rebasing onto origin/mainDockerfile:28- Dockerfile:28 — the fallbackuserdel -r ubuntu || userdel ubuntufails the build in exactly the case it was added to survive.userdel -rexits 12 when it removes the account entry but cannot remove the home directory or mail spool; the account is already gone at that point, so the fallbackuserdel ubuntuexits 6 ("user 'ubuntu' does not exist") and the RUN layer aborts. The fallback only helps in the narrow case where-rfailed before touching /etc/passwd, which is not the common failure. Useuserdel -r ubuntu || true— if UID 1000 really is still occupied, the lateruseradd --uid $USER_UID(Dockerfile:39) fails loudly with a clear message. Separately, this line is not part of the PR Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim #756 content the intent describes as content-faithfully cherry-picked; it modifies commit 4fe52e6 (PR [v9.4] Fix Ubuntu 24.04 user ID collision #782), already merged on v9.4, so reverting it to the upstream form is also a valid resolution. Choosing between fix and revert is the author's call.src/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md:813- src/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md:813-818 — thestatic_tf_map_to_odomexcerpt omits thecondition=IfCondition(PythonExpression(["not ", slam]))that the real node carries (robot_drivers_to_persist_sim.launch.py:278), and its comment says "only used when neither SLAM nor AMCL is publishing it" while the same doc (line 848 note) correctly states AMCL is never launched on v9.4. A reader copying this snippet gets an unconditional map->odom static that fights slam_toolbox underslam:=True. Add the condition to the snippet and drop the AMCL clause from its comment.src/hangar_sim/config/fuse/fuse.yaml:95- src/hangar_sim/config/fuse/fuse.yaml:95 — withpublish_tf: false, fuse's only remaining output is theodom_filteredtopic (fuse.yaml:85), and nothing subscribes to it: nav2_params.yaml:48/435 read/odomandodom, and no launch file or script referencesodom_filtered. Combined with the removal of_check_fuse_publish_odom,use_fuse:=truenow has no observable effect on the running system. This is the documented consequence of making robot_state_publisher the sole owner of the base transform (upstream PR Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim #756 design), not a defect introduced here — noting it so the dead launch argument is a conscious carry-forward rather than a surprise.🔧 Fix: Fix userdel fallback; add slam condition to TF doc snippet
1 info still open:
src/hangar_sim/launch/sim/robot_drivers_to_persist_sim.launch.py:285- src/hangar_sim/launch/sim/robot_drivers_to_persist_sim.launch.py:285 — the new comment says a live map->odom correction "shifts the whole robot subtree", butworldis not just the robot's root:ur5e_ridgeback.xacro:38attaches the hangar via<xacro:hangar_urdf parent="world"/>, andhangar_urdf.xacro:5-9fixescollision_SM_Floor_376(the floor + structure collision geometry) directly to that parent. Reparentingworldunderodomtherefore also puts MoveIt's entire static collision environment under the correction. Concrete path: launch withslam:=True->static_tf_map_to_odomis skipped (line 278) and slam_toolbox owns map->odom -> any nonzero correction translates the hangar floor/structure relative tomapand to the MuJoCo physics, which stay anchored atmj_worldviastatic_tf_world_to_map(line 261). Dormant on every path exercised here:slamdefaults to "False" (line 130), nothing in the workspace passesslam:=True, and with ground-truth odometry the correction is ~0 anyway. Under the oldmj_world->worldlink the environment was rigid regardless of localization. Noting it because the comment's "robot subtree" wording understates the scope; this is upstream PR Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim #756's design, cherry-picked faithfully, so no change is required for this backport.src/hangar_sim/description/picknik_ur_mujoco_ros2_control.xacro:41- Confirmed live: with this branch's config on the PUBLISHED v9.4 image (core without moveit_pro#21734), test_base_link_has_single_tf_parent still fails — MuJoCo's lidar fill-in chain keeps publishing base_platform -> ridgeback_base_link, leaving 2 parents. The odom_publish_tf half of the change does take effect (the 'odom' parent is gone), so the config is partially live but does not achieve single ownership. This matches the PR body's stated dependency and the prior run's captain decision; it is merge/deploy ordering context, not a defect in the change.src/hangar_sim/test/objectives_integration_test.py:337- Two pre-existing, unrelated instabilities showed up while running the hangar_sim backend and could make this integration test flaky in CI: (1) nav2's planner_server fails to load with 'nav2_navfn_planner/NavfnPlanner ... does not exist' on Jazzy (the ROS-distro nav2 params problem that open PR Fix: Select hangar_sim nav2 params by ROS distro so Jazzy navigation starts #863 fixes), and (2) one run out of six had the vacuum_gripper controller spawner die with exit 1, tearing the whole launch down as a critical process; an immediate retry passed. Neither is caused by this change — the TF assertion itself was deterministic across every completed run.pytest objectives_integration_test.py -k test_base_link_has_single_tf_parentagainst the base-commit hangar_sim install on the v9.4 + moveit_pro#21734 image (expected FAIL: 3 TF parents)pytest objectives_integration_test.py -k "single_tf_parent or solution_move_forward_2m"against this branch on the v9.4 + moveit_pro#21734 image (2 passed — single TF parent plus the base-motion regression)pytest objectives_integration_test.py -k test_base_link_has_single_tf_parentagainst this branch on the publishedpicknikciuser/moveit-studio:v9.4-jazzy-amd64-cuda13.2-cudnn9image (expected FAIL: 2 TF parents, the documented core dependency)Live TF census script bringing upgenerate_robot_drivers_launch_description()+generate_agent_launch_description()for hangar_sim, recording every /tf and /tf_static edge plus /odom pose z for 20 s, run once per commitdocker build --target base --build-arg USER_UID=1000 --build-arg USER_GID=1000with the base-commit Dockerfile (fails:groupadd: GID '1000' already exists) and with the branch Dockerfile (userdel step succeeds, user creation succeeds) on the same v9.4 base imageros2 launch hangar_sim robot_drivers_to_persist_sim.launch.py use_fuse:=trueon both commits (base commit: guard message + exit 1; branch: still running at the 45 s harness timeout)colcon build --packages-up-to hangar_simin both the backported and published v9.4 containers afterrosdep installover hangar_sim's dependency set✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.