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

Jump to bottom

Implement rclcpp::is_initialized() #522

Merged
merged 2 commits into from
Jul 26, 2018
Merged

Implement rclcpp::is_initialized() #522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5eca004
Implement rclcpp::is_initialized()
chapulina Jul 19, 2018
f15da86
linter
chapulina Jul 25, 2018
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
8 changes: 8 additions & 0 deletions rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,14 @@ if(BUILD_TESTING)
target_link_libraries(test_utilities ${PROJECT_NAME})
endif()

ament_add_gtest(test_init test/test_init.cpp
APPEND_LIBRARY_DIRS "${append_library_dirs}")
if(TARGET test_init)
ament_target_dependencies(test_init
"rcl")
target_link_libraries(test_init ${PROJECT_NAME})
endif()

ament_add_gtest(test_multi_threaded_executor test/executors/test_multi_threaded_executor.cpp
APPEND_LIBRARY_DIRS "${append_library_dirs}")
if(TARGET test_multi_threaded_executor)
Expand Down
6 changes: 6 additions & 0 deletions rclcpp/include/rclcpp/utilities.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ RCLCPP_PUBLIC
bool
ok();

/// Returns true if init() has already been called.
/** \return True if init() has been called, false otherwise. */
RCLCPP_PUBLIC
bool
is_initialized();

/// Notify the signal handler and rmw that rclcpp is shutting down.
RCLCPP_PUBLIC
void
Expand Down
6 changes: 6 additions & 0 deletions rclcpp/src/rclcpp/utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,12 @@ rclcpp::ok()
return ::g_signal_status == 0;
}

bool
rclcpp::is_initialized()
{
return rcl_ok();
}

static std::mutex on_shutdown_mutex_;
static std::vector<std::function<void(void)>> on_shutdown_callbacks_;

Expand Down
4 changes: 2 additions & 2 deletions rclcpp/test/test_executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

using namespace std::chrono_literals;

class TestExcutors : public ::testing::Test
class TestExecutors : public ::testing::Test
{
protected:
static void SetUpTestCase()
Expand All @@ -50,7 +50,7 @@ class TestExcutors : public ::testing::Test
};

// Make sure that executors detach from nodes when destructing
TEST_F(TestExcutors, detachOnDestruction) {
TEST_F(TestExecutors, detachOnDestruction) {
{
rclcpp::executors::SingleThreadedExecutor executor;
executor.add_node(node);
Expand Down
29 changes: 29 additions & 0 deletions rclcpp/test/test_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2018 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 <gtest/gtest.h>

#include "rclcpp/utilities.hpp"

TEST(TestInit, is_initialized) {
EXPECT_FALSE(rclcpp::is_initialized());

rclcpp::init(0, nullptr);

EXPECT_TRUE(rclcpp::is_initialized());

rclcpp::shutdown();

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

@wjwwood wjwwood Jul 19, 2018 •
edited
Loading

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

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:

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

}
2 changes: 2 additions & 0 deletions rclcpp/test/test_utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,6 @@ TEST(TestUtilities, init_with_args) {

ASSERT_EQ(1u, other_args.size());
ASSERT_EQ(std::string{"process_name"}, other_args[0]);

EXPECT_TRUE(rclcpp::is_initialized());
}