refactor: wait for message #447
Conversation
refactor: use custom wait_for_message method in ros2 topic api instead of flawed wait_for_message from rclpy
maciejmajek
force-pushed
the
refactor/wait_for_message
branch
from
0aeba73 to
d81fff0
Compare
|
@coderabbitai full review |
|
Error: Could not generate a valid Mermaid diagram after multiple attempts. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rai_core/rai/communication/ros2/api.py (1)
294-302: Consider improving topic name normalization.The topic availability check strips all forward slashes from topic names when comparing. This could potentially cause issues with complex topic hierarchies.
- topic = topic.replace("/", "") - if topic in [te[0].replace("/", "") for te in topic_endpoints]: + # Normalize topic names by ensuring they start with a single '/' + normalized_topic = '/' + topic.strip('/') + if normalized_topic in [te[0] for te in topic_endpoints] or topic in [te[0] for te in topic_endpoints]:This would properly normalize topic names while maintaining compatibility with both forms (with and without leading slash).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/rai_core/rai/communication/ros2/api.py(3 hunks)tests/communication/ros2/test_api.py(0 hunks)tests/communication/ros2/test_connectors.py(0 hunks)tests/messages/test_transport.py(2 hunks)tests/tools/ros2/test_topic_tools.py(0 hunks)
💤 Files with no reviewable changes (3)
- tests/communication/ros2/test_connectors.py
- tests/tools/ros2/test_topic_tools.py
- tests/communication/ros2/test_api.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test-ros2 (humble)
🔇 Additional comments (7)
tests/messages/test_transport.py (4)
23-23: Good import addition.The import
ROS2ARIConnectorfromrai.communication.ros2.connectorsaligns with the PR objective to refactor message handling.
99-101: Improved implementation using ROS2ARIConnector.The connector approach is a better design choice than the previous
RaiBaseNodeimplementation, providing a cleaner API for message reception and management.
106-106: Simplified message reception API.Replacing
get_raw_message_from_topicwithreceive_messageprovides a more intuitive interface. This change aligns with the PR objective to create a more stable implementation.
109-109: Proper resource cleanup.Adding
connector.shutdown()ensures that resources are properly cleaned up, which is important for preventing resource leaks in tests.src/rai_core/rai/communication/ros2/api.py (3)
209-209: Good addition of retry mechanism.Adding the
retry_countparameter allows for more robust message reception, especially in environments where message delivery might be inconsistent or delayed.
245-263: Well-implemented topic verification and retry logic.The implementation now properly verifies topic availability before attempting to receive messages, and uses a retry mechanism to improve reliability. This addresses the PR objective to create a more stable implementation.
One minor concern: the timeout for each retry attempt is divided by the retry count, which means individual attempts get shorter timeouts as retry_count increases. This is a reasonable trade-off to maintain the overall timeout budget.
265-293: Well-implemented message waiting mechanism.The
_wait_for_messagemethod is a clean implementation that:
- Creates a subscription with a proper callback
- Polls for messages within the timeout period
- Properly cleans up the subscription before returning
This custom implementation provides better control over the message waiting process compared to relying on external libraries.
There was a problem hiding this comment.
NOTE:
rai git:(refactor/wait_for_message) ✗ rg wait_for_message
src/rai_core/rai/communication/ros2/api.py
251: success, msg = self._wait_for_message(
265: def _wait_for_message(
src/rai_core/rai/tools/utils.py
46:# Copied from https://github.com/ros2/rclpy/blob/jazzy/rclpy/rclpy/wait_for_message.py, to support humble
47:def wait_for_message(
213: success, msg = wait_for_message(
src/rai_core/rai/tools/ros/deprecated.py
34:from rai.tools.ros.utils import wait_for_message
68: success, msg = wait_for_message(
src/rai_core/rai/tools/ros/utils.py
106:# Copied from https://github.com/ros2/rclpy/blob/jazzy/rclpy/rclpy/wait_for_message.py, to support humble
107:def wait_for_message(I can see that wait_for_message is only used in deprecated.py now.
Then deprecated.SingleMessageGrabber used only by GetOccupanceGridTool, which is not used anywhere.
This PR is a good enabler to to clean up the deprecated code. But I'm fine with doing it in another PR.
Purpose
The 2.0 architecture lacked topic discovery mechanism making synthetic tests harder.
Proposed Changes
New, internal implementation of wait_for_message which seems more stable than the rclpy version.
Testing
CI/pytest
Summary by CodeRabbit
New Features
Refactor
Tests