mujoco sim module #1035

jca0 · 2026-01-16T01:33:59Z

mujoco simulation module

created

mujoco backend to bring up mujoco
mujoco manipulation interface to easily use with other robots
mujoco driver to make it a module/determine topics to publish and subscribe to
xarm sim driver for xarm specific needs

command to test: python3 -m dimos.simulation.sim_blueprints

todos:

load an environment from a config file/folder on simulation bringup
add support for other robots
dealing with sensor topics

greptile-apps · 2026-01-16T01:39:25Z

Greptile Summary

Implements a comprehensive MuJoCo simulation module for robotic manipulators with a layered architecture: MujocoSimBackend handles the core MuJoCo engine, MujocoManipInterface provides a uniform API, and MujocoDriver wraps it as a dimos Module with LCM transport integration.

Key Changes:

Created modular simulation backend with proper threading and synchronization
Implemented position and velocity control modes with 100Hz control/monitor loops
Added xArm-specific driver with correct joint limits
Includes test scripts for validating pub/sub functionality

Issues Found:

Duplicate return True at mujoco_manip_interface.py:183 (unreachable code)
Previous review flagged: mujoco_driver.py:156 incorrectly uses msg.positions for velocity commands (already noted in previous thread)

Confidence Score: 4/5

Safe to merge after fixing the duplicate return statement
The architecture is solid with proper threading, locking, and separation of concerns. The duplicate return is a minor syntax issue that doesn't affect runtime. The velocity command bug was already flagged in a previous review.
Pay attention to mujoco_driver.py (velocity command bug from previous review) and mujoco_manip_interface.py (duplicate return)

Important Files Changed

Filename	Overview
dimos/simulation/manipulators/mujoco_manip_interface.py	Adapter interface with duplicate return statement at line 183
dimos/simulation/manipulators/mujoco_sim_backend.py	Core MuJoCo backend with proper threading and synchronization
dimos/simulation/manipulators/mujoco_driver.py	Module driver - previous issue flagged on line 156 using `msg.positions` instead of velocities field

Sequence Diagram

sequenceDiagram
    participant User
    participant MujocoDriver
    participant MujocoManipInterface
    participant MujocoSimBackend
    participant MuJoCo
    participant LCM

    User->>MujocoDriver: start()
    MujocoDriver->>MujocoManipInterface: connect()
    MujocoManipInterface->>MujocoSimBackend: create backend
    MujocoSimBackend->>MuJoCo: load robot model
    MujocoSimBackend->>MujocoSimBackend: start sim_thread
    MujocoSimBackend-->>MujocoManipInterface: connected
    MujocoManipInterface-->>MujocoDriver: connected
    
    MujocoDriver->>MujocoDriver: start control_thread
    MujocoDriver->>MujocoDriver: start monitor_thread
    MujocoDriver->>LCM: subscribe to joint_position_command
    MujocoDriver->>LCM: subscribe to joint_velocity_command
    
    loop Monitor Loop (100Hz)
        MujocoDriver->>MujocoManipInterface: read_joint_positions()
        MujocoManipInterface->>MujocoSimBackend: joint_positions
        MujocoSimBackend-->>MujocoManipInterface: positions
        MujocoManipInterface-->>MujocoDriver: positions
        MujocoDriver->>LCM: publish JointState
        MujocoDriver->>LCM: publish RobotState
    end
    
    LCM->>MujocoDriver: JointCommand received
    MujocoDriver->>MujocoDriver: store pending_positions
    
    loop Control Loop (100Hz)
        MujocoDriver->>MujocoDriver: check pending commands
        MujocoDriver->>MujocoManipInterface: write_joint_positions()
        MujocoManipInterface->>MujocoSimBackend: set_joint_position_targets()
        MujocoSimBackend->>MujocoSimBackend: update _joint_position_targets
    end
    
    loop Simulation Loop (model timestep)
        MujocoSimBackend->>MujocoSimBackend: _apply_control()
        MujocoSimBackend->>MuJoCo: mj_step()
        MuJoCo-->>MujocoSimBackend: simulation state
        MujocoSimBackend->>MujocoSimBackend: _update_joint_state()
    end
    
    User->>MujocoDriver: stop()
    MujocoDriver->>MujocoDriver: stop control/monitor threads
    MujocoDriver->>MujocoManipInterface: disconnect()
    MujocoManipInterface->>MujocoSimBackend: disconnect()
    MujocoSimBackend->>MujocoSimBackend: stop sim_thread

greptile-apps

_{8 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

dimos/simulation/manipulators/mujoco_manip_interface.py

greptile-apps · 2026-01-16T01:39:25Z

dimos/simulation/manipulators/mujoco_driver.py

+    def _on_joint_velocity_command(self, msg: JointCommand) -> None:
+        with self._command_lock:
+            self._pending_velocities = list(msg.positions)
+            self._pending_positions = None


syntax: JointCommand doesn't have a velocities field - it only has positions. This will cause an AttributeError when velocity commands are received.

Suggested change

def _on_joint_velocity_command(self, msg: JointCommand) -> None:

with self._command_lock:

self._pending_velocities = list(msg.positions)

self._pending_positions = None

def _on_joint_velocity_command(self, msg: JointCommand) -> None:

with self._command_lock:

self._pending_velocities = list(msg.positions)

self._pending_positions = None

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

greptile-apps

_{8 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

dimos/simulation/manipulators/mujoco_manip_interface.py

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

mustafab0 · 2026-01-22T21:07:21Z

dimos/simulation/manipulators/sim_manip_interface.py

+        self._dof = int(spec.dof) if spec.dof is not None else 0
+        self._connected = False
+        self._servos_enabled = False
+        self._control_mode = ControlMode.POSITION


Why do we need ControlMode.Position?

just a default until user changes it via set_control_mode

mustafab0 · 2026-01-22T21:14:07Z

dimos/simulation/plugins/mujoco_xarm.py

This is not a scalable approach. Having to build plugin for each oem manufacturer + sim engine is not maintainable.

The robot arms should not be specific to any SimEngine

mustafab0 · 2026-01-22T21:16:59Z

dimos/simulation/plugins/mujoco_xarm.py

+def _xarm_limits(dof: int) -> JointLimits:
+    if dof == 7:
+        lower_deg = [-360, -118, -360, -233, -360, -97, -360]
+        upper_deg = [360, 118, 360, 11, 360, 180, 360]
+    elif dof == 6:
+        lower_deg = [-360, -118, -225, -11, -360, -97]
+        upper_deg = [360, 118, 11, 225, 360, 180]
+    else:
+        lower_deg = [-360, -118, -225, -97, -360]
+        upper_deg = [360, 118, 11, 180, 360]
+
+    lower_rad = [math.radians(d) for d in lower_deg[:dof]]
+    upper_rad = [math.radians(d) for d in upper_deg[:dof]]
+    max_vel_rad = math.radians(180.0)
+    return JointLimits(
+        position_lower=lower_rad,
+        position_upper=upper_rad,
+        velocity_max=[max_vel_rad] * dof,
+    )


The sim module should get a Robot config that has all its limits, assets etc.

Robot shouldn't need a registry to be loaded, but should be defined in the blueprint

spomichter · 2026-01-22T21:03:38Z

dimos/simulation/engines/base.py

+    model: str | None = None
+
+
+class SimulationEngine(ABC):


spomichter · 2026-01-22T21:04:39Z

dimos/simulation/engines/base.py

+
+
+@dataclass(frozen=True)
+class RobotSpec:


Shouldn't this extend some existing general robot spec @mustafab0 ? Yeah pretty sure this should subclass, and shouldnt this take urdf @jca0

spomichter · 2026-01-22T21:10:47Z

dimos/simulation/engines/mujoco_engine.py

+            for i in range(limit):
+                self._joint_position_targets[i] = float(positions[i])
+
+    def write_joint_velocities(self, velocities: list[float]) -> None:


Needs to be changed to list of some velocity type. I assume Vector3

doesn't make sense to. Vector3 is 3D, Twist is 6D, joints can only go in 2 directions

Ideally a JointState message

Both write_joint_position & write_joint_velocities can be refactored into 1 write_joint_command/etc and take in a standard JointState message that can set Pos/Vel/Effort

spomichter · 2026-01-22T21:24:07Z

dimos/simulation/manipulators/sim_module.py

+            self._pending_positions = None
+
+    def _control_loop(self) -> None:
+        period = 1.0 / max(self._control_rate, 1.0)


The second 1.0 here is the minimum control rate so should be an env var called MIN CONTROL RATE or something

spomichter · 2026-01-22T21:24:19Z

dimos/simulation/manipulators/sim_module.py

+            time.sleep(period)
+
+    def _monitor_loop(self) -> None:
+        period = 1.0 / max(self._monitor_rate, 1.0)


Same as above here

spomichter · 2026-01-22T21:29:53Z

dimos/simulation/manipulators/sim_module.py

+        return [f"{self._joint_prefix}{i + 1}" for i in range(dof)]
+
+
+def get_blueprint() -> dict[str, Any]:


Just return blueprint how we normally do idk if we support returning blueprint via dict like this or if converting this back to .blueprint() type even works.

spomichter · 2026-01-22T21:30:39Z

dimos/simulation/manipulators/sim_module.py

+                    joints=[float(p) for p in positions],
+                )
+            )
+            time.sleep(period)


