Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added vel and acc scaling to GetCartesianPath.srv on Humble #184

Open
wants to merge 1 commit into
base: humble
Choose a base branch
from

Conversation

liamcarlos
Copy link

@liamcarlos liamcarlos commented Oct 30, 2024

I'm adding these to match a pull request on moveit/moveit2 to add velocity and acceleration control to cartesian path planning on the Humble branch.

I found this portion on the Iron branch here but couldn't find references to them in moveit/moveit2 so I just left them out if anybody's wondering.

# Maximum cartesian speed for the given link.
# If max_cartesian_speed <= 0 the trajectory is not modified. string cartesian_speed_limited_link
float64 max_cartesian_speed # m/s

I'm adding these to match a pull request on moveit/moveit2 to add velocity and acceleration control to cartesian path planning on the Humble branch.

I found this portion on the Iron branch here but couldn't find references to them in moveit/moveit2 so I just left them out if anybody's wondering.

# Maximum cartesian speed for the given link.
# If max_cartesian_speed <= 0 the trajectory is not modified.
string cartesian_speed_limited_link
float64 max_cartesian_speed # m/s
@liamcarlos
Copy link
Author

Hey @rhaschke can we merge this to the humble branch so humble users can use the velocity and acceleration scaling options for cartesian path planning? This coincides with this pr #3051, but merging this one wouldn't break anything if done first right?

@rhaschke
Copy link
Contributor

rhaschke commented Nov 18, 2024

I just comment on ROS2 and leave merging to other maintainers: @sjahr, @sea-bass.
Having said this, I'm not sure they accept message changes in Humble as that breaks existing API.

@liamcarlos
Copy link
Author

@rhaschke These changes wouldn't break the current API since these new variables aren't being used at all on the Humble branch for Cartesian Planning. If the pull request I put up here (#3051) got merged first, then things would break. Regardless, Iron has all of these changes I'm suggesting and has already passed its EOL, so I don't understand why these changes can't be made for Humble which has 2 more years until EOL. Shouldn't the Humble branch still be maintained?

@sea-bass
Copy link

Maintained as in bug fixes? Yes.

But modifying function interfaces and message types generally is not done on non-Rolling branches

@liamcarlos
Copy link
Author

@sea-bass This is a bug. Set scaled velocity and acceleration would work if they were added to this srv file and implemented in cartesian_path_service_capability.cpp and move_group_interface.cpp Otherwise, Humble users have no control over the speed and acceleration of their arms using cartesian planning.. Is there any good reason for omitting this control?

@mikeferguson
Copy link
Contributor

Changing message definitions also changes the checksum on them - so if a sender and receiver are not both on the exact same set of messages, they can't actually communicate (and so the effect is very similar to API/ABI changes).

MoveIt has an official policy on API/ABI stability that was adopted after quite a bit of discussion at developer meetings - you can find it here: https://moveit.ai/moveit%202/ros/2023/05/31/balancing-stability-and-development.html - it also has some details about how to use the main branch on older distributions.

@liamcarlos
Copy link
Author

Hey @mikeferguson I've tried using the main branch with humble as suggested here but I'm getting an error about an undefined reference in the libmoveit_warehouse library when I try to build my move group action server. Have you guys seen this issue before?

/usr/bin/ld: /opt/ros/humble/lib/libmoveit_warehouse.so.2.7.4: undefined reference to `warehouse_ros::DatabaseLoader::~DatabaseLoader()'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants