From 70eba5e788af95c0e9f0dfe71af9ae630a9d4d76 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 8 Jul 2020 10:20:08 -0300 Subject: [PATCH] Remove domain_id and localhost_only from node_options (#708) Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/context.h | 2 +- rcl/include/rcl/init_options.h | 2 +- rcl/include/rcl/node_options.h | 13 ------------ rcl/src/rcl/context.c | 2 +- rcl/src/rcl/init_options.c | 2 +- rcl/src/rcl/node.c | 16 +++++---------- rcl/src/rcl/node_options.c | 2 -- rcl/test/rcl/test_node.cpp | 37 +++------------------------------- 8 files changed, 12 insertions(+), 64 deletions(-) diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h index 36dd33d58..ec5c89ff2 100644 --- a/rcl/include/rcl/context.h +++ b/rcl/include/rcl/context.h @@ -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. /** diff --git a/rcl/include/rcl/init_options.h b/rcl/include/rcl/init_options.h index c6ccb5938..1b97b82ac 100644 --- a/rcl/include/rcl/init_options.h +++ b/rcl/include/rcl/init_options.h @@ -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. /** diff --git a/rcl/include/rcl/node_options.h b/rcl/include/rcl/node_options.h index c9c0edfb0..9960fb2da 100644 --- a/rcl/include/rcl/node_options.h +++ b/rcl/include/rcl/node_options.h @@ -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; diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c index b400c09ef..6ffde3613 100644 --- a/rcl/src/rcl/context.c +++ b/rcl/src/rcl/context.c @@ -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); diff --git a/rcl/src/rcl/init_options.c b/rcl/src/rcl/init_options.c index fbdd00f15..0c5f2a3c7 100644 --- a/rcl/src/rcl/init_options.c +++ b/rcl/src/rcl/init_options.c @@ -132,7 +132,7 @@ rcl_init_options_fini(rcl_init_options_t * init_options) } 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) { RCL_CHECK_ARGUMENT_FOR_NULL(init_options, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(init_options->impl, RCL_RET_INVALID_ARGUMENT); diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 46d248026..18347fcb1 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -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" @@ -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(); @@ -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); diff --git a/rcl/src/rcl/node_options.c b/rcl/src/rcl/node_options.c index 77bedeff0..9cd3f12e8 100644 --- a/rcl/src/rcl/node_options.c +++ b/rcl/src/rcl/node_options.c @@ -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, }; @@ -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; diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index 7a0253692..87817cae1 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -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)); 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. @@ -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) << rcl_get_error_string().str; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { osrf_testing_tools_cpp::memory_tools::disable_monitoring_in_all_threads(); @@ -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( @@ -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; @@ -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); @@ -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. @@ -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)); @@ -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, ¬_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(