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 base class for Node in gazebo plugins #771

Merged
merged 8 commits into from Jul 20, 2018

Conversation

@kev-the-dev
Copy link
Collaborator

commented Jul 17, 2018

Adds a class gazebo_ros::Node, which is a child of rclcpp::Node but manages a rclcpp::exeuctor among all it's instances. This allows gazebo plugins to create ROS nodes without worrying about setting up a thread to spin, like in the ros1 version of this package. It also handles ROS initialization and shutdown.

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Do you know how github figured out that a file had been renamed and displayed the appropriate diff here? It seems that git mv wouldn't help with that... I'm asking because it would be more convenient to review the diffs that way...

Copy link
Contributor

left a comment

Style and documentation comments

namespace gazebo_ros
{

/// Executor run in a seperate thread to handle events from all #gazebo_ros::Node instances

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

separate

/// Create an instance and start the internal thread
Executor();

/// Stops the internal exeuctor and joins with the internal thread

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

executor


private:
/// Thread where the exeuctor spins until destruction
std::thread spin_thread_;

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

Style nitpick: members should come after functions, see guide

* \param[in] node_name Name for the new node to create
* \return A shared pointer to a new #gazebo_ros::Node
*/
static SharedPtr Create(const std::string& node_name);

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

I believe the pointer style rule here also applies in this case, i.e. std::string & node_name. I also looked through existing ROS2 code and that seems to be the case.

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

Addressed by adding the linters

template <typename ...Args>
Node::SharedPtr Node::Create(Args && ...args)
{
// Throw exception is Node is created before ROS is initialized

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

is Node -> if Node?

return node;
}


This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

Nit: 2 blank lines

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

How do you recommend this PR to be tested manually?

Also, ideally we would start writing automated tests for all new functionality. This PR seems like a good place to start.

@kev-the-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2018

I have an example plugin using this in kev-the-dev#2, I'll try to convert that into a test

@kev-the-dev kev-the-dev force-pushed the kev-the-dev:ros2-devel-node branch from a7af591 to cd236f7 Jul 18, 2018
@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

@ironmig , heads up, while you work on a test, I'm enabling linters and fixing linter issues 😉

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Check out dee249e for the linters, feel free to pull the changes into this PR.


/// Create a #gazebo_ros::Node and add it to the global #gazebo_ros::Executor.
/**
* \todo Implement

This comment has been minimized.

Copy link
@chapulina

chapulina Jul 18, 2018

Contributor

How about we only add the declaration once the function is implemented? It can be mentioned on the design document so that we don't lose it.

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

This is looking good, @ironmig ! I have some suggestions on d34452a, could you take a look and let me know what you think? The main ideas are:

  • Remove the wrapper around the rclcpp::init call (i.e. Node::InitROS) so developers can use the original function directly
  • Rearranged the test directory and added a couple more test cases
@kev-the-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2018

Appreciate the additional test cases. I have two thoughts on the init change:

  • Do we need to worry about concurrent attempts to call rclcpp::Init? One nice thing about the wrapper is that it is mutex locked
  • Perhaps we should keep it wrapped in gazebo_ros::Node until the is_initialized function is merged/released with a TODO to eventually let rclcpp track the state
@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

That's a good point about concurrency. But in any case, the Node's mutex can't protect from init calls coming from outside the class, right?

I'm working right now on calling the init function from within the node without any arguments in case it hasn't been called yet and I can wrap that in a mutex. I think from our end, that's the most we can do for now. Once we move on to is_initialized, it should make it robust against init calls outside the class.

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

@ironmig , check out the latest version of branch ros2-devel-node_chapulina. The node calls init if it hasn't been called yet, that call is protected by a mutex, and I adjusted tests accordingly. I also printed a message as you suggested.

@kev-the-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2018

Looks good to me, integrated your changes

Copy link
Contributor

left a comment

LGTM

@chapulina chapulina merged commit 60bd978 into ros-simulation:ros2 Jul 20, 2018
@dhood dhood added the ros2 label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.