Manipulation demo: Update launching steps #332
Conversation
|
@CodeRabbit full review |
0d7c290 to
3e3495d
Compare
WalkthroughThe pull request introduces a new repository entry for a manipulation demo and modifies related documentation and code files. It adds the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🔇 Additional comments (2)demos.repos (1)
Adding the Run the following script to confirm repository accessibility: ✅ Verification successfulRepository and branch configuration is valid The repository URL is accessible and the 'development' branch exists, confirming the configuration is correct. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if the repository URL is correct and the 'development' branch exists.
# Test: Clone the repository's 'development' branch. Expect: Successful clone.
git ls-remote https://github.com/RobotecAI/rai-manipulation-demo.git &> /dev/null
if [ $? -ne 0 ]; then
echo "Repository URL is invalid or inaccessible."
else
BRANCH_EXISTS=$(git ls-remote --heads https://github.com/RobotecAI/rai-manipulation-demo.git development)
if [ -z "$BRANCH_EXISTS" ]; then
echo "Branch 'development' does not exist in the repository."
else
echo "Repository and branch are valid."
fi
fi
Length of output: 416 src/rai/rai/tools/ros/manipulation.py (1)
Reconsider changing calibration defaults to zero Setting Confirm if zeroing these calibration values is intentional and won't negatively impact the system's operation. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
examples/manipulation-demo.launch.py (1)
16-20: Organize imports according to PEP 8 guidelinesFor better readability and maintenance, consider grouping standard library imports, third-party imports, and local imports separately.
Apply this diff to reorganize imports:
from launch import LaunchDescription from launch.actions import ( DeclareLaunchArgument, ExecuteProcess, IncludeLaunchDescription, ) +from launch.event_handlers import OnProcessExit +from launch.actions import RegisterEventHandler from launch.launch_description_sources import PythonLaunchDescriptionSource from launch.substitutions import LaunchConfiguration from launch_ros.actions import Node, RosTimer from launch_ros.substitutions import FindPackageShareexamples/manipulation-demo.py (1)
26-26: Parameterconversion_ratiois declared but not utilizedThe new parameter
conversion_ratiois declared but not used anywhere in the code. This may lead to confusion or maintenance issues.Consider using the parameter or removing it if it's not needed.
node = RaiBaseNode(node_name="manipulation_demo") -node.declare_parameter("conversion_ratio", 1.0)docs/demos/manipulation.md (3)
12-14: Update instruction formatting for consistencyEnsure that the shell commands are properly formatted and indented for better readability.
Apply this diff to adjust the code block indentation:
2. Download additional dependencies: ```shell poetry install --with openset vcs import < demos.repos ```
47-51: Clarify the path to the game launcher executableIn the command to start the demo, specify how to find or set the
path/to/RAIManipulationDemo.GameLauncherto prevent confusion.Provide guidance on obtaining the correct path:
ros2 launch examples/manipulation-demo.launch.py game_launcher:=/absolute/path/to/RAIManipulationDemo.GameLauncherAdd a note:
> **Note**: Replace `/absolute/path/to/RAIManipulationDemo.GameLauncher` with the actual path to your downloaded game launcher executable.
52-59: Ensure users source the setup script before running the interactive promptRemind users to source the environment in the second terminal as well to avoid runtime issues.
Add the following note before step 2:
> **Note**: In the second terminal, make sure to run `source setup_shell.sh` before executing the next command.src/rai/rai/tools/ros/manipulation.py (2)
Line range hint
83-85: Updatetaskparameter options for clarityIn the
_runmethod, thetaskparameter options have changed from"grab"and"place"to"grab"and"drop". Ensure that this change is reflected consistently throughout the codebase and documentation.Update the method signature and internal logic if necessary.
def _run( self, x: float, y: float, z: float, - task: Literal["grab", "place"], + task: Literal["grab", "drop"], ) -> str:Also, update any conditional checks that compare
taskto"place".
Line range hint
83-114: Handle unexpectedtaskparameter valuesCurrently, if
taskis neither"grab"nor"drop", the function defaults to the else branch, which might lead to unintended behavior.Consider adding validation for the
taskparameter.if task == "grab": request.initial_gripper_state = True # open request.final_gripper_state = False # closed -else: +elif task == "drop": request.initial_gripper_state = False # closed request.final_gripper_state = True # open +else: + return f"Invalid task: {task}. Expected 'grab' or 'drop'."
🛑 Comments failed to post (2)
examples/manipulation-demo.launch.py (2)
48-54: 🛠️ Refactor suggestion
Use
FindPackageSharefor consistent package path resolutionThe hardcoded path in lines 51~ can lead to issues if the package is installed in a different location. It's recommended to use
FindPackageShareto dynamically locate the package path.Apply this diff to improve path resolution:
IncludeLaunchDescription( PythonLaunchDescriptionSource( [ - "src/examples/rai-manipulation-demo/Project/Examples/panda_moveit_config_demo.launch.py", + FindPackageShare("rai-manipulation-demo"), + "/Project/Examples/panda_moveit_config_demo.launch.py", ] ), ),Committable suggestion skipped: line range outside the PR's diff.
56-65: 🛠️ Refactor suggestion
Replace
RosTimerwith event handling for reliable synchronizationUsing a fixed delay with
RosTimermay not guarantee that the MoveIt node is fully initialized before therobotic_manipulationnode starts. This can lead to race conditions. It's better to use an event handler to launch the node after the MoveIt node is ready.Consider using
RegisterEventHandlerandOnProcessExitas shown below:+from launch.event_handlers import OnProcessExit +from launch.actions import RegisterEventHandler # Include the MoveIt launch file and assign it to a variable moveit_launch = IncludeLaunchDescription( PythonLaunchDescriptionSource( [ FindPackageShare("rai-manipulation-demo"), "/Project/Examples/panda_moveit_config_demo.launch.py", ] ), ) return LaunchDescription( [ # Previous launch actions... moveit_launch, - # Launch the robotic_manipulation node after 3 seconds - RosTimer( - period=3.0, - actions=[ - Node( - package="robotic_manipulation", - executable="robotic_manipulation", - name="robotic_manipulation_node", - output="screen", - ), - ], - ), + # Launch the robotic_manipulation node after MoveIt is ready + RegisterEventHandler( + OnProcessExit( + target_action=moveit_launch, + on_exit=[ + Node( + package="robotic_manipulation", + executable="robotic_manipulation", + name="robotic_manipulation_node", + output="screen", + ), + ], + ) + ), # Remaining launch actions... ] )Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Works as intended. Please create a PR for the rai-manipulation-demo to include the necessary dependencies.
Below is the list of commands I had to run to get it working:
2037 2024-12-02 12:40:04 sudo apt install ros-jazzy-ackermann-msgs
2039 2024-12-02 12:40:27 sudo apt install ros-jazzy-gazebo-msgs ros-jazzy-fastrtps*
2041 2024-12-02 12:42:18 sudo apt install ros-jazzy-gazebo-msgs ros-jazzy-control-toolbox
2079 2024-12-02 12:58:14 sudo apt install ros-jazzy-moveit-configs-utils
2082 2024-12-02 12:59:37 sudo apt install ros-jazzy-moveit-resources-panda-*
2083 2024-12-02 12:59:58 sudo apt install ros-jazzy-moveit*
2084 2024-12-02 13:03:00 sudo apt install ros-jazzy-sdformat-*
Note: Some commands use *, which installs every package with the specified prefix. This may include unnecessary packages. Please determine and specify only the required packages.
To replicate the setup on a clean Ubuntu installation with ROS, first uninstall all ROS packages:
sudo apt remove ros-*Then, reinstall ROS using the following command:
sudo apt install ros-(humble|jazzy)-ros-baseAfter reinstalling, use a trial-and-error approach (or some other method) to identify the minimal set of additional packages required to run the demo.
As a last step, please add rosdep instruction to the demo's README to install required dependencies.
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
11181c6 to
1f1cf77
Compare
Purpose
Launching the manipulation demo required to launch the simulation and a few different nodes separately, this PR simplifies all this into one launchfile and one script for interaction with the robot.
Proposed Changes
docs/demos/manipulation.mdIssues
N/A
Testing
The launchfile was ran successfully and the demo was tested manually.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes