Several improvements to be more ROS compatible. #5

sem23 · 2017-05-16T18:26:20Z

Hey David,

I made some improvements towards ROS compatibility and added a naive path follow controller for testing (and using) integrated path planning. Furthermore I added continuous integration using travis-ci.
Feel free to test yourself before merge!

I added a Github Project for RatSLAM ROS compatibility on my fork, so feel free to add the issues you never had time to do ;-) . It would be great to get some input from the original author!

Best Regards,
Peter

#2. * [mod] added boost archive feature to PosecellVisualTemplate class * [mod] added tf map->odom publisher to experience mapper/localizer * [mod] added save/load functions to experience map * [mod] added save/load functions to local view matcher * [mod] added save/load functions to pose cell network

* [mod] reformatted code style using clang-format

* [mod] modified CMakeLists.txt to link against Boost

* [add] added python path follow controller node

* [add] added travis CI configuration provided by ROS Industrial team

* [fix] fixed wrong dependency to opencv (this fixes rosdep issues) * [mod] added dependency to irrlicht

* [mod] added missing dependency to OpenGL

* [add] added readme in ROS doc style, including travis-ci build status

* [mod] moved boost linking to ratslam library target in CMakeLists.txt * [mod] enabled Cxx11 support in CMakeLists.txt * [mod] added "src" as include directory in CMakeLists.txt

davidmball · 2017-05-17T11:28:48Z

Thanks! I've had a quick look through and this looks good. I'll do a more detailed review soon.

I definitely agree with the code reformating, however, mixing it in with the functional changes make it much harder to review. I normally try and separate functional from reformting changes. Any thoughts on separating it into two prs?

sem23 · 2017-05-17T18:50:01Z

Edit: LOL ... I got you that wrong with my first answer... Sorry! I thought you meant split code into two parts! and not just 2 pull requests 😄 ... ok my original answer is related to splitting up code into core library and ROS wrappers... and for the 2 PRs, I got it now: the formatting hides the changes ... I think I was not aware of this ... damn ... I have a look what I can do!

Yeah, had already some thoughts. In best case we build a nice cpp library containing ratslam and build the ROS wrappers separately. But therefor we would need a new repository setup I think. The easiest solution (at least for me 😄 ) would be to maintain just the ROS version from now on. But I agree that easy is most often not good 😄 . I will wait for you till I prepare the bloom release! ... I really want to get this code into the official ROS distributions, I think it would be great! When I was a student, I used it a lot with many kinds of robots.

So best case would look like this:

ratslam repo
ratslam ros repo (this repo includes first one, like gmapping or others are doing)
I must have a look if the ROS buildfarm tools are using their own wstool, if yes then we could a the ratslam repo as a git sumodule without any troubles.

sem23 · 2017-05-20T10:35:18Z

Hey guys,
Ok, I think the easiest way to review this as intended is to use the github feature to review a range of commits from the PR. At least the order of the commits allows to review the changes without being hidden. Is this ok for you? I will start preparing the bloom release in parallel.
Regards,
Peter

davidmball

Thanks for the big pr to update open ratslam. It definitely needed this update. It mostly looks good to me. I've made some review comments thoughout.

Note that the irat example doesn't work. I had to add a namespace and a remap to make it work. This could be part of an additional pr though.

davidmball · 2017-05-20T19:58:53Z

.travis.yml

@@ -0,0 +1,26 @@
+sudo: required
+dist: trusty


Note I don't know travis syntax so you'll have to check this.

Have you had a look at the travis page which shows the running builds and its status? You'll see it is working 😉 . I took this script from the official ROS industrial repository. On the ROS wiki it is said that this is a very good entry point for prerelease testing. So it is tested quite well. Also you get a "build status" badge/banner which can be shown (I think I display this in the README.md).

davidmball · 2017-05-20T20:01:27Z

CMakeLists.txt

@@ -3,6 +3,8 @@ project(ratslam_ros)

 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/cmake)

+# Enable Cxx11 support
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")


I prefer the longer version here where you get an error if C++11 isn't found.

include(CheckCXXCompilerFlag) CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11) CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X) if(COMPILER_SUPPORTS_CXX11) set(CMAKE_CXX_FLAGS "-std=c++11") elseif(COMPILER_SUPPORTS_CXX0X) set(CMAKE_CXX_FLAGS "-std=c++0x") else() message(FATAL_ERROR "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.") endif()

Agree! I wanted to replace raw pointers by smart ones in near future. Will take your snippet! Thanks!

davidmball · 2017-05-20T20:02:12Z

CMakeLists.txt

  ${catkin_INCLUDE_DIRS}
 )

 #ratslam library
 add_library(ratslam src/ratslam/experience_map.cpp src/ratslam/posecell_network.cpp src/ratslam/local_view_match.cpp src/ratslam/visual_odometry.cpp)

+target_link_libraries(ratslam
+  ${Boost_LIBRARIES}
+)


Including boost is fine. But note that I would take this in a direction to minimise boost dependencies.

davidmball · 2017-05-20T20:04:26Z

CMakeLists.txt

  ${catkin_INCLUDE_DIRS}
 )

 #ratslam library
 add_library(ratslam src/ratslam/experience_map.cpp src/ratslam/posecell_network.cpp src/ratslam/local_view_match.cpp src/ratslam/visual_odometry.cpp)

+target_link_libraries(ratslam
+  ${Boost_LIBRARIES}
+)


I think you also need to add here

${IRRLICHT_LIBRARIES} ${OpenCV_LIBRARIES}

Otherwise it wouldn't compile for me. Unless these are called in the main ones below then you might be able to remove these references below.

That's interesting as the travis was able to build it in this configuration. I will check this!

davidmball · 2017-05-20T20:05:30Z

README.md

+OpenRatSLAM has only been tested on Ubuntu and as ROS support for platforms other than Linux is still limited, this will probably not change.
+
+* Maintainer status: maintained
+* Maintainer: Peter Rudolph <semael23 AT gmail DOT com>


I'm happy to be a maintainer as well for the moment. davidmichaelball AT gmail DOT com

Of course, I will add you! To excuse myself, I never thought to get an answer from you 😄 . Shall I add scott, too?

I'll leave that up to Scott.

davidmball · 2017-05-20T21:29:40Z

README.md

+
+#### 4.3.1 Subscribed Topics
+
+"image" ([sensor_msgs/Image](http://docs.ros.org/api/sensor_msgs/html/msg/Image.html))


To make this work with the bag files it needs a topic root name_space. Maybe this should be an additional issue.

The topic_root parameter can be replaced by correct usage of the namespace of the node I think. I will try to setup some example launch file, so that you can get what I mean. 😃

davidmball · 2017-05-20T21:31:08Z

script/path_follower.py

+            x = goal_pose.pose.position.x
+            y = goal_pose.pose.position.y
+            dist = np.sqrt(x**2+y**2)
+            angle = np.math.atan2(y, x)


You should probably clip the angle between pi and -pi?

Isn't atan2 doing this for me?! 😄 I will check this!

davidmball · 2017-05-20T21:33:53Z

src/main_em.cpp

-void odo_callback(nav_msgs::OdometryConstPtr odo, ratslam::ExperienceMap *em)
+std::string map_frame = std::string("map");
+std::string odom_frame = std::string("odom");
+std::string base_frame = std::string("base_footprint");


I think base_link would be better

You are the boss! 😃 I think you are right. It's more convenient.

davidmball · 2017-05-20T21:36:04Z

src/main_em.cpp


  if (argc < 2)
  {
-    ROS_FATAL_STREAM("USAGE: " << argv[0] << " <config_file>");
+    std::cout << "USAGE: " << argv[0] << " <config_file>";


The ROS ones might be better?

Oh yeah! I think this is a "debug" output leftover, damn. Will be fixed!

Checked it. I changed it because the ROS node is not initialized before. These macros make use of rosout publisher which is initialized with node or first node handle.

davidmball · 2017-05-20T21:37:59Z

src/main_lv.cpp

  image_transport::ImageTransport it(node);
-  image_transport::Subscriber sub = it.subscribe(topic_root + "/camera/image", 0, boost::bind(image_callback, _1, &pub_vt));
+  image_transport::Subscriber sub = it.subscribe("image", 0, image_callback);


Note that the bag files for the examples use camera. So this is okay here but the launch files should be modified to remap and have a namespace.

Sorry for not doing this! I tested all the things for a convenient usage with my little ANKI Cozmo robot. So I didn't care a lot about the existing ones. Definitely my fault. Will be checked & fixed!

scott-- · 2017-05-21T22:03:33Z

src/main_em.cpp

+  {
+    boost::archive::binary_oarchive binary_archive(ofs);
+    // write class instance to archive
+    binary_archive << *em;


An issue that I had when implementing save/load in the ROS version of RatSLAM was synchronising the different nodes together. Does this race condition (the nodes out of sync) affect loading/saving?

Oh man. Seems that I am really a lucky bastard! I did not thought about this while implementing. But I tested it many times live and experienced no issues. I will check if the implementation is sufficient, otherwise I will try to implement a more robust way of communication. Also running the nodes as nodelets in a nodelet_manager would be an excellent improvement (which is already planned 😄 ).

* [mod] modified C++11 compiler support include in CMakeLists.txt * [mod] added David as maintainer in README.md * [mod] linked original paper in external doc. part of README.md * [mod] changed default base_frame param to "base_link" * [mod] set authors and maintainers in package.xml * [mod] increased version number in package.xml * [mod] backward compat with original examples (if topic_root is set, old topic names are assumed)

* [mod] added ROS kinetic to .travis.yml

* [mod] added testing section to CMakeLists.txt * [add] added irat_aus.test rostest file * [add] added copy of irat_aus config with graphics disabled for testing

* [mod] added missing dependencies to wget tool in package.xml

* [mod] added missing dependencies to compressed_image_transport in package.xml

* [mod] added args to OpenCV find_package in CMakeLists.txt * [mod] added build and run dependency to opencv3 in package.xml

* [mod] removed version arg from OpenCV find_package in CMakeLists.txt * [mod] added build and run dependency to cv_bridge in package.xml

davidmball · 2017-06-28T18:54:29Z

How is this PR going? Seems like a lot of additional commits. It might be worth trying to finish this off then doing other stuff in later separate smaller PRs?

Rudolph Peter z003n7ht added 9 commits May 12, 2017 20:46

Reformatted code according ROS guidelines. This closes #3.

185e7db

* [mod] reformatted code style using clang-format

Added linking against boost libraries.

ef68617

* [mod] modified CMakeLists.txt to link against Boost

Added naive path follower. This closes #5.

a7909d9

* [add] added python path follow controller node

Added Travis CI configuration.

c752a66

* [add] added travis CI configuration provided by ROS Industrial team

Fixed package.xml.

a7e5ea8

* [fix] fixed wrong dependency to opencv (this fixes rosdep issues) * [mod] added dependency to irrlicht

Further fixes in package.xml.

92f8d1a

* [mod] added missing dependency to OpenGL

Added readme. This closes #6.

4f68852

* [add] added readme in ROS doc style, including travis-ci build status

CMakeLists.txt cleanup.

6ffb356

* [mod] moved boost linking to ratslam library target in CMakeLists.txt * [mod] enabled Cxx11 support in CMakeLists.txt * [mod] added "src" as include directory in CMakeLists.txt

davidmball assigned scott-- and davidmball May 17, 2017

davidmball requested review from scott-- and davidmball May 17, 2017 11:26

davidmball requested changes May 20, 2017

View reviewed changes

scott-- reviewed May 21, 2017

View reviewed changes

Rudolph Peter z003n7ht added 7 commits May 26, 2017 16:37

Added ROS kinetic to travis config.

553fbf6

* [mod] added ROS kinetic to .travis.yml

Added simple rostest to check basic functionality. This closes #4.

10549b4

* [mod] added testing section to CMakeLists.txt * [add] added irat_aus.test rostest file * [add] added copy of irat_aus config with graphics disabled for testing

Updated package.xml.

6268f27

* [mod] added missing dependencies to wget tool in package.xml

Updated package.xml.

867e3f4

* [mod] added missing dependencies to compressed_image_transport in package.xml

Changed build dependency from OpenCV2 to OpenCV3.

eb0b281

* [mod] added args to OpenCV find_package in CMakeLists.txt * [mod] added build and run dependency to opencv3 in package.xml

fixup! Changed build dependency from OpenCV2 to OpenCV3.

edc9f81

* [mod] removed version arg from OpenCV find_package in CMakeLists.txt * [mod] added build and run dependency to cv_bridge in package.xml

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several improvements to be more ROS compatible. #5

Several improvements to be more ROS compatible. #5

sem23 commented May 16, 2017

davidmball commented May 17, 2017

sem23 commented May 17, 2017 •

edited

Loading

sem23 commented May 20, 2017

davidmball left a comment

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 20, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

davidmball May 20, 2017

sem23 May 22, 2017

sem23 May 26, 2017

davidmball May 20, 2017

sem23 May 22, 2017

scott-- May 21, 2017

sem23 May 22, 2017

davidmball commented Jun 28, 2017


		#### 4.3.1 Subscribed Topics

		"image" ([sensor_msgs/Image](http://docs.ros.org/api/sensor_msgs/html/msg/Image.html))

Several improvements to be more ROS compatible. #5

Are you sure you want to change the base?

Several improvements to be more ROS compatible. #5

Conversation

sem23 commented May 16, 2017

davidmball commented May 17, 2017

sem23 commented May 17, 2017 • edited Loading

sem23 commented May 20, 2017

davidmball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmball commented Jun 28, 2017

sem23 commented May 17, 2017 •

edited

Loading