Skip to content

Conversation

@alexlin2
Copy link
Contributor

@alexlin2 alexlin2 commented Feb 10, 2026

  • Refactored ROSNav module to use ROSTransport instead of direct rclpy
    publishers/subscribers, removing all ROS message imports and the internal ROS node
  • Removed from_ros_msg / to_ros_msg methods and ROS message imports from all 18 message types
    under dimos/msgs/
  • Removed ROS conversion tests from all 14 test files under dimos/msgs/

DIM-449

Closes DIM-449

@greptile-apps
Copy link

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Greptile Summary

This PR successfully removes all ROS message dependencies from DimOS message types and refactors the ROSNav module to use the centralized ROSTransport infrastructure. The changes eliminate ~3,600 lines of code by:

  • Removing to_ros_msg() and from_ros_msg() methods from 18 message types across geometry_msgs, nav_msgs, sensor_msgs, std_msgs, and tf2_msgs
  • Removing ROS message imports from all message type files
  • Deleting the legacy ROSBridge class and its 442-line test suite (functionality now provided by ROSTransport)
  • Refactoring ROSNav to use declarative ROS In/Out ports with ROSTransport instead of direct rclpy publishers/subscribers
  • Removing ROS conversion tests from 14 test files (94 removed calls to to_ros_msg/from_ros_msg)

The refactoring properly leverages the existing DimosROS pubsub implementation (in dimos/protocol/pubsub/impl/rospubsub.py) which handles automatic conversion between DimOS and ROS message formats using the centralized conversion logic in rospubsub_conversion.py. This architectural improvement centralizes ROS-specific code and makes message types transport-agnostic.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - well-structured refactoring that removes legacy code
  • Score reflects a large-scale architectural refactoring that removes 3,600 lines of code and centralizes ROS message conversion logic. The changes are systematic and follow a clear pattern across all affected message types. The confidence score is 4 (not 5) because: (1) the PR removes all ROS-specific tests without replacement tests for the new ROSTransport-based approach, and (2) this is a significant architectural change affecting core navigation functionality that warrants thorough integration testing to verify the new ROSTransport properly handles all message conversion edge cases previously tested.
  • Pay close attention to dimos/navigation/rosnav.py - ensure the new ROSTransport integration is tested with actual ROS navigation stack

Important Files Changed

Filename Overview
dimos/navigation/rosnav.py Refactored to use ROSTransport instead of direct rclpy node, removing ROS message imports and conversion calls. New ports added for ROS communication via ROSTransport.
dimos/robot/ros_bridge.py File deleted - functionality replaced by ROSTransport infrastructure
dimos/navigation/demo_ros_navigation.py Updated to use new rosnav.deploy() function instead of manual setup, removed rclpy imports and shutdown calls
dimos/msgs/geometry_msgs/Twist.py Removed ROS message imports and to_ros_msg/from_ros_msg conversion methods
dimos/msgs/sensor_msgs/PointCloud2.py Removed 200+ lines of ROS conversion code including complex PointCloud2 conversion logic
dimos/msgs/std_msgs/Bool.py Removed ROS message imports and conversion methods

Sequence Diagram

sequenceDiagram
    participant ROS as ROS Navigation Stack
    participant RT as ROSTransport
    participant RN as ROSNav Module
    participant LCM as LCM Transport
    participant App as Application

    Note over ROS,App: Goal Setting Flow
    App->>LCM: goal_req (PoseStamped)
    LCM->>RN: goal_req.subscribe()
    RN->>RN: _on_goal_pose()
    RN->>RT: ros_goal_pose.publish()
    RT->>ROS: /goal_pose (converts DimOS→ROS)

    Note over ROS,App: Navigation Updates Flow
    ROS->>RT: /cmd_vel (TwistStamped)
    RT->>RN: ros_cmd_vel (converts ROS→DimOS)
    RN->>LCM: cmd_vel.publish(Twist)
    LCM->>App: cmd_vel updates

    Note over ROS,App: Point Cloud Data Flow
    ROS->>RT: /registered_scan (PointCloud2)
    RT->>RN: ros_registered_scan
    RN->>RN: RxPY sampling
    RN->>LCM: pointcloud.publish()
    LCM->>App: Point cloud data

    Note over ROS,App: Goal Reached Flow
    ROS->>RT: /goal_reached (Bool)
    RT->>RN: ros_goal_reached
    RN->>RN: Update navigation state
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

38 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@paul-nechifor
Copy link
Contributor

@alexlin2 You have to say "Closes DIM-449" in the description to link it in Linear.

@leshy
Copy link
Contributor

leshy commented Feb 11, 2026

So great to clean up ros in general, but do we need rosnav.py at all? can't blueprints directly subscribe to ros messages? we can just use ros transport for ros topics and speak to ros directly? (can do in a follow up)

The idea behind ros transports is that we don't need bridge-like modules at all (actually not sure @paul-nechifor if we'd need some spec level "module" that maybe does nothing? just specifies ros topics available, so that autoconnect knows what's available - then this same module can run dockerized ros in the future?)

can merge but good convo to sort out

@leshy leshy merged commit 381f0f4 into dev Feb 11, 2026
15 checks passed
@alexlin2
Copy link
Contributor Author

alexlin2 commented Feb 11, 2026

So great to clean up ros in general, but do we need rosnav.py at all? can't blueprints directly subscribe to ros messages? we can just use ros transport for ros topics and speak to ros directly? (can do in a follow up)

The idea behind ros transports is that we don't need bridge-like modules at all (actually not sure @paul-nechifor if we'd need some spec level "module" that maybe does nothing? just specifies ros topics available, so that autoconnect knows what's available - then this same module can run dockerized ros in the future?)

can merge but good convo to sort out

So I looked at it and I wanted to keep the same specs as before so that it works exactly the same as the go2 nav interface. Also we need to keep like the navigation state as well as sending the joystick commands to put the robot into autonomy mode and softstop mode, these will still need to be in a module since the commanding nodes should not need to know about the joystick messages or the softstop for the cancel_navigation.

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