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

Remove domain_id and localhost_only from node_options #708

Merged
merged 6 commits into from
Jul 8, 2020
Merged

Remove domain_id and localhost_only from node_options #708

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ff2c77
Delete domain_id and localhost_only from node API
ivanpauno Jul 6, 2020
cfbfad7
Adapt tests
ivanpauno Jul 6, 2020
d309e2a
Delete usage of domain_id in node_options
ivanpauno Jul 6, 2020
6669c06
Add const qualifiers of functions
ivanpauno Jul 6, 2020
c0858f6
Add missing const qualifier
ivanpauno Jul 7, 2020
4cf9b22
Add error message if assertion fails
ivanpauno Jul 7, 2020
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
2 changes: 1 addition & 1 deletion rcl/include/rcl/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ rcl_context_fini(rcl_context_t * context);
RCL_PUBLIC
RCL_WARN_UNUSED
const rcl_init_options_t *
rcl_context_get_init_options(rcl_context_t * context);
rcl_context_get_init_options(const rcl_context_t * context);

/// Returns an unsigned integer that is unique to the given context, or `0` if invalid.
/**
Expand Down
2 changes: 1 addition & 1 deletion rcl/include/rcl/init_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ rcl_init_options_fini(rcl_init_options_t * init_options);
RCL_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t
rcl_init_options_get_domain_id(rcl_init_options_t * init_options, size_t * domain_id);
rcl_init_options_get_domain_id(const rcl_init_options_t * init_options, size_t * domain_id);

/// Set a domain id in the init options provided.
/**
Expand Down
13 changes: 0 additions & 13 deletions rcl/include/rcl/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,6 @@ typedef struct rcl_node_options_t
/// If true, no parameter infrastructure will be setup.
// bool no_parameters;

/// If set, then this value overrides the ROS_DOMAIN_ID environment variable.
/**
* It defaults to RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID, which will cause the
* node to use the ROS domain ID set in the ROS_DOMAIN_ID environment
* variable, or on some systems 0 if the environment variable is not set.
*
* \todo TODO(wjwwood):
* Should we put a limit on the ROS_DOMAIN_ID value, that way we can have
* a safe value for the default RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID?
* (currently max size_t)
*/
size_t domain_id;

/// Custom allocator used for internal allocations.
rcl_allocator_t allocator;

Expand Down
2 changes: 1 addition & 1 deletion rcl/src/rcl/context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ rcl_context_fini(rcl_context_t * context)
// See `rcl_shutdown()` for invalidation of the context.

const rcl_init_options_t *
rcl_context_get_init_options(rcl_context_t * context)
rcl_context_get_init_options(const rcl_context_t * context)
{
RCL_CHECK_ARGUMENT_FOR_NULL(context, NULL);
RCL_CHECK_FOR_NULL_WITH_MSG(context->impl, "context is zero-initialized", return NULL);
Expand Down
16 changes: 5 additions & 11 deletions rcl/src/rcl/node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ extern "C"

#include "rcl/arguments.h"
#include "rcl/error_handling.h"
#include "rcl/domain_id.h"
#include "rcl/init_options.h"
#include "rcl/localhost.h"
#include "rcl/logging.h"
#include "rcl/logging_rosout.h"
Expand Down Expand Up @@ -121,7 +121,6 @@ rcl_node_init(
const rcl_node_options_t * options)
{
size_t domain_id = 0;
rmw_localhost_only_t localhost_only = RMW_LOCALHOST_ONLY_DEFAULT;
const rmw_guard_condition_t * rmw_graph_guard_condition = NULL;
rcl_guard_condition_options_t graph_guard_condition_options =
rcl_guard_condition_get_default_options();
Expand Down Expand Up @@ -254,21 +253,16 @@ rcl_node_init(
RCL_CHECK_FOR_NULL_WITH_MSG(
node->impl->logger_name, "creating logger name failed", goto fail);

domain_id = node->impl->options.domain_id;
if (RCL_DEFAULT_DOMAIN_ID == domain_id) {
if (RCL_RET_OK != rcl_get_default_domain_id(&domain_id)) {
goto fail;
}
ret = rcl_init_options_get_domain_id(rcl_context_get_init_options(context), &domain_id);
if (RCL_RET_OK != ret) {
goto fail;
}
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id);
node->impl->actual_domain_id = domain_id;

localhost_only = context->impl->init_options.impl->rmw_init_options.localhost_only;

node->impl->rmw_node_handle = rmw_create_node(
&(node->context->impl->rmw_context),
name, local_namespace_, domain_id,
localhost_only == RMW_LOCALHOST_ONLY_ENABLED);
name, local_namespace_);

RCL_CHECK_FOR_NULL_WITH_MSG(
node->impl->rmw_node_handle, rmw_get_error_string().str, goto fail);
Expand Down
2 changes: 0 additions & 2 deletions rcl/src/rcl/node_options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ rcl_node_get_default_options()
{
// !!! MAKE SURE THAT CHANGES TO THESE DEFAULTS ARE REFLECTED IN THE HEADER DOC STRING
static rcl_node_options_t default_options = {
.domain_id = RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID,
.use_global_arguments = true,
.enable_rosout = true,
};
Expand All @@ -54,7 +53,6 @@ rcl_node_options_copy(
RCL_SET_ERROR_MSG("Options out must be zero initialized");
return RCL_RET_INVALID_ARGUMENT;
}
options_out->domain_id = options->domain_id;
options_out->allocator = options->allocator;
options_out->use_global_arguments = options->use_global_arguments;
options_out->enable_rosout = options->enable_rosout;
Expand Down
37 changes: 3 additions & 34 deletions rcl/test/rcl/test_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
{
EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options)) << rcl_get_error_string().str;
});
EXPECT_EQ(RCL_RET_OK, rcl_init_options_set_domain_id(&init_options, 42));
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
rcl_context_t invalid_context = rcl_get_zero_initialized_context();
ret = rcl_init(0, nullptr, &init_options, &invalid_context);
ASSERT_EQ(RCL_RET_OK, ret); // Shutdown later after invalid node.
Expand All @@ -103,19 +104,8 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
const char * namespace_ = "/ns";
const char * fq_name = "/ns/test_rcl_node_accessors_node";
rcl_node_options_t default_options = rcl_node_get_default_options();
default_options.domain_id = 42; // Set the domain id to something explicit.
ret = rcl_node_init(&invalid_node, name, namespace_, &invalid_context, &default_options);
if (is_windows && is_opensplice) {
// On Windows with OpenSplice, setting the domain id is not expected to work.
ASSERT_NE(RCL_RET_OK, ret);
// So retry with the default domain id setting (uses the environment as is).
default_options.domain_id = rcl_node_get_default_options().domain_id;
ret = rcl_node_init(&invalid_node, name, namespace_, &invalid_context, &default_options);
ASSERT_EQ(RCL_RET_OK, ret);
} else {
// This is the normal check (not windows and windows if not opensplice)
ASSERT_EQ(RCL_RET_OK, ret);
}
ASSERT_EQ(RCL_RET_OK, ret);
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
osrf_testing_tools_cpp::memory_tools::disable_monitoring_in_all_threads();
Expand Down Expand Up @@ -251,7 +241,6 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
EXPECT_NE(nullptr, actual_options);
if (actual_options) {
EXPECT_EQ(default_options.allocator.allocate, actual_options->allocator.allocate);
EXPECT_EQ(default_options.domain_id, actual_options->domain_id);
}
rcl_reset_error();
EXPECT_NO_MEMORY_OPERATIONS(
Expand All @@ -261,7 +250,6 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
EXPECT_NE(nullptr, actual_options);
if (actual_options) {
EXPECT_EQ(default_options.allocator.allocate, actual_options->allocator.allocate);
EXPECT_EQ(default_options.domain_id, actual_options->domain_id);
}
// Test rcl_node_get_domain_id().
size_t actual_domain_id;
Expand All @@ -281,10 +269,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors)
ret = rcl_node_get_domain_id(&node, &actual_domain_id);
});
EXPECT_EQ(RCL_RET_OK, ret);
if (RCL_RET_OK == ret && (!is_windows || !is_opensplice)) {
// Can only expect the domain id to be 42 if not windows or not opensplice.
EXPECT_EQ(42u, actual_domain_id);
}
EXPECT_EQ(42u, actual_domain_id);
// Test rcl_node_get_rmw_handle().
rmw_node_t * node_handle;
node_handle = rcl_node_get_rmw_handle(nullptr);
Expand Down Expand Up @@ -427,19 +412,6 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_life_cycle)
EXPECT_EQ(RCL_RET_OK, ret);
ret = rcl_node_fini(&node);
EXPECT_EQ(RCL_RET_OK, ret);
// Try with a specific domain id.
rcl_node_options_t options_with_custom_domain_id = rcl_node_get_default_options();
options_with_custom_domain_id.domain_id = 42;
ret = rcl_node_init(&node, name, namespace_, &context, &options_with_custom_domain_id);
if (is_windows && is_opensplice) {
// A custom domain id is not expected to work on Windows with Opensplice.
EXPECT_NE(RCL_RET_OK, ret);
} else {
// This is the normal check.
EXPECT_EQ(RCL_RET_OK, ret);
ret = rcl_node_fini(&node);
EXPECT_EQ(RCL_RET_OK, ret);
}
}

/* Tests the node name restrictions enforcement.
Expand Down Expand Up @@ -754,7 +726,6 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_options) {

EXPECT_TRUE(default_options.use_global_arguments);
EXPECT_TRUE(default_options.enable_rosout);
EXPECT_EQ(RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID, default_options.domain_id);
EXPECT_TRUE(rcutils_allocator_is_valid(&(default_options.allocator)));

EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_node_options_copy(nullptr, &default_options));
Expand All @@ -767,11 +738,9 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_options) {
EXPECT_EQ(
RCL_RET_OK,
rcl_parse_arguments(argc, argv, default_options.allocator, &(default_options.arguments)));
default_options.domain_id = 42u;
default_options.use_global_arguments = false;
default_options.enable_rosout = false;
EXPECT_EQ(RCL_RET_OK, rcl_node_options_copy(&default_options, &not_ini_options));
EXPECT_EQ(42u, not_ini_options.domain_id);
EXPECT_FALSE(not_ini_options.use_global_arguments);
EXPECT_FALSE(not_ini_options.enable_rosout);
EXPECT_EQ(
Expand Down