search

rst-tu-dortmund / teb_local_planner

An optimal trajectory planner considering distinctive topologies for mobile robots based on Timed-Elastic-Bands (ROS Package)
http://wiki.ros.org/teb_local_planner
BSD 3-Clause "New" or "Revised" License
985 stars 546 forks source link

[fix] only limit with max_vel_trans for holonomic vehicles #355

Closed remco-r closed 2 years ago

remco-r commented 2 years ago

a new parameter max_vel_trans was added in commit f737130

According to the documentation it should not affect for non-holonomic robots. But this was not implemented, and my robots vel_x was limited nevertheless. Maximum linear velocity of the robot. Ignore this parameter for non-holonomic robots (max_vel_trans == max_vel_x).

corot commented 2 years ago

mmm... the idea was to have a top limit for all linear velocities, overruling both x and y max vel! but the comment is completely wrong. I propose to replace this PR with rewriting the comment with

"Maximum linear velocity of the robot, overruling both max_vel_x, max_vel_y and its linear combination."

renan028 commented 2 years ago

mmm... the idea was to have a top limit for all linear velocities, overruling both x and y max vel! but the comment is completely wrong. I propose to replace this PR with rewriting the comment with

"Maximum linear velocity of the robot, overruling both max_vel_x, max_vel_y and its linear combination."

I see. The problem is that, for non-holonomic robots, we now need to set both max_vel_x and max_vel_trans to the same value, and as @corot said the comment is wrong. I suggest to change the default value of max_vel_trans to 0 or a negative value, and check the value inside the parameter reconfiguration. If max_vel_trans <= 0 do max_vel_trans = max_vel_x. @remco-r what do you think?

remco-r commented 2 years ago

We agree the parameter documentation has to be updated. I included a commit to do that.

I can imagine that many users forget to set the max_vel_trans variable, and try to figure out why their robot is going slow (as i did). Especially people with a running configuration that update the package. And with this fix it does become backwards compatible for the non-holonomic case. So i have included a commit for this as well.