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

Open
wants to merge 40 commits into
base: dashing-devel
from

Conversation

Projects
None yet
3 participants
@routiful
Copy link

commented May 15, 2019

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 added some commits May 15, 2019

add colcon_build
Signed-off-by: Darby Lim <thlim@robotis.com>
port turtlesim to ROS2
Signed-off-by: Darby Lim <thlim@robotis.com>
update for sync dashing api
Signed-off-by: Darby Lim <thlim@robotis.com>
add author
Signed-off-by: Darby Lim <thlim@robotis.com>

@routiful routiful referenced this pull request May 15, 2019

Closed

turtlesim for ROS2 #331

Show resolved Hide resolved turtlesim/src/turtle.cpp Outdated

routiful added some commits May 17, 2019

modified arguments to generate Duration
Signed-off-by: Darby Lim <thlim@robotis.com>
modified msg ptr from shared to unique
Signed-off-by: Darby Lim <thlim@robotis.com>
make shared to unique ptr
Signed-off-by: Darby Lim <thlim@robotis.com>
@routiful

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

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

@dirk-thomas
Copy link
Member

left a comment

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

routiful added some commits Jun 11, 2019

@routiful

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

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

@dirk-thomas
Copy link
Member

left a comment

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.

Show resolved Hide resolved turtlesim/include/turtlesim/turtle.h Outdated

routiful and others added some commits Jun 13, 2019

@@ -246,26 +248,26 @@ void TurtleFrame::updateTurtles()
if (modified)
{
update();
}

}

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 13, 2019

Member

Unnecessary diff.

This comment has been minimized.

Copy link
@routiful

routiful Jun 18, 2019

Author

Accepted

clear();

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 13, 2019

Member

Unnecessary diff.

This comment has been minimized.

Copy link
@routiful

routiful Jun 18, 2019

Author

Accepted

turtles_.clear();
id_counter_ = 0;
spawnTurtle("", width_in_meters_ / 2.0, height_in_meters_ / 2.0, 0);
clear();

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 13, 2019

Member

Unnecessary diff.

This comment has been minimized.

Copy link
@routiful

routiful Jun 18, 2019

Author

Accepted

return QApplication::exec();
}

private:
std::shared_ptr<turtlesim::TurtleFrame> frame_;

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 13, 2019

Member

Why was the frame changed from a local variable in exec to a member variable? If there is no reason please revert.

This comment has been minimized.

Copy link
@routiful

routiful Jun 18, 2019

Author

I've reverted

#define PI 3.141592

void poseCallback(const turtlesim::PoseConstPtr& pose)
class DrawSquare : public rclcpp::Node

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 13, 2019

Member

As of here the patch contains still largely unnecessary changes. Please keep the patch as close as possible to the upstream branch.

This applies to pretty much everything below.

This comment has been minimized.

Copy link
@routiful

routiful Jun 18, 2019

Author

Accepted

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

(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 added some commits Jun 18, 2019

@routiful

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.