feat: unified ROS2 hri message #416
Conversation
|
@coderabbitai full review |
WalkthroughThis pull request introduces several modifications to enhance the ROS2 communication framework. It changes the default message type in the Changes
Sequence Diagram(s)sequenceDiagram
participant Pub as HRIMessagePublisher
participant Conn as ROS2HRIConnector
participant Msg as ROS2HRIMessage
participant Sub as HRIMessageSubscriber
Pub->>Conn: Publish HRI message payload
Conn->>Msg: Convert message using to_ros2_dict()
Msg-->>Conn: Return ROS2 message dict
Conn->>Sub: Transmit message over ROS2 network
Sub->>Msg: Process message using from_ros2()
Msg-->>Sub: Return structured HRI message
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/rai_interfaces/msg/AudioMessage.msg (1)
1-19: Confirm channel type.
Usingint16for channels is unusual, since the channel count is generally non-negative. If only positive values are expected, consider usinguint16. Otherwise, this is fine as-is.tests/communication/ros2/helpers.py (2)
38-52: Consider adding error handling for image conversion.The
publish_messagemethod assumes image conversion will always succeed. Consider adding try-catch blocks for potential CvBridge errors.def publish_message(self) -> None: msg = HRIMessage() - image = self.cv_bridge.cv2_to_imgmsg(np.zeros((100, 100, 3), dtype=np.uint8)) - msg.images = [image] + try: + image = self.cv_bridge.cv2_to_imgmsg(np.zeros((100, 100, 3), dtype=np.uint8)) + msg.images = [image] + except Exception as e: + self.get_logger().error(f"Failed to convert image: {e}") + return msg.audios = [AudioSegment.silent(duration=1000)] msg.text = "Hello, HRI!" self.publisher.publish(msg)
54-64: Add cleanup for subscription in HRIMessageSubscriber.The class should implement proper cleanup to prevent resource leaks.
class HRIMessageSubscriber(Node): def __init__(self, topic: str): super().__init__("test_hri_message_subscriber") self.subscription = self.create_subscription( HRIMessage, topic, self.handle_test_message, 10 ) self.received_messages: List[HRIMessage] = [] def handle_test_message(self, msg: HRIMessage) -> None: self.received_messages.append(msg) + + def destroy(self) -> None: + """Cleanup subscription when the node is destroyed.""" + self.destroy_subscription(self.subscription) + super().destroy_node()tests/communication/ros2/test_connectors.py (2)
176-214: Consider adding more test cases and improving assertions.While the test covers basic functionality, consider:
- Testing edge cases (empty images/audio)
- Testing invalid message author
- Using more descriptive assertion messages
def test_ros2hri_default_message_publish( ros_setup: None, request: pytest.FixtureRequest ): topic_name = f"{request.node.originalname}_topic" # type: ignore connector = ROS2HRIConnector(targets=[topic_name]) hri_message_receiver = HRIMessageSubscriber(topic_name) executors, threads = multi_threaded_spinner([hri_message_receiver]) try: images = [Image.new("RGB", (100, 100), color="red")] audios = [AudioSegment.silent(duration=1000)] text = "Hello, HRI!" payload = HRIPayload(images=images, audios=audios, text=text) message = ROS2HRIMessage(payload=payload, message_author="ai") connector.send_message(message, target=topic_name) time.sleep(1) # wait for the message to be received - assert len(hri_message_receiver.received_messages) > 0 + assert len(hri_message_receiver.received_messages) > 0, "No messages received" recreated_message = ROS2HRIMessage.from_ros2( hri_message_receiver.received_messages[0], message_author="ai" ) - assert message.text == recreated_message.text - assert message.message_author == recreated_message.message_author - assert len(message.images) == len(recreated_message.images) - assert len(message.audios) == len(recreated_message.audios) + assert message.text == recreated_message.text, "Text mismatch" + assert message.message_author == recreated_message.message_author, "Author mismatch" + assert len(message.images) == len(recreated_message.images), "Image count mismatch" + assert len(message.audios) == len(recreated_message.audios), "Audio count mismatch"
191-191: Replace fixed sleep with dynamic waiting.Instead of using a fixed
time.sleep(1), consider implementing a wait_for_messages helper that polls with a timeout.- time.sleep(1) # wait for the message to be received + def wait_for_messages(timeout: float = 1.0) -> bool: + start_time = time.time() + while time.time() - start_time < timeout: + if len(hri_message_receiver.received_messages) > 0: + return True + time.sleep(0.1) + return False + + assert wait_for_messages(), f"No messages received within {timeout} seconds"src/rai_interfaces/CMakeLists.txt (1)
24-25: Approved: New Message Definitions AddedThe addition of
"msg/AudioMessage.msg"and"msg/HRIMessage.msg"to therosidl_generate_interfacescall successfully expands the project’s ROS2 interface definitions in a way that aligns with the PR objectives. Please ensure that the corresponding message files exist in thesrc/rai_interfaces/msg/directory and that they conform to the correct ROS2 message syntax. It would also be useful to verify that any downstream dependencies (e.g., in the connector or topic configuration) have been appropriately updated for these new message types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/rai_core/rai/communication/ros2/api.py(1 hunks)src/rai_core/rai/communication/ros2/connectors.py(6 hunks)src/rai_interfaces/CMakeLists.txt(1 hunks)src/rai_interfaces/msg/AudioMessage.msg(1 hunks)src/rai_interfaces/msg/HRIMessage.msg(1 hunks)tests/communication/ros2/helpers.py(2 hunks)tests/communication/ros2/test_api.py(3 hunks)tests/communication/ros2/test_connectors.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/rai_core/rai/communication/ros2/connectors.py
273-273: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
274-274: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test-ros2 (humble)
🔇 Additional comments (8)
src/rai_core/rai/communication/ros2/connectors.py (5)
18-20: Imports look good.
No immediate concerns with the new dependencies. The addition of NumPy, PIL, and pydub appears consistent with the new image/audio handling functionality.Also applies to: 25-27, 32-32, 49-52
213-240: Verify audio sample width.
Currently,sample_width=2is hard-coded as 16-bit audio (int16). Make sure this is always correct for all incomingAudioMessages. If not, consider dynamically determining the sample width or enforcing a specific audio format in the interface.
319-322: Sending updated HRI messages looks correct.
Usingmessage.to_ros2_dict()for publishing ensures consistency with the new ROS2 message structure.
340-340: Receive message logic is clean.
Converting the raw ROS2 message intoROS2HRIMessageviafrom_ros2is consistent with the new interface.
367-372: Shutdown sequence looks good.
Graceful termination of the executor, thread, and APIs is correct and ensures clean resource release.src/rai_interfaces/msg/HRIMessage.msg (1)
1-20: Overall structure looks good.
This message fields layout comprehensively covers text, images, and audio for HRI use cases.tests/communication/ros2/test_api.py (1)
74-74: LGTM! Clean transition to keyword arguments.The changes consistently update TopicConfig instantiation to use keyword arguments, improving code clarity.
Also applies to: 89-90, 111-112
tests/communication/ros2/test_connectors.py (1)
190-190: Consider validating message_author.Add validation for the message_author parameter to ensure it's not empty or invalid.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if message_author validation exists rg "message_author.*validation" --type pyLength of output: 43
Action Required: Add Validation for the message_author Parameter
Our search did not reveal any validation logic formessage_authorin the codebase. In particular, the command to look for patterns like"message_author.*validation"produced no output. This confirms that the current implementation lacks a check to ensure that themessage_authorparameter is neither empty nor invalid before sending messages. Please consider adding the necessary validation logic to the relevant functions (e.g., within the connector’ssend_messageimplementation) to avoid potenti 8000 al issues at runtime.
Docstrings generation was requested by @rachwalk. * #416 (comment) The following files were modified: * `src/rai_core/rai/communication/ros2/connectors.py` * `tests/communication/ros2/helpers.py` * `tests/communication/ros2/test_api.py` * `tests/communication/ros2/test_connectors.py`
|
Note Generated docstrings for this pull request at #417 |
There was a problem hiding this comment.
Thank you for adding the much needed HRIMessages to rai_interfaces, few changes regarding type hints. I made docstrings using code rabbit https://github.com/RobotecAI/rai/pull/417/files please review them, and change them where applicable regarding the requested changes. I think also that coderabbit might be generating the docs overeagerly with docstrings
| @classmethod | ||
| def from_ros2(cls, msg: Any, message_author: Literal["ai", "human"]): | ||
| from rai_interfaces.msg import HRIMessage as ROS2HRIMessage_ | ||
|
|
||
| if not isinstance(msg, ROS2HRIMessage_): | ||
| raise ValueError( | ||
| "ROS2HRIMessage can only be created from rai_interfaces/msg/HRIMessage" | ||
| ) |
There was a problem hiding this comment.
Why is this not a top level import? If it can be it should be, and the type annotations should utilise it.
There was a problem hiding this comment.
8e26366
I think it's ok now
| def to_ros2_dict(self): | ||
| cv_bridge = CvBridge() |
There was a problem hiding this comment.
Add return type annotation, since it's enforeced to be OrderedDict (alternatively consider a cast to TypedDict, since in this case it's guaranteed to succeed, and will provide additional information to API user).
There was a problem hiding this comment.
The message definition is considered "floating" (python code is built from .msg file during colcon build), therefore it would be hard to cast it to TypedDict. I will update the return type
Further work: #418 refactor(partial): refactor openset tools to rai2.0 ros2ariconnector api refactor: finalize with simplifications fix: missing argument, missing client workaround: don't remove subscription due to rclpy issue ros2/rclpy#1142 replace ros2 future waiting with more robust callback based mechanism remove waiting from launch fix order in connector shutdown ``` Exception in thread Thread-1 (spin): Traceback (most recent call last): File "/usr/lib/python3.10/threading.py", line 1016, in _bootstrap_inner self.run() File "/usr/lib/python3.10/threading.py", line 953, in run self._target(*self._args, **self._kwargs) File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 294, in spin self.spin_once() File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 794, in spin_once self._spin_once_impl(timeout_sec) File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 786, in _spin_once_impl self._executor.submit(handler) File "/usr/lib/python3.10/concurrent/futures/thread.py", line 167, in submit raise RuntimeError('cannot schedule new futures after shutdown') RuntimeError: cannot schedule new futures after shutdown ``` refactor: cleanup manipulation launchfile chore: pre-commit feat: benchmark class models Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: naming changes Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: naming changes Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: redefine benchmark model Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> Co-authored-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> refactor: SceneConfig to BaseModel class feat: add SceneSetup to store initial scene setup build: poetry initialization of rai_benchmarks and rai_simulations chore: add licence lines build: create packages from rai_benchmarks and rai_simulations chore: removed mistakenly added file feat: scene config implementation Add O3DEEngineConnector features Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Remove an unused file Signed-off-by: Kacper 6D47 Dąbrowski <kacper.dabrowski@robotec.ai> Add binary path caching Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Add two example scenes Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> feat: replace binary path with ros2 launch command + binary path refactor: renamed rai_sim and rai_bench fix: fixed shutdown of binary chore: make pose mandatory chore: remove rai_bench because it is developed on another branch ci: add missing license lines laoding and spawning frm benchmark rabsed naming change grab xyz benchmark benchmarks run new sim every scenario for now adjust to new tools Remove psutil conflicting __del__ method from o3de connector See this issue: giampaolo/psutil#2437 stream langgraph agent extent output of tool runner enable all scenarios in the benchmark removed name providing for manipulator launch added move arm func to connectors
Further work: #418 refactor(partial): refactor openset tools to rai2.0 ros2ariconnector api refactor: finalize with simplifications fix: missing argument, missing client workaround: don't remove subscription due to rclpy issue ros2/rclpy#1142 replace ros2 future waiting with more robust callback based mechanism remove waiting from launch fix order in connector shutdown ``` Exception in thread Thread-1 (spin): Traceback (most recent call last): File "/usr/lib/python3.10/threading.py", line 1016, in _bootstrap_inner self.run() File "/usr/lib/python3.10/threading.py", line 953, in run self._target(*self._args, **self._kwargs) File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 294, in spin self.spin_once() File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 794, in spin_once self._spin_once_impl(timeout_sec) File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 786, in _spin_once_impl self._executor.submit(handler) File "/usr/lib/python3.10/concurrent/futures/thread.py", line 167, in submit raise RuntimeError('cannot schedule new futures after shutdown') RuntimeError: cannot schedule new futures after shutdown ``` refactor: cleanup manipulation launchfile chore: pre-commit feat: benchmark class models Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: naming changes Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: naming changes Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> chore: redefine benchmark model Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> Co-authored-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> refactor: SceneConfig to BaseModel class feat: add SceneSetup to store initial scene setup build: poetry initialization of rai_benchmarks and rai_simulations chore: add licence lines build: create packages from rai_benchmarks and rai_simulations chore: removed mistakenly added file feat: scene config implementation Add O3DEEngineConnector features Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Remove an unused file Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Add binary path caching Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Add two example scenes Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> feat: replace binary path with ros2 launch command + binary path refactor: renamed rai_sim and rai_bench fix: fixed shutdown of binary chore: make pose mandatory chore: remove rai_bench because it is developed on another branch ci: add missing license lines laoding and spawning frm benchmark rabsed naming change grab xyz benchmark benchmarks run new sim every scenario for now adjust to new tools Remove psutil conflicting __del__ method from o3de connector See this issue: giampaolo/psutil#2437 stream langgraph agent extent output of tool runner enable all scenarios in the benchmark removed name providing for manipulator launch added move arm func to connectors rebased and adjusted to new code added metrics to tasks, created rai_bench package added comments
Purpose
HRIMessage is used for human-robot interaction. In the current implementation, a simple String message was used instead which is very limiting.
Proposed Changes
Testing
Summary by CodeRabbit