Implement rclcpp::is_initialized() by chapulina · Pull Request #522 · ros2/rclcpp

chapulina · 2018-07-19T23:15:44Z

Addresses #518.

Named the function is_initialized instead of isInitialized because that seemed to be the style throughout the utilities
It turns out that rcl_ok was already doing what was desired, so rclcpp::is_initialized is just a wrapper around that.
Added new test file so it could run isolated

mikaelarguedas · 2018-07-19T23:19:56Z

hmm that's where the naming will get confusing..
Change is good as rcl_ok actually means "is initialized". It's just confusing that it doesn't have the same semantic as rclcpp::ok.

👍 lgtm with green CI

wjwwood · 2018-07-19T23:25:41Z

@mikaelarguedas that's why originally I had suggested that should be the semantic of rclcpp::ok(): #3 (comment)

I'm fine with having an is_initialized() as @chapulina has implemented here, but I think ok should probably also change in rclcpp to match how it worked (or at least how it is documented) in roscpp.

wjwwood · 2018-07-19T23:28:38Z

Or the name in rcl should also change to rcl_is_initialized(). That would also be ok by me. At the very least we should ticket the need to change the name of rcl_ok if we merge this without doing it directly.

wjwwood · 2018-07-19T23:32:20Z

rclcpp/test/test_init.cpp

+
+  rclcpp::shutdown();
+
+  EXPECT_TRUE(rclcpp::is_initialized());


I would expect is_initialized() to be false after shutdown, is that not the expectation? I believe rcl_ok should return false if you call rcl_shutdown (maybe we're not doing that properly yet).

Also, what happens if you do:

init

shutdown

init

EXPECT_TRUE(rclcpp::is_initialized())

I'd then expect it to be true, but not before doing the second init.

Sorry the previous comment was really two different questions, let me edit it real quick.

Yeah, it looks like we're not calling rcl_shutdown ever, which until now hasn't had any consequence (none of our middlewares do anything on rcl_shutdown/rmw_shutdown):

rclcpp/rclcpp/src/rclcpp/utilities.cpp

Lines 270 to 288 in 8f2052d

void

rclcpp::shutdown()

{

trigger_interrupt_guard_condition(SIGINT);

{

std::lock_guard<std::mutex> lock(on_shutdown_mutex_);

for (auto & on_shutdown_callback : on_shutdown_callbacks_) {

on_shutdown_callback();

}

}

}

void

rclcpp::on_shutdown(std::function<void(void)> callback)

{

std::lock_guard<std::mutex> lock(on_shutdown_mutex_);

on_shutdown_callbacks_.push_back(callback);

}

But now it means that rcl_ok()'s return value doesn't change when rclcpp::shutdown() is used, despite that really being the intention:

https://github.com/ros2/rcl/blob/d3a734f2661a05fb1ab0746907a3477a02de8dbd/rcl/include/rcl/rcl.h#L137

https://github.com/ros2/rcl/blob/d3a734f2661a05fb1ab0746907a3477a02de8dbd/rcl/test/rcl/test_rcl.cpp#L140-L143

I added the last expectation just to make it clear that the current behaviour is that once initialized, there's no way back.

I don't think init -> shutdown -> init works, I tried shutting down in the end of TestUtilities.init_with_args and adding a new init test after that but an exception is thrown.

I'm not sure what the consequences of calling rcl_shutdown could be and whether it would be worth doing it on this PR. Whatcha think?

I don't know, but I don't want to block this. I think it's clear more work needs to be done though (after merging this).

wjwwood · 2018-07-24T23:53:18Z

Btw, @chapulina should this be in review? or are you still working on it?

chapulina · 2018-07-24T23:57:36Z

Not working on it, unless the maintainers think the shutdown issue must be fixed before getting this in.

wjwwood · 2018-07-25T00:00:31Z

@mikaelarguedas said it lgth, and I don't want to block it. I'll just make a follow up issue.

wjwwood · 2018-07-25T00:03:02Z

CI:

Linux
Linux-aarch64
macOS
Windows

chapulina · 2018-07-25T22:24:25Z

Fixed linter errors in f15da86

chapulina · 2018-07-25T23:21:30Z

Linux
Linux-aarch64
macOS
Windows

Thanks for the CI crash-course, @wjwwood !

wjwwood · 2018-07-26T20:18:55Z

I think this issue covers the improvements I talked about before (being able to do init-shutdown-init, etc.): #154

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

Implement rclcpp::is_initialized()

5eca004

dirk-thomas added the in progress Actively being worked on (Kanban column) label Jul 19, 2018

wjwwood reviewed Jul 19, 2018

View reviewed changes

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 25, 2018

linter

f15da86

wjwwood approved these changes Jul 25, 2018

View reviewed changes

wjwwood merged commit fba891c into ros2:master Jul 26, 2018

wjwwood removed the in review Waiting for review (Kanban column) label Jul 26, 2018

wjwwood mentioned this pull request Jul 26, 2018

Add way to check if init has been called (i.e. isInitialized) #518

Closed

chapulina deleted the is_initialized branch July 26, 2018 20:48

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

ready_fn and self.proc_info will be removed in future (ros2#522)

7859398

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

	void
	rclcpp::shutdown()
	{
	trigger_interrupt_guard_condition(SIGINT);

	{
	std::lock_guard<std::mutex> lock(on_shutdown_mutex_);
	for (auto & on_shutdown_callback : on_shutdown_callbacks_) {
	on_shutdown_callback();
	}
	}
	}

	void
	rclcpp::on_shutdown(std::function<void(void)> callback)
	{
	std::lock_guard<std::mutex> lock(on_shutdown_mutex_);
	on_shutdown_callbacks_.push_back(callback);
	}

Conversation

chapulina commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented Jul 19, 2018

Uh oh!

wjwwood commented Jul 19, 2018

Uh oh!

wjwwood commented Jul 19, 2018

Uh oh!

wjwwood Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

wjwwood Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

chapulina Jul 20, 2018

Choose a reason for hiding this comment

Uh oh!

wjwwood Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Jul 24, 2018

Uh oh!

chapulina commented Jul 24, 2018

Uh oh!

wjwwood commented Jul 25, 2018

Uh oh!

wjwwood commented Jul 25, 2018

Uh oh!

chapulina commented Jul 25, 2018

Uh oh!

chapulina commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Jul 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chapulina commented Jul 19, 2018 •

edited

Loading

wjwwood Jul 19, 2018 •

edited

Loading

chapulina commented Jul 25, 2018 •

edited

Loading