-
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 8 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]>
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski
|
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 |
45cb87f to
43a74ea
Compare
|
This pull request is in conflict. Could you fix it @Decwest? |
43a74ea to
45cb87f
Compare
|
I was unable to fix the DCO issue. |
Doesn't work? Maybe it would be best to get your work, squash it into a single commit that you sign off on and force push back? This is an important thing we need for contributors to verify that they have permission to contribute their work to the project. Sorry for the delay, I'm not sure why this didn't come up in my notification tray. |
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.
I think you need to check linting / indents a bit, but this is MUCH improved, thank you!
nav2_regulated_pure_pursuit_controller/src/regulated_pure_pursuit_controller.cpp
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
| const std::string old_name = plugin_name_ + ".desired_linear_vel"; | ||
| const std::string new_name = plugin_name_ + ".max_linear_vel"; | ||
| const auto nan_val = std::numeric_limits<double>::quiet_NaN(); | ||
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".desired_linear_vel", rclcpp::ParameterValue(0.5)); | ||
| node, old_name, rclcpp::ParameterValue(nan_val)); | ||
| double old_val = nan_val; | ||
| if (node->get_parameter(old_name, old_val) && | ||
| !std::isnan(old_val)) | ||
| { | ||
| RCLCPP_WARN( | ||
| logger_, | ||
| "Parameter '%s' is deprecated. Use '%s' instead.", | ||
| old_name.c_str(), new_name.c_str()); | ||
| } | ||
| double default_val = std::isnan(old_val) ? 0.5 : old_val; | ||
| declare_parameter_if_not_declared( | ||
| node, plugin_name_ + ".max_linear_vel", rclcpp::ParameterValue(default_val)); |
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 totally works, no issues, but I think we can do this better
`declare_parameter_if_not_declared(node, plugin_name_ + ".desired_linear_vel", rclcpp::PARAMETER_DOUBLE);`
double max_vel_value = 0.5;
try {
node->get_parameter(plugin_name_ + ".desired_linear_vel", max_vel_value);
// Then its set, so you should log the error
} catch {
// Then its not set, so can just pass
}
declare_parameter_if_not_declared(
node, plugin_name_ + ".max_linear_vel", rclcpp::ParameterValue(max_vel_value));
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.
Sincenode->get_parameterdoes not appear to throw an exception, I tried implementing it as shown below. What do you think?
| // Declare max_linear_vel with backward compatibility |
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.
If you declare without a default value it should throw. But maybe only if you use this API?
auto my_param = get_parameter("name").as<double>();
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, I modified to use node->get_parameter(old_name).as_double(), and use try-catch statement
| auto desired_linear_vel = node->get_parameter(old_name).as_double(); |
...ler/include/nav2_regulated_pure_pursuit_controller/dynamic_window_pure_pursuit_functions.hpp
Outdated
Show resolved
Hide resolved
...ler/include/nav2_regulated_pure_pursuit_controller/dynamic_window_pure_pursuit_functions.hpp
Outdated
Show resolved
Hide resolved
7d9fbee to
6923056
Compare
|
This pull request is in conflict. Could you fix it @Decwest? |
@SteveMacenski |
|
If that works for you, I will go ahead and resolve the current conflicts. |
|
Just to short cut this issue, maybe it would be best to create a new branch & PR in which you migrate your commits over to that, sign off on it, and open a new PR to get around this. I'm not 100% what situation your git setup is in and rather than coaching you through it remotely sometimes just saying "screw this" and starting from a clean, current |
6923056 to
45cb87f
Compare
Signed-off-by: Decwest <[email protected]>
Signed-off-by: Decwest <[email protected]>
|
@SteveMacenski I’ve addressed all of your comments except for the migration guide. |

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-*.