diff --git a/rclcpp/src/rclcpp/parameter_service.cpp b/rclcpp/src/rclcpp/parameter_service.cpp index 0cbb9667e4..a766696f64 100644 --- a/rclcpp/src/rclcpp/parameter_service.cpp +++ b/rclcpp/src/rclcpp/parameter_service.cpp @@ -41,12 +41,13 @@ ParameterService::ParameterService( const std::shared_ptr request, std::shared_ptr response) { - for (const auto & name : request->names) { - // Default construct param to NOT_SET - rclcpp::Parameter param; - node_params->get_parameter(name, param); - // push back NOT_SET when get_parameter() call fails - response->values.push_back(param.get_value_message()); + try { + auto parameters = node_params->get_parameters(request->names); + for (const auto param : parameters) { + response->values.push_back(param.get_value_message()); + } + } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { + RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameters: %s", ex.what()); } }, qos_profile, nullptr); @@ -67,7 +68,7 @@ ParameterService::ParameterService( return static_cast(type); }); } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { - RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to get parameter types: %s", ex.what()); + RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameter types: %s", ex.what()); } }, qos_profile, nullptr); @@ -88,7 +89,7 @@ ParameterService::ParameterService( result = node_params->set_parameters_atomically( {rclcpp::Parameter::from_parameter_msg(p)}); } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { - RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what()); + RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what()); result.successful = false; result.reason = ex.what(); } @@ -116,7 +117,7 @@ ParameterService::ParameterService( auto result = node_params->set_parameters_atomically(pvariants); response->result = result; } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { - RCLCPP_WARN( + RCLCPP_DEBUG( rclcpp::get_logger("rclcpp"), "Failed to set parameters atomically: %s", ex.what()); response->result.successful = false; response->result.reason = "One or more parameters were not declared before setting"; @@ -136,7 +137,7 @@ ParameterService::ParameterService( auto descriptors = node_params->describe_parameters(request->names); response->descriptors = descriptors; } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { - RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to describe parameters: %s", ex.what()); + RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to describe parameters: %s", ex.what()); } }, qos_profile, nullptr); diff --git a/rclcpp/test/rclcpp/test_parameter_client.cpp b/rclcpp/test/rclcpp/test_parameter_client.cpp index ba7cc99c29..c7f8ad6581 100644 --- a/rclcpp/test/rclcpp/test_parameter_client.cpp +++ b/rclcpp/test/rclcpp/test_parameter_client.cpp @@ -218,6 +218,30 @@ TEST_F(TestParameterClient, async_parameter_get_parameter_types) { ASSERT_TRUE(callback_called); } +/* + Coverage for async get_parameter_types with allow_undeclared_ enabled + */ +TEST_F(TestParameterClient, async_parameter_get_parameter_types_allow_undeclared) { + auto asynchronous_client = + std::make_shared(node_with_option); + bool callback_called = false; + auto callback = [&callback_called](std::shared_future> result) + { + if (result.valid() && result.get().size() == 1 && + result.get()[0] == rclcpp::PARAMETER_NOT_SET) + { + callback_called = true; + } + }; + std::vector names{"foo"}; + std::shared_future> future = + asynchronous_client->get_parameter_types(names, callback); + auto return_code = rclcpp::spin_until_future_complete( + node_with_option, future, std::chrono::milliseconds(100)); + ASSERT_EQ(rclcpp::FutureReturnCode::SUCCESS, return_code); + ASSERT_TRUE(callback_called); +} + /* Coverage for async get_parameters */ @@ -226,7 +250,8 @@ TEST_F(TestParameterClient, async_parameter_get_parameters) { bool callback_called = false; auto callback = [&callback_called](std::shared_future> result) { - if (result.valid() && result.get().size() == 1 && result.get()[0].get_name() == "foo") { + // We expect the result to be empty since we tried to get a parameter that didn't exist. + if (result.valid() && result.get().size() == 0) { callback_called = true; } }; @@ -239,6 +264,28 @@ TEST_F(TestParameterClient, async_parameter_get_parameters) { ASSERT_TRUE(callback_called); } +/* + Coverage for async get_parameters with allow_undeclared_ enabled + */ +TEST_F(TestParameterClient, async_parameter_get_parameters_allow_undeclared) { + auto asynchronous_client = + std::make_shared(node_with_option); + bool callback_called = false; + auto callback = [&callback_called](std::shared_future> result) + { + if (result.valid() && result.get().size() == 1 && result.get()[0].get_name() == "foo") { + callback_called = true; + } + }; + std::vector names{"foo"}; + std::shared_future> future = asynchronous_client->get_parameters( + names, callback); + auto return_code = rclcpp::spin_until_future_complete( + node_with_option, future, std::chrono::milliseconds(100)); + ASSERT_EQ(rclcpp::FutureReturnCode::SUCCESS, return_code); + ASSERT_TRUE(callback_called); +} + /* Coverage for async set_parameters_atomically */ @@ -292,12 +339,197 @@ TEST_F(TestParameterClient, async_parameter_list_parameters) { */ TEST_F(TestParameterClient, sync_parameter_get_parameter_types) { node->declare_parameter("foo", 4); + node->declare_parameter("bar", "this is bar"); auto synchronous_client = std::make_shared(node); - std::vector names{"foo"}; - std::vector parameter_types = - synchronous_client->get_parameter_types(names, 10s); - ASSERT_EQ(1u, parameter_types.size()); - ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[0]); + + { + std::vector names{"none"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(0u, parameter_types.size()); + } + + { + std::vector names{"none", "foo", "bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(0u, parameter_types.size()); + } + + { + std::vector names{"foo"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(1u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[0]); + } + + { + std::vector names{"bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(1u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_STRING, parameter_types[0]); + } + + { + std::vector names{"foo", "bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(2u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[0]); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_STRING, parameter_types[1]); + } +} + +/* + Coverage for sync get_parameter_types with allow_undeclared_ enabled + */ +TEST_F(TestParameterClient, sync_parameter_get_parameter_types_allow_undeclared) { + node_with_option->declare_parameter("foo", 4); + node_with_option->declare_parameter("bar", "this is bar"); + auto synchronous_client = std::make_shared(node_with_option); + + { + std::vector names{"none"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(1u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_NOT_SET, parameter_types[0]); + } + + { + std::vector names{"none", "foo", "bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(3u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_NOT_SET, parameter_types[0]); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[1]); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_STRING, parameter_types[2]); + } + + { + std::vector names{"foo"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(1u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[0]); + } + + { + std::vector names{"bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(1u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_STRING, parameter_types[0]); + } + + { + std::vector names{"foo", "bar"}; + std::vector parameter_types = + synchronous_client->get_parameter_types(names, 10s); + ASSERT_EQ(2u, parameter_types.size()); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_INTEGER, parameter_types[0]); + ASSERT_EQ(rclcpp::ParameterType::PARAMETER_STRING, parameter_types[1]); + } +} + +/* + Coverage for sync get_parameters + */ +TEST_F(TestParameterClient, sync_parameter_get_parameters) { + node->declare_parameter("foo", 4); + node->declare_parameter("bar", "this is bar"); + auto synchronous_client = std::make_shared(node); + + { + std::vector names{"none"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(0u, parameters.size()); + } + + { + std::vector names{"none", "foo", "bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(0u, parameters.size()); + } + + { + std::vector names{"foo"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(1u, parameters.size()); + ASSERT_EQ("foo", parameters[0].get_name()); + ASSERT_EQ(4u, parameters[0].as_int()); + } + + { + std::vector names{"bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(1u, parameters.size()); + ASSERT_EQ("bar", parameters[0].get_name()); + ASSERT_EQ("this is bar", parameters[0].as_string()); + } + + { + std::vector names{"foo", "bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(2u, parameters.size()); + ASSERT_EQ("foo", parameters[0].get_name()); + ASSERT_EQ(4u, parameters[0].as_int()); + ASSERT_EQ("bar", parameters[1].get_name()); + ASSERT_EQ("this is bar", parameters[1].as_string()); + } +} + +/* + Coverage for sync get_parameters with allow_undeclared_ enabled + */ +TEST_F(TestParameterClient, sync_parameter_get_parameters_allow_undeclared) { + node_with_option->declare_parameter("foo", 4); + node_with_option->declare_parameter("bar", "this is bar"); + auto synchronous_client = std::make_shared(node_with_option); + + { + std::vector names{"none"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(1u, parameters.size()); + } + + { + std::vector names{"none", "foo", "bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(3u, parameters.size()); + ASSERT_EQ("foo", parameters[1].get_name()); + ASSERT_EQ(4u, parameters[1].as_int()); + ASSERT_EQ("bar", parameters[2].get_name()); + ASSERT_EQ("this is bar", parameters[2].as_string()); + } + + { + std::vector names{"foo"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(1u, parameters.size()); + ASSERT_EQ("foo", parameters[0].get_name()); + ASSERT_EQ(4u, parameters[0].as_int()); + } + + { + std::vector names{"bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(1u, parameters.size()); + ASSERT_EQ("bar", parameters[0].get_name()); + ASSERT_EQ("this is bar", parameters[0].as_string()); + } + + { + std::vector names{"foo", "bar"}; + std::vector parameters = synchronous_client->get_parameters(names, 10s); + ASSERT_EQ(2u, parameters.size()); + ASSERT_EQ("foo", parameters[0].get_name()); + ASSERT_EQ(4u, parameters[0].as_int()); + ASSERT_EQ("bar", parameters[1].get_name()); + ASSERT_EQ("this is bar", parameters[1].as_string()); + } } /*