Duplicate and inconsistent use of max_velocity parameter

[miguelprada]

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.

[gavanderhoorn]

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.

[miguelprada]

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.

[gavanderhoorn]

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.

[gavanderhoorn]

Let's wait on @henningkayser a bit.

[miguelprada]

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.

[gavanderhoorn]

:bellhop_bell: @henningkayser.

[gavanderhoorn]

Ping @henningkayser.