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

Removing robot state publishers and robot desciptors from driver launch? #275

Closed
apalomer opened this issue Mar 28, 2019 · 4 comments
Closed

Comments

@apalomer
Copy link

apalomer commented Mar 28, 2019

I am integrating a UR10 with other sensors (in my case a Kinect). While I found that this package works perfectly I think it is inconvenient that there is no driver launch without a robot_descriptionand a robot_state_publisher.

In my case, the final robot is integrated into moveit (with the perception and planning for Kinect frame) and I have conflicts because when I launch moveit I need the full robot description with the Kinect, therefore I have to kill robot_state_publisher launched by ur_common.launch, override the parameter robot_description with the one of the UR10+kinect and rerun robot_state_publisher (I know I could remap robot description within moveit but this is much more tedious that what I am about to propose). Moreover, the driver does not need the robot description at all and removing the robot description part would unlink this repository from universal_robots one.

For these reasons, I believe that removing line 22 of ur_common.launch as well as lines 18-20 of files ur10_bringup.launch, ur10_bringup_compatible.launch, ur10_ros_control.launch, ur3_bringup.launch, ur3_ros_control.launch, ur5_bringup.launch, ur5_bringup_compatible.launch and ur5_ros_control.launch improves the usability of this package. Another option would be to add a parameter to select running or not the robot_state_publisher in ur_common.launch, then that launch could be used instead of urXX_bringup.launch, urXX_bringup_compatible.launch, urXX_bringup_joint_limited.launchand urXX_ros_control.launch or move the robot_state_publishers to each urXX_bringut.launch. The final option would be to move the robot state publisher to the launch files where the robot parameter descriptor is loaded, for example ur10e_upload.launch. This is, from my point of view, the best option.

What are your thoughts? Should I just do it and create a pull request?

EDIT: I am not 100% sure if the urXX_ros_control.launch actually do need the robot description for ros control to work.

@miguelprada
Copy link
Contributor

miguelprada commented Mar 29, 2019

As you already suggest in your edit, the ros_control-flavoured launchfiles do need the URDF to be loaded, if only because the default joint trajectory controller needs it. The model would also be required if someone finds time to add joint limit support.

Also, even I would be very happy to get rid of the dependency on universal_robot, removing the URDF dependency is not enough, since this driver also depends on the messages defined in ur_msgs.

We could include an extra argument to select whether the URDF model (in non-ros_control launch files) and robot_state_publisher are loaded. I'll be glad to review it if you submit a PR with that.

Being honest, though, if I were in your situation I would resort to creating your own set of launch files with the specific configuration required by your setup.

@gavanderhoorn
Copy link
Member

As @miguelprada already mentions (and you note yourself in your edit), ros_control usage of the driver does require the robot_description parameter to exist. So that would already seem to make this "impossible".


Looking at your suggestion, the main question seems to be: who do we want to make responsible for making sure that all required parameters (and other resources) are present when they are needed? The driver? Or the users of the driver?

If the latter, it would mean that we'd have to explain/document that and every user of the driver would need to make sure that they've followed that procedure. That is a lot of duplication of effort, especially for users that merely want to "get things moving" (it's probably less of a problem for those users integrating their robot in a larger application, similar to what @miguelprada writes in his last sentence).

Personally I'd like to minimise the dependency on universal_robot as well, as it decouples the driver from the robot support packages and makes it easier to switch components.

A complicating factor is exactly what I described above: we delegate the responsibility for loading parts of the driver stack to users, which complicates one of the main usages of the driver (ie: connecting robot, installing package, running launch file), as users now first have to create something themselves to get even the basic functionality to work.

Note: I'm not veto-ing this change at all, as I'm actually in favour of a more minimalistic approach (see my related ros-industrial/ros_industrial_issues#49 fi).

@gavanderhoorn
Copy link
Member

@miguelprada wrote:

We could include an extra argument to select whether the URDF model (in non-ros_control launch files) and robot_state_publisher are loaded. I'll be glad to review it if you submit a PR with that.

Hm, let's not add more arguments to the launch files, shall we :) We already have between 12 and 17 arguments.

@miguelprada
Copy link
Contributor

Hm, let's not add more arguments to the launch files, shall we :) We already have between 12 and 17 arguments.

Since I'm responsible for some of those, I didn't want to veto adding new ones either, but that's a perfectly valid point.

@apalomer also suggests moving the robot_state_publisher to the urX_upload.launch files. I see nothing particularly wrong with this, but I don't see how it helps, either. These files are going to be included in the ur_modern_driver launchfiles anyways, for the reasons pointed out above. Am I missing something?

I guess we can close this and advance in the broader discussion about organization of driver and support packages to be had in ros-industrial/ros_industrial_issues#49. Feel free to comment further or reopen if you think I rushed this.

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

No branches or pull requests

3 participants