-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use parameterized frames #1729
Use parameterized frames #1729
Conversation
Signed-off-by: ymd-stella <world.applepie@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1729 +/- ##
==========================================
+ Coverage 63.41% 64.11% +0.69%
==========================================
Files 192 192
Lines 10321 10330 +9
==========================================
+ Hits 6545 6623 +78
+ Misses 3776 3707 -69
Continue to review full report at Codecov.
|
Hi @ymd-stella, thank you for submitting the PR. LGTM |
@@ -60,6 +60,8 @@ class GoalReachedCondition : public BT::ConditionNode | |||
{ | |||
node_ = config().blackboard->get<rclcpp::Node::SharedPtr>("node"); | |||
node_->get_parameter_or<double>("goal_reached_tol", goal_reached_tol_, 0.25); | |||
node_->get_parameter_or<std::string>("global_frame", global_frame_, "map"); |
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.
How are you planning on getting these parameters?
It seems like these would better be input ports for the BT node (see this for example https://github.com/ros-planning/navigation2/blob/71f9a62d0f8dcb0267935d6c6068138b5afad3d5/nav2_behavior_tree/plugins/action/spin_action.cpp#L49) with a default value to map and base_link. Then if you wanted to change, you change the frames in the BT XML file.
I think this could work as well, but you'd need to figure out what namespace this parameter appears in to add to your config file, which actually sounds like the best situation. Figure out where this parameterization is going to be and then add it to our nav2_params.yaml file.
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'm not familiar with nav2_behavior_tree so it's going to take a bit of time.
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'd recommend just looking at this spin_dist
as a literal example. All you need to do is add a string templated BT::InputPort
to the provide input ports function. The order of args is variable name, default value, description string. Then in the constructor of the node use getInput
to get the input of the 2 variables to store. Then you're done!
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.
@ymd-stella you can also use this as a direct guide: #1741 as @naiveHobo did this for the other controller based on your request.
@@ -45,6 +45,13 @@ RecoveryServer::RecoveryServer() | |||
declare_parameter( | |||
"plugin_types", | |||
rclcpp::ParameterValue(plugin_types)); | |||
|
|||
declare_parameter( |
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.
Please add these to our nav2_params.yaml file (and the 2 multi-robot param example files) and verify changing them works properly.
Looks good, just need to make sure we expose these params in the default configs |
@naiveHobo do you mind making these updates here too to speed this along? It block #1735 which I'd like @Marwan99 to get in today. @ymd-stella had a really good start but now there's merge conflicts to resolve and I recall them mentioning they weren't very confident in their C++/navigation. |
#1742 extends and resolves issues |
Signed-off-by: ymd-stella world.applepie@gmail.com
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes