From 77193ed70b4e68ec9703a41dbb1362becf382916 Mon Sep 17 00:00:00 2001 From: zhenpeng ge Date: Thu, 16 Sep 2021 11:34:27 +0800 Subject: [PATCH 1/2] add use_global_arguments for node options of component nodes Signed-off-by: zhenpeng ge keep use_global_arguments false default Signed-off-by: zhenpeng ge update warning message Signed-off-by: zhenpeng ge update warnning message Signed-off-by: zhenpeng ge add test Signed-off-by: zhenpeng ge --- rclcpp_components/src/component_manager.cpp | 12 ++++ .../test/test_component_manager_api.cpp | 56 +++++++++++++++++-- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/rclcpp_components/src/component_manager.cpp b/rclcpp_components/src/component_manager.cpp index cc2157f0d5..2fa3195381 100644 --- a/rclcpp_components/src/component_manager.cpp +++ b/rclcpp_components/src/component_manager.cpp @@ -160,6 +160,18 @@ ComponentManager::create_node_options(const std::shared_ptr r "Extra component argument 'use_intra_process_comms' must be a boolean"); } options.use_intra_process_comms(extra_argument.get_value()); + } else if (extra_argument.get_name() == "use_global_arguments") { + if (extra_argument.get_type() != rclcpp::ParameterType::PARAMETER_BOOL) { + throw ComponentManagerException( + "Extra component argument 'use_global_arguments' must be a boolean"); + } + options.use_global_arguments(extra_argument.get_value()); + if (extra_argument.get_value()) { + RCLCPP_WARN( + get_logger(), "use_global_arguments is true by default in nodes, but is not " + "recommended in a component manager. If true, this will cause this node's behavior " + "to be influenced by global arguments, not only those targeted at this node."); + } } } diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 84b11fe7e9..41244a650c 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -211,6 +211,50 @@ TEST_F(TestComponentManager, components_api) EXPECT_EQ(result->unique_id, 0u); } + { + // use_global_arguments + auto request = std::make_shared(); + request->package_name = "rclcpp_components"; + request->plugin_name = "test_rclcpp_components::TestComponentFoo"; + request->node_name = "test_component_global_arguments"; + rclcpp::Parameter use_global_arguments("use_global_arguments", + rclcpp::ParameterValue(true)); + request->extra_arguments.push_back(use_global_arguments.to_parameter_msg()); + + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); + EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); + EXPECT_EQ(result->success, true); + EXPECT_EQ(result->error_message, ""); + std::cout << result->full_node_name << std::endl; + EXPECT_EQ(result->full_node_name, "/test_component_global_arguments"); + EXPECT_EQ(result->unique_id, 7u); + } + + { + // use_global_arguments is not a bool type parameter + auto request = std::make_shared(); + request->package_name = "rclcpp_components"; + request->plugin_name = "test_rclcpp_components::TestComponentFoo"; + request->node_name = "test_component_global_arguments_str"; + + rclcpp::Parameter use_global_arguments("use_global_arguments", + rclcpp::ParameterValue("hello")); + request->extra_arguments.push_back(use_global_arguments.to_parameter_msg()); + + auto future = composition_client->async_send_request(request); + auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. + auto result = future.get(); + EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); + EXPECT_EQ(result->success, false); + EXPECT_EQ( + result->error_message, + "Extra component argument 'use_global_arguments' must be a boolean"); + EXPECT_EQ(result->full_node_name, ""); + EXPECT_EQ(result->unique_id, 0u); + } + auto node_names = node->get_node_names(); auto find_in_nodes = [node_names](std::string name) { @@ -239,20 +283,22 @@ TEST_F(TestComponentManager, components_api) auto result_node_names = result->full_node_names; auto result_unique_ids = result->unique_ids; - EXPECT_EQ(result_node_names.size(), 6u); + EXPECT_EQ(result_node_names.size(), 7u); EXPECT_EQ(result_node_names[0], "/test_component_foo"); EXPECT_EQ(result_node_names[1], "/test_component_bar"); EXPECT_EQ(result_node_names[2], "/test_component_baz"); EXPECT_EQ(result_node_names[3], "/ns/test_component_bing"); EXPECT_EQ(result_node_names[4], "/test_component_remap"); EXPECT_EQ(result_node_names[5], "/test_component_intra_process"); - EXPECT_EQ(result_unique_ids.size(), 6u); + EXPECT_EQ(result_node_names[6], "/test_component_global_arguments"); + EXPECT_EQ(result_unique_ids.size(), 7u); EXPECT_EQ(result_unique_ids[0], 1u); EXPECT_EQ(result_unique_ids[1], 2u); EXPECT_EQ(result_unique_ids[2], 3u); EXPECT_EQ(result_unique_ids[3], 4u); EXPECT_EQ(result_unique_ids[4], 5u); EXPECT_EQ(result_unique_ids[5], 6u); + EXPECT_EQ(result_unique_ids[6], 7u); } } @@ -306,18 +352,20 @@ TEST_F(TestComponentManager, components_api) auto result_node_names = result->full_node_names; auto result_unique_ids = result->unique_ids; - EXPECT_EQ(result_node_names.size(), 5u); + EXPECT_EQ(result_node_names.size(), 6u); EXPECT_EQ(result_node_names[0], "/test_component_bar"); EXPECT_EQ(result_node_names[1], "/test_component_baz"); EXPECT_EQ(result_node_names[2], "/ns/test_component_bing"); EXPECT_EQ(result_node_names[3], "/test_component_remap"); EXPECT_EQ(result_node_names[4], "/test_component_intra_process"); - EXPECT_EQ(result_unique_ids.size(), 5u); + EXPECT_EQ(result_node_names[5], "/test_component_global_arguments"); + EXPECT_EQ(result_unique_ids.size(), 6u); EXPECT_EQ(result_unique_ids[0], 2u); EXPECT_EQ(result_unique_ids[1], 3u); EXPECT_EQ(result_unique_ids[2], 4u); EXPECT_EQ(result_unique_ids[3], 5u); EXPECT_EQ(result_unique_ids[4], 6u); + EXPECT_EQ(result_unique_ids[5], 7u); } } } From 32f449ea3f30cc89bc99768e87718f91fec4f433 Mon Sep 17 00:00:00 2001 From: zhenpeng ge Date: Tue, 7 Dec 2021 09:59:29 +0800 Subject: [PATCH 2/2] use forward_global_arguments instead of use_global_arguments Signed-off-by: zhenpeng ge --- rclcpp_components/src/component_manager.cpp | 6 +++--- .../test/test_component_manager_api.cpp | 15 +++++++-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/rclcpp_components/src/component_manager.cpp b/rclcpp_components/src/component_manager.cpp index 2fa3195381..18949cd36e 100644 --- a/rclcpp_components/src/component_manager.cpp +++ b/rclcpp_components/src/component_manager.cpp @@ -160,15 +160,15 @@ ComponentManager::create_node_options(const std::shared_ptr r "Extra component argument 'use_intra_process_comms' must be a boolean"); } options.use_intra_process_comms(extra_argument.get_value()); - } else if (extra_argument.get_name() == "use_global_arguments") { + } else if (extra_argument.get_name() == "forward_global_arguments") { if (extra_argument.get_type() != rclcpp::ParameterType::PARAMETER_BOOL) { throw ComponentManagerException( - "Extra component argument 'use_global_arguments' must be a boolean"); + "Extra component argument 'forward_global_arguments' must be a boolean"); } options.use_global_arguments(extra_argument.get_value()); if (extra_argument.get_value()) { RCLCPP_WARN( - get_logger(), "use_global_arguments is true by default in nodes, but is not " + get_logger(), "forward_global_arguments is true by default in nodes, but is not " "recommended in a component manager. If true, this will cause this node's behavior " "to be influenced by global arguments, not only those targeted at this node."); } diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 41244a650c..e7cfab502c 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -212,14 +212,14 @@ TEST_F(TestComponentManager, components_api) } { - // use_global_arguments + // forward_global_arguments auto request = std::make_shared(); request->package_name = "rclcpp_components"; request->plugin_name = "test_rclcpp_components::TestComponentFoo"; request->node_name = "test_component_global_arguments"; - rclcpp::Parameter use_global_arguments("use_global_arguments", + rclcpp::Parameter forward_global_arguments("forward_global_arguments", rclcpp::ParameterValue(true)); - request->extra_arguments.push_back(use_global_arguments.to_parameter_msg()); + request->extra_arguments.push_back(forward_global_arguments.to_parameter_msg()); auto future = composition_client->async_send_request(request); auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. @@ -227,21 +227,20 @@ TEST_F(TestComponentManager, components_api) EXPECT_EQ(ret, rclcpp::FutureReturnCode::SUCCESS); EXPECT_EQ(result->success, true); EXPECT_EQ(result->error_message, ""); - std::cout << result->full_node_name << std::endl; EXPECT_EQ(result->full_node_name, "/test_component_global_arguments"); EXPECT_EQ(result->unique_id, 7u); } { - // use_global_arguments is not a bool type parameter + // forward_global_arguments is not a bool type parameter auto request = std::make_shared(); request->package_name = "rclcpp_components"; request->plugin_name = "test_rclcpp_components::TestComponentFoo"; request->node_name = "test_component_global_arguments_str"; - rclcpp::Parameter use_global_arguments("use_global_arguments", + rclcpp::Parameter forward_global_arguments("forward_global_arguments", rclcpp::ParameterValue("hello")); - request->extra_arguments.push_back(use_global_arguments.to_parameter_msg()); + request->extra_arguments.push_back(forward_global_arguments.to_parameter_msg()); auto future = composition_client->async_send_request(request); auto ret = exec->spin_until_future_complete(future, 5s); // Wait for the result. @@ -250,7 +249,7 @@ TEST_F(TestComponentManager, components_api) EXPECT_EQ(result->success, false); EXPECT_EQ( result->error_message, - "Extra component argument 'use_global_arguments' must be a boolean"); + "Extra component argument 'forward_global_arguments' must be a boolean"); EXPECT_EQ(result->full_node_name, ""); EXPECT_EQ(result->unique_id, 0u); }