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.

Sign up for GitHub

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

Jump to bottom

Make moveit_config compatible to moveit_configs_builder #998

Merged
merged 9 commits into from
Jun 11, 2024
Merged

Make moveit_config compatible to moveit_configs_builder #998

merged 9 commits into from
Jun 11, 2024

Conversation

fmauch
Copy link
Collaborator

@fmauch fmauch commented May 16, 2024 •
edited
Loading

As mentioned in #917 our current config is not compatible to the moveit_configs_builder.

This PR aims at making our config compatible with it.

The current state is a working version which allows loading the description from the robot_state_publisher's topic and can be used in the moveit_configs_builder

Things still need to be done

  • Lazy starting of move_group. Currently, it crashes when the description topic isn't yet published
  • [ ] Add upstream issue about the crash when no description topic is provided
  • Add moveit_servo

Things that significantly change in this PR:

  • the tf_prefix has been removed. It didn't work correctly, anyway. Since there are joints inside config files such as the joint_limits.yaml and they to my knowledge cannot handle argument substitution during loading inside the configs builder, this doesn't work.

Things suggested in #917 (comment) I did not implement

  • I did not add different IK solutions, as it seems not to be possible to pass them to the configs builder dynamically. As this is merely an example repo, I'd rather not add this. Users can still define their own when they implement their own launchfiles with the configs_builder in them.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented May 16, 2024 •
edited
Loading

Unorthodox perhaps, but what about not using the config builder at all and creating a more ROS-1-like MoveIt config pkg? Tyler made a nice example showing how to do this, right around the time I was also looking into it.

See ROSCON 2023 - Retro ROS 2 Launch (and the package itself).

I'm wondering whether hiding what goes on in a launch file -- by encapsulating its 'insides' in a separate library -- and making it harder to customise a MoveIt configuration isn't actually doing a disservice to users.

Whenever we feel the need to start creating a reusable Python package because it feels like things are getting more verbose than XML, we may have a bit of a problem ..

@fmauch
Copy link
Collaborator Author

fmauch commented May 21, 2024

Thank you for the feedback @gavanderhoorn!

We have considered for quite a while now whether we want to migrate our config package or not. Personally, I am also not the greatest fan of the way the configs builder hides the required config files away. Though once looking at its source code it became less hidden for me, so a better documentation could potentially improve things here. In my opinion there are a couple of arguments that justify making this step:

  • configs-builder seems to be the standard used by MoveIt! nowadays. Of course, that doesn't mean that we'll have to follow that and we do a couple of things differently than a standard moveit_config package would, but it might help new users to get familiar with the structure more easily. And I expect that package to be used by less experienced MoveIt! users anyway, as more experienced users would probably create their own moveit_config package rightaway.
  • Reusing this to write your own launchfiles launching your custom nodes should be much more convenient, especially if you are used to using the configs builder. The main point about this PR is changing the structure, not necessarily using the configs builder in our own launchfiles. That's also (from my understanding) what the OP in Adding MoveItConfigsBuilder compatible example to ur_moveit_config #917 was expecting.
  • Tyler's package I think is also from a time where the configs builder didn't exist, yet. The launchfiles become much less cluttered once the description is re-used from the description topic.
  • Using one big config file for everything isn't necessarily reducing complexity in my opinion.

@fmauch fmauch mentioned this pull request May 27, 2024
Merged
@fmauch
Copy link
Collaborator Author

fmauch commented May 27, 2024

I decided against an upstream issue regarding the description topic as it seems intentional to have a default of 10 seconds timeout.

@fmauch fmauch marked this pull request as ready for review May 27, 2024 14:36
@fmauch fmauch requested a review from VinDp May 27, 2024 14:40
@fmauch fmauch linked an issue May 27, 2024 that may be closed by this pull request
Adding MoveItConfigsBuilder compatible example to ur_moveit_config #917
Closed
5 tasks
fmauch and others added 4 commits June 3, 2024 13:52
@fmauch @VinDp
Make the config moveit_configs_builder compatible
09318d1 
- Make sure it can be used in conjunction with the configs builder
- Still use a parametrized srdf
- Re-use the description from the robot state publisher
@fmauch @VinDp
Wait for robot_description in moveit startup
f7fd5e0 
Since we built our moveit config around receiving the description via topic, we also add a script that waits for the robot description to be published.
This helps when components support subscribing to the description, but crash if it is not present.
@fmauch @VinDp
Added servo node
cfcd962
@mrjogo @VinDp
Add servo node config to disable advertising /get_planning_scene
18472cc
VinDp
VinDp approved these changes Jun 3, 2024
View reviewed changes
Copy link
Collaborator

@VinDp VinDp left a comment

Choose a reason for hiding this comment

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

Works nicely and looks good to me, I only have some small suggestions.

ur_moveit_config/package.xml Outdated Show resolved Hide resolved
ur_moveit_config/package.xml Outdated Show resolved Hide resolved
ur_moveit_config/package.xml Outdated Show resolved Hide resolved
ur_moveit_config/package.xml Outdated Show resolved Hide resolved
ur_moveit_config/package.xml
<author email="[email protected]">Lovro Ivanov</author>
<author email="[email protected]">Andy Zelenak</author>

<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>ament_cmake_python</buildtool_depend>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<buildtool_depend>ament_cmake_python</buildtool_depend>
<exec_depend>launch</exec_depend>
<exec_depend>launch_ros</exec_depend>

ur_moveit_config/CMakeLists.txt Outdated Show resolved Hide resolved
ur_moveit_config/CMakeLists.txt Outdated
ament_python_install_module(${PROJECT_NAME}/launch_common.py)

ament_package()
install(DIRECTORY config DESTINATION share/${PROJECT_NAME})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
install(DIRECTORY config DESTINATION share/${PROJECT_NAME})

@VinDp VinDp mentioned this pull request Jun 10, 2024
Merged
@fmauch fmauch merged commit 4c2971d into UniversalRobots:main Jun 11, 2024
11 checks passed
@fmauch fmauch deleted the moveit_configs_builder branch June 11, 2024 09:56
This was referenced Jun 13, 2024
Closed
Closed
@fmauch fmauch mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VinDp VinDp VinDp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding MoveItConfigsBuilder compatible example to ur_moveit_config
4 participants
@fmauch @gavanderhoorn @VinDp @mrjogo