moveit / moveit2

:robot: MoveIt for ROS 2
https://moveit.ai/
BSD 3-Clause "New" or "Revised" License
1.06k stars 516 forks source link

Load kinematics config from topic #1085

Open SammyRamone opened 2 years ago

SammyRamone commented 2 years ago

Is your feature request related to a problem? Please describe. When MoveIt is used from a different node, e.g. to solve IK, this node requires to have the kinematics.yaml parameters (robot_description_kinematics) itself. Otherwise one will get the following error

[WARN] [moveit_ros.robot_model_loader]: No kinematics plugins defined. Fill and load kinematics.yaml! (loadKinematicsSolvers() at /tmp/binarydeb/ros-rolling-moveit-ros-planning-2.4.0/robot_model_loader/src/robot_model_loader.cpp:272)
[ERROR] [moveit_robot_state.robot_state]: No kinematics solver instantiated for group 'LeftLeg' (setFromIK() at /tmp/binarydeb/ros-rolling-moveit-core-2.4.0/robot_state/src/robot_state.cpp:1450)

The robot urdf and srdf are already optionally loaded from a topic, if no parameter is available at the node (https://github.com/ros-planning/moveit2/pull/765). I was wondering if it would be possible to load these parameters optional from a topic (provided by the move_group node) if they are not available. This way it would be much easier to use MoveIt from other nodes.

Describe the solution you'd like I see two options:

  1. The move group node should publish the robot_description_kinematics parameters similarly as the robot_description_semantic. The kinematic_plugin_loader should then use the rdf_loader (or a similar mechanism) to get them instead of just looking for parameters.
  2. The kinematic_plugin_loader uses the get_paramter service of the move_group node to get the parameters.

Describe alternatives you've considered The system could be kept the same as now and users would be enforced to load the parameters manually to their nodes. I'm not sure how many other people are directly using MoveIt internal methods from their own nodes.

Additional context I ran into this issue two times.

  1. I have a ros node that provides bipedal walking. It uses the MoveIt IK interface (https://github.com/bit-bots/bitbots_motion/blob/ros2/bitbots_quintic_walk/src/walk_ik.cpp). This works as long as I additionally load the kinematics.yaml parameters in the launch file to this node (https://github.com/bit-bots/bitbots_motion/blob/d5718dae9f3bf961bee505a471d4a0149d9af058/bitbots_quintic_walk/launch/test.launch#L26)
  2. We also created a Python interface for the IK call, e.g. to solve look-at goals for the head of the robot from python behavior code. This is not started via ros launch and it has an internal C++ ros node. Therefore it is complicated to load the kinematics.yaml there manually. https://github.com/bit-bots/bitbots_motion/blob/d5718dae9f3bf961bee505a471d4a0149d9af058/bitbots_moveit_bindings/src/bitbots_moveit_bindings.cpp#L41-L115
SammyRamone commented 2 years ago

I did solve this for myself by copying the parameters from the move_group node to my node using services. If someone with the same problem stumbles on this issue, you can see the code here https://github.com/bit-bots/bitbots_motion/blob/00cd3eea5dd906bff8ae615eeab734c556dd40ff/bitbots_moveit_bindings/src/bitbots_moveit_bindings.cpp#L24-L40

I still think, that it would be nice if the kinematic_plugin_loader would do something similar by itself, as it would make using it more straight forward. But it is possible to circumvent this issue, so I also understand if you do not want to include this in moveit itself.

v4hn commented 2 years ago

I agree this is an issue. Just because you can work around it, it doesn't mean your use case was considered or is meant to be standard. I believe @DLu worked on the synchronized string parameters to allow for online changes of the URDF, but obviously the code did not consider the other parameters yet that are also needed to have a full and accurate RobotModel instance. You mention the kinematics.yaml content, but joint_limits.yaml is missing as well and is not even loaded in the moveit_resources demos yet! :/

I would think either (1) we go with your approach and synchronize parameters if they are not available or (2) add more synchronized string parameters for other files or (3) I just misunderstood something in the structural ROS2 changes. My favorite would be (1) for everything but the urdf, so that we don't run into synchronization issues when someone changes the URDF in move_group.

Another alternative (4) that would make a lot of sense to me is to migrate away from the additional config files in ROS2 altogether. Either by merging them into the srdf which could easily consider these values, or even by merging everything into the URDF with an additional <moveit> tag on the top level, similar to how Gazebo handles it. xacro can easily handle merging urdf and MoveIt config files. Gazebo went that way a long time ago, so I don't see why MoveIt shouldn't in the next generation code. @tylerjw went a minor step in that direction when he added his new mycobot MoveIt config to the robot_description package :)

SammyRamone commented 2 years ago

Just some thoughts from my perspective: (4) would be nicer for the user as everything is together in one file. Currently a lot of yamls need to be loaded correctly and it is easy to do a mistake there that is hard to debug (had this during switch to ROS2). But it would probably be more difficult to change parameters (e.g. IK timeout) during runtime, since they are not parameters anymore. Still, approach (1) has the same issue if they are only copied during start. At the same time, I am not sure how important it is to change these parameter during runtime. I think I never did it myself and in ROS1 they were not reconfigurable.

v4hn commented 2 years ago

(1) has the same issue if they are only copied during start

That's why I would propose to use the urdf topic as a trigger signal to request all other parameters from the receiver side as well. We would probably even want a unique urdf identifier for requesting the parameters to make sure the URDF did not change yet again.

But it would probably be more difficult to change parameters (e.g. IK timeout) during runtime, since they are not parameters anymore

Your example is not that useful because if you require different IK timeouts in a project's logic they should most likely explicitly set the method parameters for it instead of changing the more global ROS parameters. The design beauty of adding everything to this one/two configuration xmls is that every change becomes a robot change in the same structure with one name. At the moment it is already all the same structure usually called "robot_description" with sub-parameters in robot_description_{semantics,limits,kinematics,planning}. The downside is that we might want some indication upon changes what part changed because reloading the whole URDF, initializing IK solvers for all groups, etc. is a lot of overhead if the only change was to add a new planning group... I basically mentioned (4) because ROS2 should help streamline interfaces that grew (especially in MoveIt) over a decade and there is still a lot of room for improvement there.

gavanderhoorn commented 2 years ago

Just wanted to note that "everything in a single file" seems like the equivalent of "just write a monolithic program", but for models/parameters.

Yes, splitting things up into separate files/entities/objects/classes comes at a cost, but we've figured out that cost is worth it in many (almost all?) cases.

I would personally not recommend lumping everything into the URDF/SRDF. ros2_control did the same thing, and it's one aspect of the current design I really dislike.

v4hn commented 2 years ago

I actually rather like the ros2_control approach and it's one of the reasons why I think it might make sense for MoveIt as well. I did not have to work with it yet though. I agree with you in parts, but a segfault in a huge binary is about the whole system crashing, whereas configuration - by their very nature - has to be consistent across modules OR the system crashes. In terms of modularity it would probably be even better to split things by planning group instead of referring to the same planning groups from 5 different files...

gavanderhoorn commented 2 years ago

I agree with you in parts, but a segfault in a huge binary is about the whole system crashing

creating applications by composing parts has many more advantages than just (runtime) fault isolation.

One disadvantage of the approach ros2_control uses is that it encourages forks of what could otherwise be pure read-only upstream dependencies, just so people can version "their ros control configuration". This can all be worked around by using xacro and a launch file which accepts a parameter which lets you merge in a different xacro snippet just for the ros2_control parameters, but it's all additional complexity.

If configuration data was stored in separate files, those can be hosted by separate packages.

Keeping all your dependencies RO as much as possible reduces time-to-build, reduces time-to-deploy, facilitates replication of (runtime) environments, etc, etc.

v4hn commented 2 years ago

I do see your arguments, though no nice solution to the synchronization question. I smell an interesting discussion for the next maintainer meeting. I hope you will be around. :-)

v4hn commented 2 years ago

Highly related: https://github.com/ros-planning/moveit2/pull/1074