Skip to content

refactor: split control blueprints + added env variables#1601

Merged
ruthwikdasyam merged 16 commits intodevfrom
ruthwik/arm_blueprints_clean
Mar 19, 2026
Merged

refactor: split control blueprints + added env variables#1601
ruthwikdasyam merged 16 commits intodevfrom
ruthwik/arm_blueprints_clean

Conversation

@ruthwikdasyam
Copy link
Contributor

@ruthwikdasyam ruthwikdasyam commented Mar 18, 2026

Problem

The monolithic dimos/control/blueprints.py was difficult to navigate,
and had hardcoded IP addresses scattered throughout

Closes DIM-639

Solution

  • Split into a dimos/control/blueprints/ package: basic.py, dual.py, teleop.py, mobile.py
  • Extracted shared hardware component factories
  • Replaced hardcoded IPs with env vars (XARM7_IP, XARM6_IP, CAN_PORT) in default.env

Breaking Changes

None

How to Test

  • dimos run _blueprint_name_ works

Contributor License Agreement

  • I have read and approved the CLA.

@ruthwikdasyam ruthwikdasyam marked this pull request as ready for review March 18, 2026 20:00
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR refactors the monolithic dimos/control/blueprints.py (692 lines) into a proper Python package, splitting the blueprints into four focused modules (basic.py, dual.py, mobile.py, teleop.py) with a shared hardware factory layer (_hardware.py). It also replaces hardcoded IP addresses with environment variables (XARM7_IP, XARM6_IP, CAN_PORT) registered in default.env.

The split is clean and the public API is fully preserved via __init__.py re-exports, making this a non-breaking change.

Key findings:

  • The section comments in __init__.py's __all__ list are misaligned with the alphabetically-sorted entries, causing several coordinators to appear under the wrong category heading (e.g., coordinator_mock listed under "Dual arm", coordinator_piper under "Mobile manipulation").
  • XARM7_IP and XARM6_IP are read once at module import time in _hardware.py. If they are unset, address=None is silently passed to HardwareComponent. Adding a guard in the xarm7() and xarm6() factory functions would surface the misconfiguration immediately with a clear error message rather than a confusing low-level failure at connection time.

Confidence Score: 4/5

  • Safe to merge — the refactor is a clean structural split with no logic changes and a fully preserved public API.
  • Score of 4 reflects that the core refactor is correct and non-breaking, but two minor issues (misleading all comments and no env-var validation for real hardware addresses) should ideally be addressed before merging to avoid developer confusion.
  • dimos/control/blueprints/init.py (misleading all comments) and dimos/control/blueprints/_hardware.py (no guard for unset XARM7_IP/XARM6_IP).

Important Files Changed

Filename Overview
default.env Adds three new env var keys: XARM7_IP, XARM6_IP (empty), and CAN_PORT (defaulting to can0) to replace previously hardcoded hardware addresses.
dimos/control/blueprints/init.py New package entry point re-exporting all blueprint coordinators; section comments in all are misaligned with the alphabetically-sorted entries, causing misleading groupings.
dimos/control/blueprints/_hardware.py Centralises hardware component factories and reads env vars (XARM7_IP, XARM6_IP, CAN_PORT) at module import time; missing validation means None addresses are silently passed through when env vars are unset.
dimos/control/blueprints/basic.py Clean extraction of single-arm trajectory blueprints (mock, XArm7, XArm6, Piper) from the old monolithic file; no issues found.
dimos/control/blueprints/dual.py Clean extraction of dual-arm trajectory blueprints (dual mock, dual XArm, Piper+XArm); no issues found.
dimos/control/blueprints/mobile.py Clean extraction of mobile manipulation blueprints (mock twist base, mock arm + base); no issues found.
dimos/control/blueprints/teleop.py Clean extraction of advanced teleop/cartesian IK blueprints (servo, velocity, CartesianIK, TeleopIK); no issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    ENV["default.env\nXARM7_IP / XARM6_IP / CAN_PORT"]

    subgraph PKG["dimos/control/blueprints/ (package)"]
        INIT["__init__.py\n(re-exports all)"]
        HW["_hardware.py\nfactories: mock_arm, xarm7, xarm6, piper, mock_twist_base\nmodel paths: PIPER, XARM6, XARM7"]

        BASIC["basic.py\ncoordinator_basic\ncoordinator_mock\ncoordinator_xarm7\ncoordinator_xarm6\ncoordinator_piper"]
        DUAL["dual.py\ncoordinator_dual_mock\ncoordinator_dual_xarm\ncoordinator_piper_xarm"]
        MOBILE["mobile.py\ncoordinator_mock_twist_base\ncoordinator_mobile_manip_mock"]
        TELEOP["teleop.py\ncoordinator_teleop_xarm6\ncoordinator_velocity_xarm6\ncoordinator_combined_xarm6\ncoordinator_cartesian_ik_mock\ncoordinator_cartesian_ik_piper\ncoordinator_teleop_xarm7\ncoordinator_teleop_piper\ncoordinator_teleop_dual"]
    end

    ENV --> HW
    HW --> BASIC
    HW --> DUAL
    HW --> MOBILE
    HW --> TELEOP
    BASIC --> INIT
    DUAL --> INIT
    MOBILE --> INIT
    TELEOP --> INIT
    INIT --> CLI["dimos run coordinator-*"]
Loading

Last reviewed commit: "fix: pre-commit chec..."

@ruthwikdasyam ruthwikdasyam changed the title refactor: split control blueprints into modular package + adding env variables refactor: split control blueprints + adding env variables Mar 18, 2026
@ruthwikdasyam ruthwikdasyam changed the title refactor: split control blueprints + adding env variables refactor: split control blueprints + add env variables Mar 18, 2026
@ruthwikdasyam ruthwikdasyam changed the title refactor: split control blueprints + add env variables refactor: split control blueprints + added env variables Mar 18, 2026
mustafab0
mustafab0 previously approved these changes Mar 19, 2026
default.env Outdated
ROBOT_IP=
XARM7_IP=
XARM6_IP=
CAN_PORT=can0
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the can_port also be blank in the interest of standardization?

I do realize for pretty much most can based interface can_port is can0. just a nitpick

Comment on lines +48 to +50
tick_rate=100.0,
publish_joint_state=True,
joint_state_frame_id="coordinator",
Copy link
Contributor

Choose a reason for hiding this comment

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

tick_rate/publish_joint_state/joint_state_frame_id are always using the default values. Why always specify them if it's always the default?

Comment on lines +58 to +60
tick_rate=100.0,
publish_joint_state=True,
joint_state_frame_id="coordinator",
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file you're also always using defaults.

)
from dimos.utils.data import LfsPath

XARM7_IP = os.getenv("XARM7_IP")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be nicer if these vars were properties on global_config like robot_ip is so that you can pass them as env vars or on the cli (e.g. --robot-ip=...).

Just something to think about.

paul-nechifor
paul-nechifor previously approved these changes Mar 19, 2026
@ruthwikdasyam ruthwikdasyam dismissed stale reviews from paul-nechifor and mustafab0 via 2d8ec35 March 19, 2026 07:50
@ruthwikdasyam ruthwikdasyam merged commit 4d0d260 into dev Mar 19, 2026
12 checks passed
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.

3 participants