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.

Sign up for GitHub

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

Jump to bottom

Return service from node_type_description_service_init #1112

Merged
merged 5 commits into from
Oct 30, 2023
Merged

Return service from node_type_description_service_init #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43dfecc
Make the fini of the type description service explicit
mjcarroll Oct 26, 2023
3d215eb
Remove fini and get methods
mjcarroll Oct 27, 2023
90888a2
Update tests
mjcarroll Oct 27, 2023
6f7da3a
Update documentation based on reviewer feedback
mjcarroll Oct 28, 2023
25174df
Fix and document test
mjcarroll Oct 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 6 additions & 54 deletions rcl/include/rcl/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message);
* must register rcl_node_type_description_service_handle_request or a custom callback
* to handle incoming requests, via that client's executor/waitset capabilities.
*
* This will initialize the node's type cache, if it has not been initialized already.
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
* Note that the returned service must be cleaned up by the caller by calling
* rcl_service_fini.
*
* <hr>
* Attribute | Adherence
Expand All @@ -572,6 +573,7 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message);
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] service the handle to the type description service to be initialized
* \param[in] node handle to the node for which to initialize the service
* \return #RCL_RET_OK if the service was successfully initialized, or
* \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
Expand All @@ -581,59 +583,9 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message);
*/
RCL_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node);

/// Finalizes the node's ~/get_type_description service.
/**
* This function finalizes the node's private ~/get_type_description service.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] node the handle to the node whose type cache should be initialized
* \return #RCL_RET_OK if service was deinitialized successfully, or
* \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
* \return #RCL_RET_SERVICE_INVALID if the service is invalid, or
* \return #RCL_RET_NODE_INVALID if the node is invalid, or
* \return #RCL_RET_ERROR if an unspecified error occurs.
*/
RCL_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node);


/// Returns a pointer to the node's ~/get_type_description service.
/**
* On success, sets service_out to the initialized service.
* rcl_node_type_description_service_init must be called before this.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] node the handle to the node
* \param[out] service_out Handle to pointer that will be set
* \return #RCL_RET_OK if valid service was returned successfully, or
* \return #RCL_RET_NODE_INVALID if node is invalid, or
* \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
* \return #RCL_RET_NOT_INIT if the service hasn't yet been initialized, or
* \return #RCL_RET_ERROR if an unspecified error occurs.
*/
RCL_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t rcl_node_get_type_description_service(
const rcl_node_t * node,
rcl_service_t ** service_out);

rcl_ret_t rcl_node_type_description_service_init(
rcl_service_t * service,
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
const rcl_node_t * node);

/// Process a single pending request to the GetTypeDescription service.
/**
Expand Down
48 changes: 7 additions & 41 deletions rcl/src/rcl/node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ rcl_node_init(
node->impl, "allocating memory failed", ret = RCL_RET_BAD_ALLOC; goto fail);
node->impl->options = rcl_node_get_default_options();
node->impl->registered_types_by_type_hash = rcutils_get_zero_initialized_hash_map();
node->impl->get_type_description_service = rcl_get_zero_initialized_service();
node->context = context;
// Initialize node impl.
ret = rcl_node_options_copy(options, &(node->impl->options));
Expand Down Expand Up @@ -580,14 +579,15 @@ void rcl_node_type_description_service_handle_request(
response->successful = true;
}

rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node)
rcl_ret_t rcl_node_type_description_service_init(
rcl_service_t * service,
const rcl_node_t * node)
{
RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID);

rcl_ret_t ret;

if (rcl_service_is_valid(&node->impl->get_type_description_service)) {
if (rcl_service_is_valid(service)) {
return RCL_RET_ALREADY_INIT;
}
rcl_reset_error(); // Reset the error message set by rcl_service_is_valid()
Expand All @@ -601,7 +601,7 @@ rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node)
rcl_allocator_t allocator = node->context->impl->allocator;

// Construct service name
ret = rcl_node_resolve_name(
rcl_ret_t ret = rcl_node_resolve_name(
node, "~/get_type_description",
allocator, true, true, &service_name);
if (RCL_RET_OK != ret) {
Expand All @@ -612,43 +612,9 @@ rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node)

// Initialize service
ret = rcl_service_init(
&node->impl->get_type_description_service, node,
service, node,
type_support, service_name, &service_ops);
allocator.deallocate(service_name, allocator.state);

return ret;
}

rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node)
{
RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID);
if (!rcl_service_is_valid(&node->impl->get_type_description_service)) {
rcl_reset_error();
return RCL_RET_NOT_INIT;
}

const rcl_ret_t ret =
rcl_service_fini(&node->impl->get_type_description_service, node);
if (RCL_RET_OK == ret) {
node->impl->get_type_description_service = rcl_get_zero_initialized_service();
}

return ret;
}

rcl_ret_t rcl_node_get_type_description_service(
const rcl_node_t * node,
rcl_service_t ** service_out)
{
RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID);
RCL_CHECK_ARGUMENT_FOR_NULL(service_out, RCL_RET_SERVICE_INVALID);

if (!rcl_service_is_valid(&node->impl->get_type_description_service)) {
return RCL_RET_NOT_INIT;
}

*service_out = &node->impl->get_type_description_service;
return RCL_RET_OK;
}
1 change: 0 additions & 1 deletion rcl/src/rcl/node_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ struct rcl_node_impl_s
const char * logger_name;
const char * fq_name;
rcutils_hash_map_t registered_types_by_type_hash;
rcl_service_t get_type_description_service;
};

#endif // RCL__NODE_IMPL_H_
57 changes: 37 additions & 20 deletions rcl/test/rcl/test_get_type_description_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,6 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi
rcl_node_options_t node_options = rcl_node_get_default_options();
ret = rcl_node_init(this->node_ptr, name, "", this->context_ptr, &node_options);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ret = rcl_node_type_description_service_init(node_ptr);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

const char * node_fqn = rcl_node_get_fully_qualified_name(this->node_ptr);
snprintf(
Expand All @@ -174,9 +172,7 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi

void TearDown()
{
rcl_ret_t ret = rcl_node_type_description_service_fini(node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ret = rcl_node_fini(this->node_ptr);
rcl_ret_t ret = rcl_node_fini(this->node_ptr);
delete this->node_ptr;
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ret = rcl_shutdown(this->context_ptr);
Expand All @@ -197,23 +193,30 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi
TEST_F(
CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION),
test_service_init_and_fini_functions) {
rcl_service_t service = rcl_get_zero_initialized_service();

// Service does not initially exist
EXPECT_TRUE(
service_exists(
service_not_exists(
this->node_ptr, this->get_type_description_service_name,
GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5)));
EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_fini(this->node_ptr));

// Once the type descrition service is init, then it appear in the graph
EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr));
EXPECT_TRUE(
service_not_exists(
service_exists(
this->node_ptr, this->get_type_description_service_name,
GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5)));
EXPECT_EQ(RCL_RET_NOT_INIT, rcl_node_type_description_service_fini(this->node_ptr));

EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(this->node_ptr));
// Once the type description service is fini, then it no longer appears in the graph
EXPECT_EQ(RCL_RET_OK, rcl_service_fini(&service, this->node_ptr));
EXPECT_TRUE(
service_exists(
service_not_exists(
this->node_ptr, this->get_type_description_service_name,
GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5)));
EXPECT_EQ(RCL_RET_ALREADY_INIT, rcl_node_type_description_service_init(this->node_ptr));

// Repeatedly destroying the service should not cause faults.
EXPECT_EQ(RCL_RET_OK, rcl_service_fini(&service, this->node_ptr));
}

/* Basic nominal test of the ~/get_type_description service. */
Expand All @@ -222,6 +225,10 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no
const rosidl_service_type_support_t * ts = ROSIDL_GET_SRV_TYPE_SUPPORT(
type_description_interfaces, srv, GetTypeDescription);

// Create type description service.
auto service = rcl_get_zero_initialized_service();
EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr));

// Create client.
rcl_client_t client = rcl_get_zero_initialized_client();
rcl_client_options_t client_options = rcl_client_get_default_options();
Expand All @@ -233,6 +240,9 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no
{
rcl_ret_t ret = rcl_client_fini(&client, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_service_fini(&service, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
});
ASSERT_TRUE(wait_for_server_to_be_available(this->node_ptr, &client, 10, 1000));

Expand Down Expand Up @@ -262,8 +272,8 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no

// This scope simulates handling request in a different context
{
auto service = &node_ptr->impl->get_type_description_service;
ASSERT_TRUE(wait_for_service_to_be_ready(service, context_ptr, 10, 100));
auto * service_ptr = &service;
ASSERT_TRUE(wait_for_service_to_be_ready(service_ptr, context_ptr, 10, 100));

type_description_interfaces__srv__GetTypeDescription_Response service_response;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
Expand All @@ -278,7 +288,7 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no
type_description_interfaces__srv__GetTypeDescription_Request__fini(&service_request);
});
rmw_service_info_t header;
ret = rcl_take_request_with_info(service, &header, &service_request);
ret = rcl_take_request_with_info(service_ptr, &header, &service_request);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

rcl_node_type_description_service_handle_request(
Expand All @@ -287,7 +297,7 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no
&service_request,
&service_response);

ret = rcl_send_response(service, &header.request_id, &service_response);
ret = rcl_send_response(service_ptr, &header.request_id, &service_response);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

Expand Down Expand Up @@ -315,6 +325,10 @@ TEST_F(
const rosidl_service_type_support_t * ts = ROSIDL_GET_SRV_TYPE_SUPPORT(
type_description_interfaces, srv, GetTypeDescription);

// Create type description service.
auto service = rcl_get_zero_initialized_service();
EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr));

// Create client.
rcl_client_t client = rcl_get_zero_initialized_client();
rcl_client_options_t client_options = rcl_client_get_default_options();
Expand All @@ -326,6 +340,9 @@ TEST_F(
{
rcl_ret_t ret = rcl_client_fini(&client, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_service_fini(&service, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
});
ASSERT_TRUE(wait_for_server_to_be_available(this->node_ptr, &client, 10, 1000));

Expand All @@ -346,8 +363,8 @@ TEST_F(

// This scope simulates handling request in a different context
{
auto service = &node_ptr->impl->get_type_description_service;
ASSERT_TRUE(wait_for_service_to_be_ready(service, context_ptr, 10, 100));
auto * service_ptr = &service;
ASSERT_TRUE(wait_for_service_to_be_ready(service_ptr, context_ptr, 10, 100));

type_description_interfaces__srv__GetTypeDescription_Response service_response;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
Expand All @@ -362,7 +379,7 @@ TEST_F(
type_description_interfaces__srv__GetTypeDescription_Request__fini(&service_request);
});
rmw_service_info_t header;
ret = rcl_take_request_with_info(service, &header, &service_request);
ret = rcl_take_request_with_info(service_ptr, &header, &service_request);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

rcl_node_type_description_service_handle_request(
Expand All @@ -371,7 +388,7 @@ TEST_F(
&service_request,
&service_response);

ret = rcl_send_response(service, &header.request_id, &service_response);
ret = rcl_send_response(service_ptr, &header.request_id, &service_response);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

Expand Down