-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Centralize path handler logic in controller server #5446
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
base: main
Are you sure you want to change the base?
Conversation
|
@SteveMacenski before I go too far, can you take a quick look to make sure I am on the right track? I have only pushed the changes in nav2_controller, graceful_controller and RPP |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
I have spent some time reading into MPPI's path handler, I can see there are some improvements like #3659, #3443. Also, there is a related PR for RPP in #4763, maybe we need to check if these changes are applicable to all controllers? If yes, I guess it will be easier to move them in Controller Server I also noticed that the DWB/MPPI need the path from planner to find its goal, should we pass this plan via the setplan api or a new argument in computevelocitycommands? |
|
Hi, sorry to jump in here but just sharing my two cents: |
I think path_handler implementation from MPPI is more comprehensive as it also considers path up to inversion point. This would also be helpful when user uses Hybrid A* with reeds-shepp as the global planner. |
Thanks for sharing, I will try to implement this and compare with my current method |
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
SteveMacenski
left a comment
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.
Sorry it took so long!
| goal_checker_loader_("nav2_core", "nav2_core::GoalChecker"), | ||
| default_goal_checker_ids_{"goal_checker"}, | ||
| default_goal_checker_types_{"nav2_controller::SimpleGoalChecker"}, | ||
| default_goal_checker_types_{"nav2_controller::PathCompleteGoalChecker"}, |
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 by 'default' I mean that this should just be built into the simple goal checker so that this just given to everyone 'for free'
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.
So I should remove this new plugin and add the path_length_tolerance to simple goal checker, do I understand this correctly?
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.
Correct!
I agree with this general sentiment. I think it would be nice to review all the path handlers and make sure the key features of all are being respected in the server's implementation. There's a number of CI failures that should also be looked at, but I'm sure you know :-) |
Yes, I will continue this PR once #5496 and other future PRs targeting path handler are done |
145b2d9 to
ea45821
Compare
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
@SteveMacenski I think you can start reviewing this when you have time, in the following days I plan to:
Things to debate:
I think this becomes problematic when enforce_path_inversion is set to true. In that case, the local goal ends up being the cusping point, and the robot may stop there once it satisfies the XY and yaw tolerances. Because of this, I believe we still need the global goal for the goal checker. But as @SteveMacenski suggested, we could add this directly into the simple goal checker, so by default the simple goal checker is checking both the global goal and local path length. Do you agree with this approach? @mamariomiamo
Previously, the transform tolerance in the controller server was obtained from
Before merging this, I also need to:
|
|
@mini-1235, your PR has failed to build. Please check CI outputs and resolve issues. |
|
OK will do! This is admittedly alot so this next round is going to be more high level.
Ah ok. That makes sense for the goal checker, though probably not the other elements right? When enforcing inversions is off, its the same either way since we just prune to the prune distance / costmap bounds. When is it enabled, then we prune to that which we want from the control algorithm's perspective, but not the goal checker.
Why not have it still acquire it from costmap2D, just stored and used at the controller server level? I suppose I prefer less parameters to more, but I'm OK either way.
Do both - first after the integrated distance, as long as that distance is within the costmap bounds. |
| */ | ||
| ~ParameterHandler(); | ||
|
|
||
| std::mutex & getMutex() {return mutex_;} |
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.
Is this being properly locked when needed in the main function(s) so that dynamic parameters aren't being updated during execution?
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.
Yes I forgot to add this, will add it in the next update
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.
@mini-1235 is this still open?
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 think so, I have added in ControllerServer L424
Yes, when it is on, the distance to the local goal can become very small. That's why I think this local goal is not suitable to use in the goal checker
For the controller plugins as well?
Not sure if I understand this, can you elaborate? |
Isn't that in effect what is done today?
Basically combine the two: navigation2/nav2_controller/src/path_handler.cpp Lines 134 to 146 in 3404f3a
|
|
A good suggestion I received today: It would be great to make the path handler itself a plugin so that other users can replace this implementation if they see fit. A nice side effect is it makes us have to think a bit more about the API interfaces for it to make sure its generally good for a broad range of purposes and include all the information for the APIs other implementations may want that we have access to. Another would be possibly changing the pruning distance to be proportional to time instead, to prune the distance set out by that time by the maximum velocity. That way it wasn't something that needed to be tuned for changing velocity limits. Finally, maybe it would be good to provide feedback about the current idx of the path we're on to use in the BT Navigator for computing ETA, path length remaining, and so forth. That way we handle the issue of path looping using the wrong index globally |
|
I will think about it and reply tomorrow |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
I think I have addressed most of the review comments here, but there are two items that I want to discuss.
navigation2/nav2_rotation_shim_controller/src/nav2_rotation_shim_controller.cpp Lines 273 to 281 in 8dbc929
I am not sure if we actually have use cases where forward_sampling_distance is large enough and need the raw global path here. geometry_msgs::msg::TwistStamped computeVelocityCommands(
const geometry_msgs::msg::PoseStamped & pose,
const geometry_msgs::msg::Twist & velocity,
nav2_core::GoalChecker * goal_checker,
nav2_core::PathHandler * path_handler)
I am thinking how we can benefit from this API change, especially after #5387 is merged. For example: auto [start, end] = path_handler.findPlanSegmentIterators() -> return the first and last point of the local plan in map frame
nav_msgs::msg::Path transformed_plan = path_handler.transformLocalPlan(start,end) -> transform that segment to odom frame and check if it is inside costmapThen, // Use robot pose and local plan in odom frame
tracking_feedback_msg->tracking_error = distance_to_path_segment(robot_pose, transformed_plan[0], transformed_plan[1]);
// global plan and start pose in map frame
tracking_feedback_msg->current_path_index = std::distance(global_plan.poses.begin(), start);Is that your understanding as well? |
|
Hi, vacation and ROSCon and all - on this now. Can you summarize what you think are the next steps here? The thread here is long and I've not thought about this in long enough it would be great to have a summary of what's left or if you think this is good to go with another review. Can you take a look at the merge conflicts? They're quite large now |
|
This pull request is in conflict. Could you fix it @mini-1235? |
nav2_bringup/params/nav2_params.yaml
Outdated
| yaw_goal_tolerance: 0.25 | ||
| path_length_tolerance: 1.0 | ||
| path_handler: | ||
| plugin: "nav2_controller::SimplePathHandler" |
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.
Can we workshop a better name here? I don't think this is particularly 'simple'.
- FeasiblePathHandler -- since we consider rotation / cusp pruning
- PruningPathHandler -- since we prune from a distance away and to the robot's current position
FeasiblePruningPathHandler? Its a mouthful though.
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 will go with FeasiblePathHandler
SteveMacenski
left a comment
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.
A couple more items. I have approved 73/95 files (and I think ~6 are just the yaml files for renaming the "Simple" path handler)
| path.poses[idx - 1].pose.orientation != | ||
| path.poses[idx].pose.orientation) |
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.
Would it be at all sensible to have an option for any of the controllers to skip the rotations in place as a inversion point? I don't think most of the controllers had this condition before. How do the controllers act when a rotate in place command is found from a cusp like this?
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.
What about setting it to -1 here
navigation2/nav2_controller/plugins/simple_path_handler.cpp
Lines 58 to 59 in b221638
| minimum_rotation_angle_ = node->declare_or_get_parameter(plugin_name + ".minimum_rotation_angle", | |
| -1.0); |
If it's -1, then we skip the in place rotation check
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.
Or do you prefer to add enforce_rotation, I will need to add a new argument here then
navigation2/nav2_util/include/nav2_util/path_utils.hpp
Lines 85 to 87 in 6ac89a4
| inline unsigned int findFirstPathConstraint( | |
| nav_msgs::msg::Path & path, | |
| float rotation_threshold) |
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 that could make sense to pull in the work from the other PR, yes :-)
| source_pose.header.stamp = input_path.header.stamp; | ||
| source_pose.pose = input_pose.pose; | ||
|
|
||
| if (!nav2_util::transformPoseInTargetFrame( |
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.
Doesn't tf2_geometry_msgs include some transform templates for path types? I dont' think we need to iterate for each pose and do the transform there. We can use TF2
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 checked the geometry2 repository but couldn't find any helpers for nav_msgs/msg/Path, tf2_geometry_msgs only support individual pose types, I think
| in_rotation_ = false; | ||
| auto cmd_vel = primary_controller_->computeVelocityCommands(pose, velocity, goal_checker); | ||
| auto cmd_vel = primary_controller_->computeVelocityCommands(pose, velocity, goal_checker, | ||
| transformed_global_plan, global_goal); |
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.
| transformed_global_plan, global_goal); | |
| transformed_global_plan, global_goal); |
| * @class nav2_controller::ParameterHandler | ||
| * @brief Handles parameters and dynamic parameters for Controller Server | ||
| */ | ||
| class ParameterHandler |
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.
use your new template type?
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
This pull request is in conflict. Could you fix it @mini-1235? |
|
@SteveMacenski, before I continue, can I get some feedback here and here 😃, I think these are the last few things blocking this PR |
Done #5446 (comment) From #5446 (comment):
I agree. We can use the pruned, transformed one given in |
This PR continues the work in #4789
Fixes #4757
Fixes #4304