WIP Ros transport #1034
WIP Ros transport #1034
Conversation
leshy
commented
Jan 16, 2026
leshy
commented
Greptile SummaryThis PR adds ROS 2 transport support to dimos, enabling seamless interoperability between LCM and ROS messaging systems. The implementation includes automatic message conversion between Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Blueprint
participant ROSTransport
participant DimosROS
participant RawROS
participant ROS2
User->>Blueprint: build() with default_transport="ros"
Blueprint->>Blueprint: _connect_transports()
Blueprint->>Blueprint: _get_transport_for(name, type, "ros")
Blueprint->>Blueprint: _get_ros_type(lcm_type())
Note over Blueprint: Creates temp instance to get ROS type
Blueprint->>ROSTransport: new ROSTransport(topic, ros_type)
User->>ROSTransport: broadcast(msg)
ROSTransport->>ROSTransport: start()
ROSTransport->>DimosROS: new DimosROS()
DimosROS->>RawROS: __init__()
DimosROS->>RawROS: start()
RawROS->>ROS2: rclpy.init()
RawROS->>ROS2: Node(node_name)
RawROS->>ROS2: SingleThreadedExecutor()
RawROS-->>RawROS: spawn _spin thread
ROSTransport->>DimosROS: publish(topic, lcm_msg)
DimosROS->>DimosROS: encode(lcm_msg)
Note over DimosROS: LCM2ROSMixin converts to ROS msg
DimosROS->>RawROS: publish(topic, ros_msg)
RawROS->>ROS2: publisher.publish(ros_msg)
User->>ROSTransport: subscribe(callback)
ROSTransport->>DimosROS: subscribe(topic, callback)
DimosROS->>RawROS: subscribe(topic, internal_callback)
RawROS->>ROS2: create_subscription(topic, ros_callback)
ROS2-->>RawROS: message received
RawROS-->>DimosROS: ros_callback(ros_msg)
DimosROS->>DimosROS: decode(ros_msg)
Note over DimosROS: LCM2ROSMixin converts to LCM msg
DimosROS-->>User: callback(lcm_msg)
User->>ROSTransport: stop()
ROSTransport->>DimosROS: stop()
DimosROS->>RawROS: stop()
RawROS->>ROS2: rclpy.shutdown()
Note over RawROS: Shuts down ALL ROS nodes in process!
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| class ROSTransport(PubSubTransport[T]): | ||
| """Transport that publishes/subscribes via ROS 2. | ||
| Automatically converts between dimos_lcm messages and ROS messages. | ||
| Usage: | ||
| from geometry_msgs.msg import Vector3 as ROSVector3 | ||
| ROSTransport("/my_topic", ROSVector3) | ||
| """ | ||
|
|
||
| _started: bool = False | ||
| _ros: DimosROS | None = None | ||
|
|
||
| def __init__(self, topic: str, ros_type: type, **kwargs) -> None: # type: ignore[no-untyped-def] | ||
| if not ROS_AVAILABLE: | ||
| raise ImportError("ROS not available. Install rclpy to use ROSTransport.") | ||
| super().__init__(ROSTopic(topic, ros_type)) | ||
| self._kwargs = kwargs | ||
|
|
||
| def _ensure_ros(self) -> DimosROS: | ||
| if self._ros is None: | ||
| self._ros = DimosROS(**self._kwargs) | ||
| return self._ros | ||
|
|
||
| def start(self) -> None: | ||
| if not self._started: | ||
| self._ensure_ros().start() | ||
| self._started = True | ||
|
|
||
| def stop(self) -> None: | ||
| if self._ros is not None: | ||
| self._ros.stop() | ||
|
|
||
| def __reduce__(self): # type: ignore[no-untyped-def] | ||
| return (ROSTransport, (self.topic.topic, self.topic.ros_type)) | ||
|
|
||
| def broadcast(self, _, msg) -> None: # type: ignore[no-untyped-def] | ||
| """Publish a dimos_lcm message to ROS (auto-converts to ROS message).""" | ||
| self.start() | ||
| self._ensure_ros().publish(self.topic, msg) | ||
|
|
||
| def subscribe(self, callback: Callable[[T], None], selfstream: In[T] = None) -> None: # type: ignore[assignment, override] | ||
| """Subscribe to ROS topic (auto-converts ROS messages to dimos_lcm).""" | ||
| self.start() | ||
| return self._ensure_ros().subscribe(self.topic, lambda msg, topic: callback(msg)) # type: ignore[return-value] | ||
|
|
There was a problem hiding this comment.
logic: Each ROSTransport instance creates its own DimosROS node. When multiple topics use ROS transport, this creates multiple ROS nodes which is resource-intensive and can cause node name conflicts.
Consider using a singleton pattern or shared ROS node instance across all ROSTransport instances, similar to how LCM shares a single LCM instance.
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!
| def _spin(self) -> None: | ||
| """Background thread for spinning the ROS executor.""" | ||
| while self._running and self._executor: | ||
| self._executor.spin_once(timeout_sec=0) # Non-blocking for max throughput |
There was a problem hiding this comment.
style: timeout_sec=0 causes busy-waiting which consumes CPU even when idle. Consider using a small timeout (e.g., 0.01 or 0.1 seconds) to reduce CPU usage while maintaining responsiveness.
| if not rclpy.ok(): | ||
| rclpy.init() |
There was a problem hiding this comment.
logic: rclpy.init() is called without checking if it was already initialized by another instance. If multiple RawROS instances are created, this will cause an error on the second call.
| if not rclpy.ok(): | |
| rclpy.init() | |
| if not rclpy.ok(): | |
| try: | |
| rclpy.init() | |
| except RuntimeError: | |
| # Already initialized by another instance | |
| pass |
| if rclpy.ok(): | ||
| rclpy.shutdown() |
There was a problem hiding this comment.
logic: rclpy.shutdown() is called unconditionally, which will shut down rclpy for all ROS nodes in the process. If multiple RawROS instances exist, shutting down one will break the others.
Consider not calling rclpy.shutdown() here, or implementing reference counting to only shut down when the last instance stops.
|
|
||
| ros_type = _get_ros_type(type()) # Get ROS type from an instance |
There was a problem hiding this comment.
logic: Creating a temporary instance with type()() to call _get_ros_type assumes the LCM type has a no-argument constructor. This will fail for types that require constructor arguments.
Consider modifying _get_ros_type to accept a type instead of an instance, or handle the case where instantiation fails.
