made tf2_py compile on ROS2 by BrannonKing · Pull Request #26 · ros2/geometry2

BrannonKing · 2017-06-21T16:49:08Z

I'm not sure it's being deployed correctly. Can somebody verify that?

There were a lot of pre-existing warnings about uninitialized structures. Those are still there.

I would like to finish converting geometry2 and run the tests. Talk it up if you want me to find time for that.

I removed the metapackages. I assume there will not be any equivalent of them in ROS2.

bringing up to date

following head

made tf2_py compile with ROS2

clalancette · 2017-06-22T18:00:33Z

Thanks for doing this, this will be helpful for getting some of the tf2 debugging tools going. As a meta-comment, I think it would be best not to touch the pieces that don't absolutely require touching. This will make it easier for us to merge in stuff from the base ros/geometry2 repository as needed. In this PR, I would say that we shouldn't bother removing the meta-package stuff. I'll do a review of the rest of the stuff in here.

clalancette

Nothing majorly wrong here, but I would like it to reuse some more existing code, and get parts of this in upstream ros/geometry2.

clalancette · 2017-06-22T18:04:54Z

tf2/include/tf2/buffer_core.h



-};
+}


This is present in the upstream geometry2 code. Would you mind opening a PR there to remove it (and then we'll inherit it the next time we merge)? If you don't have time, let me know and I can do it instead.

I confess I'm not too interested in fixing this for ROS1. My team at ASI isn't using ROS1, and I'm not too familiar with it.

clalancette · 2017-06-22T18:06:40Z

tf2_py/CMakeLists.txt

-# Dynamic linking with tf worked OK, except for exception propagation, which failed in the unit test.
-# so build with the objects directly instead.
+if(NOT WIN32)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14 -Wall -Wextra -Wpedantic")


There's no way you could have known this, but we have changed this idiom. See the top of i.e. https://github.com/ros2/turtlebot2_demo/blob/master/turtlebot2_drivers/CMakeLists.txt for the new idiom.

clalancette · 2017-06-22T18:07:45Z

tf2_py/CMakeLists.txt

+find_package(tf2 REQUIRED)
+find_package(tf2_msgs REQUIRED)
+find_package(PythonLibs 3 REQUIRED)
+include_directories(${PYTHON_INCLUDE_PATH})


Nit, but please alphabetize the find_package() invocations. Also a newline between them and include_directories()

clalancette · 2017-06-22T18:12:45Z

tf2_py/CMakeLists.txt

-## Testing ##
-#############
+add_library(${PROJECT_NAME} src/tf2_py.cpp)
+set_target_properties(${PROJECT_NAME} PROPERTIES LINKER_LANGUAGE CXX)


Why did you need to add the LINKER_LANGUAGE when the upstream doesn't have it? A comment might be helpful, and is it something we should add to upstream?

Fixed. The LINKER_LANGUAGE was required for the unique_id stuff, which I borrowed from.

clalancette · 2017-06-22T18:13:02Z

tf2_py/CMakeLists.txt

-# if(TARGET ${PROJECT_NAME}-test)
-#   target_link_libraries(${PROJECT_NAME}-test ${PROJECT_NAME})
-# endif()
+ament_target_dependencies(${PROJECT_NAME} "rclcpp" "rclpy" "geometry_msgs" "tf2_msgs" "python")


Nit: we prefer one dependency per-line.

clalancette · 2017-06-22T18:14:34Z

tf2_py/package.xml

 <?xml version="1.0"?>
-<package>
+<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+<package format="2">


It would be good to change the upstream package.xml to format 2 (and clean it up while we are at it), and then just change the dependencies as necessary here. Again, if you don't have time to do that, let me know and I can.

clalancette · 2017-06-22T18:15:11Z

tf2_py/src/tf2_py.cpp

-  "_tf2.BufferCore",               /*name*/
-  sizeof(buffer_core_t),           /*basicsize*/
+  "_tf2.BufferCore",         /* name */
+  sizeof(buffer_core_t),     /* basicsize */


This seems like a spurious change.

There's actually still a bug here. That structure isn't initialized completely. I pasted in (and aligned comments to) what I thought was the full initialization from http://python3porting.com/cextensions.html , but it still showed a lot of warnings and seemed incomplete.

clalancette · 2017-06-22T18:17:52Z

tf2_py/src/tf2_py.cpp

  WRAP(source_id = bc->_validateFrameId("get_latest_common_time", source_frame));
-  int r = bc->_getLatestCommonTime(target_id, source_id, time, &error_string);
-  if (r == 0) {
+  auto r = bc->_getLatestCommonTime(target_id, source_id, time, &error_string);


This change to "auto" (and elsewhere in the file) again look like something that could go into upstream tf2_py.

These changes to auto were made because I was too lazy to figure out what the ROS2 equivalent types were (and what header they were defined in).

clalancette · 2017-06-22T18:23:54Z

tf2_py/src/tf2_py.cpp

+  }
+}
+
+static int rostime_converter_blt(PyObject *obj, builtin_interfaces::msg::Time *rt)


We already have an interface to convert from builtin_interfaces::msg::Time -> tf2::TimePoint: https://github.com/ros2/geometry2/blob/ros2/tf2_ros/include/tf2_ros/buffer_interface.h#L58 . I think it would be best to reuse that, as having the conversions scattered around seems to be more fragile to me. That being said, that would introduce a new dependency on tf2_py, namely tf2_ros; I'm not sure if that is a problem or not.

