ROS2 port by mlautman · Pull Request #48 · moveit/srdfdom

mlautman · 2019-03-13T00:47:13Z

This ports srdfdom to ROS2. Both python and C++ tests pass

mlautman · 2019-03-13T00:48:12Z

This was based on #45 by @vmayoral

mlautman · 2019-03-14T17:39:48Z

rhaschke

I cannot judge this PR. I'm not an expert for ROS 2.
I suggest to split reformatting from actual changes. Right now, it's hard to review this PR at all.
Travis is not yet succeeding.

rhaschke · 2019-03-14T18:49:46Z

+    self.assertTrue( xml_matches(robot.to_xml_string(False),expected))
+
+
+  # def test_simple_srdf(self):


Why disabling this test?

Thanks for catching this. I restored it

rhaschke · 2019-03-14T18:51:32Z

+    self.assertTrue(len(robot.end_effectors)==0)
+
  def test_complex_srdf(self):
-        datadir=rospkg.RosPack().get_path('srdfdom')+"/test/resources/"


This file contains a lot of unnecessary reformatting.

I believe the reformatting is necessary as it is bad style to change indentation mid file. I have refactored my changes so that the whitespace changes are in a separate commit

@rhaschke is right, this should be in a separate PR

mlautman · 2019-03-14T21:31:02Z

@henningkayser Please review

mlautman · 2019-03-14T21:49:03Z

@rhaschke

I suggest to split reformatting from actual changes. Right now, it's hard to review this PR at all.

I have split up the PR into separate commits for formatting and porting

Travis is not yet succeeding.

This is because moveit_ci is still broken:
The command "docker pull moveit/moveit:crystal-ci" failed with error code 1.

Porting srdfdom is one step in getting moveit_ci ported to ROS2.

rhaschke · 2019-03-14T21:50:44Z

moveit_ci is still broken

I think, the only thing you need is to provide the corresponding docker container. Didn't you work on that already?

Porting srdfdom is one step in getting moveit_ci ported to ROS2.

srdfdom is completely unrelated to moveit_ci.

mlautman · 2019-03-14T22:03:13Z

moveit_ci is still broken

I think, the only thing you need is to provide the corresponding docker container. Didn't you work on that already?

It is a WIP. We realized that quite a few of the dependencies (eg srdfdom) weren't building correctly so we are porting them now.

Porting srdfdom is one step in getting moveit_ci ported to ROS2.

srdfdom is completely unrelated to moveit_ci.

Not exactly. The travis.yml for srdfdom clones moveit_ci and runs travis.sh from that repo. Porting everything is going to be kind of a chicken and the egg problem so we choose to start with the dependencies

mlautman · 2019-03-14T23:03:47Z

@vmayoral Can you give this a quick review?

rhaschke · 2019-03-15T08:32:05Z

Porting everything is going to be kind of a chicken and the egg problem.

There is a clear dependency chain, isn't it?

moveit_ci (which is independent of any particular repo)
geometry2, shapes, srdf, urdf, etc.
MoveIt

mlautman · 2019-03-15T19:55:07Z

moveit_ci pulls in https://raw.githubusercontent.com/ros-planning/moveit/$ROS_DISTRO-devel/moveit.rosinstall

It also tries do download the docker image for MoveIt which isn't for crystal yet. If we get the base moveit docker image building and update the rosinstall (without CI passing) then the chain will be broken and we'll be all clear to port moveit_ci

rhaschke · 2019-03-16T10:12:55Z

What is pulled in by moveit_ci is defined in .travis.yml. If you want to break the circle start there.
Another dependency is indeed the docker image. For now, I suggest to remove/disable the wstool call and installing of MoveIt dependencies there as well. This was introduced in the Dockerfile to speed up Travis.
travis.sh itself updates those dependencies as well.

henningkayser

Please fix clang. Also rosdep still fails because of urdfdom_py, but I guess this will only be fixed by ros/urdf_parser_py#41

henningkayser · 2019-03-26T15:36:13Z

  srdf::Model s;
-  urdf::ModelInterfaceSharedPtr u = loadURDF(std::string(TEST_RESOURCE_LOCATION) + "/pr2_desc.urdf");
+
+  std::string package_name = "srdfdom";


const or constexpr char[] ?

seems like over-optimization ;-)

I'm with Dave here

I'm with Henning. Adding a const doesn't harm and clearly states that this variable will not be modified.

davetcoleman · 2019-03-26T20:50:48Z

@@ -1,3 +0,0 @@
-<launch>


I understand that ros2 uses a different test approach, but is the replacement in this PR? I don't see it. If not, I think we should keep this with an in-line TODO (and maybe matching Issue).

ros2 tests the .cpp without needing .test files

davetcoleman · 2019-03-26T20:51:18Z

+    self.assertTrue(len(robot.end_effectors)==0)
+
  def test_complex_srdf(self):
-        datadir=rospkg.RosPack().get_path('srdfdom')+"/test/resources/"


@rhaschke is right, this should be in a separate PR

davetcoleman · 2019-03-26T20:51:49Z

+
+TEST(TestCpp, testSimpleUTF)
+{
+  testSimple("nl_NL.UTF-8");


davetcoleman · 2019-03-26T20:52:12Z

  srdf::Model s;
-  urdf::ModelInterfaceSharedPtr u = loadURDF(std::string(TEST_RESOURCE_LOCATION) + "/pr2_desc.urdf");
+
+  std::string package_name = "srdfdom";


seems like over-optimization ;-)

rhaschke

I'm curious, how the new ROS2 approach to integration tests looks like.
The approach you took here, explicitly coding the two environment options in the cpp files, works here, but for more complex scenarios that's not a feasible approach anymore. Sometimes, we need to launch several nodes to perform an integration test...

rhaschke · 2019-03-26T21:01:01Z

+endif()

+if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+  add_compile_options(-Wall -Wextra -Wpedantic)


These shouldn't be part of a CMakeLists file, but rather defined by the user.

rhaschke · 2019-03-26T21:04:05Z

 find_package(console_bridge REQUIRED)
 find_package(urdfdom_headers REQUIRED)
+find_package(urdf REQUIRED)
+find_package(tinyxml_vendor REQUIRED)


What are these *_vendor packages?
You add many new find_packages, but don't use them in include_directories or target_link_libraries.
I don't think they are really needed. Please clean up if possible or better explain.

rhaschke · 2019-03-29T07:58:45Z


 before_script:
-  - git clone -q --depth=1 https://github.com/ros-planning/moveit_ci.git .moveit_ci
+  - git clone -q -b ros2 --depth=1 -b ros2 https://github.com/ros-planning/moveit_ci.git .moveit_ci


One --branch ros2 is sufficient 😉

rhaschke · 2019-03-29T08:00:07Z


  <build_depend>boost</build_depend>
-  <build_depend>cmake_modules</build_depend>
+  <build_depend>python_cmake_module</build_depend>


cmake_modules wasn't related to python in ROS1. Do you really need this package? What does it do?

rhaschke · 2019-03-29T08:01:18Z

  srdf::Model s;
-  urdf::ModelInterfaceSharedPtr u = loadURDF(std::string(TEST_RESOURCE_LOCATION) + "/pr2_desc.urdf");
+
+  std::string package_name = "srdfdom";


I'm with Henning. Adding a const doesn't harm and clearly states that this variable will not be modified.

mlautman mentioned this pull request Mar 13, 2019

Update CI to ROS2 moveit/moveit2#37

Closed

rhaschke reviewed Mar 14, 2019

View reviewed changes

Michael Lautman added 2 commits March 14, 2019 15:03

trim whitespace

27cd1d9

fixing whitespace in python file

9cf1b24

mlautman force-pushed the ros2 branch from 693f265 to 6079c12 Compare March 14, 2019 21:26

mlautman closed this Mar 14, 2019

mlautman changed the base branch from ros2 to crystal-devel March 14, 2019 21:30

mlautman reopened this Mar 14, 2019

mlautman mentioned this pull request Mar 14, 2019

Port to ROS 2 #45

Closed

mlautman force-pushed the ros2 branch from 6079c12 to 1895c09 Compare March 14, 2019 21:52

Michael Lautman added 6 commits March 25, 2019 13:51

adding python generated files to gitignore

d2a2a12

updating cmake and package.xml for ros2

3daf790

porting test.py to ros2

8549826

removing unused .test files

47ef93e

porting cpp tests to ros2

f32c5e9

updating travis.yml to crystal

efc6f39

mlautman force-pushed the ros2 branch from 460cc95 to efc6f39 Compare March 25, 2019 19:51

adding braces to satisfy -Wdangling-else

e8d055c

mlautman requested a review from henningkayser March 25, 2019 21:07

henningkayser requested changes Mar 26, 2019

View reviewed changes

davetcoleman reviewed Mar 26, 2019

View reviewed changes

rhaschke reviewed Mar 26, 2019

View reviewed changes

Mike Lautman added 2 commits March 28, 2019 15:22

clang

dde1989

pointing moveit_ci to the correct branch

eb9adfa

rhaschke requested changes Mar 29, 2019

View reviewed changes

rhaschke closed this Nov 17, 2019

		self.assertTrue( xml_matches(robot.to_xml_string(False),expected))


		# def test_simple_srdf(self):

Conversation

mlautman commented Mar 13, 2019

Uh oh!

mlautman commented Mar 13, 2019

Uh oh!

mlautman commented Mar 14, 2019

Uh oh!

rhaschke 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!

mlautman commented Mar 14, 2019

Uh oh!

mlautman commented Mar 14, 2019

Uh oh!

rhaschke commented Mar 14, 2019

Uh oh!

mlautman commented Mar 14, 2019

Uh oh!

mlautman commented Mar 14, 2019

Uh oh!

rhaschke commented Mar 15, 2019

Uh oh!

mlautman commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhaschke commented Mar 16, 2019

Uh oh!

henningkayser 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!

Uh oh!

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!

rhaschke 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mlautman commented Mar 15, 2019 •

edited

Loading