refactor: internal ros communication #335
Conversation
boczekbartek
force-pushed
the
refactor/ros_communication
branch
2 times, most recently
from
3a28ad0 to
da1e490
Compare
boczekbartek
force-pushed
the
refactor/ros_communication
branch
from
fc74566 to
c7974de
Compare
boczekbartek
force-pushed
the
refactor/ros_communication
branch
from
60208a5 to
f9da367
Compare
|
@maciejmajek I responded to your comments. Could you check again? |
|
@coderabbitai full review |
WalkthroughThe pull request introduces significant architectural changes to the ROS 2 interaction framework, focusing on improving node management, action handling, and tool integration. The modifications span multiple files, with key updates in node structure, action handling, and discovery mechanisms. The changes enhance the flexibility and robustness of ROS 2 interactions, introducing new helper classes like Changes
Sequence DiagramsequenceDiagram
participant Node as RaiStateBasedLlmNode
participant ActionsHelper as Ros2ActionsHelper
participant TopicsHandler as Ros2TopicsHandler
participant ROS2 as ROS2 System
Node->>ActionsHelper: run_action()
ActionsHelper->>ROS2: Send action goal
ActionsHelper->>ActionsHelper: Wait for future result
ActionsHelper-->>Node: Return action result
Node->>TopicsHandler: get_raw_message_from_topic()
TopicsHandler->>ROS2: Subscribe to topic
ROS2-->>TopicsHandler: Receive message
TopicsHandler-->>Node: Return message
This sequence diagram illustrates the high-level interaction between the new 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/rai/rai/node.py (2)
391-394: Simplifyhas_subscriptionmethod usinganyYou can simplify the
has_subscriptionmethod by using a generator expression withany(), making the code more concise and Pythonic.Apply this diff to update the method:
-def has_subscription(self, topic: str) -> bool: - for sub in self.node._subscriptions: - if sub.topic == topic: - return True - return False +def has_subscription(self, topic: str) -> bool: + return any(sub.topic == topic for sub in self.node._subscriptions)🧰 Tools
🪛 Ruff (0.8.2)
391-394: Use
return any(sub.topic == topic for sub in self.node._subscriptions)instead offorloopReplace with
return any(sub.topic == topic for sub in self.node._subscriptions)(SIM110)
675-677: Remove unnecessaryNonecheck onlast_subscription_msgs_bufferThe attribute
self.topics_handler.last_subscription_msgs_bufferis initialized as an empty dictionary and should not beNone. The checkif self.topics_handler.last_subscription_msgs_buffer is Noneis unnecessary and can be removed to simplify the code.Apply this diff to remove the unnecessary condition:
- if self.topics_handler.last_subscription_msgs_buffer is None: - self.state_dict = state_dict - returnsrc/rai/rai/utils/ros_async.py (1)
21-37: Consider optimizing the future handling implementation.The implementation could be improved in several ways:
- Add early return when future is done to avoid unnecessary sleep.
- Make the sleep duration configurable.
- Clean up the callback on timeout.
Consider this implementation:
def get_future_result( future: rclpy.task.Future, timeout_sec: float = 5.0 ) -> Optional[Any]: """Replaces rclpy.spin_until_future_complete""" result = None + sleep_duration = 0.1 # Make this a parameter def callback(future: rclpy.task.Future) -> None: nonlocal result result = future.result() future.add_done_callback(callback) ts = time.perf_counter() while result is None and time.perf_counter() - ts < timeout_sec: + if future.done(): # Early return if future is done + break - time.sleep(0.1) + time.sleep(sleep_duration) + + if result is None: # Clean up callback on timeout + future.remove_done_callback(callback) return resultsrc/rai/rai/utils/ros_logs.py (1)
77-81: Consider adding timeout parameter for service calls.The service call is using the default timeout from get_future_result. For better control, consider exposing the timeout parameter.
- response: Optional[rai_interfaces.srv.StringList.Response] = get_future_result( - future - ) + response: Optional[rai_interfaces.srv.StringList.Response] = get_future_result( + future, + timeout_sec=self.node.get_parameter('service_timeout').value + )examples/rosbot-xl-demo.py (2)
93-93: Consider enhancing collision detection handling.The collision detection rule is good, but consider adding specific guidance on what actions to take when a collision is detected.
Add more specific rules:
- - if you detect collision, please stop operation + - if you detect collision: + 1. Immediately stop all motion by canceling running actions + 2. Publish zero velocity on /cmd_vel topic + 3. Report collision details and location + 4. Wait for operator instructions
138-138: Document the purpose of conversion_ratio parameter.The parameter is declared but its purpose and usage are not documented.
Add a comment explaining the parameter's purpose:
+ # Conversion ratio for transforming between different unit systems (e.g., meters to millimeters) node.declare_parameter("conversion_ratio", 1.0)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/rosbot-xl-demo.py(4 hunks)src/rai/rai/agents/tool_runner.py(1 hunks)src/rai/rai/node.py(17 hunks)src/rai/rai/tools/ros/native.py(1 hunks)src/rai/rai/tools/ros/native_actions.py(5 hunks)src/rai/rai/tools/ros/utils.py(3 hunks)src/rai/rai/utils/ros.py(1 hunks)src/rai/rai/utils/ros_async.py(1 hunks)src/rai/rai/utils/ros_logs.py(4 hunks)src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/rai/rai/node.py
391-394: Use return any(sub.topic == topic for sub in self.node._subscriptions) instead of for loop
Replace with return any(sub.topic == topic for sub in self.node._subscriptions)
(SIM110)
🔇 Additional comments (8)
src/rai/rai/tools/ros/utils.py (1)
161-180: LGTM: Updatedget_transformfunction implementationThe updated implementation of
get_transformenhances robustness by adding a timeout and handling asynchronous transform lookup correctly. Resource cleanup withtf_listener.unregister()is appropriately handled.src/rai/rai/utils/ros_logs.py (1)
59-61: LGTM! Good improvement in constructor signature.The addition of the callback_group parameter enhances the flexibility of ROS2 service client creation.
examples/rosbot-xl-demo.py (1)
71-71: Good addition of transform verification rule.The rule to check transforms as a first step improves safety and reliability.
src/rai/rai/agents/tool_runner.py (1)
65-65: LGTM! Enhanced logging improves debugging.Adding tool arguments to the log message provides better context for debugging tool execution.
src/rai/rai/tools/ros/native_actions.py (2)
144-144: LGTM! Method calls updated to use public interface.The changes from private methods (
_run_action,_is_task_complete,_get_task_result,_cancel_task) to their public counterparts improve the API's clarity and accessibility.Also applies to: 154-154, 164-164, 174-174
189-191: LGTM! Well-structured input model for transform operations.The new
GetTransformInputmodel with clear field descriptions and sensible defaults enhances the API's usability.src/rai/rai/tools/ros/native.py (1)
187-187: LGTM! Consistent callback group usage.The addition of
callback_groupparameter to both publisher and timer creation ensures proper callback handling within the same executor group. This is a good practice for ROS2 applications as it provides better control over execution scheduling.Also applies to: 195-197
src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py (1)
29-29: Verify compatibility with RaiBaseNode.The change from
RaiAsyncToolsNodetoRaiBaseNodesuggests a significant architectural shift. While this aligns with the refactoring goals, we should verify that all required async functionality is still available in the base node.Also applies to: 84-84
✅ Verification successful
Compatibility with RaiBaseNode is verified and safe
The change from
RaiAsyncToolsNodetoRaiBaseNodeis compatible as the tool accesses node functionality through delegation (self.node), andRaiBaseNodeprovides all necessary Node capabilities through inheritance fromrclpy.node.Node. No async-specific functionality fromRaiAsyncToolsNodeis being used.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if RaiBaseNode provides all necessary methods used in GroundingDinoBaseTool rg -A 5 "class RaiBaseNode" rg -A 5 "def (create_client|create_publisher|get_logger|get_parameter)"Length of output: 979
Script:
#!/bin/bash # Check GroundingDinoBaseTool implementation and inheritance ast-grep --pattern 'class GroundingDinoBaseTool extends RaiBaseNode { $$$ }' # Search for async patterns and method usage rg -l "async def" src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py rg "(create_client|create_publisher|get_parameter)" src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py # Check Node base class rg "from rclpy.node import Node" src/rai/rai/node.py -A 2Length of output: 684
|
@maciejmajek I have noticed that open-set detection and |
- with current method based on callbacks the default executor is good
Co-authored-by: Maciej Majek <[email protected]>
- rollback open-set detection removal - fix it by setting conversion_ratio=1.0
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
boczekbartek
force-pushed
the
refactor/ros_communication
branch
from
fff5085 to
d91204b
Compare
Purpose
Proposed Changes
rclpy.Futurefor nodes that are being span byexecutor.spin()RaiNodeand extract methods to separate classes to improve code clarityros2 actions by parsingnav2error codesIssues
Testing
Summary by CodeRabbit
New Features
Improvements
Technical Updates