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

[ros2] Add check if rclcpp::init() has been called #859

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

otamachan
Copy link

Hi,

I would like to use both gazebo_ros_init and gazebo_ros_factory plugins at the same time. But when I tried that, it failed.

tamaki@pc:~$ gzserver --verbose -s libgazebo_ros_init.so -s libgazebo_ros_factory.so
Gazebo multi-robot simulator, version 9.0.0
Copyright (C) 2012 Open Source Robotics Foundation.
Released under the Apache 2 License.
http://gazebosim.org

terminate called after throwing an instance of 'rclcpp::ContextAlreadyInitialized'
  what():  context is already initialized
Aborted (core dumped)

This PR adds check if rclcpp::init() has been called before calling it.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for catching the problem and making a PR!

The new test failed on the build farm, it looks like the message is published too late:

19:54:56 12: �[0m/tmp/ws/src/gazebo_ros_pkgs/gazebo_ros/test/test_plugins.cpp:63: Failure
19:54:56 12: Expected: (msg) != (nullptr), actual: 16-byte object <00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00> vs 8-byte object <00-00 00-00 00-00 00-00>
19:54:56 12: test
19:54:57 12: [WARN] [hello_ros_world]: Publishing

rclcpp::init(argc, argv);
if (!rclcpp::is_initialized()) {
rclcpp::init(argc, argv);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call! It may also be worth it adding a check to gazebo_ros_init, and in both cases, printing a warning if it's been initialized, so the user knows their arguments won't be loaded.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I'll add a check to gazebo_ros_init, too.
I think using both gazebo_ros_init and gazebo_ros_factory is a common use case for ROS users. In that case it might be annoying when a warning is always printed. Is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's go without the warning, thanks!

@chapulina chapulina added the ros2 label Dec 14, 2018
@chapulina chapulina self-assigned this Dec 14, 2018
@otamachan
Copy link
Author

Thank you for your reviewing.

  1. Test failure
    I could reproduce the test failure locally, but after upgrading rclcpp to master branch it never happens. So it might not be related to this change. (Resolve startup race condition for sim time ros2/rclcpp#608 ?)
    The test is still failing but [ros2] Improve executor destruction #860 will solve the problem.
  2. I removed calling rclcpp::init() from gazebo_ros_factory rather than checking and calling because gazebo_ros::Node::Get() will call rclcpp::init() automatically.
  3. I added warning to gazebo_ros_init if rclcpp::init is already called as I think gazebo_ros_init should be the only way to call rclcpp::init with user defined arguments.

Thanks for your advice in advance.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for iterating, this looks good to me.

rclcpp::init(argc, argv);
impl_->ros_node_ = gazebo_ros::Node::Get();
} else {
impl_->ros_node_ = gazebo_ros::Node::Get();
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this line can be moved out of the if/else

@chapulina chapulina merged commit 8421aac into ros-simulation:ros2 Dec 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants