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:

ur_modern_driver/src/ros_main.cpp

Lines 118 to 119 in 77fa08a

	ros::param::param(MAX_VEL_CHANGE_ARG, args.max_vel_change, 15.0); // rad/s
	ros::param::param(MAX_VEL_CHANGE_ARG, args.max_velocity, 10.0);

If so: I would say that is probably a copy-pasta. Introduced in 1e4934a (part of Zagitta#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:

ur_modern_driver/src/ros_main.cpp

Lines 118 to 119 in 77fa08a

	ros::param::param(MAX_VEL_CHANGE_ARG, args.max_vel_change, 15.0); // rad/s
	ros::param::param(MAX_VEL_CHANGE_ARG, args.max_velocity, 10.0);

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:

ur_modern_driver/launch/ur_common.launch

Lines 32 to 33 in 77fa08a

	<!-- The max_velocity parameter is only used for debugging in the ur_driver. It's not related to actual velocity limits -->
	<arg name="max_velocity" default="10.0"/> <!-- [rad/s] -->

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]

🛎️ @henningkayser.

[gavanderhoorn]

Ping @henningkayser.