-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Implementation of Dynamic Window Pure Pursuit (DWPP) #5591
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
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.
@doisyg FYI - would this be of interest? See the video
Also, please add unit testing for the functions and features you added. Check out test/ in the package to see some examples. Your main function should be tested for all edge cases at the bare minimum
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
Absolutely ! @Decwest, we were discussing internally your DWPP repo (at Dexory) and I am super happy that you guys are working toward a nav2 integration. |
…n2 into feature/implement_dwpp
Signed-off-by: Decwest <[email protected]>
This reverts commit 53c5cd4.
This reverts commit 547c54e.
Signed-off-by: Decwest <[email protected]>
Codecov Report❌ Patch coverage is
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Let me know when you want me to take a look again after the review comments are addressed! Only things at a glance: Your very large block of code may be more concise as or I'd suggest reviewing your code a bit for how it can be as concise and self-descriptive as possible |
|
@SteveMacenski |
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@Decwest, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
Let me know when I should take a look again! Note that you should probably rebase and/or pull in main so that CI can pass :-) |
|
@Decwest, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: Decwest <[email protected]>
…, add test (WIP) Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@Decwest, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski Hi, In addition, I have a couple of points I’d like to ask about:
Thank you very much for your time and support. |
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski friendly ping:) |
|
Please fix the merge conflict Sorry for the delay. ROSCon + vacation took me out of the office for some time and takes a moment to get up to speed on all the PRs again that have piled up This looks much better now though! |
| | `min_linear_vel` | The minimum linear velocity to use. | | ||
| | `max_angular_vel` | The maximum angular velocity to use. | | ||
| | `min_angular_vel` | The minimum angular velocity to use. | | ||
| | `max_linear_accel` | The maximum linear acceleration to use. | | ||
| | `max_linear_decel` | The maximum linear deceleration to use. | | ||
| | `max_angular_accel` | The maximum angular acceleration to use. | | ||
| | `max_angular_decel` | The maximum angular deceleration to use. | |
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.
Mention any that are specific to your technique
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 added "used when use_dynamic_window is true" except max_linear_vel and max_angular_accel
| cost_scaling_gain: 1.0 | ||
| inflation_cost_scaling_factor: 3.0 | ||
| min_distance_to_obstacle: 0.0 | ||
| use_dynamic_window: true |
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.
Default this to false please
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, I set use_dynamic_window's default value in parameter_handler.cpp and setting in README as false.
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, I set proper default values in nav2.docs.org
ros-navigation/docs.nav2.org#802
| logger_ = logger; | ||
|
|
||
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".desired_linear_vel", rclcpp::ParameterValue(0.5)); |
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.
We should have a migration log here. If desired_linear_vel is set, log a warning that it has been changed to max_linear_vel and that they should migrate the use.
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 modified that if user uses desired_linear_vel, set max_linear_vel value equals to desired_linear_vel and print a migration log.
https://github.com/Decwest/navigation2/blob/feature/implement_dwpp/nav2_regulated_pure_pursuit_controller/src/parameter_handler.cpp#L39-L56
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".max_linear_decel", rclcpp::ParameterValue(0.5)); | ||
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".max_angular_accel", rclcpp::ParameterValue(1.0)); |
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.
Make these reasonable, we had defaults here already, do not override them. Ditto for all the acceleration settings 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.
I set these same as current RPP's default value.
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".min_linear_vel", rclcpp::ParameterValue(0.0)); | ||
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".max_angular_vel", rclcpp::ParameterValue(1.0)); |
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.
Ditto defaults here, see other controllers for sensible defaults
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 set these same as velocity smoother's default values
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
Outdated
Show resolved
Hide resolved
| // considering velocity and acceleration constraints (DWPP) | ||
| const double regulated_linear_vel = linear_vel; | ||
| geometry_msgs::msg::Twist current_speed; | ||
| if (params_->velocity_feedback == "CLOSED_LOOP") { |
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.
Shouldn't be be made of use more generally if we're enabling open vs closed loop? This is not the only place speed is utilized
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.
Indeed, since the current velocity is also used in other parts of RPP, for example when computing the look ahead distance, we should have an issue or a PR to discuss OPEN_LOOP vs CLOSED_LOOP at the level of the entire RPP controller.
For now, I am using OPEN_LOOP in the Dynamic Window computation.
Signed-off-by: Fumiya Ohnishi <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski I have one issue that I would like to ask for your advice on. However, after syncing Nav2 to the latest version today and building and running it in this environment, I encountered a segmentation fault, and Nav2 no longer starts. Below is a detailed description of the situation.
ROLLING ubuntu:~/ros2_ws$ ros2 run nav2_controller controller_server \
--ros-args --log-level debug \
-r __ns:=/ \
-p use_sim_time=False
[DEBUG] [1764069038.870090908] [rclcpp]: signal handler installed
[DEBUG] [1764069038.870102907] [rclcpp]: deferred_signal_handler(): waiting for SIGINT/SIGTERM or uninstall
[DEBUG] [1764069038.870473289] [rcl]: Initializing node 'controller_server' in namespace ''
[DEBUG] [1764069038.870496899] [rcl]: Using domain ID of '0'
[ros2run]: Segmentation faultFrom this, it seems that the process runs at least up to
If you have any ideas or suggestions, I would be grateful to hear them. |
|
Unfortunately I'm not sure - it sounds like you took some of the right debugging steps to try to isolate the issue. The next steps I would try are to compile with debug flags and run with GDB to see what that says. This tutorial may be helpful: https://docs.nav2.org/tutorials/docs/get_backtrace.html If its the core docker image, which it sounds like it may be, the right answer would be try to use an older version. Another option would be to build that image yourself locally and see if that does the trick. |
|
We also have a nav2 docker that hasn't been updated in some time and we keep older images around for 30 or 60 days so you should be able to find one from October even after I get it turning over right now https://github.com/ros-navigation/nav2_docker (annoyingly github disables jobs after 30 days of the repo being inactive, even though its job is fully automated so there's not really any active pushes required for a year at time) |
|
Thank you very much for providing such helpful information. After trying a very, very large number of things, I found that building BehaviorTree.CPP from source resolved the issue described above. GDB did not produce any readable clues, and neither the Rolling version nor the Nav2 version had anything to do with the issue. Anyway, the clue that led me to the solution came from the fact that the nav2_docker you introduced builds BehaviorTree.CPP from source. I would like to express my sincere gratitude once again. I will continue addressing the comments you provided. |
|
Ah, I know why: BehaviorTree/BehaviorTree.CPP#1028. It supposedly was resolved but needs to be synced up to the binaries to resolve. It sounds like the issue wasn't a controller server crash but a BT Navigator crash. |
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski I have one question. |

Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Added Parameters
"OPEN_LOOP": Uses the last commanded velocity (recommended)"CLOSED_LOOP": Uses odometry velocity (may hinder proper acceleration/deceleration)Description of how this change was tested
Simulation video
DWPP_simulation.mp4
Future work that may be required in bullet points
For Maintainers:
backport-*.