refactor: tools #521
refactor: tools #521
Conversation
|
@coderabbitai full review |
WalkthroughThis pull request reorganizes the ROS-related code to favor ROS2 functionality. The changes update the import paths and remove legacy ROS modules while introducing new ROS2 utilities and tools. Specifically, a function import in the communication API is updated, and several ROS navigation, waypoint, and message utility files are removed. In addition, the ROS2 modules are enhanced by updating their export lists and incorporating new tools for manipulation and navigation, including changes to class inheritance and added helper functions for image conversion, message waiting, and transform lookup. Changes
Sequence Diagram(s)sequenceDiagram
participant Node
participant ROS2Utils
participant Converter
Node->>ROS2Utils: wait_for_message(msg_type, topic, qos_profile, time_to_wait)
ROS2Utils-->>Node: (Message Received)
Node->>ROS2Utils: convert_ros_img_to_ndarray(msg, encoding)
ROS2Utils->>Converter: Process image conversion
Converter-->>ROS2Utils: Return ndarray
ROS2Utils-->>Node: ndarray result
Node->>ROS2Utils: get_transform(target_frame, source_frame, timeout_sec)
ROS2Utils-->>Node: TransformStamped result
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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: 2
🔭 Outside diff range comments (1)
src/rai_core/rai/tools/ros2/moveit2/manipulation.py (1)
156-156:⚠️ Potential issueFix format string in format_pose method.
There's a typo in the f-string formatting - missing a '.' before '2f' for y and z coordinates, which will result in incorrect formatting.
- def format_pose(pose: Pose): - return f"Centroid(x={pose.position.x:.2f}, y={pose.position.y:2f}, z={pose.position.z:2f})" + def format_pose(pose: Pose): + return f"Centroid(x={pose.position.x:.2f}, y={pose.position.y:.2f}, z={pose.position.z:.2f})"
🧹 Nitpick comments (3)
src/rai_core/rai/tools/ros2/nav2/__init__.py (1)
1-1: Update the copyright year to match the current year.The copyright year is set to 2025, which is in the future. This should be updated to the current year (2024).
-# Copyright (C) 2025 Robotec.AI +# Copyright (C) 2024 Robotec.AIsrc/rai_core/rai/tools/ros2/utils.py (2)
65-94: Add support for additional image encodings.While the function handles common image encodings, it could be enhanced to support additional formats like 'bgra8' that might be encountered in ROS systems.
def convert_ros_img_to_ndarray( msg: sensor_msgs.msg.Image, encoding: str = "" ) -> np.ndarray: if encoding == "": encoding = msg.encoding.lower() if encoding == "rgb8": image_data = np.frombuffer(msg.data, np.uint8) image = image_data.reshape((msg.height, msg.width, 3)) elif encoding == "bgr8": image_data = np.frombuffer(msg.data, np.uint8) image = image_data.reshape((msg.height, msg.width, 3)) image = cv2.cvtColor(image, cv2.COLOR_BGR2RGB) + elif encoding == "bgra8": + image_data = np.frombuffer(msg.data, np.uint8) + image = image_data.reshape((msg.height, msg.width, 4)) + image = cv2.cvtColor(image, cv2.COLOR_BGRA2RGB) elif encoding == "rgba8": image_data = np.frombuffer(msg.data, np.uint8) image = image_data.reshape((msg.height, msg.width, 4)) image = cv2.cvtColor(image, cv2.COLOR_RGBA2RGB)🧰 Tools
🪛 GitHub Actions: build and test
[warning] 'audioop' is deprecated and slated for removal in Python 3.13
[warning] Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
97-104: Consider simplifying the type casting in the CvBridge conversion.The current implementation uses a cast to
cv2.Matwhich might be unnecessary and could be simplified.def convert_ros_img_to_cv2mat(msg: sensor_msgs.msg.Image) -> cv2.typing.MatLike: bridge = CvBridge() - cv_image = cast(cv2.Mat, bridge.imgmsg_to_cv2(msg, desired_encoding="passthrough")) # type: ignore + cv_image = bridge.imgmsg_to_cv2(msg, desired_encoding="passthrough") if cv_image.shape[-1] == 4: cv_image = cv2.cvtColor(cv_image, cv2.COLOR_BGRA2RGB) else: cv_image = cv2.cvtColor(cv_image, cv2.COLOR_BGR2RGB) return cv_image🧰 Tools
🪛 GitHub Actions: build and test
[warning] 'audioop' is deprecated and slated for removal in Python 3.13
[warning] Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/rai_core/rai/communication/ros2/api.py(1 hunks)src/rai_core/rai/tools/ros/nav2/basic_navigator.py(0 hunks)src/rai_core/rai/tools/ros/nav2/navigator.py(0 hunks)src/rai_core/rai/tools/ros/tools.py(0 hunks)src/rai_core/rai/tools/ros/utils.py(0 hunks)src/rai_core/rai/tools/ros2/__init__.py(3 hunks)src/rai_core/rai/tools/ros2/moveit2/__init__.py(2 hunks)src/rai_core/rai/tools/ros2/moveit2/manipulation.py(3 hunks)src/rai_core/rai/tools/ros2/nav2/__init__.py(2 hunks)src/rai_core/rai/tools/ros2/utils.py(2 hunks)
💤 Files with no reviewable changes (4)
- src/rai_core/rai/tools/ros/nav2/navigator.py
- src/rai_core/rai/tools/ros/nav2/basic_navigator.py
- src/rai_core/rai/tools/ros/tools.py
- src/rai_core/rai/tools/ros/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/rai_core/rai/communication/ros2/api.py (1)
src/rai_core/rai/tools/ros2/utils.py (1)
import_message_from_str(60-62)
src/rai_core/rai/tools/ros2/__init__.py (1)
src/rai_core/rai/tools/ros2/moveit2/manipulation.py (3)
GetObjectPositionsTool(137-185)MoveToPointTool(52-128)MoveToPointToolInput(42-49)
src/rai_core/rai/tools/ros2/moveit2/__init__.py (1)
src/rai_core/rai/tools/ros2/moveit2/manipulation.py (3)
GetObjectPositionsTool(137-185)MoveToPointTool(52-128)MoveToPointToolInput(42-49)
src/rai_core/rai/tools/ros2/moveit2/manipulation.py (1)
src/rai_core/rai/tools/ros2/base.py (1)
BaseROS2Tool(23-59)
src/rai_core/rai/tools/ros2/nav2/__init__.py (1)
src/rai_core/rai/tools/ros2/nav2/navigation.py (6)
CancelNavigateToPoseTool(158-167)GetNavigateToPoseFeedbackTool(138-144)GetNavigateToPoseResultTool(147-155)GetOccupancyGridTool(170-304)Nav2Toolkit(40-59)NavigateToPoseTool(69-135)
src/rai_core/rai/tools/ros2/utils.py (3)
src/rai_core/rai/communication/ros2/connectors/ari_connector.py (1)
node(225-226)tests/communication/test_hri_message.py (1)
image(26-28)src/rai_core/rai/communication/sound_device/api.py (1)
wait(226-239)
🪛 GitHub Actions: build and test
src/rai_core/rai/tools/ros2/moveit2/manipulation.py
[warning] 37-37: rai_open_set_vision is not installed, GetGrabbingPointTool will not work
src/rai_core/rai/tools/ros2/utils.py
[warning] 'audioop' is deprecated and slated for removal in Python 3.13
[warning] Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
🪛 Ruff (0.8.2)
src/rai_core/rai/tools/ros2/utils.py
165-166: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
169-170: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (14)
src/rai_core/rai/communication/ros2/api.py (1)
75-75: LGTM: Updated import path to use ros2 module.This change aligns with the PR's objective of providing a cleaner structure by updating the import path from
rai.tools.ros.utilstorai.tools.ros2.utils. Theimport_message_from_strfunction has been moved to a more appropriate location.src/rai_core/rai/tools/ros2/__init__.py (2)
32-36: LGTM: Added imports for manipulation tools from moveit2.This change correctly imports new manipulation tools from the moveit2 module, making them available at the ros2 package level.
55-55: LGTM: Updated all list to include new tools.The all list has been properly updated to include the newly imported manipulation tools, ensuring they are exposed in the public API.
Also applies to: 64-65
src/rai_core/rai/tools/ros2/moveit2/__init__.py (3)
15-15: LGTM: Updated imports to use manipulation module.This change correctly imports the manipulation tools from the local manipulation module.
17-21: LGTM: Updated all list to reflect new structure.The all list has been properly updated to include only the tools now available from the manipulation module.
1-1:⚠️ Potential issueFix incorrect copyright year.
The copyright year is set to 2025, which is in the future.
-# Copyright (C) 2025 Robotec.AI +# Copyright (C) 2024 Robotec.AILikely an incorrect or invalid review comment.
src/rai_core/rai/tools/ros2/moveit2/manipulation.py (4)
23-23: LGTM: Updated base class import.Changed the import to use the ROS2-specific base class, which is more appropriate for these tools.
52-52: LGTM: Changed MoveToPointTool to inherit from BaseROS2Tool.This is a good refactoring decision as it uses a more specific ROS2 base class that provides appropriate ROS2 functionality.
137-137: LGTM: Changed GetObjectPositionsTool to inherit from BaseROS2Tool.This is a good refactoring decision as it uses a more specific ROS2 base class that provides appropriate ROS2 functionality.
34-39:❓ Verification inconclusive
Verify that GetGrabbingPointTool is properly installed in production.
The pipeline shows a warning that
rai_open_set_visionis not installed, which meansGetGrabbingPointToolwill not work. This might impactGetObjectPositionsToolfunctionality since it depends onget_grabbing_point_tool.
🏁 Script executed:
#!/bin/bash # Check if the GetObjectPositionsTool is used anywhere in the codebase echo "Checking for usage of GetObjectPositionsTool..." rg -A 2 "GetObjectPositionsTool" # Check if rai_open_set_vision is mentioned in any setup or requirements files echo "Checking for rai_open_set_vision in requirements..." rg "rai_open_set_vision" --type=txt --type=pythonLength of output: 6355
🏁 Script executed:
#!/bin/bash echo "Searching for rai_open_set_vision in Python files..." rg "rai_open_set_vision" -g "*.py"Length of output: 2320
Action Required: Confirm Dependency Installation for Production
The current implementation in
src/rai_core/rai/tools/ros2/moveit2/manipulation.pywarns whenrai_open_set_visionis absent, which disablesGetGrabbingPointTool. SinceGetObjectPositionsTooldepends on the functionality provided byGetGrabbingPointTool, it's essential that the production environment includes therai_open_set_visionpackage. Please verify that:
- Production Dependencies: The
rai_open_set_visionpackage is either installed or properly integrated into the production setup.- Fallback Handling: If the package remains optional, ensure that any functionality depending on
GetGrabbingPointToolwill gracefully handle its absence without disrupting critical operations.- Documentation & Tests: Update documentation and CI tests to reflect this dependency, especially given its usage in demo scripts (e.g., in
examples/rosbot-xl-demo.pyandexamples/manipulation-demo.py).🧰 Tools
🪛 GitHub Actions: build and test
[warning] 37-37: rai_open_set_vision is not installed, GetGrabbingPointTool will not work
src/rai_core/rai/tools/ros2/nav2/__init__.py (2)
15-22: Looks good - clean import structure.The imports from the navigation module are properly organized and provide a clear interface to the navigation tools.
24-31: Excellent use of__all__to define public API.Explicitly declaring the exported symbols with
__all__improves encapsulation and makes it clear to users which tools are intended for public use. This is a good practice that enhances the module's interface.src/rai_core/rai/tools/ros2/utils.py (2)
60-62: Looks good - clean utility function for message type importing.This utility function provides a convenient way to import ROS2 message types from string representations.
🧰 Tools
🪛 GitHub Actions: build and test
[warning] 'audioop' is deprecated and slated for removal in Python 3.13
[warning] Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
127-179:✅ Verification successful
Simplify nested if statements and address pending TODO.
The code has a TODO comment about an unresolved GitHub issue. Also, according to static analysis hints, the nested if statements could be simplified.
# Copied from https://github.com/ros2/rclpy/blob/jazzy/rclpy/rclpy/wait_for_message.py, to support humble def wait_for_message( msg_type: Type[object], node: "Node", topic: str, *, qos_profile: Union[QoSProfile, int] = 1, time_to_wait: float = -1, ) -> tuple[bool, Optional[object]]: # ... try: # ... - if guards_ready: - if sigint_gc.handle.pointer in guards_ready: - return False, None + if guards_ready and sigint_gc.handle.pointer in guards_ready: + return False, None - if subs_ready: - if sub.handle.pointer in subs_ready: - msg_info = sub.handle.take_message(sub.msg_type, sub.raw) - if msg_info is not None: - return True, msg_info[0] + if subs_ready and sub.handle.pointer in subs_ready: + msg_info = sub.handle.take_message(sub.msg_type, sub.raw) + if msg_info is not None: + return True, msg_info[0] finally: # TODO(boczekbartek): uncomment when rclpy resolves: https://github.com/ros2/rclpy/issues/1142 # node.destroy_subscription(sub) pass
🏁 Script executed:
#!/bin/bash # Check if the GitHub issue has been resolved gh issue view 1142 --repo ros2/rclpy --json state,title,url | jq '.'Length of output: 249
Simplify nested if statements and defer cleanup until upstream is fixed.
The nested if conditions have been simplified as suggested:
- Instead of separate
ifstatements, combine conditions for guard and subscription checks.- The
node.destroy_subscription(sub)call remains commented out because the related upstream issue (ros2/rclpy#1142) is still open. Once that issue is resolved, please revisit and uncomment the subscription destruction.No further changes are needed at this time.
🧰 Tools
🪛 Ruff (0.8.2)
165-166: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
169-170: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🪛 GitHub Actions: build and test
[warning] 'audioop' is deprecated and slated for removal in Python 3.13
[warning] Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
maciejmajek
force-pushed
the
style/tools
branch
2 times, most recently
from
27f6178 to
43b31a0
Compare
maciejmajek
force-pushed
the
style/tools
branch
from
43b31a0 to
dd94045
Compare
maciejmajek
force-pushed
the
style/tools
branch
from
6e1ab33 to
ad7a2c8
Compare
refactor: update __init__ imports to better reflect public api
…se class for manipulation tools refactor: move nav2 tools into nav2 catalog, rename to navigation
chore: remove ros2 tools utils
maciejmajek
force-pushed
the
style/tools
branch
from
cb62e06 to
d2493f7
Compare
Merge after #520
Purpose
To provide a clean experience for the developers.
Proposed Changes
Issues
Testing
Summary by CodeRabbit
New Features
Refactor