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

Add enable_rosout into NodeOptions. #900

Merged
merged 6 commits into from
Dec 3, 2019
Merged

Add enable_rosout into NodeOptions. #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0d034a
Add disable_rosout into NodeOptions.
Barry-Xu-2018 Oct 22, 2019
103565b
Update comments
Barry-Xu-2018 Oct 24, 2019
56a71d9
keep implementation consistency by using enable_rosout name
Barry-Xu-2018 Oct 29, 2019
23fc30a
fix error comment
Barry-Xu-2018 Nov 22, 2019
6f4c9cb
add test case for node options
Barry-Xu-2018 Nov 25, 2019
74ae606
fix source about copy value and reset rcl_node_options, add more test…
Barry-Xu-2018 Nov 28, 2019
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
18 changes: 18 additions & 0 deletions rclcpp/include/rclcpp/node_options.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,22 @@ class NodeOptions
NodeOptions &
use_global_arguments(bool use_global_arguments);

/// Return the enable_rosout flag.
RCLCPP_PUBLIC
bool
enable_rosout() const;

/// Set the enable_rosout flag, return this for parameter idiom.
/**
* If false this will cause the node not to use rosout logging.
*
* Defaults to true for now, as there are still some cases where it is
* desirable.
*/
RCLCPP_PUBLIC
NodeOptions &
enable_rosout(bool enable_rosout);

/// Return the use_intra_process_comms flag.
RCLCPP_PUBLIC
bool
Expand Down Expand Up @@ -312,6 +328,8 @@ class NodeOptions

bool use_global_arguments_ {true};

bool enable_rosout_ {true};

bool use_intra_process_comms_ {false};

bool start_parameter_services_ {true};
Expand Down
16 changes: 16 additions & 0 deletions rclcpp/src/rclcpp/node_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ NodeOptions::operator=(const NodeOptions & other)
this->arguments_ = other.arguments_;
this->parameter_overrides_ = other.parameter_overrides_;
this->use_global_arguments_ = other.use_global_arguments_;
this->enable_rosout_ = other.enable_rosout_;
this->use_intra_process_comms_ = other.use_intra_process_comms_;
this->start_parameter_services_ = other.start_parameter_services_;
this->allocator_ = other.allocator_;
Expand All @@ -90,6 +91,7 @@ NodeOptions::get_rcl_node_options() const
node_options_->allocator = this->allocator_;
node_options_->use_global_arguments = this->use_global_arguments_;
node_options_->domain_id = this->get_domain_id_from_env();
node_options_->enable_rosout = this->enable_rosout_;

int c_argc = 0;
std::unique_ptr<const char *[]> c_argv;
Expand Down Expand Up @@ -198,6 +200,20 @@ NodeOptions::use_global_arguments(bool use_global_arguments)
return *this;
}

bool
NodeOptions::enable_rosout() const
{
return this->enable_rosout_;
}

NodeOptions &
NodeOptions::enable_rosout(bool enable_rosout)
{
this->node_options_.reset(); // reset node options to make it be recreated on next access.
this->enable_rosout_ = enable_rosout;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Barry-Xu-2018 thinking about the test made me realize you probably want to node_options_.reset() here.

return *this;
}

bool
NodeOptions::use_intra_process_comms() const
{
Expand Down
32 changes: 32 additions & 0 deletions rclcpp/test/test_node_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,35 @@ TEST(TestNodeOptions, bad_ros_args) {
options.get_rcl_node_options(),
rclcpp::exceptions::UnknownROSArgsError);
}

TEST(TestNodeOptions, enable_rosout) {
{
auto options = rclcpp::NodeOptions();
EXPECT_TRUE(options.enable_rosout());
EXPECT_TRUE(options.get_rcl_node_options()->enable_rosout);
}

{
auto options = rclcpp::NodeOptions().enable_rosout(false);
EXPECT_FALSE(options.enable_rosout());
EXPECT_FALSE(options.get_rcl_node_options()->enable_rosout);
}

{
auto options = rclcpp::NodeOptions().enable_rosout(true);
EXPECT_TRUE(options.enable_rosout());
EXPECT_TRUE(options.get_rcl_node_options()->enable_rosout);
}

{
auto options = rclcpp::NodeOptions();
EXPECT_TRUE(options.enable_rosout());
EXPECT_TRUE(options.get_rcl_node_options()->enable_rosout);
options.enable_rosout(false);
EXPECT_FALSE(options.enable_rosout());
EXPECT_FALSE(options.get_rcl_node_options()->enable_rosout);
options.enable_rosout(true);
EXPECT_TRUE(options.enable_rosout());
EXPECT_TRUE(options.get_rcl_node_options()->enable_rosout);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Barry-Xu-2018 this is fine, but I think it'd be slightly better to test if after an options.enable_rosout(boolean_value) that gets reflected in options.get_rcl_node_options().enable_rosout.