time.sleep(period) ignores work time; loop runs slower than target.

spomichter · 2026-01-22T21:31:25Z

dimos/simulation/plugins/mujoco_xarm.py

+def _xarm_limits(dof: int) -> JointLimits:
+    if dof == 7:
+        lower_deg = [-360, -118, -360, -233, -360, -97, -360]
+        upper_deg = [360, 118, 360, 11, 360, 180, 360]
+    elif dof == 6:
+        lower_deg = [-360, -118, -225, -11, -360, -97]
+        upper_deg = [360, 118, 11, 225, 360, 180]
+    else:
+        lower_deg = [-360, -118, -225, -97, -360]
+        upper_deg = [360, 118, 11, 180, 360]
+
+    lower_rad = [math.radians(d) for d in lower_deg[:dof]]
+    upper_rad = [math.radians(d) for d in upper_deg[:dof]]
+    max_vel_rad = math.radians(180.0)
+    return JointLimits(
+        position_lower=lower_rad,
+        position_upper=upper_rad,
+        velocity_max=[max_vel_rad] * dof,
+    )
+
+


spomichter · 2026-01-22T21:49:58Z

@jca0 Plus these changes add a blueprint to add_blueprints so devs can easily run, and then need an e2e test that runs everything and puts the arm through some motions, as well as a few unit tests where needed

mustafab0 · 2026-01-24T01:58:09Z

dimos/simulation/assets/xarm7/xarm7.png

Should also be in lfs

mustafab0 · 2026-01-24T02:09:22Z

dimos/simulation/manipulators/sim_module.py

 from dimos.msgs.sensor_msgs import JointCommand, JointState, RobotState
+from dimos.simulation.engines.mujoco_engine import MujocoEngine


sim_module should not have "Mujoco" references.

mustafab0 · 2026-01-24T02:50:41Z

dimos/simulation/utils/xml_parser.py

Should be in a utils folder

mustafab0 · 2026-01-24T02:52:04Z

dimos/simulation/sim_blueprints.py

    engine="mujoco",
-    robot="xarm7",
-    config_path=None,
+    config_path="dimos/simulation/assets/xarm7/scene.xml",


Hard coded paths are a No-No

please look at how the manipulation branch does it

mustafab0 · 2026-01-24T02:58:29Z

dimos/e2e_tests/test_simulation_module.py

Please see test_control_module.py in e2e tests for reference.

The e2e test should confirm an instance of Sim engine is running and it can initialize correctly, and the module can communicate on its respective streams.

mustafab0 · 2026-01-24T03:02:37Z

dimos/simulation/engines/mujoco_engine.py

+    def connect(self) -> None:
+        logger.info(f"{self.__class__.__name__}: connect()")
+        with self._lock:
+            self._connected = True
+            self._stop_event.clear()
+
+        if self._sim_thread is None or not self._sim_thread.is_alive():
+            self._sim_thread = threading.Thread(
+                target=self._sim_loop,
+                name=f"{self.__class__.__name__}Sim",
+                daemon=True,
+            )
+            self._sim_thread.start()
+
+    def disconnect(self) -> None:
+        logger.info(f"{self.__class__.__name__}: disconnect()")
+        with self._lock:
+            self._connected = False
+
+        self._stop_event.set()
+        if self._sim_thread and self._sim_thread.is_alive():
+            self._sim_thread.join(timeout=2.0)
+        self._sim_thread = None


These should return either a bool or an int to track if call was successful

mustafab0 · 2026-01-24T03:10:38Z

dimos/simulation/engines/mujoco_engine.py

+            for i in range(limit):
+                self._joint_position_targets[i] = float(positions[i])
+
+    def write_joint_velocities(self, velocities: list[float]) -> None:


