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

Added remapping resolution for action names #1170

Open
wants to merge 6 commits into
base: rolling
Choose a base branch
from
Open

Added remapping resolution for action names #1170

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
44dcad3
Added remapping resolution for action names
JustusBraun Jul 14, 2024
9498aa4
Fix cpplint/uncrustify
JustusBraun Jul 16, 2024
95b0fba
Simplified returned error codes in case name resolution failes.
JustusBraun Jul 17, 2024
e3b6fe4
Renamed action_name field to remapped_action_name.
JustusBraun Jul 18, 2024
3c216d6
Removed unnecessary resolved_action_name stack variable
JustusBraun Jul 23, 2024
bc2fdd5
Added tests for action name remapping.
JustusBraun Jul 27, 2024
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
28 changes: 25 additions & 3 deletions rcl_action/src/rcl_action/action_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ _rcl_action_client_fini_impl(
// \internal Initializes an action client specific service client.
#define CLIENT_INIT(Type) \
char * Type ## _service_name = NULL; \
ret = rcl_action_get_ ## Type ## _service_name(action_name, allocator, &Type ## _service_name); \
ret = rcl_action_get_ ## Type ## _service_name( \
resolved_action_name, allocator, &Type ## _service_name); \
if (RCL_RET_OK != ret) { \
rcl_reset_error(); \
RCL_SET_ERROR_MSG("failed to get " #Type " service name"); \
Expand Down Expand Up @@ -145,7 +146,8 @@ _rcl_action_client_fini_impl(
// \internal Initializes an action client specific topic subscription.
#define SUBSCRIPTION_INIT(Type) \
char * Type ## _topic_name = NULL; \
ret = rcl_action_get_ ## Type ## _topic_name(action_name, allocator, &Type ## _topic_name); \
ret = rcl_action_get_ ## Type ## _topic_name( \
resolved_action_name, allocator, &Type ## _topic_name); \
if (RCL_RET_OK != ret) { \
rcl_reset_error(); \
RCL_SET_ERROR_MSG("failed to get " #Type " topic name"); \
Expand Down Expand Up @@ -212,8 +214,19 @@ rcl_action_client_init(

// Avoid uninitialized pointers should initialization fail
*action_client->impl = _rcl_action_get_zero_initialized_client_impl();

// Remap/Expand the action name
char * resolved_action_name = NULL;
ret = rcl_node_resolve_name(node, action_name, allocator, false, false, &resolved_action_name);
JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
if (RCL_RET_OK != ret) {
if (RCL_RET_TOPIC_NAME_INVALID == ret || RCL_RET_SERVICE_NAME_INVALID == ret) {
ret = RCL_RET_ACTION_NAME_INVALID;
}
goto fail;
JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
}

JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
// Copy action client name and options.
action_client->impl->action_name = rcutils_strdup(action_name, allocator);
action_client->impl->action_name = rcutils_strdup(resolved_action_name, allocator);
if (NULL == action_client->impl->action_name) {
RCL_SET_ERROR_MSG("failed to duplicate action name");
ret = RCL_RET_BAD_ALLOC;
Expand All @@ -230,6 +243,10 @@ rcl_action_client_init(
SUBSCRIPTION_INIT(feedback);
SUBSCRIPTION_INIT(status);

// The resolved action name is no longer needed
allocator.deallocate(resolved_action_name, allocator.state);
resolved_action_name = NULL;

JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
if (RCL_RET_OK != rcl_node_type_cache_register_type(
node, type_support->get_type_hash_func(type_support),
type_support->get_type_description_func(type_support),
Expand All @@ -244,6 +261,11 @@ rcl_action_client_init(
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client initialized");
return ret;
fail:
// Deallocate the resolved action name
if (NULL != resolved_action_name) {
allocator.deallocate(resolved_action_name, allocator.state);
}

fini_ret = _rcl_action_client_fini_impl(action_client, node, allocator);
if (RCL_RET_OK != fini_ret) {
RCL_SET_ERROR_MSG("failed to cleanup action client");
Expand Down
26 changes: 23 additions & 3 deletions rcl_action/src/rcl_action/action_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ rcl_action_get_zero_initialized_server(void)

#define SERVICE_INIT(Type) \
char * Type ## _service_name = NULL; \
ret = rcl_action_get_ ## Type ## _service_name(action_name, allocator, &Type ## _service_name); \
ret = rcl_action_get_ ## Type ## _service_name( \
resolved_action_name, allocator, &Type ## _service_name); \
if (RCL_RET_OK != ret) { \
if (RCL_RET_BAD_ALLOC == ret) { \
ret = RCL_RET_BAD_ALLOC; \
Expand Down Expand Up @@ -80,7 +81,8 @@ rcl_action_get_zero_initialized_server(void)

#define PUBLISHER_INIT(Type) \
char * Type ## _topic_name = NULL; \
ret = rcl_action_get_ ## Type ## _topic_name(action_name, allocator, &Type ## _topic_name); \
ret = rcl_action_get_ ## Type ## _topic_name( \
resolved_action_name, allocator, &Type ## _topic_name); \
if (RCL_RET_OK != ret) { \
if (RCL_RET_BAD_ALLOC == ret) { \
ret = RCL_RET_BAD_ALLOC; \
Expand Down Expand Up @@ -163,6 +165,16 @@ rcl_action_server_init(
action_server->impl->type_hash = rosidl_get_zero_initialized_type_hash();

rcl_ret_t ret = RCL_RET_OK;
// Resolve action name
char * resolved_action_name = NULL;
ret = rcl_node_resolve_name(node, action_name, allocator, false, false, &resolved_action_name);
if (RCL_RET_OK != ret) {
if (RCL_RET_TOPIC_NAME_INVALID == ret || RCL_RET_SERVICE_NAME_INVALID == ret) {
ret = RCL_RET_ACTION_NAME_INVALID;
}
JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
goto fail;
}

// Initialize services
SERVICE_INIT(goal);
SERVICE_INIT(cancel);
Expand All @@ -189,12 +201,16 @@ rcl_action_server_init(
}

// Copy action name
action_server->impl->action_name = rcutils_strdup(action_name, allocator);
action_server->impl->action_name = rcutils_strdup(resolved_action_name, allocator);
JustusBraun marked this conversation as resolved.
Show resolved Hide resolved
if (NULL == action_server->impl->action_name) {
ret = RCL_RET_BAD_ALLOC;
goto fail;
}

// The resolved_action_name is no longer needed
allocator.deallocate(resolved_action_name, allocator.state);
resolved_action_name = NULL;

// Store type hash
if (RCL_RET_OK != rcl_node_type_cache_register_type(
node, type_support->get_type_hash_func(type_support),
Expand All @@ -210,6 +226,10 @@ rcl_action_server_init(
return ret;
fail:
{
// Deallocate the resolved action name
if (NULL != resolved_action_name) {
allocator.deallocate(resolved_action_name, allocator.state);
}
// Finalize any services/publishers that were initialized and deallocate action_server->impl
rcl_ret_t ret_throwaway = rcl_action_server_fini(action_server, node);
// Since there is already a failure, it is likely that finalize will error on one or more of
Expand Down
2 changes: 1 addition & 1 deletion rcl_action/test/rcl_action/test_action_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ class TestActionClientFixture : public TestActionClientBaseFixture
TestActionClientBaseFixture::TearDown();
}

const char * const action_name = "test_action_client_name";
const char * const action_name = "/test_action_client_name";
rcl_action_client_options_t action_client_options;
rcl_action_client_t invalid_action_client;
rcl_action_client_t action_client;
Expand Down
2 changes: 1 addition & 1 deletion rcl_action/test/rcl_action/test_action_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ TEST_F(TestActionServer, test_action_server_get_action_name)
// Get action_name for a valid action server
action_name = rcl_action_server_get_action_name(&this->action_server);
ASSERT_NE(action_name, nullptr) << rcl_get_error_string().str;
EXPECT_STREQ(action_name, "test_action_server_name");
EXPECT_STREQ(action_name, "/test_action_server_name");
}

TEST_F(TestActionServer, test_action_server_get_options)
Expand Down