Dummy robot demo by Karsten1987 · Pull Request #118 · ros2/demos

Karsten1987 · 2017-03-11T18:10:07Z

This is a PR for a robot demo which includes a map_server, joint_state_publisher, robot_state_publisher and a laser_scan_publisher. It basically resembles the rrbot.

The following PRs are dependent on this:

The following new packages have to be compiled for this:

robot state publisher: https://github.com/ros2/robot_state_publisher.git
robot_model: https://github.com/ros2/robot_model.git
orocos_kdl: https://github.com/ros2/orocos_kinematics_dynamics.git
urdfdom: https://github.com/ros2/urdfdom.git
urdfdom_headers: https://github.com/ros/urdfdom_headers.git
tinyxml_vendor: (optional) https://github.com/ros2/tinyxml_vendor.git

Depending on the success for the chocolatey packages, we can also use the upstream version of urdfdom* and tinyxml

Buldfarm Status:

linux:
mac: (unrelated test failure)
windows: In progress, we need tinyxml and eigen3 as chocolate packages

Karsten1987 · 2017-03-11T18:11:02Z

@ros2/team I leave this PR labeled as in progress due to open PRs, but ask for feedback already.

mikaelarguedas · 2017-03-12T03:09:09Z

same as ros2/ci#34 (comment), should all the dependencies listed above be added to the new repos file and this demo target a different branch than master?

mikaelarguedas

Looks good!
Haven't tried the code just read through it. Except a few minor comments it looks good to me.

Note: I think the dependencies necessary to run this code should be listed in the package.xml of all packages using them.
Note2: Could you detail the motivation for not leveraging the map_server ported recently and implementing a dummy one here instead ?

Nitpick: there is a .swp file in this PR that should be removed. We could add .gitignore files on the repos if we think it makes sense.

mikaelarguedas · 2017-03-12T03:10:08Z

dummy_robot/dummy_map_server/CMakeLists.txt

+project(dummy_map_server)
+
+if(NOT WIN32)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14 -Wall -Wextra")


Nit: please use -Wpedantic if complying with it is not too much work, same for every package of this PR

mikaelarguedas · 2017-03-12T03:11:44Z

dummy_robot/dummy_map_server/CMakeLists.txt

+find_package(ament_cmake REQUIRED)
+find_package(rclcpp REQUIRED)
+find_package(rmw REQUIRED)
+find_package(nav_msgs REQUIRED)


Nit: alphabetical

mikaelarguedas · 2017-03-12T03:13:47Z

dummy_robot/dummy_map_server/package.xml

+  <buildtool_depend>rosidl_default_generators</buildtool_depend>
+
+  <build_depend>rclcpp</build_depend>
+  <build_depend>nav_msgs</build_depend>


alphabetical order, same below

mikaelarguedas · 2017-03-12T03:16:12Z

dummy_robot/dummy_robot_bringup/launch/dummy_robot_bringup.py

@@ -0,0 +1,51 @@
+#!/usr/bin/env python3


avoid shebang lines for multiplatform compatibility

mikaelarguedas · 2017-03-12T03:17:08Z

dummy_robot/dummy_robot_bringup/package.xml

+  <license>Apache License 2.0</license>
+
+  <buildtool_depend>ament_cmake</buildtool_depend>
+


should declare launch as an exec dependency

mikaelarguedas · 2017-03-12T03:28:58Z

dummy_robot/dummy_map_server/src/dummy_map_server.cpp

+#include "rcl/rcl.h"
+#include "rclcpp/rclcpp.hpp"
+
+#include "nav_msgs/msg/occupancy_grid.hpp"


Nit: this should go before the rcl* imports in alphabetical order, same below

mikaelarguedas · 2017-03-12T03:50:22Z

dummy_robot/dummy_sensors/src/dummy_laser.cpp

+
+#include "sensor_msgs/msg/laser_scan.hpp"
+
+#define DEG2RAD M_PI / 180.0


Nit: Should this macro be function-like rather than a scalar?

Should math.h be included to use this?

copied from sick_lms code.

Ok, while not changing the macro structure, I still think we should import math.h in here to use M_PI

mikaelarguedas · 2017-03-12T03:51:07Z

dummy_robot/dummy_sensors/src/dummy_joint_states.cpp

+
+#include "sensor_msgs/msg/joint_state.hpp"
+
+#define DEG2RAD M_PI / 180.0


Not used in this file

mikaelarguedas · 2017-03-12T03:54:23Z

dummy_robot/dummy_sensors/src/dummy_laser.cpp

+    distance = std::abs(amplitude * std::sin(counter));
+
+    for (size_t i = 0; i < msg->ranges.size(); ++i) {
+      msg->ranges[i] = distance;


Given that msg->intensities has been allocated memory accordingly, would it make sense to fill it too here ?

it publishes 0 right now and are not necessary for the rviz demo.

sounds good, in that case maybe we can get rid of / comment out L51

mikaelarguedas · 2017-03-12T03:57:57Z

dummy_robot/dummy_sensors/src/dummy_laser.cpp

+    } else {
+      msg->header.stamp.sec =
+        static_cast<builtin_interfaces::msg::Time::_sec_type>(now.count() / 1000000000);
+      msg->header.stamp.nanosec = now.count() % 1000000000;


Same as above comments https://github.com/ros2/demos/pull/118/files#r105547867 and https://github.com/ros2/demos/pull/118/files#r105547888

Karsten1987 · 2017-03-13T06:55:21Z

Thanks for the feedback!

I didn't use the new map_server as this code was written before williams PR was merged. I'll definitely plan on using it, have to create a dummy map. Do we have any tutorial like demo maps?

For the tutorial repo file, I am not sure. I don't know if we have a real policy on what goes in there and what not. I still think that the robot_state_publisher is a rather crucial ROS(1/2) package, which should be in master - IMHO.

I'll run the build jobs again, to make sure the pedantic flag doesn't cause any trouble.

gerkey · 2017-03-13T18:20:14Z

@Karsten1987 if you're looking for an example map, the old Willow map is often helpful:

https://github.com/turtlebot/turtlebot_apps/tree/indigo/turtlebot_navigation/maps

mikaelarguedas · 2017-03-14T15:17:40Z

dummy_robot/dummy_map_server/src/dummy_map_server.cpp


-#include "rcl/rcl.h"
 #include "rclcpp/rclcpp.hpp"
+#include "rcl/rcl.h"


Nit: rcl before rclcpp was good. nav_msgs is the include I was referring to (that should be before the rcl* ones)

Karsten1987 · 2017-03-21T22:58:24Z

Tinyxml and Eigen3 chocolate packages can be found here:
https://github.com/ros2/choco-packages/releases

… dummy_robot_demo

Karsten1987 · 2017-03-22T20:18:59Z

Winner Winner Chicken Dinner!

Linux:
OSX:
Windows:

As mentioned in the previous comments, TinyXML and Eigen3 can simply be installed from brew or aptitude, for Windows we provide the choco packages.
I ran the demo successfully on Windows and piped it to RViz via the bridge!

@ros2/team If somebody could review that please.

wjwwood

lgtm, though I just skimmed over it.

Only comment is that there is still a .swp file committed in there that should be removed.

mikaelarguedas · 2017-03-22T21:25:38Z

👍 🎉

Karsten1987 · 2017-03-22T23:14:46Z

The changes for the installation wikipages can be found here:

linux: https://github.com/ros2/ros2/wiki/Linux-Development-Setup/_compare/bd41aa5615a3f59010aaacacf94a24366c7966bf...8fdf92a642716874ec549efc2b71d39bb282d16b

osx: https://github.com/ros2/ros2/wiki/OSX-Development-Setup/_compare/ec78654805e790f7d5a65fdd11192006654f255f...61ea04cecead0538005264eb270b172ba08b6c4e

windows: https://github.com/ros2/ros2/wiki/Windows-Development-Setup/_compare/01f788d93d7c7cef5a1bf6196973e5a1a514d0cc...38159b4f08531297271440d6dd042f32bd575b38

wjwwood · 2017-03-22T23:26:46Z

For the Windows one, do you think it would be better to specify that you mean to download the two .nuget files from that link? Or you could just link directly to the download of both.

Otherwise lgtm, thanks!

Karsten1987 · 2017-03-23T00:29:40Z

And finally the wiki page

https://github.com/ros2/ros2/wiki/dummy-robot-demo

dirk-thomas · 2017-06-26T20:25:46Z

dummy_robot/dummy_robot_bringup/launch/dummy_robot_bringup.py

+file_path = os.path.dirname(os.path.realpath(__file__))
+
+
+def launch():


Wrong signature for a launch file (see #140).

Karsten1987 added 6 commits March 2, 2017 16:00

inital dummy robot commit

8fe63fb

extend for dummy joint state pub

c037cbd

add linters

edf2b62

add urdf file

252f3bb

correct rrbot laser frame

6460859

fix flake8

5f30339

Karsten1987 self-assigned this Mar 11, 2017

dhood added the in progress Actively being worked on (Kanban column) label Mar 11, 2017

mikaelarguedas reviewed Mar 12, 2017

View reviewed changes

Karsten1987 force-pushed the dummy_robot_demo branch from 586ca06 to be463d9 Compare March 13, 2017 06:47

address comments

e3b2dfc

Karsten1987 force-pushed the dummy_robot_demo branch from be463d9 to e3b2dfc Compare March 13, 2017 06:51

mikaelarguedas reviewed Mar 14, 2017

View reviewed changes

Karsten1987 and others added 3 commits March 21, 2017 11:27

fix compiler warnings on msvc

5e40b87

use rclcpp::Time::now() to fill timestamp header

aeb059e

happy linters

d7f037f

Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 22, 2017

use rclcpp::Time::now() to fill timestamp header

a1113b3

Karsten1987 force-pushed the dummy_robot_demo branch from d7f037f to a1113b3 Compare March 22, 2017 16:38

Karsten1987 added 2 commits March 22, 2017 11:25

Merge branch 'dummy_robot_demo' of https://github.com/ros2/demos into…

74df57e

… dummy_robot_demo

plausible map values

e4d037a

wjwwood approved these changes Mar 22, 2017

View reviewed changes

delete .swp file

b42064a

Karsten1987 mentioned this pull request Mar 22, 2017

Add tinyxml and Eigen3 as dependencies ros2/ci#34

Merged

Karsten1987 mentioned this pull request Mar 22, 2017

Robot state publisher ros2/ros2#325

Merged

Karsten1987 merged commit 277e43c into master Mar 22, 2017

Karsten1987 removed the in review Waiting for review (Kanban column) label Mar 22, 2017

Karsten1987 deleted the dummy_robot_demo branch March 22, 2017 22:34

dhood mentioned this pull request Mar 23, 2017

Add tf2::TimePoint <-> builtin_interfaces::msg::Time conversions ros2/geometry2#2

Merged

dirk-thomas reviewed Jun 26, 2017

View reviewed changes

		<license>Apache License 2.0</license>

		<buildtool_depend>ament_cmake</buildtool_depend>


		#include "sensor_msgs/msg/laser_scan.hpp"

		#define DEG2RAD M_PI / 180.0


		#include "sensor_msgs/msg/joint_state.hpp"

		#define DEG2RAD M_PI / 180.0

		file_path = os.path.dirname(os.path.realpath(__file__))


		def launch():

Conversation

Karsten1987 commented Mar 11, 2017

Uh oh!

Karsten1987 commented Mar 11, 2017

Uh oh!

mikaelarguedas commented Mar 12, 2017

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerkey commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Mar 21, 2017

Uh oh!

Karsten1987 commented Mar 22, 2017

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Mar 22, 2017

Uh oh!

Karsten1987 commented Mar 22, 2017

Uh oh!

wjwwood commented Mar 22, 2017

Uh oh!

Karsten1987 commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Karsten1987 commented Mar 13, 2017 •

edited

Loading