Ideally a JointState message

Both write_joint_position & write_joint_velocities can be refactored into 1 write_joint_command/etc and take in a standard JointState message that can set Pos/Vel/Effort

mustafab0 · 2026-01-24T03:28:08Z

dimos/simulation/manipulators/sim_module.py

+            self.joint_state.publish(
+                JointState(
+                    frame_id=self.frame_id,
+                    name=names,
+                    position=positions,
+                    velocity=velocities,
+                    effort=efforts,
+                )
+            )


Should be part of the control loop, as joint states would be needed at the same control frequency

mustafab0 · 2026-01-24T03:34:16Z

dimos/simulation/manipulators/sim_module.py

+                    joints=[float(p) for p in positions],
+                )
+            )
+            time.sleep(period)


time.sleep(period) ignores work time; loop runs slower than target.

paul-nechifor · 2026-01-24T05:04:53Z

dimos/simulation/sim_blueprints.py

+
+xarm7_trajectory_sim = simulation(
+    engine="mujoco",
+    config_path=str(get_data("xarm7") / "scene.xml"),


Keep config_path as a Path. In MujocoEngine you're converting it back to a Path anyway.

paul-nechifor · 2026-01-24T05:12:03Z

dimos/simulation/manipulators/sim_module.py

+        if not self.config.engine:
+            raise ValueError("engine is required for SimulationModule")


If engine is required, then its type should be str, not str | None = None.

There's no need to verify it here if you disallow the class from being instantiated with an invalid config.

paul-nechifor · 2026-01-24T05:33:17Z

dimos/simulation/manipulators/sim_module.py

+        engine_key = self.config.engine.lower()
+        if engine_key not in self._ENGINES:
+            raise ValueError(f"Unknown simulation engine: {self.config.engine}")


I think it's better to use the type system.

EngineType = Literal["mujoco", "some_other"] _ENGINES: dict[EngineType, SimulationEngine] = { "mujoco": MujocoEngine, } ... # No need for None check, lower(), or key existence. engine_cls = self._ENGINES[self.config.engine]

paul-nechifor · 2026-01-24T05:42:23Z

dimos/simulation/manipulators/sim_module.py

+        except Exception as exc:
+            import logging
+
+            logging.getLogger(self.__class__.__name__).warning(


We use logger = setup_logger() for logging.

But what exceptions are you expecting here? I would expect just this to work without if and try:

self._disposables.add( Disposable( self.joint_position_command.subscribe(self._on_joint_position_command) ) )

If that doesn't work, it would be good to know why.

paul-nechifor · 2026-01-24T06:04:35Z

dimos/simulation/manipulators/sim_manip_interface.py

+        return self._error_code, self._error_message
+
+    def write_joint_positions(self, positions: list[float], velocity: float = 1.0) -> bool:
+        _ = velocity


Is this to silence the velocity parameter not being used? If so, it's better to rename it to _velocity. I assume it's required in the interface?

Oh, velocity isn't used at all. Just delete it?

paul-nechifor · 2026-01-24T06:15:55Z

dimos/simulation/manipulators/sim_manip_interface.py

+        if hasattr(self._engine, "hold_current_position"):
+            self._engine.hold_current_position()  # type: ignore[attr-defined]
+            return True


Checking if a method exists with hasattr is often a sign that the code isn't ideal.

It looks like hold_current_position exists on MujocoEngine, but not on SimulationEngine and you're using hasattr to break the abstraction.

The better way to accomplish this (adding special handling just for Mujoco) without ignoring types is to check if self._engine is an instance of MujocoEngine .

But isn't it better to just add hold_current_position to SimulationEngine?

paul-nechifor · 2026-01-24T06:35:09Z

dimos/simulation/engines/mujoco_engine.py

+        with self._lock:
+            pos_targets = list(self._joint_position_targets)
+        with self._lock:


Why two? Seems like you're releasing and acquiring unnecessary the second time.

paul-nechifor · 2026-01-24T07:04:52Z

dimos/simulation/engines/mujoco_engine.py

+            limit = min(len(positions), self._num_joints)
+            for i in range(limit):
+                self._joint_position_targets[i] = float(positions[i])


I see this pattern a lot in the code where you're copying values only up to a point.

But I think this just silently ignores errors. Is there a valid reason why write_joint_positions would be sent more positions than can be stored? I think you should let the code throw.

dimos/e2e_tests/test_simulation_module.py

Co-authored-by: Paul Nechifor <paul@nechifor.net>

jca0 added 4 commits January 15, 2026 13:45

add original files

07c18f6

rebased with dev for new controller orchestrator arch

4821255

clean up code + comments

b9adc9b

tests for testing

ac1c647

jca0 requested a review from a team January 16, 2026 01:34

greptile-apps bot reviewed Jan 16, 2026

View reviewed changes

jca0 marked this pull request as draft January 16, 2026 01:59

jca0 and others added 3 commits January 15, 2026 23:41

address race condition

ca898c5

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

fix mypy errors

8116d1f

Merge branch 'jing-sim' of github.com:dimensionalOS/dimos into jing-sim

d7fd12e

jca0 marked this pull request as ready for review January 16, 2026 07:44

greptile-apps bot reviewed Jan 16, 2026

View reviewed changes

dimos/simulation/manipulators/mujoco_manip_interface.py Outdated Show resolved Hide resolved

jca0 and others added 10 commits January 15, 2026 23:50

syntax error :(

3461348

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

CI code cleanup

f46ec17

skeleton for modular sim arch

97508ee

mujoco engine/bringup script

89f462a

manipulation interface that plugs into different sims

6073d80

simulation model that supports any sim engine

328a019

add support for xarm in sim

c153c13

clean up code

6d9e135

more cleanup

86a3c06

mypy error fix

4121640

mustafab0 requested changes Jan 22, 2026

View reviewed changes

spomichter requested changes Jan 22, 2026

View reviewed changes

jca0 added 4 commits January 22, 2026 16:51

delete joint limits

a2b66f7

remove get_blueprint() and set constant

91cd6a3

sim module takes in xml file and parses it for robot properties

bcbe1be

Track xarm7 assets with LFS

f392af0

move xarm7 arm to data folder

90c8297

mustafab0 requested changes Jan 24, 2026

View reviewed changes

jca0 added 2 commits January 23, 2026 19:03

move xml parser to utils folder

b61e4db

remove 'mujoco' from sim module

460f24a

mustafab0 requested changes Jan 24, 2026

View reviewed changes

jca0 added 2 commits January 23, 2026 20:16

no time.sleeps for clock. monotonic time instead

ca4afc9

connect() and disconnect() return bool

2d118e4

paul-nechifor reviewed Jan 24, 2026

View reviewed changes

consolidate write commands into one and fix velocity control

7fec092

paul-nechifor reviewed Jan 24, 2026

View reviewed changes

jca0 added 2 commits January 23, 2026 22:46

config_path changed to Path type and mandatory

104c4fe

engine typing

e62a249

paul-nechifor reviewed Jan 24, 2026

View reviewed changes

jca0 and others added 10 commits January 23, 2026 23:17

typing fix, no default so set kw_only=True

22d71ec

add hold_current_position() to base

f06fb46

fix locks and make sure array length is as expected

7a6662d

revise e2e tests to spin up blueprint

bc98dd3

fix sim module tests

499ede5

Merge branch 'dev' into jing-sim

ede93fc

fix mypy errors

2c52669

fetch LFS when module starts

645a5f0

Merge branch 'jing-sim' of github.com:dimensionalOS/dimos into jing-sim

6ced566

clean up

9f5f919

paul-nechifor reviewed Jan 25, 2026

View reviewed changes

dimos/e2e_tests/test_simulation_module.py Show resolved Hide resolved

Update test_simulation_module.py

2245752

Co-authored-by: Paul Nechifor <paul@nechifor.net>

		return [f"{self._joint_prefix}{i + 1}" for i in range(dof)]


		def get_blueprint() -> dict[str, Any]:

		from dimos.msgs.sensor_msgs import JointCommand, JointState, RobotState
		from dimos.simulation.engines.mujoco_engine import MujocoEngine

		if not self.config.engine:
		raise ValueError("engine is required for SimulationModule")

mujoco sim module #1035

Are you sure you want to change the base?

mujoco sim module #1035

Uh oh!

Conversation

jca0 commented Jan 16, 2026

Uh oh!

greptile-apps bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spomichter commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

greptile-apps bot commented Jan 16, 2026 •

edited

Loading

paul-nechifor Jan 24, 2026 •

edited

Loading

paul-nechifor Jan 24, 2026 •

edited

Loading