ur_modern_driver
ur_modern_driver copied to clipboard
ros-industrial
Duplicate and inconsistent use of max_velocity parameter
The max_velocity parameter, if found, is used to set both ActionServer::max_velocity_ and VelocityInterface::max_vel_change_ (a.k.a. max acceleration?).
This doesn't make much sense, and to make matters worse, the default values provided for when the parameter is not found are different for both variables.
Are you referring to this:
https://github.com/ros-industrial/ur_modern_driver/blob/77fa08ae9c846344310d3b50824a7affdc3eda47/src/ros_main.cpp#L118-L119
If so: I would say that is probably a copy-pasta. Introduced in 1e4934a1 (part of https://github.com/Zagitta/ur_modern_driver/pull/1), that should most likely not have reused that parameters name.
@henningkayser: you submitted that PR. Do you remember why you used MAX_VEL_CHANGE_ARG there? That's definitely not right.
Are you referring to this:
https://github.com/ros-industrial/ur_modern_driver/blob/77fa08ae9c846344310d3b50824a7affdc3eda47/src/ros_main.cpp#L118-L119
Yes.
Furthermore, looking a bit deeper, those two lines attempt to read from max_vel_change parameter, which is not set by any of the launch files. Instead, the launch files set max_velocity parameter, which is not read anywhere.
Having something check that incoming trajectories do not violate the maximum allowable velocity before executing them is probably a good idea. I'd like to see that functionality kept/restored/(re)implemented.
As noted here:
https://github.com/ros-industrial/ur_modern_driver/blob/77fa08ae9c846344310d3b50824a7affdc3eda47/launch/ur_common.launch#L32-L33
that is not a safety feature, but more a sanity-check.
Edit: also: this is not related to maximum joint velocities, but more an additional sanity check that makes sure incoming trajectories are not corrupted / malformed / illegal. It's a carry-over from ur_driver.
Having something check that incoming trajectories do not violate the maximum allowable velocity before executing them is probably a good idea. I'd like to see that functionality kept/restored/(re)implemented.
Definitely, yes.