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

added fake_node pkg, tb3_house #90

Merged
merged 17 commits into from
Sep 16, 2019
Merged

added fake_node pkg, tb3_house #90

merged 17 commits into from
Sep 16, 2019

Conversation

JaehyunShim
Copy link
Contributor

No description provided.

@robotpilot
Copy link
Member

@rjshim
From next PR, raise PR from your special branch to ros2-devel. ros2 is the branch we work on for release.

Copy link

@routiful routiful left a comment

Choose a reason for hiding this comment

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

I attached some comment.
Best regards

turtlebot3_fake_node/CMakeLists.txt Show resolved Hide resolved
turtlebot3_fake_node/src/turtlebot3_fake_node.cpp Outdated Show resolved Hide resolved
turtlebot3_fake_node/src/turtlebot3_fake_node.cpp Outdated Show resolved Hide resolved
turtlebot3_fake_node/src/turtlebot3_fake_node.cpp Outdated Show resolved Hide resolved
turtlebot3_fake_node/package.xml Show resolved Hide resolved
@routiful
Copy link

routiful commented Sep 9, 2019

@rjshim Please check your files to pass CI. And describe what is changed this PR.

@JaehyunShim
Copy link
Contributor Author

@robotpilot I didn't know how the system works. I will close this PR and reupload it.
@routiful I will. Thanks!

Ryan

* limitations under the License.
*******************************************************************************/

/* Authors: Yoonseok Pyo */
Copy link
Member

Choose a reason for hiding this comment

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

add your name to Authors list

@@ -0,0 +1,39 @@
# /*******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a mixture of Python and C ++. Please use Python comments.

@@ -0,0 +1,78 @@
# /*******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a mixture of Python and C ++. Please use Python comments.

Node(
package='turtlebot3_fake_node',
node_executable='turtlebot3_fake_node',
# node_name='tb3_fake_node',
Copy link
Member

Choose a reason for hiding this comment

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

Why is node_name commented?

joint_states_frame: "base_footprint"
odom_frame: "odom"
base_frame: "base_footprint"

Copy link
Member

Choose a reason for hiding this comment

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

There is no EOF.

joint_states_frame: "base_footprint"
odom_frame: "odom"
base_frame: "base_footprint"

Copy link
Member

Choose a reason for hiding this comment

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

There is no EOF.

joint_states_frame: "base_footprint"
odom_frame: "odom"
base_frame: "base_footprint"

Copy link
Member

Choose a reason for hiding this comment

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

There is no EOF.

void Turtlebot3Fake::init_variables()
{
// Initialise variables
wheel_speed_cmd_[LEFT] = 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

Delete unnecessary indents.

@JaehyunShim JaehyunShim closed this Sep 9, 2019
@JaehyunShim
Copy link
Contributor Author

Added the following modifications.

  1. test lines removed from CMakeLists.txt
  2. unnecessary indentations removed
  3. author name added
  4. missing empty lines added to EOF
  5. python file format edited (tested with ament_flake8)
  6. rviz config changed (background colour changed to white, tf display disabled)

@JaehyunShim JaehyunShim reopened this Sep 9, 2019
@robotpilot
Copy link
Member

The current CI may fail depending on the ROS Distro update situation.
The next ROS Distro update for ROS 2 Dashing will be September 10th, so this PR's merge will continue after 10th.

Signed-off-by: Pyo <pyo@robotis.com>
Copy link
Member

@robotpilot robotpilot left a comment

Choose a reason for hiding this comment

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

Good! Please correct some of the comments mentioned above.

turtlebot3_fake_node/src/turtlebot3_fake_node.cpp Outdated Show resolved Hide resolved
turtlebot3_fake_node/src/turtlebot3_fake_node.cpp Outdated Show resolved Hide resolved
@robotpilot
Copy link
Member

Today ROS 2 Dashing patch 3 has been released, and ROS 2 packages are synced. I modified CI settings for ROS 2 Dashing patch 3 and dependency packages.

@routiful
Copy link

@rjshim After commit to fix your code, please comment on each conversation. That makes reviewer would be more easy to check what you fixed.

@JaehyunShim
Copy link
Contributor Author

@routiful Thank you for the advice 👍

Added the following modifications.

  1. Lines with seconds() have been changed to use nanoseconds() in the turtlebot3_fake_node package. ref

@robotpilot
Copy link
Member

Build Status

ros2-devel ros2 Dashing + Ubuntu Bionic
Build Status Build Status Build Status

@robotpilot
Copy link
Member

@rjshim I'm working on a release. Please correct the last @routiful's comment.

@routiful
Copy link

@rjshim
I think package name and launch file name should be correct to ROS 1 version

http://emanual.robotis.com/docs/en/platform/turtlebot3/simulation/#turtlebot3-simulation-using-fake-node

@JaehyunShim
Copy link
Contributor Author

@routiful

I actually thought about it and what I thought was the current name (fake_node rather than fake) can better express what the package is (also, eManual for ROS 2 can be written differently from ROS 1).

If that name is commonly used for mobile robots in general or the author who originally wrote it wants it that way, I will just follow the advice and reupload the package. @robotpilot

The modification made for this will be uploaded together.

Regards,
Ryan

@robotpilot
Copy link
Member

@rjshim I accept your opinion. Please proceed with the changed name.

@JaehyunShim
Copy link
Contributor Author

@robotpilot @routiful Thank you for the advice. I kept the name 'turtlebot3_fake_node' for the package name and launch file name. Here are the changes made.

  1. rclcpp::Node::now() are used instead of rclcpp::Clock::now() as advised.

@robotpilot robotpilot merged commit 33d7470 into ros2 Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants