UniversalRobots / Universal_Robots_ROS2_Gazebo_Simulation

BSD 3-Clause "New" or "Revised" License
54 stars 24 forks source link

Provide an own set of controllers.yaml file to reduce dependencies #3

Closed fmauch closed 2 years ago

fmauch commented 2 years ago

Currently, this repo depends on the driver's repository (the ur_bringup package) merely because of the ur_controllers.yaml file and the ur_moveit.launch.py files being included / loaded in this package's launchfiles. Do we want to keep it that way?

As also risen in https://github.com/UniversalRobots/Universal_Robots_ROS2_Description/pull/5#discussion_r792606985 I think, keeping the packages as less depending on each other as possible is desireble.

This would also reduce problems of keeping individual repositories in sync with each other, as for example the one fixed in 76df17236438ff83b1e2d26acf14a2526a40f7be (Sorry for missing a PR there).

Providing an own ur_controllers.yaml is pretty simple and doesn't seem like a bad idea to me, but the moveit launchfile is rather hard to factor out. However I'd like to raise the question whether we might want to remove this from this repo completely, as I see this repo more like a component provider for a larger application (a simulated hardware device), while the moveit startup is more part of the application itself. Any (counter?)-opinions on this?

gavanderhoorn commented 2 years ago

What about storing the base .xacros in the description package, and have the driver and the simulation packages provide the necessary companion .xacros which add the ros2_control elements?

Similar to how it's done in universal_robot?

However I'd like to raise the question whether we might want to remove this from this repo completely, as I see this repo more like a component provider for a larger application (a simulated hardware device), while the moveit startup is more part of the application itself.

probably what you are suggesting here (or similar), but I'm not sure I completely understand what you're saying here.

fmauch commented 2 years ago

What about storing the base .xacros in the description package, and have the driver and the simulation packages provide the necessary companion .xacros which add the ros2_control elements?

@gavanderhoorn I've got to admit, I am confused by your comment. I was not talking about the .xacro files in this PR (although the controllers.yaml file ends up in the xacro as mentioned in https://github.com/UniversalRobots/Universal_Robots_ROS2_Description/pull/5#discussion_r792606985).

Your comment, however, would match https://github.com/UniversalRobots/Universal_Robots_ROS2_Description/pull/5 concerning the changes in urdf/ur.urdf.xacro.

Providing our own controllers.yaml file as it is done in universal_robot is what I am proposing here.

destogl commented 2 years ago

Currently, this repo depends on the driver's repository (the ur_bringup package) merely because of the ur_controllers.yaml file and the ur_moveit.launch.py files being included / loaded in this package's launchfiles. Do we want to keep it that way?

I am personally not a friend of doubling files and I don't see many issues, especially in the future, with having dependency to ur_bringup because it will be installed from binaries. Moreover, the main point of the simulation is that it uses as much as possible the same configuration as one would use on real hardware. If we double files this could open other potential issue like "my setup is working with sim, but not with hardware" which could be almost impossible to debug without more in-depth knowledge of the user's setup. Still, I would be OK with this if really neccesary…. For MoveIt launch, see comment below.

This comment is very much opinion based and take it as such — on the end we should "vote" or whatever to decide on this :D

On the other hand, I fully agree on UniversalRobots/Universal_Robots_ROS2_Description#5 (comment)! We should remove it there. Otherwise there are circular dependencies.

Providing an own ur_controllers.yaml is pretty simple and doesn't seem like a bad idea to me, but the moveit launchfile is rather hard to factor out. However I'd like to raise the question whether we might want to remove this from this repo completely, as I see this repo more like a component provider for a larger application (a simulated hardware device), while the moveit startup is more part of the application itself. Any (counter?)-opinions on this?

Regarding MoveIt launch file… I think it is very valuable to have it because many people with use the simulation with MoveIt (I assume that many users are more interested into high-level applications and not control itself). This file can be reused on one hand and serve as example for integrating simulation with other nodes on the other hand. I don't have a better idea where to place the file. One option would be to separate MoveIt setup from the driver. The only question is, do we want to have an additional repository?

fmauch commented 2 years ago

I am personally not a friend of doubling files and I don't see many issues, especially in the future, with having dependency to ur_bringup because it will be installed from binaries.

I do see your point and I could live with the dependency to ur_bringup. On the other hand (from a logical dependency view) the bringup package should depend on the things it brings up, not the other way around (see also comment below).

Copying the file here would give a working standalone configuration if you just want to have a peek at this package. If we shape ur_bringup in a way that it brings up all the supported setups (simulated / real robot, MoveIt, etc), those launchfiles from ur_bringup can use the common configuration inside ur_bringup which will then be the same for all scenarios to keep simulation and real robots as close as possible.

Regarding MoveIt launch file… I think it is very valuable to have it because many people with use the simulation with MoveIt (I assume that many users are more interested into high-level applications and not control itself). This file can be reused on one hand and serve as example for integrating simulation with other nodes on the other hand. I don't have a better idea where to place the file. One option would be to separate MoveIt setup from the driver. The only question is, do we want to have an additional repository?

I absolutely do agree, that this is valuable and we should provide something like that. However, I don't think that this is the place to put this into. I think it would be better suited inside the ur_bringup package, as it actually brings up a simulated robot together with a working MoveIt! stack.

destogl commented 2 years ago

I absolutely do agree, that this is valuable and we should provide something like that. However, I don't think that this is the place to put this into. I think it would be better suited inside the ur_bringup package, as it actually brings up a simulated robot together with a working MoveIt! stack.

As we discussed in the meeting: Adding the launch files for simulation into ur_bringup would introduce simulation dependencies to it. And this would result with installing simulators all-around the place, even in cases when those are not needed. Keep this separated as it is now, reduces dependency footprint very much.

fmauch commented 2 years ago

Merging this in for now, but will probably create other PRs regarding runtime dependencies and CI upstream workspaces.