AndrejOrsula / pymoveit2

Basic Python interface for MoveIt 2 built on top of ROS 2 actions and services
BSD 3-Clause "New" or "Revised" License
137 stars 54 forks source link

Added setting of cartesian speed and acceleration #53

Closed Zarnack closed 6 months ago

Zarnack commented 6 months ago

Description

This PR fixes the issue #45 by changing the max_cartesian_speed setter to set the max_velocity_scaling_factor of the cartesian_path_request. This PR also adds a max_cartesian_acceleration setter and getter to set the max_acceleration_scaling_factor of the cartesian_path_request.

Testing

Tested with a real and simulated franka panda robot via the move_to_pose command with the cartesian flag set to true. I used the BiTRRT algorithm from the OMPL and the PTP algorithm from the PILZ trajectory generator.

amalnanavati commented 6 months ago

The pattern used for all other cartesian path parameters is that the property setters/getters set them in self.__move_action_goal.request and then the _plan_cartesian_path function copies them into self.__cartesian_path_request. See here for multiple such examples.

Thus, I'd recommend reverting back to the original property setters/getters, and instead copying those values from self.__move_action_goal.request to self.__cartesian_path_request in _plan_cartesian_path. That way, we continue to maintain the same functionality regardless of whether one is using the MoveGroup action or just calling the planning service in isolation.

Zarnack commented 6 months ago

I do agree and rebased the commit. But to avoid any further confusion the setter and getter for _max_cartesianspeed should be removed and the corresponding value in the MotionPlanRequest be set to a default value (0.0) as it is currently not used within moveit2.

--> According to the message description of MotionPlanRequest.msg the parameter _cartesian_speed_limitedlink and _max_cartesianspeed requires the _default_planner_requestadapters/SetMaxCartesianEndEffectorSpeed which only exists for moveit1.

amalnanavati commented 6 months ago

Removing the setter/getter of those properties, as they are not yet used in MoveIt2, seems reasonable to me.

(Although to be clear, I am just a contributor to this library, not an author/maintainer, so I'd recommend waiting for @AndrejOrsula to weigh in before making that change, since that would be a breaking change.)

AndrejOrsula commented 6 months ago

Thank you @Zarnack for this contribution and for explaining why max_cartesian_speed does not currently function. @amalnanavati thanks as well for mentioning the pattern.

But to avoid any further confusion the setter and getter for max_cartesian_speed should be removed and the corresponding value in the MotionPlanRequest be set to a default value (0.0) as it is currently not used within moveit2.

This is a valid point. It can be removed if it is not used. Would you be able to add this change to this PR @Zarnack? Including a comment describing the reason would be appreciated. This change would then be included in the next major release.

Zarnack commented 6 months ago

Done and tested in simulation.