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.

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

Implement rclcpp::is_initialized() #522

Merged
merged 2 commits into from
Jul 26, 2018
Merged

Implement rclcpp::is_initialized() #522

merged 2 commits into from
Jul 26, 2018

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jul 19, 2018

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

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

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
Copy link
Member

wjwwood commented Jul 19, 2018

@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
Copy link
Member

wjwwood commented Jul 19, 2018

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.


rclcpp::shutdown();

EXPECT_TRUE(rclcpp::is_initialized());
Copy link
Member

@wjwwood wjwwood Jul 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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):

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

wjwwood commented Jul 24, 2018

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

@chapulina
Copy link
Contributor Author

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

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

wjwwood commented Jul 25, 2018

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

@wjwwood
Copy link
Member

wjwwood commented Jul 25, 2018

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@chapulina
Copy link
Contributor Author

Fixed linter errors in f15da86

@chapulina
Copy link
Contributor Author

chapulina commented Jul 25, 2018

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Thanks for the CI crash-course, @wjwwood !

@wjwwood wjwwood merged commit fba891c into ros2:master Jul 26, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jul 26, 2018
@wjwwood
Copy link
Member

wjwwood commented Jul 26, 2018

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

@chapulina 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
Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants