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

turtlesim for ROS2 #53

Merged
merged 61 commits into from
Sep 11, 2019
Merged

Conversation

routiful
Copy link

Darby Lim added 4 commits May 15, 2019 10:41
Signed-off-by: Darby Lim <[email protected]>
Signed-off-by: Darby Lim <[email protected]>
Signed-off-by: Darby Lim <[email protected]>
Signed-off-by: Darby Lim <[email protected]>
turtlesim/src/turtle.cpp Outdated Show resolved Hide resolved
@routiful
Copy link
Author

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

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

I added some comments inline but didn't go through the whole patch for now. The general feedback:

  • Please avoid any unnecessary changes in this patch (including style changes, refactorings, etc.). Only the bare minimum to convert from ROS 1 to ROS 2. The reason is to keep the dashing-devel branch as close to the melodic-devel branch as possible. Otherwise maintaining both branches moving forward will be much more effort.
  • Any changes unrelated to the porting should be done in separate PRs. Preferably against the default branch from where they can be cherry-picked into the ROS 2 branch. Maybe consider getting them reviewed and merged first.

turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
turtlesim/include/turtlesim/turtle.h Outdated Show resolved Hide resolved
turtlesim/src/turtle_frame.cpp Outdated Show resolved Hide resolved
turtlesim/src/turtle_frame.cpp Outdated Show resolved Hide resolved
turtlesim/tutorials/teleop_turtle_key.cpp Outdated Show resolved Hide resolved
turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
turtlesim/CMakeLists.txt Outdated Show resolved Hide resolved
@routiful
Copy link
Author

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

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

I added another comment and some previous comments are still pending.

The main goal is to keep the diff as minimal as possible. So if a change isn't required to make it work with ROS 2 it shouldn't be part of this PR.

turtlesim/include/turtlesim/turtle.h Outdated Show resolved Hide resolved
@routiful
Copy link
Author

routiful commented Sep 11, 2019

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

@routiful
Copy link
Author

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

various changes to the pending PR
@dirk-thomas
Copy link
Member

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

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

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

  • Windows Build Status

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

@dirk-thomas dirk-thomas merged commit d88540b into ros:dashing-devel Sep 11, 2019
This was referenced Sep 11, 2019
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.

3 participants