Any other thoughts on this dependency? I think it will be okay for the sake of "always reuse code".

clalancette · 2017-06-22T18:26:06Z

tf2_py/src/tf2_py.cpp

  if (!PyArg_ParseTupleAndKeywords(args, kw, "|O&", (char**)keywords, rostime_converter, &time))
    return NULL;
-  return PyString_FromString(bc->_allFramesAsDot(time.toSec()).c_str());
+  return stringToPython(bc->_allFramesAsDot(time).c_str());


Again, looks like a good change for upstream.

That method PyString_FromString didn't seem to exist for me. I don't know where it was coming from before.

fixed package ordering, time message conversion

allenh1 · 2017-06-25T07:25:07Z

tf2_py/CMakeLists.txt

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


Do we need these flags on for some reason? I'd prefer these only get introduced if we're compiling with -DCMAKE_BUILD_TYPE=Debug.

We use these flags everywhere in the ros2 codebase, we usually try to also use Wpedantic when it doesn't introduce spurious changes or make us diverge too much for upstream. I don't see a good reason for this package to not do so What is the rationale for enforcing these flags only in debug mode?

allenh1 · 2017-06-25T07:26:22Z

Other than that one small comment, this looks good to me (after changes that @clalancette brought up).

mikaelarguedas

I made a few high level remarks on the approach but haven't reviewed the code, see comments below

mikaelarguedas · 2017-06-25T16:06:12Z

tf2_py/package.xml

-  <build_depend>tf2</build_depend>
+  <build_depend>builtin_interfaces</build_depend>
+  <build_depend>geometry_msgs</build_depend>
  <build_depend>rospy</build_depend>


This dependency should be removed because rospy is a ros1 package. All the code relying on rospy (starting with the import here https://github.com/ros2/geometry2/pull/26/files#diff-43abb9e427aa2b1429dbad6c940088e9R576) and calls to its functions should be updated to use ros2 functions

mikaelarguedas · 2017-06-25T16:06:48Z

tf2_py/package.xml

-    <!-- Other tools can request additional information be placed here -->
-
+    <build_type>ament_cmake</build_type>
+    <build_type>ament_python</build_type>


ament packages cannot have several build types, in this case I think you want an ament_cmake build type and install the python modules through cmake

Are there any examples of installing ROS2 python modules through cmake?

Please see the following two CMake functions and their docblocks: https://github.com/ament/ament_cmake/tree/master/ament_cmake_python/cmake

BrannonKing · 2017-06-26T17:30:52Z

Looking at this further today, the code still relies on rospy.Time. I don't see an equivalent object in rclpy. Is there one?

wjwwood · 2017-06-26T20:13:55Z

Looking at this further today, the code still relies on rospy.Time. I don't see an equivalent object in rclpy. Is there one?

Not at the moment, but that's just because we haven't had time to implement it. A full "ROS Time" interface is planned for Beta 3. I think that was mostly for C++, but we'll probably do Python at the same time.

fixed dll deployment

BrannonKing · 2017-06-26T22:28:52Z

I've changed it to deploy the shared library as part of the cmake_process. I moved the init.py file to match the default ament package deployment. I also had to add an install line, which I think should be part of the ament deployment instead. The leading underscore on the shared library -- is that the way we've always done C/C++ implementations of python modules? That seems a little weird to me (but matches what we did in ROS1).

mikaelarguedas · 2017-08-09T18:22:31Z

@tfoote to pull upstream changes. Moving in backlog given that we don't have time in python right now

sloretz · 2019-08-30T23:37:21Z

It looks like These commits are included in #99. I'll close this as replaced by that PR.

BrannonKing added 3 commits April 27, 2017 05:45

Merge pull request #1 from ros2/ros2

e5c2c1f

bringing up to date

Merge pull request #2 from ros2/ros2

94f52c9

following head

2202ada

made tf2_py compile with ROS2

clalancette requested changes Jun 22, 2017

View reviewed changes

clalancette added the in progress Actively being worked on (Kanban column) label Jun 22, 2017

00fbbf8

fixed package ordering, time message conversion

allenh1 reviewed Jun 25, 2017

View reviewed changes

mikaelarguedas reviewed Jun 25, 2017

View reviewed changes

0f3f12a

fixed dll deployment

mikaelarguedas added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 26, 2017

mikaelarguedas removed the ready Work is about to start (Kanban column) label Aug 9, 2017

JacobJBeck mentioned this pull request Dec 12, 2018

tf2_ros is not built for Python #87

Closed

vinnamkim mentioned this pull request Mar 16, 2019

tf2_ros is not built for Python #99

Merged

sloretz closed this Aug 30, 2019

Conversation

BrannonKing commented Jun 21, 2017

Uh oh!

clalancette commented Jun 22, 2017

Uh oh!

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

BrannonKing Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mikaelarguedas Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenh1 commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

BrannonKing commented Jun 26, 2017

Uh oh!

wjwwood commented Jun 26, 2017

Uh oh!

BrannonKing commented Jun 26, 2017

Uh oh!

mikaelarguedas commented Aug 9, 2017

Uh oh!

sloretz commented Aug 30, 2019

Uh oh!

Reviewers

Assignees

BrannonKing Jun 25, 2017 •

edited

Loading

mikaelarguedas Jun 25, 2017 •

edited

Loading

allenh1 commented Jun 25, 2017 •

edited

Loading