-
Notifications
You must be signed in to change notification settings - Fork 947
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
Add default velocity/acceleration scaling factors #1890
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! As you mentioned, the scaling factors should be limited to the range (0, 1]. Maybe, the Qt widget already handles that on the rviz side.
But, at least in the MoveGroupInterface you should add a corresponding clipping (and a warning).
My suggestion is:
value < 0
→ use defaultvalue > 1
→ clip to 1.0
moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp
Outdated
Show resolved
Hide resolved
...it_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui
Outdated
Show resolved
Hide resolved
I didn't know that it wasn't already handled that way. I added the check as suggested. |
Oh, I didn't check explicitly. Indeed, all the time parameterization methods to exactly the same checking (sorry for the noise!): ... falling back to a default of 1.0. I think this code should be refactored to have a common planning request adapt for time-parameterization, which in turn uses a specific time-parameterization plugin via a common interface. |
Hmm, I would prefer to keep this PR small since the default parameters will be useful regardless, and it seems like a separate issue. Would you prefer to revert the clipping inside the MoveGroupInterface or keep it? I can see arguments for either. |
robot_description_planning/joint_limits should list robot joints only.
Use YAML::Emitter syntax for all output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this PR shouldn't be further extended. I pushed several cleanup commits.
@felixvd, please check again from your point of view.
Looks great, thanks! Agree with everything and only found that typo. |
Co-Authored-By: Felix von Drigalski <FvDrigalski@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1890 +/- ##
==========================================
- Coverage 50.26% 50.25% -0.02%
==========================================
Files 313 313
Lines 24623 24672 +49
==========================================
+ Hits 12377 12398 +21
- Misses 12246 12274 +28
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for proposing this @felixvd !
Assuming all the node handles are in the correct scope to see the parameters (didn't check),
this looks quite good!
Please have a look at the inline feedback.
Cartesian trajectory scaling remains an open issue for this approach to change defaults, because the current API does not care about the scaling factors. But I don't think this has to be dealt with here.
else if (target <= 0.0) | ||
{ | ||
node_handle_.param<double>(std::string("robot_description_planning/default_") + name, variable, fallback); | ||
ROS_WARN_NAMED("move_group_interface", "max_%s <= 0.0! Setting to default: %.2f.", name, variable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this to be reasonable behavior for this API, so I would prefer this as a ROS_DEBUG
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If the user specifies a negative scaling factor, she should get a warning about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that clipping the value to (0, 1] is reasonable behavior, but I think users should be alerted if they attempt to set it outside of that range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated: shouldn't this be ROS_WARN_NAMED_STREAM
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using stream syntax would be more verbose. But, I don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If the user specifies a negative scaling factor, she should get a warning about that.
If you insist, I can agree with the negative factor.
In this case, I would prefer to have a separate branch for 0.0
though.
Otherwise there is no warning-free API to reset the factors to their defaults (aside from the Constructor)!
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
node_handle_.param<double>("robot_description_planning/joint_limits/default_velocity_scaling_factor", | ||
max_velocity_scaling_factor_, 0.1); | ||
node_handle_.param<double>("robot_description_planning/joint_limits/default_acceleration_scaling_factor", | ||
max_acceleration_scaling_factor_, 0.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is quite a number of places with hard-coded 0.1
now, but I'm not sure we have a nice place to define a constant for this.
0.1
seems overly slow to me though, I would have picked 0.5
myself.
Maybe we want to haggle over the exact number in the maintainer meeting next week?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we mostly need to decide which situation the default value should be for. In my opinion, it should be a safe ballpen mode for new users, which leaves them enough time to hit the emergency switch (which they will invariably not have in their hand) in case they accidentally send a motion request that does not do what they expected. E.g. the IK flipped and the robot is doing a large motion, or the robot hits something that wasn't represented in the environment (like the robot base), etc.
Choosing a "very safe" default value and expecting the user to adjust the speed to when they are ready is probably better than the opposite.
I tested a few parameter values with our UR5 to check which speeds would still be surprising a user in this type of situation. 0.5
is barely different from full speed for small motions, 0.2
seemed to be still pretty fast, and 0.1
seemed safe. 0.1
for a UR5 is about 10 cm/s
in most configurations, which I think is a good compromise.
I updated the PR with small changes. We can talk about the actual value in the meeting if you like. A last comment is that I think |
Actually you used |
Yes, I used |
There has to be a warning-free way to do so.
If this is good with you after your last change @v4hn, the only tests that are failing are the code coverage, so I think this is ready to merge. |
We wanted to discuss the actual numbers for the scaling factors... So not ready to merge yet. |
@felixvd: The maintainer meeting just agreed to have the default of 0.1. At the same time, we need prominent information (or better a tutorial page) on how to set the default scaling factors also for existing moveit configurations. |
I'll do it next week, after the IROS deadline :) I will add a ToDo list to the first post of this thread. |
@v4hn Is this ready for merging? |
I'll update it soon. I was still busy with the video submission deadline that is in 15 minutes. |
@v4hn Is this ready for merging?
We do need some prominent advertisement,
either a discourse announcement with some explanations or a tutorial.
Also, there should be an entry in the `MIGRATION.md` as part of this patch set.
So I want to wait for @felixvd to do this before merging.
Now that the IROS video-submission deadline is over too, he should have time to do that. 😁
|
I added a mention in I think the only other thing to do would be to post a pre-emptive ROS answers question "Why is MoveIt so slow?" |
You didn't push a change to the file as part of this request.
Could you do that please? Aside from these things, this is good to go. |
I forgot to push the branch from the office 🙄I just fixed that and rebased. The ROS answers question is here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Description
Since I was in the middle of touching adjacent code and @rhaschke mentioned (#1889) that he has no time to fully implement the feature, I quickly wrote this PR and it seems to work fine. As suggested in #1888 (@v4hn), this PR adds default velocity/acceleration scaling factors as ROS parameters to
joint_limits.yaml
so that the initial setting is0.1
for each, which should ensure safer robot motions for beginners. The parameter is retrieved from the server when starting up a MoveGroup and when adding a MotionPlanning widget in Rviz.It leaves the setting in the
.ui
file of the MotionPlanning widget somewhat redundant, but I guess it's not a big problem.I would suggest adding a note to
MIGRATION.md
, throughout the tutorials, and maybe even somewhere else so that new users are aware that the robot is not moving at full speed.Checklist