Fix: Make robot_state_publisher the sole owner of the ridgeback_base_link transform in hangar_sim#756
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR reworks simulated Ridgeback odom/TF ownership, configures planar MuJoCo odometry, replaces the static world transform, removes launch validation, aligns a robot joint offset, updates architecture documentation, and adds TF/odom integration assertions. ChangesOdom/TF ownership refactor
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
[written by AI] Tested with moveit_pro#20258, fuse on. |
a1c5d0f to
b9e98e1
Compare
|
|
1 similar comment
|
|
|
|
|
||
|
|
||
| BASE_LINK_FRAME = "ridgeback_base_link" | ||
| TF_PARENT_SAMPLE_DURATION_S = 4.0 |
There was a problem hiding this comment.
would this create a future CI flake if not high enough?
There was a problem hiding this comment.
unsure is a way to wait for the first transform...
There was a problem hiding this comment.
[written by AI]
Fair concern — the 4 s window was doing double duty as both "wait for the stack" and "sample the parent set", and only the second job is time-calibrated. Fixed in ffdb9da2 exactly along the lines of your second comment: the test is now two-phase. Phase 1 spins until the FIRST transform into ridgeback_base_link (and the first /odom message) arrives, bounded by a generous TF_FIRST_SIGHTING_TIMEOUT_S = 60 s that only limits startup latency on a loaded runner. Phase 2 then samples for the original 4 s to catch conflicting parents — competing publishers broadcast at 10–120 Hz, so a second parent would land hundreds of times in that window. Slow bring-up can no longer eat the sampling budget or masquerade as "wrong publishers".
There was a problem hiding this comment.
[written by AI]
There is — resolved together with the thread above: ffdb9da2 adds a first-sighting wait phase (spin until the frame appears on /tf, 60 s bound) before the 4 s parent-sampling phase.
| # the estimate into the virtual-rail joint states instead of TF. | ||
| world_frame_id: 'odom' | ||
| publish_tf: true | ||
| publish_tf: false |
There was a problem hiding this comment.
so effectively... we would never use this in sim? but maybe on a real robot that didn't need the whole-body virtual joint paths...
There was a problem hiding this comment.
[written by AI]
Right on both halves. In this config — sim or hardware — fuse's TF output stays off permanently: the whole-body virtual-rail joint chain means robot_state_publisher already owns the edge into ridgeback_base_link, and a second TF parent from fuse is exactly the conflict this PR removes. Fuse still runs and its estimate stays consumed via the odom_filtered topic; on hardware the estimate would feed the virtual-rail joint states (per the comment above this line) rather than TF.
A robot that does NOT use the whole-body virtual-joint approach — no joint path from odom to the base, so nothing else publishes that edge — is the standard fuse deployment, and there you'd set publish_tf: true and let fuse own odom → base_link as usual. So: not "never use fuse in sim", but "never let two things own the same TF edge"; which one owns it follows from whether the base pose is in the joint state.
|
[written by AI] Ran a fuse-on before/after with this PR + moveit_pro#20258:
Three-publisher conflict → single clean owner with fuse enabled, no runaway. |
bkanator
left a comment
bkanator
left a comment
There was a problem hiding this comment.
ran your test harness, and re-ran the failure case I had with fuse on to confirm fixes still working.
b9e98e1 to
ffdb9da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md (1)
893-904: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReword this as a parent-frame conflict, not just duplicate TF publication.
robot_state_publisherownsworld → … → ridgeback_base_link, while the controllers would publishodom → ridgeback_base_link; with the staticodom → worldlink, the real problem is competing parents forridgeback_base_link. If both controllers enable TF, they also duplicate the sameodom → ridgeback_base_linkedge, so keep those two cases distinct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md` around lines 893 - 904, The architecture documentation should describe the primary issue as competing parent frames for ridgeback_base_link: robot_state_publisher provides world → … → ridgeback_base_link, while controllers provide odom → ridgeback_base_link through the static odom → world link. In the Consequence of Misconfiguration section, distinguish this parent-frame conflict from the separate case where both platform_velocity_controller and platform_velocity_controller_nav2 enable TF and duplicate the same odom → ridgeback_base_link edge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md`:
- Around line 893-904: The architecture documentation should describe the
primary issue as competing parent frames for ridgeback_base_link:
robot_state_publisher provides world → … → ridgeback_base_link, while
controllers provide odom → ridgeback_base_link through the static odom → world
link. In the Consequence of Misconfiguration section, distinguish this
parent-frame conflict from the separate case where both
platform_velocity_controller and platform_velocity_controller_nav2 enable TF and
duplicate the same odom → ridgeback_base_link edge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ac4cea6-7ad9-49e2-adaa-9b152f3041a0
📒 Files selected for processing (7)
src/hangar_sim/config/config.yamlsrc/hangar_sim/config/fuse/fuse.yamlsrc/hangar_sim/description/picknik_ur_mujoco_ros2_control.xacrosrc/hangar_sim/description/ur5e_ridgeback.xacrosrc/hangar_sim/docs/NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.mdsrc/hangar_sim/launch/sim/robot_drivers_to_persist_sim.launch.pysrc/hangar_sim/test/objectives_integration_test.py
✅ Files skipped from review due to trivial changes (1)
- src/hangar_sim/config/config.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/hangar_sim/config/fuse/fuse.yaml
- src/hangar_sim/description/ur5e_ridgeback.xacro
- src/hangar_sim/description/picknik_ur_mujoco_ros2_control.xacro
- src/hangar_sim/test/objectives_integration_test.py
- src/hangar_sim/launch/sim/robot_drivers_to_persist_sim.launch.py
|
|
|
2 participants
[written by AI]
Fixes the remaining half of PickNikRobotics/moveit_pro#18174. Part of epic PickNikRobotics/moveit_pro#17431.
needs: moveit_pro/#20258
Depends on PickNikRobotics/moveit_pro#20258 (adds the
base_link_namehardware parameter). Against a moveit_pro image without it, the new xacro param is silently ignored, the lidar-chain half of the conflict remains, and the new single-parent integration test fails — theneeds:token above makes CI run against that PR's image. Do not merge until #20258 is merged and a fresh moveit_promainimage is published, or the first post-mergemainCI run here will use an image without the parameter.Motivation
ridgeback_base_linkhad three concurrent TF parents in hangar_sim: the MuJoCo lidar fill-in chain (base_platform →, 2×60 Hz), robot_state_publisher (virtual_rail_link_2 →, 20 Hz), and the MuJoCo odom TF (odom →, 150 Hz; fuse when enabled). tf2 and the web UI keep last-writer-wins, so the frame flipped between poses disagreeing by 48 mm at rest (a real model mismatch) and up to 61 mm in motion — the robot visibly oscillated belowbase_linkwhenever the UI's fixed frame sat at or below it. #18174 de-duplicated only the odom edge, and only in the opt-in fuse mode.Brief description
Makes robot_state_publisher the sole owner of the live transform into
ridgeback_base_link. Its virtual-rail chain is the one publisher that cannot be turned off per-edge (the joints are required for whole-body planning), and in sim it is already ground truth — the base's x/y/yaw are literal MuJoCo joints streamed throughjoint_state_broadcaster. Every other subsystem now expresses the base pose as data, not TF:base_link_name: ridgeback_base_linkstops the MuJoCo lidar fill-in chain at the base (lidar mounts hang off it as plain children);odom_publish_tf: falsekeeps/odommessages flowing for Nav2 but drops the competing TF edge; the deadodom_zero_ztag (never a recognized parameter — silently ignored) is replaced by the realodom_planar.mj_world → worldstatic becomesodom → world. Theodomframe previously existed only because MuJoCo broadcast the competing edge; anchoringworldunderodomgives REP-105 semantics — beluga_amcl's livemap → odomcorrection shifts the whole robot subtree, and its ownodom → baselookups resolve through the static. The #18174 launch guard is removed: it enforced the old ownership model and, when fuse was on, forcedpublish_odom: false, which also killed the/odommessages Nav2 reads (odom_topic: /odom).publish_tf: false. Nothing consumesodom_filteredtoday; the estimate stays on the topic. On hardware, the estimator feeds the virtual-rail joint states (odometry-to-joint-state bridge) rather than TF — ownership of the edge never moves.z = -0.048, matching the MJCFbase_platform_rotationanchor (the sim base has no z DOF; that offset is what puts the wheels in floor contact). Previously the RSP chain rendered the robot 48 mm above where the physics and every sensor frame actually put it.ridgeback_base_linkhas exactly one TF parent (virtual_rail_link_2), so a re-enabled competing publisher fails CI instead of reappearing as UI jitter.NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.mdupdated to the new tree.Known out-of-scope:
phoebe_sim.urdf.xacrouses the same deadodom_zero_ztag (vendored dependency); the architecture doc's executive-summary data-flow diagram still shows the historical odometry bridge.How it was tested
Live on a running hangar_sim stack (dev container, moveit_pro built from the paired branch):
/tfattribution over 4 s windows: exactly one parent forridgeback_base_link(virtual_rail_link_2, ~20 Hz, co-published edges identify robot_state_publisher) — at rest and during motion. Before the change: three parents at ~150 flips/s.Solution - Move Forward 2mobjective SUCCEEDED (whole-body pipeline: MoveIt → JTC → chained mecanum controller); base x: −0.08 → 1.65 with z pinned at −0.048 throughout.map → odomlive on the new topology;odom → ridgeback_base_linkresolves (z = −0.048, matching physics); local costmap publishes;/scan_frontat 10 Hz; zero TF warnings (message filter,TF_OLD_DATA,TF_REPEATED_DATA) in the backend log.ast.parse;colcon buildofhangar_simsucceeds.Release notes
hangar_simwhen the fixed frame wasbase_linkor below, caused by multiple simulation publishers competing over the mobile base's transform.Agent checks (this repo has no PR template; statuses mirror the moveit_pro convention):
code-reviewerrun (cross-repo dependency called out above; stale comments, doc sweeps, and a regression test applied from its findings) ·documentation-botrun (3 stale-corrections in NAV2_AND_WHOLE_BODY_PLANNING_ARCHITECTURE.md applied) ·platform-architect-botrun (CI ordering finding addressed by theneeds:token + merge-order note; stale xacro comment fixed; /odom planar assertion added to the integration test) ·roboticist-botrun (no required changes; −0.048 z verified kinematically safe: offset sits above every IK chain, waypoints are joint-space) ·test-runner: hangar_sim builds; launch file and test pass syntax checks; the integration test runs in CI against the paired image; full TF behavior live-verified on a running stack (see "How it was tested").