- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.6k
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. | 
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>
| Oops, forgot to format files | 
| I think  | 
| It didn't used to do that ... I wonder if the ament_uncrustify format got corrupted in that case. it didn't used to do that and that was the profile in use. is there anything above you want me to take a look at (except that last comment)? I wasn't sure if I was waiting or you wanted me to take a look again. | 
| This pull request is in conflict. Could you fix it @mini-1235? | 
| 
 I am not familiar with  
 Yes, please take another look when you have time. I have updated things accordingly | 
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
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.
Also check out CI, a number of tests don't have results or have failures
There are a number of open items above (make sure to expand the hidden topics since this is so long) and I commented on all of them with responses (if not closing due to completion).
| params_file = os.path.join(nav2_bringup_dir, 'params/nav2_params.yaml') | ||
|  | ||
| # Replace the `use_astar` setting on the params file | ||
| param_substitutions = { | 
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.
Why this change?
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.
This substitutes the parameter prune_distance and forward_prune_distance in DWB. Since we are centralizing it in controller server, I don't think we have these two parameters anymore in DWB
| xy_goal_tolerance: 0.25 | ||
| yaw_goal_tolerance: 0.25 | ||
| 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.
Add some params to this to be fully described? (here, other similar files in nav2 tests + Nav2 bringup)
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.
Will add in next update
| row_3_label_layout_->addWidget(new QLabel("Progress Checker")); | ||
| row_3_layout_->addWidget(progress_checker_); | ||
| row_3_label_layout_->addWidget(new QLabel("Path Handler")); | ||
| row_3_layout_->addWidget(path_handler_); | 
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.
Does the layout need to adapt due to the additional field (i.e. larger size?)
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.
| results4); | ||
| } | ||
|  | ||
| class TransformGlobalPlanTest : public ::testing::Test | 
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.
Were all the relevant tests for full coverage of the new plugin transferred?
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.
Will add in next update
| double SimplePathHandler::getCostmapMaxExtent() const | ||
| { | ||
| const double max_costmap_dim_meters = std::max( | ||
| costmap_ros_->getCostmap()->getSizeInCellsX(), | 
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 that semantic difference is really being used here -- I think you can simply use the Meters API
| */ | ||
| ~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.
@mini-1235 is this still open?
| 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.
Correct!
| current_path_ = path; | ||
| primary_controller_->setPlan(path); | ||
| path_updated_ = rotate_to_heading_once_ ? isGoalChanged(raw_global_path) : true; | ||
| current_path_ = raw_global_path; | 
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.
Should we use the new computeVelocityCommands for this instead for setting the current path?
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 this is a special case where the controller needs the full global plan, no?
navigation2/nav2_rotation_shim_controller/src/nav2_rotation_shim_controller.cpp
Lines 273 to 281 in 8dbc929
| // Find the first point at least sampling distance away | |
| for (unsigned int i = 1; i != current_path_.poses.size(); i++) { | |
| dx = current_path_.poses[i].pose.position.x - start.position.x; | |
| dy = current_path_.poses[i].pose.position.y - start.position.y; | |
| if (hypot(dx, dy) >= forward_sampling_distance_) { | |
| current_path_.poses[i].header.frame_id = current_path_.header.frame_id; | |
| current_path_.poses[i].header.stamp = clock_->now(); // Get current time transformation | |
| return current_path_.poses[i]; | |
| } | 
| auto transformed_plan = path_handler_->transformGlobalPlan( | ||
| pose, params_->max_robot_pose_search_dist, params_->interpolate_curvature_after_goal); | ||
| global_path_pub_->publish(transformed_plan); | ||
| // Transform the plan from costmap's global frame to robot base frame | 
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.
This process seems like it might be a nice utility to have in the path handler plugin itself. Given a path, convert it to another frame (i.e. the base frame) at a timestamp. If not in the path handler class, then maybe as a path utils for nav2_utils
| goal_dist_tolerance_ = pose_tolerance.position.x; | ||
| } | ||
|  | ||
| // Transform the plan from costmap's global frame to robot base frame | 
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.
That util / path handler API could then be used here too!
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>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

This PR continues the work in #4789
Fixes #4757
Fixes #4304