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

Removed launch files, moved to ur_bringup #246

Closed
wants to merge 1 commit into from
Closed

Removed launch files, moved to ur_bringup #246

wants to merge 1 commit into from

Conversation

hsd-dev
Copy link

@hsd-dev hsd-dev commented Dec 17, 2018

No description provided.

@miguelprada
Copy link
Member

@ipa-hsd I believe you should target kinetic-devel with this PR, instead of master.

@gavanderhoorn gavanderhoorn changed the base branch from master to kinetic-devel December 17, 2018 13:34
@gavanderhoorn
Copy link
Member

@miguelprada: you should be able to do that as well. I've just updated the target branch.

@ipa-hsd: should we consider doing this the other way around? So keeping the launch files here in ur_modern_driver, and then after some time, removing the ones in ur_bringup. This would align better with ros-industrial/ros_industrial_issues#49 and would make it easier to use ros-industrial/universal_robot with different drivers.

With the change suggested in this PR we again tightly couple universal_robot with a new driver, which is something we should perhaps try to avoid.

@gavanderhoorn
Copy link
Member

@ipa-hsd: the PR is failing Travis as you didn't update this line.

@miguelprada
Copy link
Member

miguelprada commented Dec 17, 2018

@miguelprada: you should be able to do that as well. I've just updated the target branch.

I wasn't sure what the rules of etiquette were regarding this. I will do it next time.

@ipa-hsd: should we consider doing this the other way around? So keeping the launch files here in ur_modern_driver, and then after some time, removing the ones in ur_bringup. This would align better with ros-industrial/ros_industrial_issues#49 and would make it easier to use ros-industrial/universal_robot with different drivers.

With the change suggested in this PR we again tightly couple universal_robot with a new driver, which is something we should perhaps try to avoid.

This sounds very reasonable.

@gavanderhoorn
Copy link
Member

@miguelprada: you should be able to do that as well. I've just updated the target branch.

I wasn't sure what the rules of etiquette were regarding this. I will do it next time.

thanks for being so considerate.

I would say it will depend a bit on the exact circumstances, but in most cases it should be more expedient for you to change the target branch.

@gavanderhoorn gavanderhoorn added the kinetic Issues with the refactor in Kinetic label Dec 17, 2018
@ipa-nhg
Copy link
Member

ipa-nhg commented Dec 18, 2018

I am sorry but I usually prefer to keep the driver packages as simple as possible and don't include launch or configuration (yaml) files. See my comment ros-industrial/ros_industrial_issues#49 (comment)

I usually divide the "bringup" layer into 3 types of packages (grouped in metapackages):

  • common: that holds description and msgs. This metapackage is super stable, in terms of release and usually hasn't to be updated for new ROS distros.
  • driver : that holds basically the driver (ccp code). This metapackage is quite stable, has to be released when a new bug is found and fixed and may require updates for new distros. It depends on cpp libs and the msgs package (that should be installed via rosdep)
  • bringup: launch(real robot and simulation), yaml and moveit config. This is the most unstable metapackage that depends on the common and the driver packages. It changes constantly and usually is built in your workspace and not installed from release.

Also a note: Now that we want to support also the E-series, probably we will need more yaml, launch, URDFs and test files that will imply more complexity for this repo and makes much difficult keep a stable release version.

@miguelprada
Copy link
Member

@ipa-nhg What you say makes sense, but that would entail adding yet another repository/metapackage with the bringup package, separate from universal_robot repo.

Having layers 1 and 3 of your proposed structure in one repository and layer 2 in another doesn't make much sense to me. In this setting both repositories have a circular dependency, which is not very nice, even if the packages themselves do not.

Unless you want to add a third repository, I vote in favor of @gavanderhoorn's suggestion of keeping launch files in ur_modern_driver.

@ipa-nhg
Copy link
Member

ipa-nhg commented Dec 18, 2018

Having layers 1 and 3 of your proposed structure in one repository and layer 2 in another doesn't make much sense to me. In this setting both repositories have a circular dependency, which is not very nice, even if the packages themselves do not.

Totally agree.

The only alternative I can propose is merge the layers 1 and 2. That means create a new metapackage (this repo probably) to hold the packages: ur_msgs, ur_description , ur_e_description and ur_modern_driver.

And then the universal_robot repo should contain ur10_moveit_config, ur3_moveit_config, ur5_moveit_config, ur_bringup, ur_gazebo, ur10_e_moveit_config, ur3_e_moveit_config, ur5_e_moveit_config, ur_e_bringup and ur_e_gazebo

similar to the schunk approach : https://github.com/ipa320/schunk_modular_robotics and https://github.com/ipa320/schunk_robots.

NOTE: if the urdf introduces a dependency to gazebo(?not sure..) and is not needed to start the driver, the description packages can also be part of the universal_robot repository

@miguelprada
Copy link
Member

The only alternative I can propose is merge the layers 1 and 2. That means create a new metapackage (this repo probably) to hold the packages: ur_msgs, ur_description , ur_e_description and ur_modern_driver.

And then the universal_robot repo should contain ur10_moveit_config, ur3_moveit_config, ur5_moveit_config, ur_bringup, ur_gazebo, ur10_e_moveit_config, ur3_e_moveit_config, ur5_e_moveit_config, ur_e_bringup and ur_e_gazebo

That would tie together the description and msgs packages with the driver, which isn't very nice either. Also, the universal_robot repo would then necessarily depend on ur_modern_driver.

I still think that the best compromise is to keep layers 1 (i.e. universal_robot) and 2 (i.e. ur_modern_driver) and merging layer 3 (i.e. bringup launch files) onto 2. An open question would be whether to include the simulation packages together with layer 2, so that the specific controller setup can be configured to match the real driver setup as much as possible for different potential drivers, or to keep a generic simulation that may differ from the specifics of each driver at runtime.

NOTE: if the urdf introduces a dependency to gazebo(?not sure..) and is not needed to start the driver, the description packages can also be part of the universal_robot repository

I believe the URDF is not used by the driver per-se right now (although it is included in the launch files for the individual controllers to use), but if we ever plan to include joint limit enforcement to the driver, which I think we should, then it will be required.

@miguelprada
Copy link
Member

Closing due to inactivity and because it's slowly drifting towards a more general discussion that would fit ros-industrial/ros_industrial_issues#49 better.

@miguelprada miguelprada closed this Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kinetic Issues with the refactor in Kinetic
Development

Successfully merging this pull request may close these issues.

None yet

4 participants