included position information in joint_limits.yaml

[Sunny478]

Description

Included the position limits from URDF to the joint_limits.yaml file in /config. Resolves issue #2486

Checklist

• [ ] Required by CI: Code is auto formatted using clang-format
• [ ] Extend the tutorials / documentation reference
• [ ] Document API changes relevant to the user in the MIGRATION.md notes
• [ ] Create tests, which fail without this PR reference
• [ ] Include a screenshot if changing a GUI
• [ ] While waiting for someone to review your request, please help review another open pull request to support the maintainers

[welcome[bot]]

Thanks for helping in improving MoveIt and open source robotics!

[codecov[bot]]

Codecov Report

Merging #2553 (8a58c2f) into master (e28714a) will increase coverage by 0.70%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2553      +/-   ##
==========================================
+ Coverage   60.15%   60.85%   +0.70%     
==========================================
  Files         351      402      +51     
  Lines       26562    29347    +2785     
==========================================
+ Hits        15976    17855    +1879     
- Misses      10586    11492     +906     

Impacted Files	Coverage Δ
...t_setup_assistant/src/tools/moveit_config_data.cpp	18.57% <0.00%> (-0.64%)	:arrow_down:
...lace/include/moveit/pick_place/manipulation_plan.h	57.15% <0.00%> (-17.85%)	:arrow_down:
..._core/collision_detection/src/collision_matrix.cpp	40.40% <0.00%> (-6.40%)	:arrow_down:
...core/planning_interface/src/planning_interface.cpp	73.69% <0.00%> (-5.72%)	:arrow_down:
...tion/work_space/pose_model_state_space_factory.cpp	72.73% <0.00%> (-5.05%)	:arrow_down:
...s/include/moveit/py_bindings_tools/serialize_msg.h	95.00% <0.00%> (-5.00%)	:arrow_down:
...g_request_adapter/src/planning_request_adapter.cpp	54.55% <0.00%> (-4.91%)	:arrow_down:
..._core/planning_interface/src/planning_response.cpp	26.93% <0.00%> (-4.89%)	:arrow_down:
...moveit/ompl_interface/detail/constraints_library.h	28.58% <0.00%> (-4.76%)	:arrow_down:
...rface/src/wrap_python_planning_scene_interface.cpp	25.65% <0.00%> (-4.66%)	:arrow_down:
... and 342 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e28714a...8a58c2f. Read the comment docs.

[v4hn]

• emitter << YAML::Key << "has_position_limits";
• emitter << YAML::Value << "false";

why false? I'm not even sure what happens when you set that while the URDF actually has a position limit

Turns out the "has_position_limits" key does not even exist in the current logic loading the parameters. It only exists inside the JointModel

Also, I never use these overrides because they are MoveIt-specific, but the actual drivers should know about them. My naive interpretation of the has_*_bounds fields in joint_limits.yaml was always that they enable/disable overriding the URDF entries, but looking at the parsing code that is not true at all! Any entry directly applies to the model and thus setting a hypothetical has_position_limits key to false would potentially make the joint continuous (potentially because the joint might still not report limits -pi/pi yet).

So the only possible alternatives for this PR seem to be to (1) either specify all joint limits explicitly, thus overriding any changes someone does in the URDF later on, or (2) adding the joint limits as YAML comments so users might rely on them if they want to. I strictly prefer the second option to avoid issues with users expecting their URDF values to be used. In both cases we actually do need a new has_position_limits parameter to describe continuous joints.

[simonschmeisser]

We use the joint_limits.yaml as a way to go from generic hardware/vendor joint limits that are distributed in the urdf/xacro in packages (debs) to the limits that apply to a particular instance/ a particular robot. How do you handle that case @v4hn ? Or do you have a branch for each of your robots?

We use (1) with our internal equivalent of MoveIt Setup Assistant

To keep this PR small we could also add default joint limits (+- pi) as a comment for continuous joints or not add them at all

@Sunny478 not sure why you closed this, you can simply commit and push further improvements to your branch and they will show up here. Also please don't be shy and ask questions if they occur

[Sunny478]

Sorry for closing, deleted my local branch accidentally.

or (2) adding the joint limits as YAML comments so users might rely on them if they want to. Should all the joint limits (including the velocity and acceleration limits also) be added as YAML comments ?

Also, I never use these overrides because they are MoveIt-specific, but the actual drivers should know about them. Did you mean that there exist other drivers that fetch the joint limits information from joint_limits.yaml and not the original URDF files?

[v4hn]

We use the joint_limits.yaml as a way to go from generic hardware/vendor joint limits that are distributed in the urdf/xacro in packages (debs) to the limits that apply to a particular instance/ a particular robot. How do you handle that case @v4hn ?

We have this particular problem only with UR robots and their descriptions provide joint-limit parameters in the xacro macros used to define them. As I always want to add more geometry to each robot instance, I will only ever use the macro anyway in my own descriptions. If you use a robot description that does not have equivalent parameters, I would expect the maintainers to be willing to add them?

To keep this PR small we could also add default joint limits (+- pi) as a comment for continuous joints or not add them at all

Sounds reasonable, though it's a hack and people might get the wrong idea and assume [-pi;pi] == continuous joint. So maybe add a simple comment like # continuous instead.

Should all the joint limits (including the velocity and acceleration limits also) be added as YAML comments ?

The current behavior adds them as statements, right? So by the same argument they should be comments as well. ttbomk URDF cannot specify acceleration at all, only effort (which of course restricts acceleration implicitly). So has_acceleration_limits: false (the default) is pretty much equivalent to commenting them out. I was unsure about it when I read your question, but yes, I do the dynamics limits should be commented too.

Did you mean that there exist other drivers that fetch the joint limits information from joint_limits.yaml and not the original URDF files?

I meant it the other way around. MoveIt will respect the limits in joint_limits.yaml - at least it should, though some time parameterization code can actually violate them afaik. But the lower-level controllers will not know about them at all, e.g., ros_control only parses the URDF. Now if MoveIt's limits are lower than the actual hardware limits (almost always) and trajectory execution on the hardware is delayed for some reason the low-level controller can legitimately violate MoveIt's limits even if the planned trajectory does not. The same is obviously true for any approach you might use in parallel to MoveIt, possibly visual servoing or trained controllers.

[gavanderhoorn]

@v4hn wrote:

But the lower-level controllers will not know about them at all, e.g., ros_control only parses the URDF.

this is actually not entirely true. See this bit of code in @davetcoleman's boilerplate, which makes use of this function to parse limits from the parameter server (docs).

The headers in joint_limits_interface actually show both the URDF and the rosparam server are supported (and I've used this structure to (almost transparently) provide overloads for other sources of limits, such as system variables on industrial robot controllers).

And the structure of those parameters is identical to MoveIt's (with the exception it also supports specifying jerk limits, and support for minimum velocity, etc). This is very convenient when loading those limits from a shared file (part of the robot description of support package fi): load it once in a suitable namespace, and both MoveIt and the hardware_interface can find it.

---

Edit: I'm of course not claiming here every hardware_interface uses that function. But I know mine do, and those derived from the boilerplate code also could be doing this.

[v4hn]

Turns out, I never sent this reply!

Thanks, I did not know ros_control supports a superset of MoveIt's specification for that! That's great! My main point remains though, you have to ensure the limits are respected by every layer of your pipeline, not just by MoveIt. It would be much better to have all the limits specified in the robot description, but that's not supported, even in ROS2 I guess? sigh.

Adding "global" limits to the moveit configuration is somewhat unexpected at least to me.

[Sunny478]

much better to have all the limits specified in the robot description, but that's not supported, even in ROS2 Then how do we expect to solve #2486 ?

[v4hn]

much better to have all the limits specified in the robot description, but that's not supported, even in ROS2 `

Then how do we expect to solve #2486 ?

That's not related. I just stated my own opinion on using the limits file for global setting. MoveIt supports the file/parameters and we are not going to change that (anytime soon). I still propose to add all the limits as comments in the joint_limits.yaml file to resolve your (valid) request.

[Sunny478]

I am unable to get my code to pass the clang-format. I have downloaded/installed the required packages (clang-format) for my text editor (I am using Atom) and I think it does work the correct way but somehow it is still unable to pass the format check on github. Could someone help me out here... Did I miss something?

[simonschmeisser]

you typically need the exact same version of clang-format (clang-format-10) in our case.

Please also check here: https://moveit.ros.org/documentation/contributing/code/ pre-commit sounds pretty nice and easy to setup

What I often do is to just have a look at the diff here https://github.com/ros-planning/moveit/pull/2553/checks?check_run_id=2363057956 and change that manually

[akashjinandra]

I might be out of the loop here, but it seems like this is already able to be done with the max_position, min_position key words in the joint_limits.yaml file. Mentioned here: https://ros-planning.github.io/moveit_tutorials/doc/time_parameterization/time_parameterization_tutorial.html

I have confirmed that this works and when planning with moveit it shows my new limits in the rviz motion planning plugin: