Skip to content
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

Ignore global parameters when spawning sub-nodes #851

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Jun 13, 2019

Fixes #842
To test, run the nav stack with node name overrides in your launch file.

rotu added 4 commits June 13, 2019 14:07
options.start_parameter_services(false);
options.start_parameter_event_publisher(false);
return rclcpp::Node::make_shared(generate_internal_node_name(prefix));
// options.start_parameter_services(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer removing commented-out code. I'm generally not a fan of leaving commented-out code around for documentation purposes, but would instead rather have it in the revision history and keep the current code as clean as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -46,7 +46,8 @@ BtNavigator::on_configure(const rclcpp_lifecycle::State & /*state*/)
auto node = shared_from_this();

// Support for handling the topic-based goal pose from rviz
client_node_ = std::make_shared<rclcpp::Node>("bt_navigator_client_node");
client_node_ =
std::make_shared<rclcpp::Node>("bt_navigator_client_node", rclcpp::NodeOptions().use_global_arguments(false));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the code is exceeding 100 columns here and will fail the cpplint tool. Please run ament_cpplint and ament_uncrustify. We're trying to get all code to pass cleanly.

@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #851 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #851   +/-   ##
======================================
  Coverage    20.5%   20.5%           
======================================
  Files         181     181           
  Lines        9305    9305           
  Branches     2259    2258    -1     
======================================
  Hits         1908    1908           
- Misses       6319    6321    +2     
+ Partials     1078    1076    -2
Impacted Files Coverage Δ
src/navigation2/nav2_util/src/lifecycle_node.cpp 77.77% <0%> (-4.58%) ⬇️
...2/nav2_lifecycle_manager/src/lifecycle_manager.cpp 16.66% <0%> (-0.26%) ⬇️
...on2/nav2_util/include/nav2_util/service_client.hpp 25% <0%> (ø) ⬆️
...navigation2/nav2_bt_navigator/src/bt_navigator.cpp 0% <0%> (ø) ⬆️
...navigation2/nav2_costmap_2d/src/costmap_2d_ros.cpp 0% <0%> (ø) ⬆️
src/navigation2/nav2_util/src/node_utils.cpp 68% <0%> (+9.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d141ffd...0f3cb80. Read the comment docs.

@rotu
Copy link
Contributor Author

rotu commented Jun 14, 2019

Scott raised some good points about this, and I have a better solution in the pipeline. Should I change this pull request or close and create a new one?

@rotu rotu closed this Jun 14, 2019
@rotu rotu mentioned this pull request Jun 14, 2019
@rotu rotu deleted the subnode_ignore_global_args branch June 14, 2019 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nav2 Nodes cannot be cleanly renamed
2 participants