From 540ead5bc5d8b7ec14741b0f0b6bdead655e99d8 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Mon, 5 Aug 2019 13:07:00 -0300 Subject: [PATCH 1/7] Use --ros-args to deal with node arguments in rclcpp. Signed-off-by: Michel Hidalgo --- rclcpp/src/rclcpp/node_options.cpp | 12 +++++++----- rclcpp/test/test_node_global_args.cpp | 4 ++-- rclcpp/test/test_utilities.cpp | 6 ++++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/rclcpp/src/rclcpp/node_options.cpp b/rclcpp/src/rclcpp/node_options.cpp index f13c411089..02451bf9bb 100644 --- a/rclcpp/src/rclcpp/node_options.cpp +++ b/rclcpp/src/rclcpp/node_options.cpp @@ -90,11 +90,14 @@ NodeOptions::get_rcl_node_options() const node_options_->use_global_arguments = this->use_global_arguments_; node_options_->domain_id = this->get_domain_id_from_env(); - std::unique_ptr c_args; + int c_argc = 0; + std::unique_ptr c_argv; if (!this->arguments_.empty()) { - c_args.reset(new const char *[this->arguments_.size()]); + c_argc = this->arguments_.size() + 1; + c_argv.reset(new const char *[c_argc]); + c_argv[0] = RCL_ROS_ARGS_FLAG; for (std::size_t i = 0; i < this->arguments_.size(); ++i) { - c_args[i] = this->arguments_[i].c_str(); + c_argv[i + 1] = this->arguments_[i].c_str(); } } @@ -103,8 +106,7 @@ NodeOptions::get_rcl_node_options() const } rmw_ret_t ret = rcl_parse_arguments( - static_cast(this->arguments_.size()), c_args.get(), this->allocator_, - &(node_options_->arguments)); + c_argc, c_argv.get(), this->allocator_, &(node_options_->arguments)); if (RCL_RET_OK != ret) { throw_from_rcl_error(ret, "failed to parse arguments"); diff --git a/rclcpp/test/test_node_global_args.cpp b/rclcpp/test/test_node_global_args.cpp index 504955f8af..19d5d7e060 100644 --- a/rclcpp/test/test_node_global_args.cpp +++ b/rclcpp/test/test_node_global_args.cpp @@ -28,8 +28,8 @@ class TestNodeWithGlobalArgs : public ::testing::Test protected: static void SetUpTestCase() { - const char * const args[] = {"proc", "__node:=global_node_name"}; - rclcpp::init(2, args); + const char * const args[] = {"proc", "--ros-args", "__node:=global_node_name"}; + rclcpp::init(3, args); } }; diff --git a/rclcpp/test/test_utilities.cpp b/rclcpp/test/test_utilities.cpp index 7b84aaf3a6..6ab3912976 100644 --- a/rclcpp/test/test_utilities.cpp +++ b/rclcpp/test/test_utilities.cpp @@ -22,8 +22,10 @@ #include "rclcpp/utilities.hpp" TEST(TestUtilities, remove_ros_arguments) { - const char * const argv[] = {"process_name", "-d", "__ns:=/foo/bar", - "__ns:=/fiz/buz", "--foo=bar", "--baz"}; + const char * const argv[] = { + "process_name", "-d", "--ros-args", "__ns:=/foo/bar", "__ns:=/fiz/buz", "--", + "--foo=bar", "--baz" + }; int argc = sizeof(argv) / sizeof(const char *); auto args = rclcpp::remove_ros_arguments(argc, argv); From 8b9a9a0a0a58298f2be7d872f3405428ed3df221 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 6 Aug 2019 11:51:57 -0300 Subject: [PATCH 2/7] Document implicit --ros-args flag in NodeOptions::arguments(). Signed-off-by: Michel Hidalgo --- rclcpp/include/rclcpp/node_options.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node_options.hpp b/rclcpp/include/rclcpp/node_options.hpp index 82ead79f1e..1a7fcfd52d 100644 --- a/rclcpp/include/rclcpp/node_options.hpp +++ b/rclcpp/include/rclcpp/node_options.hpp @@ -99,7 +99,8 @@ class NodeOptions /// Set the arguments, return this for parameter idiom. /** * These arguments are used to extract remappings used by the node and other - * settings. + * settings. Being specific to a ROS node, an implicit `--ros-args` scope flag + * always precedes these arguments. * * This will cause the internal rcl_node_options_t struct to be invalidated. */ From 2d274d08423c98de761342c103ac41ebae80b7ae Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 6 Aug 2019 16:22:52 -0300 Subject: [PATCH 3/7] Add missing size_t to int cast. Signed-off-by: Michel Hidalgo --- rclcpp/src/rclcpp/node_options.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/node_options.cpp b/rclcpp/src/rclcpp/node_options.cpp index 02451bf9bb..b89abeb1f2 100644 --- a/rclcpp/src/rclcpp/node_options.cpp +++ b/rclcpp/src/rclcpp/node_options.cpp @@ -93,7 +93,7 @@ NodeOptions::get_rcl_node_options() const int c_argc = 0; std::unique_ptr c_argv; if (!this->arguments_.empty()) { - c_argc = this->arguments_.size() + 1; + c_argc = static_cast(this->arguments_.size()) + 1; c_argv.reset(new const char *[c_argc]); c_argv[0] = RCL_ROS_ARGS_FLAG; for (std::size_t i = 0; i < this->arguments_.size(); ++i) { From 68807d50d0ca44a0c9fe7fc36880c449d034291a Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 6 Aug 2019 17:15:42 -0300 Subject: [PATCH 4/7] Only add implicit --ros-args flag if not present already. Signed-off-by: Michel Hidalgo --- rclcpp/src/rclcpp/node_options.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/rclcpp/src/rclcpp/node_options.cpp b/rclcpp/src/rclcpp/node_options.cpp index b89abeb1f2..1e18f13cac 100644 --- a/rclcpp/src/rclcpp/node_options.cpp +++ b/rclcpp/src/rclcpp/node_options.cpp @@ -93,11 +93,20 @@ NodeOptions::get_rcl_node_options() const int c_argc = 0; std::unique_ptr c_argv; if (!this->arguments_.empty()) { - c_argc = static_cast(this->arguments_.size()) + 1; + auto it = std::find( + this->arguments_.cbegin(), this->arguments_.cend(), RCL_ROS_ARGS_FLAG); + + c_argc = static_cast(this->arguments_.size()); + if (it == this->arguments_.cend()) c_argc += 1; + c_argv.reset(new const char *[c_argc]); - c_argv[0] = RCL_ROS_ARGS_FLAG; - for (std::size_t i = 0; i < this->arguments_.size(); ++i) { - c_argv[i + 1] = this->arguments_[i].c_str(); + + std::size_t i = 0; + if (it == this->arguments_.cend()) { + c_argv[i++] = RCL_ROS_ARGS_FLAG; + } + for (std::size_t j = 0; j < this->arguments_.size(); ++i, ++j) { + c_argv[i] = this->arguments_[j].c_str(); } } From 493f8ad60b4012785cf01f09be249e5173be42df Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 6 Aug 2019 18:02:22 -0300 Subject: [PATCH 5/7] Add some rclcpp::NodeOptions test coverage. Signed-off-by: Michel Hidalgo --- rclcpp/CMakeLists.txt | 5 ++ rclcpp/test/test_node_options.cpp | 110 ++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 rclcpp/test/test_node_options.cpp diff --git a/rclcpp/CMakeLists.txt b/rclcpp/CMakeLists.txt index 32b7664d20..f401b7898e 100644 --- a/rclcpp/CMakeLists.txt +++ b/rclcpp/CMakeLists.txt @@ -254,6 +254,11 @@ if(BUILD_TESTING) ) target_link_libraries(test_node_global_args ${PROJECT_NAME}) endif() + ament_add_gtest(test_node_options test/test_node_options.cpp) + 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_parameter_client test/test_parameter_client.cpp) if(TARGET test_parameter_client) ament_target_dependencies(test_parameter_client diff --git a/rclcpp/test/test_node_options.cpp b/rclcpp/test/test_node_options.cpp new file mode 100644 index 0000000000..ba3dacb97d --- /dev/null +++ b/rclcpp/test/test_node_options.cpp @@ -0,0 +1,110 @@ +// Copyright 2019 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 +#include +#include + +#include "rclcpp/node_options.hpp" + + +TEST(TestNodeOptions, implicit_ros_args) { + rcl_allocator_t allocator = rcl_get_default_allocator(); + auto options = rclcpp::NodeOptions(allocator) + .arguments({"__node:=some_node", "__ns:=/some_ns"}); + + const rcl_node_options_t * rcl_options = options.get_rcl_node_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_EQ(0, rcl_arguments_get_count_unparsed(&rcl_options->arguments)); + ASSERT_EQ(0, rcl_arguments_get_count_unparsed_ros(&rcl_options->arguments)); + + char * node_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + ASSERT_TRUE(node_name != nullptr); + EXPECT_STREQ("some_node", node_name); + allocator.deallocate(node_name, allocator.state); + + char * namespace_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + ASSERT_TRUE(namespace_name != nullptr); + EXPECT_STREQ("/some_ns", namespace_name); + allocator.deallocate(namespace_name, allocator.state); +} + +TEST(TestNodeOptions, explicit_ros_args) { + rcl_allocator_t allocator = rcl_get_default_allocator(); + auto options = rclcpp::NodeOptions(allocator) + .arguments({"--ros-args", "__node:=some_node", "__ns:=/some_ns"}); + + const rcl_node_options_t * rcl_options = options.get_rcl_node_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_EQ(0, rcl_arguments_get_count_unparsed(&rcl_options->arguments)); + ASSERT_EQ(0, rcl_arguments_get_count_unparsed_ros(&rcl_options->arguments)); + + char * node_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + ASSERT_TRUE(node_name != nullptr); + EXPECT_STREQ("some_node", node_name); + allocator.deallocate(node_name, allocator.state); + + char * namespace_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + ASSERT_TRUE(namespace_name != nullptr); + EXPECT_STREQ("/some_ns", namespace_name); + allocator.deallocate(namespace_name, allocator.state); +} + +TEST(TestNodeOptions, explicit_ros_args_and_non_ros_args) { + rcl_allocator_t allocator = rcl_get_default_allocator(); + auto options = rclcpp::NodeOptions(allocator).arguments({ + "--non-ros-flag", "--ros-args", "__node:=some_node", + "__ns:=/some_ns", "--", "non-ros-arg"}); + + const rcl_node_options_t * rcl_options = options.get_rcl_node_options(); + ASSERT_TRUE(rcl_options != nullptr); + ASSERT_EQ(0, rcl_arguments_get_count_unparsed_ros(&rcl_options->arguments)); + ASSERT_EQ(2, rcl_arguments_get_count_unparsed(&rcl_options->arguments)); + + char * node_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + ASSERT_TRUE(node_name != nullptr); + EXPECT_STREQ("some_node", node_name); + allocator.deallocate(node_name, allocator.state); + + char * namespace_name = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + ASSERT_TRUE(namespace_name != nullptr); + EXPECT_STREQ("/some_ns", namespace_name); + allocator.deallocate(namespace_name, allocator.state); + + int * output_indices = nullptr; + EXPECT_EQ(RCL_RET_OK, rcl_arguments_get_unparsed( + &rcl_options->arguments, allocator, &output_indices)); + ASSERT_TRUE(output_indices != nullptr); + const std::vector & args = options.arguments(); + EXPECT_EQ("--non-ros-flag", args[output_indices[0]]); + EXPECT_EQ("non-ros-arg", args[output_indices[1]]); + allocator.deallocate(output_indices, allocator.state); +} From 8772bd0d73d2e9bf958e7f545bf3358ba226f61f Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 6 Aug 2019 18:49:17 -0300 Subject: [PATCH 6/7] Address peer review comments. Signed-off-by: Michel Hidalgo --- rclcpp/src/rclcpp/node_options.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/node_options.cpp b/rclcpp/src/rclcpp/node_options.cpp index 1e18f13cac..7b348c4d08 100644 --- a/rclcpp/src/rclcpp/node_options.cpp +++ b/rclcpp/src/rclcpp/node_options.cpp @@ -97,7 +97,9 @@ NodeOptions::get_rcl_node_options() const this->arguments_.cbegin(), this->arguments_.cend(), RCL_ROS_ARGS_FLAG); c_argc = static_cast(this->arguments_.size()); - if (it == this->arguments_.cend()) c_argc += 1; + if (it == this->arguments_.cend()) { + c_argc += 1; + } c_argv.reset(new const char *[c_argc]); From 69773838e2b229c79676897d080455113f10bc9e Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 7 Aug 2019 11:39:42 -0300 Subject: [PATCH 7/7] Please cpplint and uncrustify. Signed-off-by: Michel Hidalgo --- rclcpp/test/test_node_options.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/rclcpp/test/test_node_options.cpp b/rclcpp/test/test_node_options.cpp index ba3dacb97d..cefe2be45e 100644 --- a/rclcpp/test/test_node_options.cpp +++ b/rclcpp/test/test_node_options.cpp @@ -17,9 +17,9 @@ #include #include -#include -#include -#include +#include "rcl/allocator.h" +#include "rcl/arguments.h" +#include "rcl/remap.h" #include "rclcpp/node_options.hpp" @@ -36,14 +36,14 @@ TEST(TestNodeOptions, implicit_ros_args) { char * node_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( - &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); ASSERT_TRUE(node_name != nullptr); EXPECT_STREQ("some_node", node_name); allocator.deallocate(node_name, allocator.state); char * namespace_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( - &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); ASSERT_TRUE(namespace_name != nullptr); EXPECT_STREQ("/some_ns", namespace_name); allocator.deallocate(namespace_name, allocator.state); @@ -61,14 +61,14 @@ TEST(TestNodeOptions, explicit_ros_args) { char * node_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( - &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); ASSERT_TRUE(node_name != nullptr); EXPECT_STREQ("some_node", node_name); allocator.deallocate(node_name, allocator.state); char * namespace_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( - &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); ASSERT_TRUE(namespace_name != nullptr); EXPECT_STREQ("/some_ns", namespace_name); allocator.deallocate(namespace_name, allocator.state); @@ -87,21 +87,21 @@ TEST(TestNodeOptions, explicit_ros_args_and_non_ros_args) { char * node_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_name( - &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); + &rcl_options->arguments, nullptr, "my_node", allocator, &node_name)); ASSERT_TRUE(node_name != nullptr); EXPECT_STREQ("some_node", node_name); allocator.deallocate(node_name, allocator.state); char * namespace_name = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_remap_node_namespace( - &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); + &rcl_options->arguments, nullptr, "my_ns", allocator, &namespace_name)); ASSERT_TRUE(namespace_name != nullptr); EXPECT_STREQ("/some_ns", namespace_name); allocator.deallocate(namespace_name, allocator.state); int * output_indices = nullptr; EXPECT_EQ(RCL_RET_OK, rcl_arguments_get_unparsed( - &rcl_options->arguments, allocator, &output_indices)); + &rcl_options->arguments, allocator, &output_indices)); ASSERT_TRUE(output_indices != nullptr); const std::vector & args = options.arguments(); EXPECT_EQ("--non-ros-flag", args[output_indices[0]]);