diff --git a/rclcpp/include/rclcpp/parameter_client.hpp b/rclcpp/include/rclcpp/parameter_client.hpp index 490c72e9b1..7f5fc7f288 100644 --- a/rclcpp/include/rclcpp/parameter_client.hpp +++ b/rclcpp/include/rclcpp/parameter_client.hpp @@ -391,6 +391,33 @@ class SyncParametersClient ); } + /// Get parameter values, reporting the wait outcome so failures are distinguishable. + /** + * Behaves like get_parameters(parameter_names, timeout), but additionally stores the + * outcome of waiting for the service response in \p result. The other overloads return + * an empty vector both when the server has no matching parameters and when the request + * does not complete (timeout or interruption); this overload lets callers tell those + * cases apart. On any non-SUCCESS result the returned vector is empty. + * + * \param[in] parameter_names the names of the parameters to get. + * \param[in] timeout how long to wait for the response before giving up. + * \param[out] result the outcome of waiting for the response. + * \return the parameter values, or an empty vector if \p result is not SUCCESS. + */ + template + std::vector + get_parameters( + const std::vector & parameter_names, + std::chrono::duration timeout, + rclcpp::FutureReturnCode & result) + { + return get_parameters( + parameter_names, + std::chrono::duration_cast(timeout), + result + ); + } + RCLCPP_PUBLIC bool has_parameter(const std::string & parameter_name); @@ -583,6 +610,13 @@ class SyncParametersClient const std::vector & parameter_names, std::chrono::nanoseconds timeout); + RCLCPP_PUBLIC + std::vector + get_parameters( + const std::vector & parameter_names, + std::chrono::nanoseconds timeout, + rclcpp::FutureReturnCode & result); + RCLCPP_PUBLIC std::vector describe_parameters( diff --git a/rclcpp/src/rclcpp/parameter_client.cpp b/rclcpp/src/rclcpp/parameter_client.cpp index e2f6a5b3c6..fd8a7146b3 100644 --- a/rclcpp/src/rclcpp/parameter_client.cpp +++ b/rclcpp/src/rclcpp/parameter_client.cpp @@ -390,14 +390,22 @@ std::vector SyncParametersClient::get_parameters( const std::vector & parameter_names, std::chrono::nanoseconds timeout) +{ + rclcpp::FutureReturnCode result; + return get_parameters(parameter_names, timeout, result); +} + +std::vector +SyncParametersClient::get_parameters( + const std::vector & parameter_names, + std::chrono::nanoseconds timeout, + rclcpp::FutureReturnCode & result) { auto f = async_parameters_client_->get_parameters(parameter_names); using rclcpp::executors::spin_node_until_future_complete; - if ( - spin_node_until_future_complete( - *executor_, node_base_interface_, f, - timeout) == rclcpp::FutureReturnCode::SUCCESS) - { + result = spin_node_until_future_complete( + *executor_, node_base_interface_, f, timeout); + if (result == rclcpp::FutureReturnCode::SUCCESS) { return f.get(); } // Return an empty vector if unsuccessful diff --git a/rclcpp/test/rclcpp/test_parameter_client.cpp b/rclcpp/test/rclcpp/test_parameter_client.cpp index d0fa5b20f9..c1aa843f68 100644 --- a/rclcpp/test/rclcpp/test_parameter_client.cpp +++ b/rclcpp/test/rclcpp/test_parameter_client.cpp @@ -445,6 +445,50 @@ TEST_F(TestParameterClient, sync_parameter_get_parameter_types_allow_undeclared) /* Coverage for sync get_parameters */ +// Regression for ros2/rclcpp#200: the get_parameters() overload that reports the wait +// outcome lets callers distinguish a request that did not complete (TIMEOUT/INTERRUPTED) +// from a successful response that happens to be empty. The plain overload returns an +// empty vector for both, conflating the two. +TEST_F(TestParameterClient, sync_parameter_get_parameters_reports_result) { + node->declare_parameter("foo", 4); + auto synchronous_client = std::make_shared(node); + + { + // SUCCESS with a value. + rclcpp::FutureReturnCode result; + std::vector names{"foo"}; + std::vector parameters = + synchronous_client->get_parameters(names, 10s, result); + EXPECT_EQ(rclcpp::FutureReturnCode::SUCCESS, result); + ASSERT_EQ(1u, parameters.size()); + EXPECT_EQ(4, parameters[0].as_int()); + } + + { + // SUCCESS but empty: the server responded, the parameter just does not exist. + rclcpp::FutureReturnCode result; + std::vector names{"does_not_exist"}; + std::vector parameters = + synchronous_client->get_parameters(names, 10s, result); + EXPECT_EQ(rclcpp::FutureReturnCode::SUCCESS, result); + EXPECT_EQ(0u, parameters.size()); + } + + { + // TIMEOUT: no parameter service exists for this remote node, so the request does + // not complete. The returned vector is empty, but result distinguishes it from the + // SUCCESS-but-empty case above. + auto client_to_missing = + std::make_shared(node, "non_existent_remote_node"); + rclcpp::FutureReturnCode result; + std::vector names{"foo"}; + std::vector parameters = + client_to_missing->get_parameters(names, 200ms, result); + EXPECT_EQ(rclcpp::FutureReturnCode::TIMEOUT, result); + EXPECT_EQ(0u, parameters.size()); + } +} + TEST_F(TestParameterClient, sync_parameter_get_parameters) { node->declare_parameter("foo", 4); node->declare_parameter("bar", "this is bar");