Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial port to ROS 2 #41

Open
wants to merge 11 commits into
base: foxy
Choose a base branch
from
Open

Initial port to ROS 2 #41

wants to merge 11 commits into from

Conversation

ahcorde
Copy link

@ahcorde ahcorde commented Jan 14, 2021

This PR depends on this other PR ros-controls/gazebo_ros2_control#44

Great news! Moveit2 and gazebo it's working! Thank you the effort done in ros2_control and gazebo_ros2_control. I have created a simple example to move the joints of rrbot with moveit2 (which is already available in Foxy like ros2_control).

rrbot_moveit

I moved these packages from gazebo_ros2_control because I think it makes more sense to keep the ros1 structure and port this repository to ROS 2. Not sure if the moveit related packages should live here too.

Signed-off-by: ahcorde [email protected]

Signed-off-by: ahcorde <[email protected]>
Copy link

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the rrbot_gazebo and rrbot_control packages will ever be used? If not, they could be removed instead of ignored.

The robot is launched on Gazebo and RViz for me, but it doesn't move. I see this error message:

[run_moveit_cpp-4] [ERROR] [1612312888.650904331] [moveit.ros_planning_interface.moveit_cpp]: Execution failed! No active controllers configured for group 'rrbot_arm'

rrbot_moveit_config/package.xml Outdated Show resolved Hide resolved
rrbot_description/package.xml Outdated Show resolved Hide resolved
rrbot_moveit_config/package.xml Show resolved Hide resolved
<package format="3">
<name>rrbot_moveit_demo_nodes</name>
<version>0.0.0</version>
<description>Config and launch files to run rrbot demos with ROS 2, gazebo and moveit2</description>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this package maybe replace rrbot_gazebo?

Also, I think you're missing some Gazebo dependencies here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahcorde , I'm not sure what the answer is. I see you also ported rrbot_gazebo, so I guess they serve different purposes. How about documenting both on the README?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved descriptions 65fc070

rrbot_moveit_demo_nodes/launch/rrbot_demo.launch.py Outdated Show resolved Hide resolved
rrbot_moveit_demo_nodes/package.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me what the goals are of this ROS2 port, but much of the example functionality I worked hard to add to Gazebo is being disabled/removed/forgotten in this PR. And I want to make sure that is deliberate.

If it is deliberate, just delete the old code rather than having it hang around as crust.

If you intend to port it soon, document that so it doesn't forgotten.

Overall I find this effort messy, as parts of this code are referenced in the Gazebo tutorials (I wrote this) and disabling it is likely to break those tutorials

README.md Outdated
@@ -1,31 +1,15 @@
# Gazebo ROS Demos

* Author: Dave Coleman <[email protected]>
* Author: Alejandro Hernández
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor of simply removing this line... we're both authors, but so are many others who have contributed to this repo.

README.md Show resolved Hide resolved
@@ -0,0 +1,117 @@
/*********************************************************************
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to copy this code into here? Or depend on it from the moveit project? It will likely get out of sync here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the group name was hardcoded in the moveit2 tutorials, I decided to move this here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should update the moveit tutorial to not hardcode the group name... @henningkayser @JafarAbdi what do you think?

@ahcorde
Copy link
Author

ahcorde commented Mar 18, 2021

The effort here was focus to create some packages that allow to run gazebo_ros2_control and moveit2.

For this reason I ported/created:

  • rrbot_description: Add the rrbot description
  • rrbot_gazebo: launch gazebo with the rrbot and ros2 controllers
  • rrbot_moveit_config: moveit2 configuration files
  • rrbot_moveit_config_demos: launch file to run gazebo (it uses rrbot_gazebo) and moveit2. Demo executable to run some moveit2 code.

@davetcoleman you are the author and maintainer of this packages, let me know how do you envision this package. I will be happy to make the appropriate changes.

Copy link
Collaborator

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, thanks for working on this. Don't let me review block this from merging, if necessary.

rrbot_description/urdf/rrbot.xacro Outdated Show resolved Hide resolved
rrbot_gazebo/package.xml Outdated Show resolved Hide resolved
rrbot_moveit_config/package.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,117 @@
/*********************************************************************
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should update the moveit tutorial to not hardcode the group name... @henningkayser @JafarAbdi what do you think?

@davetcoleman
Copy link
Collaborator

@ahcorde please reach out via email when you want me to review again

@chapulina chapulina removed their request for review April 11, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants