Skip to content

Feature: G1 control coordinator adapter for mobile base cmd vel#1410

Open
mustafab0 wants to merge 19 commits intodevfrom
feature/mustafa-g1-adapter-for-mobile-base-cmd_vel
Open

Feature: G1 control coordinator adapter for mobile base cmd vel#1410
mustafab0 wants to merge 19 commits intodevfrom
feature/mustafa-g1-adapter-for-mobile-base-cmd_vel

Conversation

@mustafab0
Copy link
Contributor

Problem

The Unitree G1 humanoid has no ControlCoordinator integration — it can only be driven through the legacy G1Connection WebRTC wrapper, with no DDS support and no unified velocity (Twist) interface.


Solution

Added two TwistBaseAdapter implementations for the G1 so it can be controlled via the ControlCoordinator's cmd_vel Twist interface:

  • unitree_g1 (DDS) — wraps LocoClient from unitree-sdk2py-dimos. Communicates over CycloneDDS on a specified network interface (enp60s0). Handles FSM state machine: checks boot state, transitions to lock stand (FSM 4), then activates locomotion (FSM 200) on enable. Never calls Damp/ZeroTorque.
  • unitree_g1_webrtc — wraps UnitreeWebRTCConnection for environments where DDS isn't available. Manages standup and walk mode via WebRTC publish requests.

Both adapters register with TwistBaseAdapterRegistry and expose 3 DOF: [vx, vy, wz].

Two coordinator blueprints added: unitree-g1-dds-coordinator and unitree-g1-webrtc-coordinator, both wiring KeyboardTeleop to the G1 via cmd_vel.

unitree-sdk2py-dimos>=1.0.3 added to the unitree extras in pyproject.toml.


Breaking Changes

None


How to Test

  1. DDS (real hardware): ROBOT_INTERFACE=enp60s0 dimos run unitree-g1-dds-coordinator — robot should stand up, WASD keys send velocity.
  2. WebRTC (real hardware): ROBOT_IP=192.168.123.164 dimos run unitree-g1-webrtc-coordinator
  3. Keyboard Teleop: Run dimos run keyboard-teleop

Please note, when G1 is booted first time. Robot must be put to damp mode manually. Subsequent blueprint runs don't need manual damping mode to be enabled.

closes DIM-592

@mustafab0 mustafab0 self-assigned this Mar 4, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR adds two TwistBaseAdapter implementations (unitree_g1 via DDS and unitree_g1_webrtc via WebRTC) that integrate the Unitree G1 humanoid robot into the existing ControlCoordinator framework, plus two matching coordinator blueprints. The adapters follow the established adapter pattern in the codebase and the registry auto-discovery mechanism works correctly for both.

Key concerns found:

  • FSM state detection false-positive (logic bug): _wait_for_fsm in the DDS adapter uses str(target_fsm) in str(data) for substring matching. Waiting for FSM 4 would falsely match responses containing 40, 14, or 400; waiting for FSM 200 would falsely match 2000 or 1200. This could cause locomotion_ready to be set True prematurely, sending velocity commands while the robot is in the wrong FSM state. The _get_fsm_id helper already does value extraction from the same response — _wait_for_fsm should use exact matching via json.loads.
  • Default network interface mismatch: The DDS coordinator blueprint defaults to "enp86s0" but both the module docstring and the PR description reference "enp60s0". One of the two needs to be corrected so documentation and code agree.
  • Fragile JSON parsing in _get_fsm_id: Manual string-splitting on : and rstrip of } is brittle; json.loads is the correct tool for parsing the SDK's JSON response.
  • Empty __init__.py for unitree_g1_webrtc: Unlike the DDS sibling package, the WebRTC package exposes no public symbols, making direct imports of UnitreeG1WebRTCTwistAdapter from the package fail — inconsistent with established package structure.
  • Private _Call SDK method: Both FSM query helpers use session.client._Call(...), a single-underscore private method not guaranteed by the public API.

