ros / ros_tutorials

Code used in tutorials found on ROS wiki
http://wiki.ros.org/ros_tutorials
805 stars 540 forks source link

turtlesim for ROS2 #53

Closed routiful closed 5 years ago

routiful commented 5 years ago

I have been ported turtlesim node(including tutorials) to ROS2. It works same as ROS but has some differences in tutorials.

Add some information when mimic node is started (https://github.com/ROBOTIS-Platform/ros_tutorials/blob/f8b600d331060f39afc44436b3016d99a9fead97/turtlesim/tutorials/mimic.cpp#L15)

Add argument to add namespace in mimic node (https://github.com/ROBOTIS-Platform/ros_tutorials/blob/f8b600d331060f39afc44436b3016d99a9fead97/turtlesim/tutorials/mimic.cpp#L64)

Add stop signal in turtle_teleop node (https://github.com/ROBOTIS-Platform/ros_tutorials/blob/f8b600d331060f39afc44436b3016d99a9fead97/turtlesim/tutorials/teleop_turtle_key.cpp#L117)

Please review this and feel free to comment for anything.

routiful commented 5 years ago

Carefully Ping I assured that turtlesim is the best simulation for ROS2 users.

routiful commented 5 years ago

@dirk-thomas Thank you for your review. I almost have accepted your required. I have tested this in dashing-release.

dirk-thomas commented 5 years ago

(I will be out of the office for the next month. So it is unlikely that you will get further feedback until I am back mid July.)

routiful commented 5 years ago

@dirk-thomas I've done to revert original code and delete some unnecessary diff.

routiful commented 5 years ago

@dirk-thomas Carefully Ping. Any comments?

routiful commented 5 years ago

@dirk-thomas

Can you please enable the flag for allowing me to push changes to your branch.

I don't understand which means of flag. Could you describe to me more specific??

dirk-thomas commented 5 years ago

I don't understand which means of flag. Could you describe to me more specific??

https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

routiful commented 5 years ago

@dirk-thomas Thanks. I'v enabled that check box.

dirk-thomas commented 5 years ago

CI confirms that the package build successfully on Linux / macOS (I doesn't have any tests):

On Windows it fails to build atm but that can be addressed in a separate PR:

Thank you for iterating on this even though it took me a while to get back at it.