From 51f24eb6f268c2bfcf218806e19c99e0fa2dfcc4 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Thu, 11 Jun 2020 13:34:34 +0900 Subject: [PATCH 01/11] Ability to configure domain_id via InitOptions. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 18 ++++++++++++ rclcpp/src/rclcpp/init_options.cpp | 40 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index 447c148e00..22577e0929 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -16,9 +16,11 @@ #define RCLCPP__INIT_OPTIONS_HPP_ #include +#include #include "rcl/init_options.h" #include "rclcpp/visibility_control.hpp" +#include "rcutils/get_env.h" namespace rclcpp { @@ -80,6 +82,21 @@ class InitOptions const rcl_init_options_t * get_rcl_init_options() const; + /// Retrieve the ROS_DOMAIN_ID environment variable. + RCLCPP_PUBLIC + bool + use_default_domain_id(); + + /// Set domain id. + RCLCPP_PUBLIC + void + set_domain_id(size_t domain_id); + + /// Return the domain id. + RCLCPP_PUBLIC + size_t + get_domain_id() const; + protected: void finalize_init_options(); @@ -87,6 +104,7 @@ class InitOptions private: std::unique_ptr init_options_; bool initialize_logging_{true}; + size_t domain_id_{std::numeric_limits::max()}; }; } // namespace rclcpp diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index ad2a900b5f..aca3a70e56 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -99,4 +99,44 @@ InitOptions::get_rcl_init_options() const return init_options_.get(); } +bool +InitOptions::use_default_domain_id() +{ + // Try to get the ROS_DOMAIN_ID environment variable. + const char * ros_domain_id = NULL; + const char * env_var = "ROS_DOMAIN_ID"; + const char * get_env_error_str = NULL; + get_env_error_str = rcutils_get_env(env_var, &ros_domain_id); + if (NULL != get_env_error_str) { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), + "failed to get env var %s with %s", env_var, get_env_error_str); + return false; + } + if (ros_domain_id) { + unsigned long number = strtoul(ros_domain_id, NULL, 0); + if (number == (std::numeric_limits::max)()) { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), + "failed to interpret %s as integral number", env_var); + return false; + } + init_options_->domain_id = (size_t) number; + } + + return true; +} + +void +InitOptions::set_domain_id(size_t domain_id) +{ + init_options_->domain_id = domain_id; +} + +size_t +InitOptions::get_domain_id() const +{ + return init_options_->domain_id; +} + } // namespace rclcpp From 8fef12ec528a2af2177df275973565de8755ec79 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Mon, 15 Jun 2020 13:51:47 +0900 Subject: [PATCH 02/11] use rcl_init_options_set/get_domain_id. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 3 +-- rclcpp/src/rclcpp/init_options.cpp | 27 ++++++++++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index 22577e0929..8160a7d868 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -84,7 +84,7 @@ class InitOptions /// Retrieve the ROS_DOMAIN_ID environment variable. RCLCPP_PUBLIC - bool + void use_default_domain_id(); /// Set domain id. @@ -104,7 +104,6 @@ class InitOptions private: std::unique_ptr init_options_; bool initialize_logging_{true}; - size_t domain_id_{std::numeric_limits::max()}; }; } // namespace rclcpp diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index aca3a70e56..6d30ccd7d9 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -99,7 +99,7 @@ InitOptions::get_rcl_init_options() const return init_options_.get(); } -bool +void InitOptions::use_default_domain_id() { // Try to get the ROS_DOMAIN_ID environment variable. @@ -111,7 +111,6 @@ InitOptions::use_default_domain_id() RCLCPP_ERROR( rclcpp::get_logger("rclcpp"), "failed to get env var %s with %s", env_var, get_env_error_str); - return false; } if (ros_domain_id) { unsigned long number = strtoul(ros_domain_id, NULL, 0); @@ -119,24 +118,36 @@ InitOptions::use_default_domain_id() RCLCPP_ERROR( rclcpp::get_logger("rclcpp"), "failed to interpret %s as integral number", env_var); - return false; } - init_options_->domain_id = (size_t) number; + rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), (size_t) number); + if (RCL_RET_OK != ret) { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), "failed to set domain id (%d).", number); + } } - - return true; } void InitOptions::set_domain_id(size_t domain_id) { - init_options_->domain_id = domain_id; + rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), domain_id); + if (RCL_RET_OK != ret) { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), "failed to set domain id (%d).", domain_id); + } } size_t InitOptions::get_domain_id() const { - return init_options_->domain_id; + size_t domain_id; + rcl_ret_t ret = rcl_init_options_get_domain_id(init_options_.get(), &domain_id); + if (RCL_RET_OK != ret) { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), "failed to get domain id."); + } + + return domain_id; } } // namespace rclcpp From 259c369ca76478f26ffcd69b4d5cd7ee41110ecd Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 30 Jun 2020 11:21:23 +0900 Subject: [PATCH 03/11] use rcl_get_default_domain_id() instead. Signed-off-by: Tomoya.Fujita --- rclcpp/src/rclcpp/init_options.cpp | 33 +++++++----------------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 6d30ccd7d9..a05a24d6be 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -102,29 +102,12 @@ InitOptions::get_rcl_init_options() const void InitOptions::use_default_domain_id() { - // Try to get the ROS_DOMAIN_ID environment variable. - const char * ros_domain_id = NULL; - const char * env_var = "ROS_DOMAIN_ID"; - const char * get_env_error_str = NULL; - get_env_error_str = rcutils_get_env(env_var, &ros_domain_id); - if (NULL != get_env_error_str) { - RCLCPP_ERROR( - rclcpp::get_logger("rclcpp"), - "failed to get env var %s with %s", env_var, get_env_error_str); - } - if (ros_domain_id) { - unsigned long number = strtoul(ros_domain_id, NULL, 0); - if (number == (std::numeric_limits::max)()) { - RCLCPP_ERROR( - rclcpp::get_logger("rclcpp"), - "failed to interpret %s as integral number", env_var); - } - rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), (size_t) number); - if (RCL_RET_OK != ret) { - RCLCPP_ERROR( - rclcpp::get_logger("rclcpp"), "failed to set domain id (%d).", number); - } + size_t domain_id; + rcl_ret_t ret = rcl_get_default_domain_id(&domain_id); + if (RCL_RET_OK != ret) { + rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get default domain id"); } + set_domain_id(domain_id); } void @@ -132,8 +115,7 @@ InitOptions::set_domain_id(size_t domain_id) { rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), domain_id); if (RCL_RET_OK != ret) { - RCLCPP_ERROR( - rclcpp::get_logger("rclcpp"), "failed to set domain id (%d).", domain_id); + rclcpp::exceptions::throw_from_rcl_error(ret, "failed to set domain id to rcl init options"); } } @@ -143,8 +125,7 @@ InitOptions::get_domain_id() const size_t domain_id; rcl_ret_t ret = rcl_init_options_get_domain_id(init_options_.get(), &domain_id); if (RCL_RET_OK != ret) { - RCLCPP_ERROR( - rclcpp::get_logger("rclcpp"), "failed to get domain id."); + rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get domain id to rcl init options"); } return domain_id; From 42ebba8d0a92dd4752bc9b6d544479e8bbd4a810 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 30 Jun 2020 11:36:48 +0900 Subject: [PATCH 04/11] get rid of unnecessary headers and fix comments. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 8 +++----- rclcpp/src/rclcpp/init_options.cpp | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index 8160a7d868..093e12ee58 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -16,11 +16,9 @@ #define RCLCPP__INIT_OPTIONS_HPP_ #include -#include #include "rcl/init_options.h" #include "rclcpp/visibility_control.hpp" -#include "rcutils/get_env.h" namespace rclcpp { @@ -82,17 +80,17 @@ class InitOptions const rcl_init_options_t * get_rcl_init_options() const; - /// Retrieve the ROS_DOMAIN_ID environment variable. + /// Retrieve default domain id and set. RCLCPP_PUBLIC void use_default_domain_id(); - /// Set domain id. + /// Set the domain id. RCLCPP_PUBLIC void set_domain_id(size_t domain_id); - /// Return the domain id. + /// Return domain id. RCLCPP_PUBLIC size_t get_domain_id() const; diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index a05a24d6be..4134c5edad 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -125,7 +125,7 @@ InitOptions::get_domain_id() const size_t domain_id; rcl_ret_t ret = rcl_init_options_get_domain_id(init_options_.get(), &domain_id); if (RCL_RET_OK != ret) { - rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get domain id to rcl init options"); + rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get domain id from rcl init options"); } return domain_id; From d2630e6f600530f96a534a8588e5185e5a89781a Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 7 Jul 2020 11:46:34 +0900 Subject: [PATCH 05/11] use RCL_DEFAULT_DOMAIN_ID as default value. Signed-off-by: Tomoya.Fujita --- rclcpp/src/rclcpp/init_options.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 4134c5edad..54412db84f 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -102,7 +102,7 @@ InitOptions::get_rcl_init_options() const void InitOptions::use_default_domain_id() { - size_t domain_id; + size_t domain_id = RCL_DEFAULT_DOMAIN_ID; rcl_ret_t ret = rcl_get_default_domain_id(&domain_id); if (RCL_RET_OK != ret) { rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get default domain id"); From 8563ea0e9c687a550afd355129958c6f2d92f294 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 28 Jul 2020 18:31:35 +0900 Subject: [PATCH 06/11] protect init_options_ with mutex. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 3 +++ rclcpp/src/rclcpp/init_options.cpp | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index 093e12ee58..bcfe9812b2 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -16,6 +16,7 @@ #define RCLCPP__INIT_OPTIONS_HPP_ #include +#include #include "rcl/init_options.h" #include "rclcpp/visibility_control.hpp" @@ -100,6 +101,8 @@ class InitOptions finalize_init_options(); private: + // This mutex is recursive so that the operator can ensure atomicity + mutable std::recursive_mutex init_options_mutex_; std::unique_ptr init_options_; bool initialize_logging_{true}; }; diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 54412db84f..2955a803c5 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -33,6 +33,7 @@ InitOptions::InitOptions(rcl_allocator_t allocator) InitOptions::InitOptions(const rcl_init_options_t & init_options) : init_options_(new rcl_init_options_t) { + std::lock_guard init_options_lock(init_options_mutex_); *init_options_ = rcl_get_zero_initialized_init_options(); rcl_ret_t ret = rcl_init_options_copy(&init_options, init_options_.get()); if (RCL_RET_OK != ret) { @@ -63,6 +64,7 @@ InitOptions & InitOptions::operator=(const InitOptions & other) { if (this != &other) { + std::lock_guard init_options_lock(init_options_mutex_); this->finalize_init_options(); rcl_ret_t ret = rcl_init_options_copy(other.get_rcl_init_options(), init_options_.get()); if (RCL_RET_OK != ret) { @@ -81,6 +83,7 @@ InitOptions::~InitOptions() void InitOptions::finalize_init_options() { + std::lock_guard init_options_lock(init_options_mutex_); if (init_options_) { rcl_ret_t ret = rcl_init_options_fini(init_options_.get()); if (RCL_RET_OK != ret) { @@ -96,6 +99,7 @@ InitOptions::finalize_init_options() const rcl_init_options_t * InitOptions::get_rcl_init_options() const { + std::lock_guard init_options_lock(init_options_mutex_); return init_options_.get(); } @@ -113,6 +117,7 @@ InitOptions::use_default_domain_id() void InitOptions::set_domain_id(size_t domain_id) { + std::lock_guard init_options_lock(init_options_mutex_); rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), domain_id); if (RCL_RET_OK != ret) { rclcpp::exceptions::throw_from_rcl_error(ret, "failed to set domain id to rcl init options"); @@ -122,6 +127,7 @@ InitOptions::set_domain_id(size_t domain_id) size_t InitOptions::get_domain_id() const { + std::lock_guard init_options_lock(init_options_mutex_); size_t domain_id; rcl_ret_t ret = rcl_init_options_get_domain_id(init_options_.get(), &domain_id); if (RCL_RET_OK != ret) { From 65b01de43e8ef49a246d7d60c3d529db26e01745 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 28 Jul 2020 20:57:01 +0900 Subject: [PATCH 07/11] add test for InitOptions. Signed-off-by: Tomoya.Fujita --- rclcpp/src/rclcpp/init_options.cpp | 2 + rclcpp/test/CMakeLists.txt | 5 ++ rclcpp/test/rclcpp/test_init_options.cpp | 87 ++++++++++++++++++++++++ 3 files changed, 94 insertions(+) create mode 100644 rclcpp/test/rclcpp/test_init_options.cpp diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 2955a803c5..5e1504cd23 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -45,6 +45,7 @@ InitOptions::InitOptions(const InitOptions & other) : InitOptions(*other.get_rcl_init_options()) { shutdown_on_sigint = other.shutdown_on_sigint; + initialize_logging_ = other.initialize_logging_; } bool @@ -71,6 +72,7 @@ InitOptions::operator=(const InitOptions & other) rclcpp::exceptions::throw_from_rcl_error(ret, "failed to copy rcl init options"); } this->shutdown_on_sigint = other.shutdown_on_sigint; + this->initialize_logging_ = other.initialize_logging_; } return *this; } diff --git a/rclcpp/test/CMakeLists.txt b/rclcpp/test/CMakeLists.txt index 840c5de0e3..e9e70f8063 100644 --- a/rclcpp/test/CMakeLists.txt +++ b/rclcpp/test/CMakeLists.txt @@ -253,6 +253,11 @@ if(TARGET test_node_options) ament_target_dependencies(test_node_options "rcl") target_link_libraries(test_node_options ${PROJECT_NAME}) endif() +ament_add_gtest(test_init_options rclcpp/test_init_options.cpp) +if(TARGET test_init_options) + ament_target_dependencies(test_init_options "rcl") + target_link_libraries(test_init_options ${PROJECT_NAME}) +endif() ament_add_gtest(test_parameter_client rclcpp/test_parameter_client.cpp) if(TARGET test_parameter_client) ament_target_dependencies(test_parameter_client diff --git a/rclcpp/test/rclcpp/test_init_options.cpp b/rclcpp/test/rclcpp/test_init_options.cpp new file mode 100644 index 0000000000..32e308497c --- /dev/null +++ b/rclcpp/test/rclcpp/test_init_options.cpp @@ -0,0 +1,87 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include +#include + +#include "rcl/allocator.h" +#include "rcl/domain_id.h" + +#include "rclcpp/init_options.hpp" + + +TEST(TestInitOptions, test_construction) { + rcl_allocator_t allocator = rcl_get_default_allocator(); + auto options = rclcpp::InitOptions(allocator); + const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_TRUE(rcl_options->impl != nullptr); + + { + auto options_copy = rclcpp::InitOptions(options); + const rcl_init_options_t * rcl_options_copy = options_copy.get_rcl_init_options(); + ASSERT_TRUE(rcl_options_copy != nullptr); + ASSERT_TRUE(rcl_options_copy->impl != nullptr); + } + + { + auto options_copy = options; + const rcl_init_options_t * rcl_options_copy = options_copy.get_rcl_init_options(); + ASSERT_TRUE(rcl_options_copy != nullptr); + ASSERT_TRUE(rcl_options_copy->impl != nullptr); + } +} + +TEST(TestInitOptions, test_initialize_logging) { + { + auto options = rclcpp::InitOptions(); + EXPECT_TRUE(options.auto_initialize_logging()); + const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_TRUE(rcl_options->impl != nullptr); + } + + { + auto options = rclcpp::InitOptions().auto_initialize_logging(true); + EXPECT_TRUE(options.auto_initialize_logging()); + const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_TRUE(rcl_options->impl != nullptr); + } + + { + auto options = rclcpp::InitOptions().auto_initialize_logging(false); + EXPECT_FALSE(options.auto_initialize_logging()); + const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_TRUE(rcl_options->impl != nullptr); + } +} + +TEST(TestInitOptions, test_domain_id) { + rcl_allocator_t allocator = rcl_get_default_allocator(); + auto options = rclcpp::InitOptions(allocator); + const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_TRUE(rcl_options->impl != nullptr); + + options.use_default_domain_id(); + EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); + options.set_domain_id(42); + EXPECT_EQ(42, options.get_domain_id()); + options.use_default_domain_id(); + EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); +} From 46d65d3775d045e31bceed3a0824649ce27096c3 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Wed, 29 Jul 2020 09:04:20 +0900 Subject: [PATCH 08/11] Delete mutex lock from InitOptions constructor. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 2 +- rclcpp/src/rclcpp/init_options.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index bcfe9812b2..7e7036807f 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -101,7 +101,7 @@ class InitOptions finalize_init_options(); private: - // This mutex is recursive so that the operator can ensure atomicity + // This mutex needs to be recursive for assignment operator mutable std::recursive_mutex init_options_mutex_; std::unique_ptr init_options_; bool initialize_logging_{true}; diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 5e1504cd23..8d9a68111d 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -33,7 +33,6 @@ InitOptions::InitOptions(rcl_allocator_t allocator) InitOptions::InitOptions(const rcl_init_options_t & init_options) : init_options_(new rcl_init_options_t) { - std::lock_guard init_options_lock(init_options_mutex_); *init_options_ = rcl_get_zero_initialized_init_options(); rcl_ret_t ret = rcl_init_options_copy(&init_options, init_options_.get()); if (RCL_RET_OK != ret) { From 14693dce1e0ed7cf3e567932bb6ea4398c09db2c Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Thu, 30 Jul 2020 15:25:49 +0900 Subject: [PATCH 09/11] use std::mutex instead of std::recursive_mutex. Signed-off-by: Tomoya.Fujita --- rclcpp/include/rclcpp/init_options.hpp | 6 ++++-- rclcpp/src/rclcpp/init_options.cpp | 18 ++++++++++++------ rclcpp/test/rclcpp/test_init_options.cpp | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/rclcpp/include/rclcpp/init_options.hpp b/rclcpp/include/rclcpp/init_options.hpp index 7e7036807f..6f87904f25 100644 --- a/rclcpp/include/rclcpp/init_options.hpp +++ b/rclcpp/include/rclcpp/init_options.hpp @@ -101,8 +101,10 @@ class InitOptions finalize_init_options(); private: - // This mutex needs to be recursive for assignment operator - mutable std::recursive_mutex init_options_mutex_; + void + finalize_init_options_impl(); + + mutable std::mutex init_options_mutex_; std::unique_ptr init_options_; bool initialize_logging_{true}; }; diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 8d9a68111d..48e5a2b1c6 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -64,8 +64,8 @@ InitOptions & InitOptions::operator=(const InitOptions & other) { if (this != &other) { - std::lock_guard init_options_lock(init_options_mutex_); - this->finalize_init_options(); + std::lock_guard init_options_lock(init_options_mutex_); + this->finalize_init_options_impl(); rcl_ret_t ret = rcl_init_options_copy(other.get_rcl_init_options(), init_options_.get()); if (RCL_RET_OK != ret) { rclcpp::exceptions::throw_from_rcl_error(ret, "failed to copy rcl init options"); @@ -84,7 +84,13 @@ InitOptions::~InitOptions() void InitOptions::finalize_init_options() { - std::lock_guard init_options_lock(init_options_mutex_); + std::lock_guard init_options_lock(init_options_mutex_); + this->finalize_init_options_impl(); +} + +void +InitOptions::finalize_init_options_impl() +{ if (init_options_) { rcl_ret_t ret = rcl_init_options_fini(init_options_.get()); if (RCL_RET_OK != ret) { @@ -100,7 +106,7 @@ InitOptions::finalize_init_options() const rcl_init_options_t * InitOptions::get_rcl_init_options() const { - std::lock_guard init_options_lock(init_options_mutex_); + std::lock_guard init_options_lock(init_options_mutex_); return init_options_.get(); } @@ -118,7 +124,7 @@ InitOptions::use_default_domain_id() void InitOptions::set_domain_id(size_t domain_id) { - std::lock_guard init_options_lock(init_options_mutex_); + std::lock_guard init_options_lock(init_options_mutex_); rcl_ret_t ret = rcl_init_options_set_domain_id(init_options_.get(), domain_id); if (RCL_RET_OK != ret) { rclcpp::exceptions::throw_from_rcl_error(ret, "failed to set domain id to rcl init options"); @@ -128,7 +134,7 @@ InitOptions::set_domain_id(size_t domain_id) size_t InitOptions::get_domain_id() const { - std::lock_guard init_options_lock(init_options_mutex_); + std::lock_guard init_options_lock(init_options_mutex_); size_t domain_id; rcl_ret_t ret = rcl_init_options_get_domain_id(init_options_.get(), &domain_id); if (RCL_RET_OK != ret) { diff --git a/rclcpp/test/rclcpp/test_init_options.cpp b/rclcpp/test/rclcpp/test_init_options.cpp index 32e308497c..0a31ae40cc 100644 --- a/rclcpp/test/rclcpp/test_init_options.cpp +++ b/rclcpp/test/rclcpp/test_init_options.cpp @@ -81,7 +81,7 @@ TEST(TestInitOptions, test_domain_id) { options.use_default_domain_id(); EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); options.set_domain_id(42); - EXPECT_EQ(42, options.get_domain_id()); + EXPECT_EQ((size_t)42, options.get_domain_id()); options.use_default_domain_id(); EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); } From cea62b0edcf395462d1d222e7c463d24c8763255 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Tue, 4 Aug 2020 13:42:53 +0900 Subject: [PATCH 10/11] get rid of unrelated changes. Signed-off-by: Tomoya.Fujita --- rclcpp/src/rclcpp/init_options.cpp | 3 --- rclcpp/test/rclcpp/test_init_options.cpp | 29 ------------------------ 2 files changed, 32 deletions(-) diff --git a/rclcpp/src/rclcpp/init_options.cpp b/rclcpp/src/rclcpp/init_options.cpp index 48e5a2b1c6..cffa06bd11 100644 --- a/rclcpp/src/rclcpp/init_options.cpp +++ b/rclcpp/src/rclcpp/init_options.cpp @@ -44,7 +44,6 @@ InitOptions::InitOptions(const InitOptions & other) : InitOptions(*other.get_rcl_init_options()) { shutdown_on_sigint = other.shutdown_on_sigint; - initialize_logging_ = other.initialize_logging_; } bool @@ -71,7 +70,6 @@ InitOptions::operator=(const InitOptions & other) rclcpp::exceptions::throw_from_rcl_error(ret, "failed to copy rcl init options"); } this->shutdown_on_sigint = other.shutdown_on_sigint; - this->initialize_logging_ = other.initialize_logging_; } return *this; } @@ -106,7 +104,6 @@ InitOptions::finalize_init_options_impl() const rcl_init_options_t * InitOptions::get_rcl_init_options() const { - std::lock_guard init_options_lock(init_options_mutex_); return init_options_.get(); } diff --git a/rclcpp/test/rclcpp/test_init_options.cpp b/rclcpp/test/rclcpp/test_init_options.cpp index 0a31ae40cc..77a147a49c 100644 --- a/rclcpp/test/rclcpp/test_init_options.cpp +++ b/rclcpp/test/rclcpp/test_init_options.cpp @@ -45,38 +45,9 @@ TEST(TestInitOptions, test_construction) { } } -TEST(TestInitOptions, test_initialize_logging) { - { - auto options = rclcpp::InitOptions(); - EXPECT_TRUE(options.auto_initialize_logging()); - const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); - ASSERT_TRUE(rcl_options != nullptr); - ASSERT_TRUE(rcl_options->impl != nullptr); - } - - { - auto options = rclcpp::InitOptions().auto_initialize_logging(true); - EXPECT_TRUE(options.auto_initialize_logging()); - const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); - ASSERT_TRUE(rcl_options != nullptr); - ASSERT_TRUE(rcl_options->impl != nullptr); - } - - { - auto options = rclcpp::InitOptions().auto_initialize_logging(false); - EXPECT_FALSE(options.auto_initialize_logging()); - const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); - ASSERT_TRUE(rcl_options != nullptr); - ASSERT_TRUE(rcl_options->impl != nullptr); - } -} - TEST(TestInitOptions, test_domain_id) { rcl_allocator_t allocator = rcl_get_default_allocator(); auto options = rclcpp::InitOptions(allocator); - const rcl_init_options_t * rcl_options = options.get_rcl_init_options(); - ASSERT_TRUE(rcl_options != nullptr); - ASSERT_TRUE(rcl_options->impl != nullptr); options.use_default_domain_id(); EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); From 300580356c5bb665d999b0c6861197dc945944c1 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Wed, 5 Aug 2020 11:16:25 +0900 Subject: [PATCH 11/11] fix test_domain_id to call rcl_get_default_domain_id(). Signed-off-by: Tomoya.Fujita --- rclcpp/test/rclcpp/test_init_options.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rclcpp/test/rclcpp/test_init_options.cpp b/rclcpp/test/rclcpp/test_init_options.cpp index 77a147a49c..53b2b9402e 100644 --- a/rclcpp/test/rclcpp/test_init_options.cpp +++ b/rclcpp/test/rclcpp/test_init_options.cpp @@ -48,11 +48,13 @@ TEST(TestInitOptions, test_construction) { TEST(TestInitOptions, test_domain_id) { rcl_allocator_t allocator = rcl_get_default_allocator(); auto options = rclcpp::InitOptions(allocator); + size_t domain_id = RCL_DEFAULT_DOMAIN_ID; + EXPECT_EQ(RCL_RET_OK, rcl_get_default_domain_id(&domain_id)); options.use_default_domain_id(); - EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); + EXPECT_EQ(domain_id, options.get_domain_id()); options.set_domain_id(42); EXPECT_EQ((size_t)42, options.get_domain_id()); options.use_default_domain_id(); - EXPECT_EQ(RCL_DEFAULT_DOMAIN_ID, options.get_domain_id()); + EXPECT_EQ(domain_id, options.get_domain_id()); }