Confidence Score: 3/5

  • The DDS adapter has a logic bug in FSM state detection that could send velocity commands to a robot in the wrong state — requires a fix before merging for hardware safety.
  • The WebRTC adapter and all blueprint/registry wiring are solid. However, the DDS adapter's _wait_for_fsm substring check is a correctness issue: premature FSM confirmation could activate locomotion mode on the G1 while the robot is still transitioning, with real physical consequences. The additional concerns (fragile JSON parsing, interface default mismatch, empty init) are lower severity but should be addressed for quality.
  • dimos/hardware/drive_trains/unitree_g1/adapter.py — FSM detection logic and JSON parsing; dimos/robot/unitree/g1/blueprints/basic/unitree_g1_dds_coordinator.py — default interface name

Important Files Changed

Filename Overview
dimos/hardware/drive_trains/unitree_g1/adapter.py New DDS-based TwistBaseAdapter for the G1; contains a substring-matching false-positive bug in _wait_for_fsm, fragile JSON parsing in _get_fsm_id, and relies on the private _Call SDK method.
dimos/hardware/drive_trains/unitree_g1_webrtc/adapter.py New WebRTC-based TwistBaseAdapter for the G1; clean implementation that wraps UnitreeWebRTCConnection with proper odometry via RxPY subscription and correct quaternion-to-yaw conversion.
dimos/hardware/drive_trains/unitree_g1_webrtc/init.py Empty init.py — inconsistent with the DDS sibling package which exports its adapter class; direct imports of UnitreeG1WebRTCTwistAdapter from the package will fail.
dimos/robot/unitree/g1/blueprints/basic/unitree_g1_dds_coordinator.py New DDS coordinator blueprint; hardcoded default interface "enp86s0" conflicts with both the module docstring and PR description which reference "enp60s0".
dimos/robot/unitree/g1/blueprints/basic/unitree_g1_webrtc_coordinator.py New WebRTC coordinator blueprint; straightforward wiring of KeyboardTeleop to the G1 via cmd_vel, no issues.
dimos/robot/all_blueprints.py Two new blueprint entries added in correct alphabetical position; no issues.

Sequence Diagram

sequenceDiagram
    participant User as User / KeyboardTeleop
    participant CC as ControlCoordinator
    participant DDS as UnitreeG1TwistAdapter (DDS)
    participant WR as UnitreeG1WebRTCTwistAdapter (WebRTC)
    participant G1 as Unitree G1 Robot

    Note over CC,DDS: DDS path (unitree-g1-dds-coordinator)
    CC->>DDS: connect()
    DDS->>G1: ChannelFactoryInitialize(0, interface)
    DDS->>G1: LocoClient.Init()
    DDS->>G1: SetFsmId(4) — lock stand
    DDS->>G1: _wait_for_fsm(4)
    G1-->>DDS: FSM state = 4
    DDS-->>CC: connected = True

    CC->>DDS: write_enable(True)
    DDS->>G1: LocoClient.Start() — FSM 200
    DDS->>G1: _wait_for_fsm(200)
    G1-->>DDS: FSM state = 200
    DDS-->>CC: enabled = True

    loop 100 Hz cmd_vel loop
        User->>CC: Twist (vx, vy, wz) via LCM /cmd_vel
        CC->>DDS: write_velocities([vx, vy, wz])
        DDS->>G1: LocoClient.Move(vx, vy, wz)
    end

    CC->>DDS: disconnect()
    DDS->>G1: StopMove()

    Note over CC,WR: WebRTC path (unitree-g1-webrtc-coordinator)
    CC->>WR: connect()
    WR->>G1: UnitreeWebRTCConnection(ip)
    WR->>G1: publish_request(FSM 4 — lock stand)
    WR-->>CC: connected = True

    CC->>WR: write_enable(True)
    WR->>G1: publish_request(FSM 200 — locomotion)
    WR-->>CC: enabled = True

    loop cmd_vel loop
        User->>CC: Twist via LCM /cmd_vel
        CC->>WR: write_velocities([vx, vy, wz])
        WR->>G1: conn.move(Twist)
    end

    CC->>WR: disconnect()
    WR->>G1: send zero velocity + liedown()
Loading

Last reviewed commit: 15c93c2

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Additional Comments (2)

dimos/robot/unitree/g1/blueprints/basic/unitree_g1_dds_coordinator.py
Default interface name conflicts with PR description and module docstring

The blueprint falls back to "enp86s0" when ROBOT_INTERFACE is not set, but:

  • The module docstring (line 17) shows ROBOT_INTERFACE=enp60s0
  • The PR description says: "ROBOT_INTERFACE=enp60s0 dimos run unitree-g1-dds-coordinator"

These three places disagree. Users following the docs will set enp60s0, which will work, but any user who doesn't set the env var will get enp86s0 as the default — likely the wrong interface. The docstring should be updated to match the actual default, or the default should be changed to match the documented value.

            address=os.getenv("ROBOT_INTERFACE", "enp60s0"),

dimos/hardware/drive_trains/unitree_g1_webrtc/__init__.py
Empty __init__.py — inconsistent with the DDS sibling package

unitree_g1/__init__.py exports UnitreeG1TwistAdapter and sets __all__. This package's __init__.py is completely empty, making UnitreeG1WebRTCTwistAdapter inaccessible via from dimos.hardware.drive_trains.unitree_g1_webrtc import UnitreeG1WebRTCTwistAdapter.

While the adapter registry discovers the class via adapter.py directly (so runtime behaviour is unaffected), the empty init is inconsistent and makes the package less usable for direct imports. Consider mirroring the DDS package structure:

"""Unitree G1 WebRTC humanoid robot adapter."""

from dimos.hardware.drive_trains.unitree_g1_webrtc.adapter import UnitreeG1WebRTCTwistAdapter

__all__ = ["UnitreeG1WebRTCTwistAdapter"]

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@mustafab0 mustafab0 changed the title Feature/mustafa g1 adapter for mobile base cmd vel Feature: G1 control coordinator adapter for mobile base cmd vel Mar 4, 2026
@mustafab0 mustafab0 force-pushed the feature/mustafa-g1-adapter-for-mobile-base-cmd_vel branch from 12653c3 to 9cd7865 Compare March 4, 2026 05:10
@mustafab0 mustafab0 requested a review from jeff-hykin March 5, 2026 02:36
(needs.ros-python.result == 'success' && (needs.check-changes.outputs.python == 'true' || needs.check-changes.outputs.ros == 'true')))
}}
from-image: ghcr.io/dimensionalos/ros-python:${{ needs.ros-python.result == 'success' && needs.check-changes.outputs.branch-tag || 'dev' }}
from-image: ghcr.io/dimensionalos/ros-python:${{ needs.ros-python.result == 'success' && (needs.check-changes.outputs.python == 'true' || needs.check-changes.outputs.ros == 'true') && needs.check-changes.outputs.branch-tag || 'dev' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls dont touch CI unless you have very good reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I debated quite a bit before adding this.

The original intended to fall back to dev when ros-python didn't build, but the problem is GitHub Actions expression evaluation: ros-python.result == 'success' is true even when should-run was false (all steps skipped, job still reports success). So the original resolves to branch-tag instead of 'dev'. This causes a bug where it expects the ros-python build but gets a stale one.

This edit adds the extra check (python == 'true' || ros == 'true') so it only uses the branch tag when those files actually changed, meaning ros-python actually built something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand your concern here. You finally got CI to work properly I am scared of breaking it again too. 😅


unitree-dds = [
"dimos[unitree]",
"unitree-sdk2py-dimos>=1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a high risk PR so needs careful review and will come in following release. Relies on someone else's code and we need to maintain a wheel. How many will use CycloneDDS? We likely do to this properly need very robust build matrix for py310 311 312 and macos arch etc etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed on this. I am not pushing this currently for merge.

I am testing this on multiple NUCs, building things from scratch. Already caught a few bugs, and will keep this to test longer.

Also will get more people to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cyclone DDS or DDS support it seems will be very important as we start interfacing with more OEM hardware.

We are already seeing there is no way around it when trying to work with Unitree. Similarly with Booster, and some initial tests show we would need it for Galaxea too.

I believe all OEMs have DDS support as ROS works over DDS and everyone supports it by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have uploaded the unitree-sdk2py-dimos package with custom wheel to PyPi. I have not required to do that for CycloneDDS.


# Build CycloneDDS 0.10.5 from source (Ubuntu 22.04 apt only has ~0.9.x,
# but the Python cyclonedds package requires >=0.10.5 headers)
RUN git clone --branch 0.10.5 --depth 1 https://github.com/eclipse-cyclonedds/cyclonedds.git /tmp/cyclonedds && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants