From cb50520e176b8fa2d76d51e079aefdc649646ae8 Mon Sep 17 00:00:00 2001 From: "Luetkebohle Ingo (CR/AEX3)" Date: Tue, 21 Apr 2020 17:35:18 +0200 Subject: [PATCH 1/4] Adapt to using rmw_service_info_t Signed-off-by: Luetkebohle Ingo (CR/AEX3) --- rcl/include/rcl/client.h | 2 +- rcl/include/rcl/service.h | 2 +- rcl/src/rcl/client.c | 2 +- rcl/src/rcl/service.c | 2 +- rcl/test/rcl/client_fixture.cpp | 2 +- rcl/test/rcl/service_fixture.cpp | 4 ++-- rcl/test/rcl/test_service.cpp | 8 ++++---- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/rcl/include/rcl/client.h b/rcl/include/rcl/client.h index ebb96b499..3d0905538 100644 --- a/rcl/include/rcl/client.h +++ b/rcl/include/rcl/client.h @@ -287,7 +287,7 @@ RCL_WARN_UNUSED rcl_ret_t rcl_take_response( const rcl_client_t * client, - rmw_request_id_t * request_header, + rmw_service_info_t * request_header, void * ros_response); /// Get the name of the service that this client will request a response from. diff --git a/rcl/include/rcl/service.h b/rcl/include/rcl/service.h index 4924eeee3..351b86016 100644 --- a/rcl/include/rcl/service.h +++ b/rcl/include/rcl/service.h @@ -247,7 +247,7 @@ RCL_WARN_UNUSED rcl_ret_t rcl_take_request( const rcl_service_t * service, - rmw_request_id_t * request_header, + rmw_service_info_t * request_header, void * ros_request); /// Send a ROS response to a client using a service. diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 889825cd8..7b0fd0319 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -291,7 +291,7 @@ rcl_send_request(const rcl_client_t * client, const void * ros_request, int64_t rcl_ret_t rcl_take_response( const rcl_client_t * client, - rmw_request_id_t * request_header, + rmw_service_info_t * request_header, void * ros_response) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Client taking service response"); diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 33f4788a4..811350b25 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -278,7 +278,7 @@ rcl_service_get_rmw_handle(const rcl_service_t * service) rcl_ret_t rcl_take_request( const rcl_service_t * service, - rmw_request_id_t * request_header, + rmw_service_info_t * request_header, void * ros_request) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Service server taking service request"); diff --git a/rcl/test/rcl/client_fixture.cpp b/rcl/test/rcl/client_fixture.cpp index a8aa9b709..bc086f4a3 100644 --- a/rcl/test/rcl/client_fixture.cpp +++ b/rcl/test/rcl/client_fixture.cpp @@ -218,7 +218,7 @@ int main(int argc, char ** argv) RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Client never became ready"); return -1; } - rmw_request_id_t header; + rmw_service_info_t header; if (rcl_take_response(&client, &header, &client_response) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( ROS_PACKAGE_NAME, "Error in send response: %s", rcl_get_error_string().str); diff --git a/rcl/test/rcl/service_fixture.cpp b/rcl/test/rcl/service_fixture.cpp index 74a1d2795..4c0d2437d 100644 --- a/rcl/test/rcl/service_fixture.cpp +++ b/rcl/test/rcl/service_fixture.cpp @@ -179,7 +179,7 @@ int main(int argc, char ** argv) { test_msgs__srv__BasicTypes_Request__fini(&service_request); }); - rmw_request_id_t header; + rmw_service_info_t header; // TODO(jacquelinekay) May have to check for timeout error codes if (rcl_take_request(&service, &header, &service_request) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( @@ -189,7 +189,7 @@ int main(int argc, char ** argv) // Sum the request and send the response. service_response.uint64_value = service_request.uint8_value + service_request.uint32_value; - if (rcl_send_response(&service, &header, &service_response) != RCL_RET_OK) { + if (rcl_send_response(&service, &header.request_id, &service_response) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( ROS_PACKAGE_NAME, "Error in send_response: %s", rcl_get_error_string().str); return -1; diff --git a/rcl/test/rcl/test_service.cpp b/rcl/test/rcl/test_service.cpp index 694d370e7..bcd8d5b69 100644 --- a/rcl/test/rcl/test_service.cpp +++ b/rcl/test/rcl/test_service.cpp @@ -205,7 +205,7 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) // Initialize a separate instance of the request and take the pending request. test_msgs__srv__BasicTypes_Request service_request; test_msgs__srv__BasicTypes_Request__init(&service_request); - rmw_request_id_t header; + rmw_service_info_t header; ret = rcl_take_request(&service, &header, &service_request); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; @@ -213,7 +213,7 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) EXPECT_EQ(2UL, service_request.uint32_value); // Simulate a response callback by summing the request and send the response.. service_response.uint64_value = service_request.uint8_value + service_request.uint32_value; - ret = rcl_send_response(&service, &header, &service_response); + ret = rcl_send_response(&service, &header.request_id, &service_response); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; test_msgs__srv__BasicTypes_Request__fini(&service_request); } @@ -223,10 +223,10 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) test_msgs__srv__BasicTypes_Response client_response; test_msgs__srv__BasicTypes_Response__init(&client_response); - rmw_request_id_t header; + rmw_service_info_t header; ret = rcl_take_response(&client, &header, &client_response); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; EXPECT_EQ(client_response.uint64_value, 3ULL); - EXPECT_EQ(header.sequence_number, 1); + EXPECT_EQ(header.request_id.sequence_number, 1); test_msgs__srv__BasicTypes_Response__fini(&client_response); } From c8864e43107a775c8a2b874f43fd3f841d069641 Mon Sep 17 00:00:00 2001 From: "Luetkebohle Ingo (CR/AEX3)" Date: Wed, 22 Apr 2020 23:17:43 +0200 Subject: [PATCH 2/4] Provide backwards compatibility. Signed-off-by: Luetkebohle Ingo (CR/AEX3) --- rcl/include/rcl/client.h | 12 +++++++++++- rcl/include/rcl/service.h | 14 ++++++++++++-- rcl/src/rcl/client.c | 15 ++++++++++++++- rcl/src/rcl/service.c | 15 ++++++++++++++- rcl/test/rcl/client_fixture.cpp | 2 +- rcl/test/rcl/service_fixture.cpp | 4 ++-- rcl/test/rcl/test_service.cpp | 4 ++-- 7 files changed, 56 insertions(+), 10 deletions(-) diff --git a/rcl/include/rcl/client.h b/rcl/include/rcl/client.h index 3d0905538..c080e92f6 100644 --- a/rcl/include/rcl/client.h +++ b/rcl/include/rcl/client.h @@ -285,11 +285,21 @@ rcl_send_request(const rcl_client_t * client, const void * ros_request, int64_t RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_take_response( +rcl_take_response_with_info( const rcl_client_t * client, rmw_service_info_t * request_header, void * ros_response); +/// backwards compatibility function that takes a rmw_request_id_t only +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_take_response( + const rcl_client_t * client, + rmw_request_id_t * request_header, + void * ros_response); + + /// Get the name of the service that this client will request a response from. /** * This function returns the client's internal service name string. diff --git a/rcl/include/rcl/service.h b/rcl/include/rcl/service.h index 351b86016..5e44c3f4b 100644 --- a/rcl/include/rcl/service.h +++ b/rcl/include/rcl/service.h @@ -232,7 +232,7 @@ rcl_service_get_default_options(void); * [1] only if required when filling the request, avoided for fixed sizes * * \param[in] service the handle to the service from which to take - * \param[inout] request_header ptr to the struct holding metadata about the request ID + * \param[inout] request_header ptr to the struct holding metadata about the request * \param[inout] ros_request type-erased ptr to an allocated ROS request message * \return `RCL_RET_OK` if the request was taken, or * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or @@ -245,11 +245,21 @@ rcl_service_get_default_options(void); RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_take_request( +rcl_take_request_with_info( const rcl_service_t * service, rmw_service_info_t * request_header, void * ros_request); +/// backwards compatibility version that takes a request_id only +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_take_request( + const rcl_service_t * service, + rmw_request_id_t * request_header, + void * ros_request); + + /// Send a ROS response to a client using a service. /** * It is the job of the caller to ensure that the type of the `ros_response` diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 7b0fd0319..ea2d2dd18 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -289,7 +289,7 @@ rcl_send_request(const rcl_client_t * client, const void * ros_request, int64_t } rcl_ret_t -rcl_take_response( +rcl_take_response_with_info( const rcl_client_t * client, rmw_service_info_t * request_header, void * ros_response) @@ -317,6 +317,19 @@ rcl_take_response( return RCL_RET_OK; } +rcl_ret_t +rcl_take_response( + const rcl_client_t * client, + rmw_request_id_t * request_header, + void * ros_response) +{ + rmw_service_info_t header; + header.request_id = *request_header; + rcl_ret_t ret = rcl_take_response_with_info(client, &header, ros_response); + *request_header = header.request_id; + return ret; +} + bool rcl_client_is_valid(const rcl_client_t * client) { diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 811350b25..6dc1c79fc 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -276,7 +276,7 @@ rcl_service_get_rmw_handle(const rcl_service_t * service) } rcl_ret_t -rcl_take_request( +rcl_take_request_with_info( const rcl_service_t * service, rmw_service_info_t * request_header, void * ros_request) @@ -308,6 +308,19 @@ rcl_take_request( return RCL_RET_OK; } +rcl_ret_t +rcl_take_request( + const rcl_service_t * service, + rmw_request_id_t * request_header, + void * ros_request) +{ + rmw_service_info_t header; + header.request_id = *request_header; + rcl_ret_t ret = rcl_take_request_with_info(service, &header, ros_request); + *request_header = header.request_id; + return ret; +} + rcl_ret_t rcl_send_response( const rcl_service_t * service, diff --git a/rcl/test/rcl/client_fixture.cpp b/rcl/test/rcl/client_fixture.cpp index bc086f4a3..650b14bf4 100644 --- a/rcl/test/rcl/client_fixture.cpp +++ b/rcl/test/rcl/client_fixture.cpp @@ -219,7 +219,7 @@ int main(int argc, char ** argv) return -1; } rmw_service_info_t header; - if (rcl_take_response(&client, &header, &client_response) != RCL_RET_OK) { + if (rcl_take_response_with_info(&client, &header, &client_response) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( ROS_PACKAGE_NAME, "Error in send response: %s", rcl_get_error_string().str); return -1; diff --git a/rcl/test/rcl/service_fixture.cpp b/rcl/test/rcl/service_fixture.cpp index 4c0d2437d..74a1d2795 100644 --- a/rcl/test/rcl/service_fixture.cpp +++ b/rcl/test/rcl/service_fixture.cpp @@ -179,7 +179,7 @@ int main(int argc, char ** argv) { test_msgs__srv__BasicTypes_Request__fini(&service_request); }); - rmw_service_info_t header; + rmw_request_id_t header; // TODO(jacquelinekay) May have to check for timeout error codes if (rcl_take_request(&service, &header, &service_request) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( @@ -189,7 +189,7 @@ int main(int argc, char ** argv) // Sum the request and send the response. service_response.uint64_value = service_request.uint8_value + service_request.uint32_value; - if (rcl_send_response(&service, &header.request_id, &service_response) != RCL_RET_OK) { + if (rcl_send_response(&service, &header, &service_response) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( ROS_PACKAGE_NAME, "Error in send_response: %s", rcl_get_error_string().str); return -1; diff --git a/rcl/test/rcl/test_service.cpp b/rcl/test/rcl/test_service.cpp index bcd8d5b69..5ad5a2a78 100644 --- a/rcl/test/rcl/test_service.cpp +++ b/rcl/test/rcl/test_service.cpp @@ -206,7 +206,7 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) test_msgs__srv__BasicTypes_Request service_request; test_msgs__srv__BasicTypes_Request__init(&service_request); rmw_service_info_t header; - ret = rcl_take_request(&service, &header, &service_request); + ret = rcl_take_request_with_info(&service, &header, &service_request); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; EXPECT_EQ(1, service_request.uint8_value); @@ -224,7 +224,7 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) test_msgs__srv__BasicTypes_Response__init(&client_response); rmw_service_info_t header; - ret = rcl_take_response(&client, &header, &client_response); + ret = rcl_take_response_with_info(&client, &header, &client_response); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; EXPECT_EQ(client_response.uint64_value, 3ULL); EXPECT_EQ(header.request_id.sequence_number, 1); From b7be43171db957d1a6c0680ce9ea6dbbbbce5625 Mon Sep 17 00:00:00 2001 From: "Luetkebohle Ingo (CR/AEX3)" Date: Fri, 24 Apr 2020 01:04:53 +0200 Subject: [PATCH 3/4] Add service test Signed-off-by: Luetkebohle Ingo (CR/AEX3) --- rcl/test/CMakeLists.txt | 4 ++++ rcl/test/rcl/test_service.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index 4e0dedb16..7a8fdc531 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -210,11 +210,15 @@ function(test_target_function) message(STATUS "Enabling message timestamp test for ${rmw_implementation}") target_compile_definitions(test_subscription${target_suffix} PUBLIC "RMW_TIMESTAMPS_SUPPORTED=1" "RMW_RECEIVED_TIMESTAMP_SUPPORTED=1") + target_compile_definitions(test_service${target_suffix} + PUBLIC "RMW_TIMESTAMPS_SUPPORTED=1" "RMW_RECEIVED_TIMESTAMP_SUPPORTED=1") else() if(rmw_implementation STREQUAL "rmw_cyclonedds_cpp") message(STATUS "Enabling only source timestamp test for ${rmw_implementation}") target_compile_definitions(test_subscription${target_suffix} PUBLIC "RMW_TIMESTAMPS_SUPPORTED=1") + target_compile_definitions(test_service${target_suffix} + PUBLIC "RMW_TIMESTAMPS_SUPPORTED=1") else() message(STATUS "Disabling message timestamp test for ${rmw_implementation}") endif() diff --git a/rcl/test/rcl/test_service.cpp b/rcl/test/rcl/test_service.cpp index 5ad5a2a78..b141ae6a8 100644 --- a/rcl/test/rcl/test_service.cpp +++ b/rcl/test/rcl/test_service.cpp @@ -228,5 +228,17 @@ TEST_F(CLASSNAME(TestServiceFixture, RMW_IMPLEMENTATION), test_service_nominal) ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; EXPECT_EQ(client_response.uint64_value, 3ULL); EXPECT_EQ(header.request_id.sequence_number, 1); +#ifdef RMW_TIMESTAMPS_SUPPORTED + EXPECT_NE(0u, header.source_timestamp); +#ifdef RMW_RECEIVED_TIMESTAMP_SUPPORTED + EXPECT_NE(0u, header.received_timestamp); +#else + EXPECT_EQ(0u, header.received_timestamp); +#endif +#else + EXPECT_EQ(0u, header.source_timestamp); + EXPECT_EQ(0u, header.received_timestamp); +#endif + test_msgs__srv__BasicTypes_Response__fini(&client_response); } From 24710fe3d72362f876f4c0355d843668fced13c9 Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Thu, 23 Apr 2020 16:47:04 -0700 Subject: [PATCH 4/4] minimize whitespace Signed-off-by: Karsten Knese --- rcl/include/rcl/client.h | 1 - rcl/include/rcl/service.h | 1 - 2 files changed, 2 deletions(-) diff --git a/rcl/include/rcl/client.h b/rcl/include/rcl/client.h index c080e92f6..df5d72b6f 100644 --- a/rcl/include/rcl/client.h +++ b/rcl/include/rcl/client.h @@ -299,7 +299,6 @@ rcl_take_response( rmw_request_id_t * request_header, void * ros_response); - /// Get the name of the service that this client will request a response from. /** * This function returns the client's internal service name string. diff --git a/rcl/include/rcl/service.h b/rcl/include/rcl/service.h index 5e44c3f4b..6c0ecb831 100644 --- a/rcl/include/rcl/service.h +++ b/rcl/include/rcl/service.h @@ -259,7 +259,6 @@ rcl_take_request( rmw_request_id_t * request_header, void * ros_request); - /// Send a ROS response to a client using a service. /** * It is the job of the caller to ensure that the type of the `ros_response`