Skip to content

Task: Update robot joint mapping #1728

Open
mustafab0 wants to merge 40 commits intodevfrom
task/mustafa/per-robot-joint-states
Open

Task: Update robot joint mapping #1728
mustafab0 wants to merge 40 commits intodevfrom
task/mustafa/per-robot-joint-states

Conversation

@mustafab0
Copy link
Copy Markdown
Contributor

Problem

The manipulation module broadcasts joint state to all robot monitors inefficiently (N robots = N redundant extraction attempts per tick), stores init joints as a single global variable (breaking multi-robot go_init), and uses ambiguous underscore-based joint naming (left_arm_joint1 — is the robot left_arm or left?).

Solution

Slash joint naming — Joint names now use / as separator: arm/joint1, base/vx instead of arm_joint1, base_vx. make_twist_base_joints, RobotConfig.joint_prefix, and all string-parsing code in coordinator and coordinator_client.

Per-robot joint state routing — _on_joint_state now splits the aggregated JointState by robot using each robot's coordinator joint names from config, then routes to the specific RobotStateMonitor with an explicit robot_id instead of broadcasting to all monitors.

Per-robot init joints — _init_joints changed from JointState | None to dict[RobotName, JointState]. All methods (go_init, get_init_joints, set_init_joints, set_init_joints_to_current, get_robot_info) are now per-robot aware.

Rename — WorldStateMonitor → RobotStateMonitor (file: robot_state_monitor.py). It monitors one robot's state, not the world.

Breaking Changes

Joint names use / separator: "arm/joint1" instead of "arm_joint1". Any code referencing old-style prefixed joint names must update.
get_init_joints() and set_init_joints() now accept an optional robot_name parameter.
WorldStateMonitor renamed to RobotStateMonitor (import path changed).

How to Test

uv run pytest dimos/control/test_control.py -x -q          
uv run pytest dimos/manipulation/test_manipulation_unit.py -x 

@mustafab0 mustafab0 force-pushed the task/mustafa/per-robot-joint-states branch 4 times, most recently from 449a2b4 to 9340ff7 Compare April 2, 2026 21:34
@mustafab0 mustafab0 marked this pull request as ready for review April 2, 2026 21:35
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR refactors the manipulation and control stack across three axes: (1) joint name separator changed from _ to / throughout (arm_joint1arm/joint1), (2) _on_joint_state now routes per-robot slices of the aggregated JointState to the correct RobotStateMonitor instead of broadcasting to all monitors, and (3) _init_joints is promoted from a single global JointState | None to a per-robot dict[RobotName, JointState]. The rename WorldStateMonitorRobotStateMonitor and the Drake model-instance cache are also included.

Key issues found:

  • P0 – Piper blueprint adapter logic is inverted (dimos/robot/manipulators/piper/blueprints.py): global_config.can_port defaults to \"can0\", which is the standard real-hardware CAN interface. The condition can_port != \"can0\" therefore selects "mock" for every user with a default-configured real robot. Contrast with the correct xArm pattern that gates on None.
  • P1 – Velocity data silently dropped (dimos/manipulation/manipulation_module.py): The new per-robot sub_msg is built with only position; msg.velocity is never forwarded. RobotStateMonitor._extract_velocities will always return None, silently regressing velocity tracking.
  • P1 – split_joint_name raises ValueError in display helpers (dimos/manipulation/control/coordinator_client.py): preview_waypoints and preview_trajectory now crash on any joint name that lacks a / separator instead of gracefully degrading as the previous split(\"_\")[-1] approach did.
  • P1 – Drake model-instance cache may share an instance across independent robots (dimos/manipulation/planning/world/drake_world.py): Two robots loaded from the same URDF at the same base pose reuse one Drake model instance. Drake identifies joints by model instance, so both robots would read/write identical joints.
  • P2 – Silent robot skip with no diagnostic log (dimos/manipulation/manipulation_module.py): When a robot's joints are not all present in the incoming message it is silently skipped on every tick, making joint-name mismatches very hard to diagnose.

Confidence Score: 2/5

Not safe to merge as-is: the inverted Piper adapter condition means real hardware silently runs as mock, and velocity data is dropped from every joint-state update.

Two P1 regressions (velocity data loss, ValueError in display helpers) and one P0 logic inversion (Piper always mock) block reliable use on real hardware. The Drake cache issue could corrupt multi-robot joint state. The core joint-separator migration and per-robot routing architecture are sound, but these implementation bugs need fixes before merging.

dimos/robot/manipulators/piper/blueprints.py (inverted adapter logic), dimos/manipulation/manipulation_module.py (velocity drop + silent skip), dimos/manipulation/planning/world/drake_world.py (model-instance cache safety), dimos/manipulation/control/coordinator_client.py (ValueError in preview helpers)

Important Files Changed

Filename Overview
dimos/manipulation/manipulation_module.py Per-robot joint routing and init-joints dict refactor; velocity data is silently dropped when building sub-messages, and robot-skip path has no warning log
dimos/robot/manipulators/piper/blueprints.py Inverted adapter-selection condition means real Piper hardware on the default "can0" port always runs as "mock"
dimos/manipulation/planning/world/drake_world.py New model-instance cache reuses Drake instances when URDF path and base pose match; may corrupt independent per-robot joint-state tracking for two robots sharing same URDF at same pose
dimos/manipulation/control/coordinator_client.py Uses split_joint_name in display helpers, which throws ValueError on pre-migration joint names instead of gracefully degrading
dimos/control/components.py Clean migration of joint separator from "_" to "/" with new split_joint_name helper; no issues found
dimos/manipulation/planning/monitor/robot_state_monitor.py Rename from WorldStateMonitor to RobotStateMonitor with updated docstrings; no logic changes, clean refactor
dimos/manipulation/blueprints.py Blueprints now respect global_config IP settings for xArm adapters; xArm pattern is correct (None-based), no issues
dimos/robot/config.py joint_prefix separator updated from "_" to "/"; _ensure_prefix called explicitly in to_hardware_component for safety; clean change
dimos/control/coordinator.py Twist-base suffix extraction now uses split_joint_name instead of rsplit("_"); correct and cleaner
dimos/manipulation/planning/spec/config.py Documentation update only (joint_name_mapping example updated to "/" separator); no logic changes

Sequence Diagram

sequenceDiagram
    participant Driver as Hardware Driver
    participant MM as ManipulationModule
    participant WM as WorldMonitor
    participant RSM1 as RobotStateMonitor(robot_1)
    participant RSM2 as RobotStateMonitor(robot_2)

    Driver->>MM: JointState(all robots, positions+velocities)
    Note over MM: Build name→index map
    MM->>MM: For each robot: extract coord_names indices
    alt All joints present
        MM->>MM: sub_msg = JointState(coord_names, positions_only ⚠️)
        MM->>WM: on_joint_state(sub_msg, robot_id=robot_1)
        WM->>RSM1: on_joint_state(sub_msg)
        RSM1->>RSM1: _extract_positions ✓
        RSM1->>RSM1: _extract_velocities → None ⚠️
    else Any joint missing
        MM-->>MM: continue (silent skip ⚠️)
    end
    MM->>MM: For robot_2: extract coord_names indices
    alt All joints present
        MM->>MM: sub_msg2 = JointState(coord_names, positions_only)
        MM->>WM: on_joint_state(sub_msg2, robot_id=robot_2)
        WM->>RSM2: on_joint_state(sub_msg2)
    end
    Note over MM: Capture _init_joints[robot_name] on first receipt
Loading

Comments Outside Diff (1)

  1. dimos/manipulation/control/coordinator_client.py, line 506 (link)

    P1 split_joint_name raises ValueError in display/diagnostic functions

    split_joint_name raises ValueError if the joint name does not contain /:

    def split_joint_name(joint_name: str) -> tuple[str, str]:
        ...
        if len(parts) != 2:
            raise ValueError(f"Joint name '{joint_name}' missing separator '/'")

    The old code was j.split("_")[-1][:6], which never raises. Using split_joint_name in preview_waypoints and preview_trajectory (display-only helpers) means a single unconverted joint name will crash the entire preview output rather than gracefully degrade. The same issue exists on line 514 in preview_trajectory.

    Consider guarding against this:

    Or wrap in a try/except for the display functions.

Reviews (1): Last reviewed commit: "trigger CI" | Re-trigger Greptile

@mustafab0 mustafab0 force-pushed the task/mustafa/per-robot-joint-states branch from b8e837a to 081d38d Compare April 6, 2026 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant