ROS2 tf2-eigen Reloaded #4

clalancette · 2017-03-24T19:19:44Z

This is a pull request to make tf2-eigen work under ROS2. I've tested these changes in CI on Linux, Windows, and OSX; you can see the results below:

Linux: http://ci.ros2.org/job/ci_linux/2362/
Windows: http://ci.ros2.org/job/ci_windows/2471/
OSX: http://ci.ros2.org/job/ci_osx/1816/

If you look closely, you'll see that they are all yellow; that's because there was an unused variable in tf2_eigen-test.cpp, which I've since removed. It was trivial enough that I didn't feel like waiting for the build farm again for it. Inline I have more comments on things people pointed out in the previous PR: #1

connects to ros2/ros2#342

clalancette · 2017-03-24T19:20:06Z

tf2/include/tf2/convert.h

 template <class A, class B>
  void convert(const A& a, B& b)
  {
    //printf("In double type convert\n");
    impl::Converter<ros::message_traits::IsMessage<A>::value, ros::message_traits::IsMessage<B>::value>::convert(a, b);
  }
+*/



As pointed out by Mikael, I've changed this to just be a block comment.

clalancette · 2017-03-24T19:20:37Z

tf2/include/tf2/transform_datatypes.h

@@ -43,7 +44,6 @@ namespace tf2
 template <typename T>
 class Stamped : public T{
 public:
-  typedef std::chrono::system_clock::time_point TimePoint;


This is new. I determined that this was already being defined elsewhere (namely time.h), so we can just use that version instead.

clalancette · 2017-03-24T19:21:07Z

tf2_eigen/CMakeLists.txt

+      ${tf2_ros_INCLUDE_DIRS}
+      ${Eigen_INCLUDE_DIR}
+    )
+  endif()


As pointed out by Mikael in the previous review, I reenabled the test here.

clalancette · 2017-03-24T19:21:32Z

tf2_eigen/CMakeLists.txt

 endif()
+
+ament_export_include_directories(include)
+ament_export_dependencies(tf2_ros)


Because tf2_eigen now depends on tf2_ros, we have to export the dependencies so downstream users get the right include headers. This is done here. This is new.

clalancette · 2017-03-24T19:22:57Z

tf2_eigen/include/tf2_eigen/tf2_eigen.h

@@ -30,15 +30,16 @@
 #define TF2_EIGEN_H


As a meta-comment in this file, I changed this to use @dhood's toMsg()/fromMsg() timestamp conversion, which cleans up the file nicely. I also removed any changes dealing with whitespace or style; this is to take care of @tfoote's concern w.r.t. cherry-picking from the ROS1 geometry2.

clalancette · 2017-03-24T19:23:44Z

tf2_eigen/package.xml

@@ -1,20 +1,26 @@
 <?xml version="1.0"?>


I'll open a PR against the ROS1 geometry2 to change it to format="2". Once that is in, we can merge it up with this one.

Opened up as: ros/geometry2#216

Tully merged that into the ros/geometry2 repository, so we can merge up with that to get those fixes in here. I'll do that once the review here is done.

I think that we should stay in sync with upstream regardless of the status of the opened PRs. Then it's up to the PR opener to rebase their branches accordingly if they happen to conflict.

clalancette · 2017-03-24T19:24:07Z

tf2_eigen/test/tf2_eigen-test.cpp

 #include <gtest/gtest.h>
 #include <tf2/convert.h>

-static const double EPS = 1e-3;
-
-
 TEST(TfEigen, ConvertVector)


Both of these fixes can go upstream to ROS1 tf2_eigen. I'll open a PR for these.

Actually, it turns out that only the EPS removal can go upstream; the PR is here: ros/geometry2#215 . The ros.h header is needed upstream to get access to the ros::Time symbol, which is still used to construct Stamped objects in ROS1.

clalancette · 2017-03-24T19:25:38Z

tf2_ros/CMakeLists.txt

@@ -171,5 +171,6 @@ add_rostest(test/transform_listener_time_reset_test.launch)
 endif()
 ament_export_include_directories(include)
 ament_export_libraries(${PROJECT_NAME})
+ament_export_dependencies(rclcpp)


tf2_ros depends on rclcpp, so we have to make sure to export the dependencies for downstream users.

clalancette · 2017-03-24T19:26:35Z

tf2_eigen/CMakeLists.txt

+  include(${tf2_eigen_SOURCE_DIR}/config/FindEigen3.cmake)
+endif()
+set(Eigen_INCLUDE_DIR "${EIGEN3_INCLUDE_DIR}")
+message("Using Eigen3 include dirs: ${Eigen_INCLUDE_DIR}")


This code is a copy of the code in orocos_kinematics_dynamics that Karsten pointed to, and seems to make finding Eigen on Windows work. Thanks Karsten.

Related to my other comment, is it just on Windows that finding Eigen does not work? Maybe the install of Eigen just needs to be added to the CMAKE_PREFIX_PATH.

I'm hesitant to include a module to find Eigen in every package that needs it, but if that's really what's required to move this forward, then we'll have to do it for now.

wjwwood · 2017-04-04T00:09:51Z

This currently needs to have conflicts resolved.

Should it be reviewed? I saw lots of email traffic surrounding tf2 in ROS2, but I wasn't following closely, so I'm uncertain about the state of this.

clalancette · 2017-04-04T13:05:53Z

Yeah, this should be reviewed. I expect the conflicts to be easy to resolve; I'll do it today.

clalancette · 2017-04-04T13:38:39Z

(I guess using the "Resolve" button in the Github UI does a merge, and not a rebase. I'll make sure to rebase these commits before pushing)

clalancette · 2017-04-10T14:52:59Z

@ros2/team This is still awaiting review so I can merge.

wjwwood

Other than concerns about how we're finding Eigen, lgtm.

wjwwood · 2017-04-10T18:24:50Z

config/FindEigen3.cmake

+
+  mark_as_advanced(EIGEN3_INCLUDE_DIR)
+
+endif(EIGEN3_INCLUDE_DIR)


Where does this file come from? I thought Eigen comes with a CMake config file in more modern releases (like on Xenial). We have a module to find Eigen in our ROS 1 repositories which points this out:

https://github.com/ros/cmake_modules/blob/0.4-devel/cmake/Modules/FindEigen.cmake#L1-L4

Yeah, on Linuxen (and I'm assuming OSX), we don't need this. This is strictly for Windows support.

I also don't love the idea of including this module in every package. Alternatives here would be to use CMAKE_PREFIX_PATH (I don't know if that will make it work on Windows or not), to include this module one time (in, say, ament), or something else? @nuclearsandwich , any thoughts here on what the correct way to find Eigen on Windows is?

are you sure that Eigen is not getting found on the windows machines after the nuget package? This should normally be found.
see here: https://github.com/ros2/robot_state_publisher/blob/ros2/CMakeLists.txt#L13

If we need it, then I propose to open another PR where we revive the cmake_modules package and store the Find*.cmake confiles in a common package.

@Karsten1987 You were right, we didn't need that anymore; Windows on CI seems to do the right thing now. So I've removed FindEigen3.cmake.

That being said, I definitely messed something else up when doing some of my cleanup, so I'm going to run one more set of complete CI jobs on this. I'll post again once those are complete and happy.

wjwwood · 2017-04-10T18:29:43Z

tf2_eigen/CMakeLists.txt

+  include(${tf2_eigen_SOURCE_DIR}/config/FindEigen3.cmake)
+endif()
+set(Eigen_INCLUDE_DIR "${EIGEN3_INCLUDE_DIR}")
+message("Using Eigen3 include dirs: ${Eigen_INCLUDE_DIR}")


Related to my other comment, is it just on Windows that finding Eigen does not work? Maybe the install of Eigen just needs to be added to the CMAKE_PREFIX_PATH.

I'm hesitant to include a module to find Eigen in every package that needs it, but if that's really what's required to move this forward, then we'll have to do it for now.

mikaelarguedas · 2017-04-10T18:40:46Z

Note @clalancette : please rebase this branch and resolve conflicts before "Squash and Merge"ing it. The farm can currently not test this branch because of the conflicts.

clalancette · 2017-04-10T19:42:19Z

I've rebased this, and it should be a fast-forward now.

clalancette · 2017-04-11T16:31:26Z

CI jobs with the latest code:

http://ci.ros2.org/job/ci_linux/2426/
http://ci.ros2.org/job/ci_linux-aarch64/57/
http://ci.ros2.org/job/ci_osx/1879/
http://ci.ros2.org/job/ci_windows/2554/

All green (or yellow, but that isn't this PRs fault). So I think this is ready to merge; I know @wjwwood already approved, but since I made changes I'll wait for another approval before merging.

mikaelarguedas · 2017-04-11T16:41:08Z

I think the new warnings on Windows are caused by this PR: http://ci.ros2.org/job/ci_windows/2554/warnings41Result/new/ and should be adressed before considering merging

mikaelarguedas · 2017-04-11T16:43:38Z

Tip: please use badges as described in the Onboarding document so that it's easy to see which jobs are green without having to click on all of them. Example below:

Linux
Linux aarch64
MacOS
Windows

clalancette · 2017-04-11T17:20:58Z

Oh, good call, I missed that somehow. I'll check into those.

Karsten1987 · 2017-04-11T18:36:05Z

tf2_ros/include/tf2_ros/buffer_interface.h

@@ -55,11 +55,10 @@ namespace tf2_ros
    return time_msg;
  }

-  inline const tf2::TimePoint& fromMsg(const builtin_interfaces::msg::Time & time_msg)
+  inline const tf2::TimePoint fromMsg(const builtin_interfaces::msg::Time & time_msg)


does this function still have to be const return?

Hm, good point. I'm guessing we actually don't want it to be const return, since it is a translation, and users may want to manipulate the returned object later. Does that make sense? If so, I'll remove the const.

It depends on what you want, but right now in line 61 you return a non-const object, so the const-return has no effect.

Oh, I see. All right, well, I'll definitely remove the const then :). Thanks.

dhood · 2017-04-11T19:26:54Z

tf2_ros/include/tf2_ros/buffer_interface.h

@@ -55,11 +55,10 @@ namespace tf2_ros
    return time_msg;
  }

-  inline const tf2::TimePoint& fromMsg(const builtin_interfaces::msg::Time & time_msg)


For some context here, when working on #2, this signature was chosen for compatibility with getTimestamp

geometry2/tf2/include/tf2/convert.h

Line 62 in a0896de

const tf2::TimePoint& getTimestamp(const T& t);

which @tfoote told me returns const ref to allow for chaining.

In particular, changing this signature will break this line: https://github.com/ros2/geometry2/pull/7/files#diff-bb6bd2d938a7c0ed4397920b3d645fd3R65
There are no tests for tf2_geometry_msgs at the moment and these template instantiations often won't give errors unless you're actually using the code. adding the tests is the next thing on my list, I just haven't gotten to it yet.

It may be that the std::move call can be moved into https://github.com/ros2/geometry2/pull/7/files#diff-bb6bd2d938a7c0ed4397920b3d645fd3R65 to adapt for this change (I am not 100% familiar with the intricacies of this).

The std::move was added to fix that compiler warning, but I admit that it wasn't fully tested on Windows. It might be that the signature can be left as it was with some tweaks for it to work on windows. It could also be that it is the way fromMsg is being used in this PR that is causing the errors.

@dhood, thanks for the context. I'm not entirely certain why VC++ is unhappy with that code; I'll take another look and see if I can get a better understanding of what's going on with it.

All right, so here is my understanding of the situation (based on [1], and what I think I understand)

A signature like: inline const tf2::TimePoint& fromMsg(...) is returning a reference to an object. This is only OK if the object is going to stick around after return from the function, i.e. this is a reference to a member value, or something like that. If the object is not going to stick around, then this can lead to a crash, or (worse) later stack corruption. We get away with it in https://github.com/ros2/geometry2/pull/7/files#diff-bb6bd2d938a7c0ed4397920b3d645fd3R70 because it is returning a reference to a passed-in object, which has a different lifetime. However, that does not hold true for fromMsg(TimePoint) here, because we are actually creating a new object. Given that, I think we must change this to return an object, and not a reference. @dhood, that also implies we'll have to fix that line of code, and API, that you referenced. Can someone else comment about whether I'm making sense here?

[1] http://stackoverflow.com/questions/752658/is-the-practice-of-returning-a-c-reference-variable-evil

@clalancette 👍

The function you mentioned with "we get away with it" also needs to be changed:

const std::string& getFrameId(const geometry_msgs::Vector3Stamped& t) {return t.header.frame_id;}

It is trivial to use this function in a way which leads to accessing invalid memory:

const std::string & bad() { geometry_msgs::Vector3Stamped t; return getFrameId(t); } const std::string & s = bad();

Trying to use s: 💥

Just for reference: https://github.com/ros2/geometry2/pull/7/files#r111020855

So after discussing this at length, it looks like the API change to fromMsg() is what we have to do to be correct. We'll also have to do further API changes to fix up additional problems that @dirk-thomas pointed out, which I'll open up a separate issue for.

clalancette · 2017-04-11T22:01:52Z

windows:
linux:
osx:

clalancette · 2017-04-12T13:09:02Z

So all of those builds for this PR are unstable, but it also looks like all of the nightlies went unstable too :(.

clalancette · 2017-04-13T18:04:22Z

windows:
linux:
osx:

clalancette · 2017-04-13T19:08:16Z

So it looks like the builds for this are passing (the one unstable for OSX is the known problem from FastRTPS). Unless I hear any objections in the next little while, I'm going to merge this up.

mikaelarguedas · 2017-04-13T19:10:55Z

cf #8 (comment)

clalancette · 2017-04-13T19:17:25Z

Oh, right, I didn't realize there were actually differences at this time. I'll rebase this as well.

clalancette · 2017-04-24T14:35:08Z

This one is waiting on the merge of indigo-devel into ros2, #13 . Once that is in, I'll merge this one up as well.

Signed-off-by: Chris Lalancette <[email protected]>

This should work properly in Windows without it. Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

This gets rid of a bool cast that VC++ was complaining about. Signed-off-by: Chris Lalancette <[email protected]>

Windows VC++ was complaining that we were returning a pointer to a temporary. To avoid this, don't return a pointer, and just construct and return an object, which C++11 should be able to elide the copy for (see http://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2017-04-24T21:55:00Z

One more CI job with the latest changes rebased:

windows:
linux:
osx:

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2017-04-24T23:22:38Z

All right, CI passed. I'm going to merge this up as well.

Required when building from source. Signed-off-by: Scott K Logan <[email protected]>

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

clalancette commented Mar 24, 2017

View reviewed changes

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

wjwwood assigned clalancette Mar 31, 2017

wjwwood approved these changes Apr 10, 2017

View reviewed changes

clalancette force-pushed the ros2-tf2-eigen-rebased branch from 7457ca5 to f1aa166 Compare April 10, 2017 19:41

Karsten1987 reviewed Apr 11, 2017

View reviewed changes

dhood reviewed Apr 11, 2017

View reviewed changes

clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 24, 2017

clalancette added 11 commits April 24, 2017 17:20

Make tf2_eigen work for ROS2.

d7e11a0

Signed-off-by: Chris Lalancette <[email protected]>

Use the definition of TimePoint from tf2/time.h.

265bbfa

Signed-off-by: Chris Lalancette <[email protected]>

Make sure tf2_ros build_export_depend rclcpp.

32ce8c4

Signed-off-by: Chris Lalancette <[email protected]>

And make sure to make ament export the dependencies.

5db956a

Signed-off-by: Chris Lalancette <[email protected]>

Remove our hand-coded FindEigen3.cmake.

0ba007e

This should work properly in Windows without it. Signed-off-by: Chris Lalancette <[email protected]>

Uncomment convert.

617aa00

Signed-off-by: Chris Lalancette <[email protected]>

Just return RUN_ALL_TESTS.

71a5b72

This gets rid of a bool cast that VC++ was complaining about. Signed-off-by: Chris Lalancette <[email protected]>

Fix silly typos.

9f3be35

Signed-off-by: Chris Lalancette <[email protected]>

More fixes.

9913962

Signed-off-by: Chris Lalancette <[email protected]>

Comment out tests that can't work right now.

709543a

Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the ros2-tf2-eigen-rebased branch from 1affb51 to 709543a Compare April 24, 2017 21:52

Define _USE_MATH_DEFINES so Windows gets M_PI symbol.

dc7b000

Signed-off-by: Chris Lalancette <[email protected]>

clalancette merged commit 735f7a4 into ros2 Apr 24, 2017

clalancette deleted the ros2-tf2-eigen-rebased branch April 24, 2017 23:23

clalancette removed the in progress Actively being worked on (Kanban column) label Apr 24, 2017

BriceRenaudeau mentioned this pull request Sep 28, 2021

TF2 Buffer crash due to timer not destructed #460

Closed

srmainwaring pushed a commit to srmainwaring/geometry2 that referenced this pull request Sep 18, 2023

Add buildtool_depend on git (ros2#4)

ebba460

Required when building from source. Signed-off-by: Scott K Logan <[email protected]>


		mark_as_advanced(EIGEN3_INCLUDE_DIR)

		endif(EIGEN3_INCLUDE_DIR)

ROS2 tf2-eigen Reloaded #4

ROS2 tf2-eigen Reloaded #4

Conversation

clalancette commented Mar 24, 2017 • edited Loading

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

clalancette Mar 24, 2017 • edited by wjwwood Loading

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

wjwwood commented Apr 4, 2017

clalancette commented Apr 4, 2017

clalancette commented Apr 4, 2017

clalancette commented Apr 10, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Apr 10, 2017

clalancette commented Apr 10, 2017

clalancette commented Apr 11, 2017

mikaelarguedas commented Apr 11, 2017

mikaelarguedas commented Apr 11, 2017

clalancette commented Apr 11, 2017

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

clalancette commented Apr 11, 2017

clalancette commented Apr 12, 2017

clalancette commented Apr 13, 2017

clalancette commented Apr 13, 2017

mikaelarguedas commented Apr 13, 2017

clalancette commented Apr 13, 2017

clalancette commented Apr 24, 2017

clalancette commented Apr 24, 2017 • edited Loading

clalancette commented Apr 24, 2017

clalancette commented Mar 24, 2017 •

edited

Loading

clalancette Mar 24, 2017 •

edited by wjwwood

Loading

Karsten1987 Apr 10, 2017 •

edited

Loading

clalancette commented Apr 24, 2017 •

edited

Loading