Change KDL IK plugin joint weight parameters specification

sea-bass commented 4 months ago

Description

This is a potential fix for #2749.

It changes the KDL parameters introduced in https://github.com/ros-planning/moveit2/pull/1671 from:

panda_arm:
  kinematics_solver: "kdl_kinematics_plugin/KDLKinematicsPlugin"
  kinematics_solver_search_resolution: 0.005
  kinematics_solver_timeout: 0.05

  joints: ["joint1", "joint2", "joint3", "joint4", "joint5", "joint6"]
  # Default weight for joints is 1.0 
  joint1:
    weight: 2.0
  joint5:
    weight: 1.5

to

panda_arm:
  kinematics_solver: "kdl_kinematics_plugin/KDLKinematicsPlugin"
  kinematics_solver_search_resolution: 0.005
  kinematics_solver_timeout: 0.05

  joints: ["joint1", "joint2", "joint3", "joint4", "joint5", "joint6"]
  # Default weight for joints is 1.0 
  weights:
    joint1:
      value: 2.0
    joint5:
      value: 1.5

Checklist

[x] 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
[x] Include a screenshot if changing a GUI
[x] While waiting for someone to review your request, please help review another open pull request to support the maintainers

sea-bass commented 4 months ago

Ugh, seems like MoveIt Setup Assistant has some failures due to the addition of the nonzero velocity here: https://github.com/ros-planning/moveit_resources/pull/198.

This is fixed by https://github.com/ros-planning/moveit2/pull/2751, which I temporarily included in this branch as well to see if Humble CI passes.

sea-bass commented 4 months ago

Do we need to update some moveit_resources packages?

Actually, no, because none of the example configs even sets joint weights in the first place!

But ler's not merge this in yet, as we could also opt to fix generate_parameter_library

rhaschke commented 4 months ago

Why don't you simply use a map from joint name to weight (instead of an additional indirection layer):

  weights:
    joint1: 2.0
    joint5: 1.5

sea-bass commented 4 months ago

Why don't you simply use a map from joint name to weight (instead of an additional indirection layer):
  weights:
    joint1: 2.0
    joint5: 1.5

That does sound good, but I'm not sure if generate_parameter_library supports doing this at the moment. Looking at the examples: https://github.com/PickNikRobotics/generate_parameter_library?tab=readme-ov-file#mapped-parameters, seems like there has to be a field underneath the mapped parameters.

rhaschke commented 4 months ago

I'm not sure if generate_parameter_library supports doing this at the moment.

If it doesn't, you probably should fix that library. A library implementation shouldn't impose constraints on the kind of parameter structures available.

Abishalini commented 4 months ago

If humble CI jobs pass, would it make sense to merge this while a fix to generate_param_library is being worked on? This would unblock other PRs in the repo

sea-bass commented 4 months ago

Seems there is interest in fixing CI for now, so will open up https://github.com/ros-planning/moveit2/issues/2756 and we'll make sure we get that in for the next MoveIt 2 release.

sea-bass commented 4 months ago

Update: I think I fixed generate_parameter_library to be able to keep the old syntax: https://github.com/PickNikRobotics/generate_parameter_library/pull/185

gavanderhoorn commented 1 month ago

@sea-bass: could the branch be deleted?

moveit / moveit2

Change KDL IK plugin joint weight parameters specification #2750

Description

Checklist