[Servo] Update MoveIt Servo to use generate_parameter_library

[ibrahiminfinite]

Description

This PR modifies Moveit Servo to use the generate_parameter_library to load the servo parameters.
Fixes #2095

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

[Abishalini]

Thanks for the initiative! I will review the PR as you progress.

Just an initial feedback to get the formatting CI check passing - install pre-commit
Here are instructions on how to run pre-commit - https://moveit.ros.org/documentation/contributing/code/#pre-commit-formatting-checks

[ibrahiminfinite]

There is a make_shared_from_pool.h file in the package which does not seem to be used anywhere.
Is that there for some future purpose ?
If not , I can just remove it in this PR itself.

[AndyZe]

There is a make_shared_from_pool.h file in the package which does not seem to be used anywhere. Is that there for some future purpose ? If not , I can just remove it in this PR itself.

Please do remove it, if possible 👍

[ibrahiminfinite]

Please do remove it, if possible 👍

Turns out it is used in pose_tracking
I did not notice this earlier since I had disabled the pose_tracking demos.

  // use the shared pool to create a message more efficiently
  auto msg = moveit::util::make_shared_from_pool<geometry_msgs::msg::TwistStamped>();

@AndyZe , could this be replaced with a geometry_msgs::msg::TwistStamped member variable which is reused ?

[AndyZe]

Can you please create an issue about it and we'll decide later. It doesn't matter for this PR, in fact it would be nice to keep it separate from this PR.

[ibrahiminfinite]

At present , the parameters do not have a "moveit_servo" prefix like before.
rotational_scale instead of moveit_servo.rotational_scale.
Should this prefix be kept , or is this something that can be dropped since the parameters are already in a node namespace.

Another question is, if the default values in the servo_parameters.yaml should be set to something generic rather than the panda specific values like panda_link8 Since the values are overridden anyway

[sea-bass]

Took another pass through and only saw a few small things.

[sea-bass]

I will also say that you can run clang-tidy locally so you don't have to wait for CI to get feedback:

First, make sure you build your workspace with the compile-commands mixin:

colcon build --mixin compile-commands

Then, from that same workspace root:

run-clang-tidy -p build/ src/

[codecov]

Codecov Report

Patch coverage: 76.31% and project coverage change: -0.35 ⚠️

Comparison is base (f432978) 50.83% compared to head (7f13e3f) 50.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2096      +/-   ##
==========================================
- Coverage   50.83%   50.48%   -0.35%     
==========================================
  Files         391      387       -4     
  Lines       32158    31819     -339     
==========================================
- Hits        16345    16061     -284     
+ Misses      15813    15758      -55     

Impacted Files	Coverage Δ
...oveit_servo/include/moveit_servo/collision_check.h	100.00% <ø> (ø)
.../moveit_servo/include/moveit_servo/pose_tracking.h	100.00% <ø> (ø)
...ros/moveit_servo/include/moveit_servo/servo_node.h	100.00% <ø> (ø)
moveit_ros/moveit_servo/src/servo_node.cpp	63.24% <68.19%> (+0.28%)	⬆️
moveit_ros/moveit_servo/src/servo_calcs.cpp	65.62% <68.27%> (-1.10%)	⬇️
moveit_ros/moveit_servo/src/collision_check.cpp	85.49% <90.91%> (ø)
moveit_ros/moveit_servo/src/pose_tracking.cpp	77.28% <100.00%> (+0.17%)	⬆️
moveit_ros/moveit_servo/src/servo.cpp	73.34% <100.00%> (-1.66%)	⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ibrahiminfinite]

Another question is, if the default values in the servo_parameters.yaml should be set to something generic rather than the panda specific values like panda_link8 Since the values are overridden anyway

Bringing this up again since I just found out the tests were passing because of the default values and not loading the config properly.

[AndyZe]

Ideally all these parameters that are specific to the robot would NOT have defaults so they throw an error if not configured. Thanks for taking the time to address these issues instead of sweeping them under the rug.

move_group_name, planning_frame, ee_frame_name, robot_link_command_frame

[ibrahiminfinite]

Ideally all these parameters that are specific to the robot would NOT have defaults so they throw an error if not configured.

The tests fail properly now if the parameters are not specified during launch.

[ibrahiminfinite]

Maybe the parameters that use default values should be removed from the configs in the config folder to keep things cleaner ?
So that the file only contains params that are overridden or those that do not have a default value.

Earlier the full list of parameters were really only available if you looked at the header file.
With this PR , everything is available in a clean readable format in src/servo_parameters.yaml along with the descriptions.
Because of this, I think it would be OK to keep the "user specified" configs like config/panda_simulated_config.yaml minimal.

It might also be good to move the files in config to the their own example folders.
eg: The pose tracking related rviz and servo config files should go in the respective pose tracking example folder

These changes should probably be done in a separate "cleanup" PR since these are mostly "nice to have".

[AndyZe]

Just waiting on a review from a second maintainer now...

[sea-bass]

Looks good to me as well -- let's do this. Thanks, @ibrahiminfinite!

[ibrahiminfinite]

Thanks for the guidance!

[tylerjw]

@ibrahiminfinite this broke the tutorials from building, would you mind updating the tutorials site?

See moveit/moveit2_tutorials#684

[ibrahiminfinite]

@ibrahiminfinite this broke the tutorials from building, would you mind updating the tutorials site?

See ros-planning/moveit2_tutorials#684

Yea sure .

[develiberta]

I have the same problem and I'm waiting the moveit2_tutorials update, especially, moveit2_tutorials/doc/how_to_guides/isaac_panda and branch humble