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

Duplicate and inconsistent use of max_velocity parameter #290

Open
miguelprada opened this issue Apr 10, 2019 · 7 comments
Open

Duplicate and inconsistent use of max_velocity parameter #290

miguelprada opened this issue Apr 10, 2019 · 7 comments
Labels
bug kinetic Issues with the refactor in Kinetic more-info-needed

Comments

@miguelprada
Copy link
Contributor

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
Copy link
Member

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 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
Copy link
Contributor Author

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.

@miguelprada miguelprada added the kinetic Issues with the refactor in Kinetic label Apr 10, 2019
@gavanderhoorn
Copy link
Member

gavanderhoorn commented Apr 10, 2019

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
Copy link
Member

Let's wait on @henningkayser a bit.

@miguelprada
Copy link
Contributor Author

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
Copy link
Member

🛎️ @henningkayser.

@gavanderhoorn
Copy link
Member

Ping @henningkayser.

@gavanderhoorn gavanderhoorn added the wrid19 World ROS-Industrial Day 2019 label Jun 29, 2019
@gavanderhoorn gavanderhoorn removed the wrid19 World ROS-Industrial Day 2019 label Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug kinetic Issues with the refactor in Kinetic more-info-needed
Projects
None yet
Development

No branches or pull requests

2 participants