New approach for toMsg()/fromMsg()#368

Closed

gleichdick wants to merge 51 commits intoros2:ros2from

gleichdick:ros2_new_toMsg_approach

Contributor

gleichdick commented Jan 25, 2021 •

edited

Loading

I did a complete rework of the conversion utilities, with the goal to avoid link-time errors and to make tf2::convert more bullet-proof. In ROS1, various issues arised, mainly because of the forward declaration of toMsg() as a templated function, followed by the implementations for Eigen, KDL and friends being traditional overloaded functions (without template<>, so no specialization). This is a port of ros/geometry2#491 and addresses #242.

Now, fromMsg() and toMsg() both have a default implementation, by redirecting into impl::ImplDetails structs. SFINAE with structs allow some more flexibility than the current approach, with the bonus of compile-time errors when a specific implementation is missing.
The converison of stamped types geometry_msgs::...Stamped and tf2::Stamped<T> now uses the conversion functions of the underlying unstamped types.

To do:

Documentation
Formatting (which .clang-format to use?)
Some more unit tests

gleichdick force-pushed the ros2_new_toMsg_approach branch from 8b44a4b to 11190f3 Compare

January 26, 2021 15:53

This was referenced Jan 26, 2021

New approach for the tf2::toMsg() hassle ros/geometry2#491

Closed

[Don't Merge] Add geometry2 moveit/moveit2#345

Closed

gleichdick force-pushed the ros2_new_toMsg_approach branch from de72a43 to 32a6d7f Compare

January 29, 2021 13:26

gleichdick marked this pull request as ready for review

January 29, 2021 16:27

gleichdick changed the title ~~[WIP] New approach for toMsg()/fromMsg()~~ New approach for toMsg()/fromMsg()

gleichdick mentioned this pull request

Fix doTransform with Eigen Quaternion #369

Merged

gleichdick force-pushed the ros2_new_toMsg_approach branch from 6cc44b9 to 5b74e32 Compare

February 12, 2021 00:27

Contributor Author

gleichdick commented Feb 12, 2021

Just rebased onto ROS2, I hope I didn't drop anything.
Friedly ping to get some comments, as the deadlines for Galactic come closer @clalancette @tfoote.

Contributor Author

gleichdick commented Mar 4, 2021

Friendly ping

clalancette assigned ahcorde

ahcorde requested changes

View reviewed changes

Contributor

ahcorde left a comment •

edited

Loading

I'm going to run the CI while I'm reviewing this PR (it's quite big)

Linux
Linux-aarch64
macOS
Windows

There is a conflict in one file, do you mind to fix it ?

test_tf2/test/test_convert.cpp Outdated Show resolved Hide resolved

test_tf2/test/test_convert.cpp Outdated Show resolved Hide resolved

ahcorde reviewed

View reviewed changes

Contributor

ahcorde left a comment

CI is failing https://ci.ros2.org/job/ci_linux-aarch64/8809/console

Maybe you need to add here sensor_msgs

gleichdick added 17 commits

March 19, 2021 10:09


          Add test for Vector/Point toMsg()


          Complete rework of toMsg()/fromMsg()

c095835


          Reorganize some headers

786fdd3


          Cleanup of Time/Duration conversion

01e8c02


          Fix toMsg forward

3904bd0


          Reenable Eigen tests

2499ae9


          Reenable sensor_msgs cpp test

592056a


          Reenable conversion test

402f4bd


          Add testcases from tf2_eigen_kdl to tf2_test

71513ec


          Add Wrench conversion

4dc5453


          Enable Transform and Quaternion for bullet

1f69cb0


          fix bullet uncrustify

8288bc4


          bullet uncrustify format

185433a


          Fix bullet test and cmake

938d908


          make Vector and Quaternion conversion reuseable

9f39eb7


          Cleanup of Transform conversions

d4d1704


          Unify doTransform() (except for tf2_geometry_msgs)

97cd92a

Contributor

ahcorde commented Apr 6, 2021

Sorry I forgot this PR. I would prefer to add the static_casts instead of disabling the warning


          Add static_cast to tf2_bullet

3278ca2

bullet uses floats, which causes narrowing warnings

gleichdick force-pushed the ros2_new_toMsg_approach branch from 3bbc20d to 3278ca2 Compare

April 6, 2021 17:28

Contributor Author

gleichdick commented Apr 6, 2021

Okay, I agree 😀

ahcorde requested changes

View reviewed changes

Contributor

ahcorde left a comment

some nits and comments

tf2/include/tf2/impl/convert.h

+                  alwaysFalse<Datatype>,
+                  "No default message type defined, "
+                  "please check your header file includes!");
+                // using type = ...;

Contributor

ahcorde Apr 8, 2021

remove ?

tf2/include/tf2/impl/convert.h

+                  alwaysFalse<Datatype>,
+                  "No default transform type defined, "
+                  "please check your header file includes!");
+                // using type = ...;

Contributor

ahcorde Apr 8, 2021

remove ?

tf2/include/tf2/impl/convert.h

Comment on lines +149 to +150

		// void toMsg(const Datatype&, Message&);
		// void fromMsg(const Message&, Datatype&);

Contributor

ahcorde Apr 8, 2021

remove ?

tf2/include/tf2/impl/convert.h

Comment on lines +193 to +197

+                // using UntampedType = ...;
+                // static UntampedType& accessMessage(StampedMsg &);
+                // static UntampedType getMessage(StampedMsg const&);
+                // static CovarianceType & accessCovariance(MsgWithCovarianceStamped &);
+                // static CovarianceType getCovariance(MsgWithCovarianceStamped const&);

Contributor

ahcorde Apr 8, 2021

remove ?

tf2/include/tf2/impl/convert.h

Comment on lines +229 to +230

		// using StampedType = ...;
		// using StampedTypeWithCovariance = ...;

Contributor

ahcorde Apr 8, 2021

remove ?
if you don't want to remove these comments at least add a comment explaining the meaning. (some for all)

tf2/include/tf2/impl/convert.h Show resolved Hide resolved

tf2/include/tf2/impl/convert.h Show resolved Hide resolved

tf2_geometry_msgs/CMakeLists.txt Outdated

                 find_package(ament_cmake_gtest REQUIRED)
                 find_package(rclcpp REQUIRED)
+                find_package(ament_cmake_cppcheck REQUIRED)

Contributor

ahcorde Apr 8, 2021

why don't you add common ? instead of run only 4 tests?

Contributor Author

gleichdick Apr 8, 2021

cppcheck needs to be called manually (with the LANGUAGE "c++" option), because of the header file suffix being .h. Otherwise it would only recognize them as plain C headers and complain about non-C code.

Contributor

ahcorde Apr 12, 2021

You should be able to configure this before.

CMakeLists.txt example

Some ament links:

tf2_sensor_msgs/CMakeLists.txt

+                find_package(ament_cmake_gtest REQUIRED)
+                find_package(ament_cmake_pytest REQUIRED)
+                #ament_add_pytest_test(test_tf2_sensor_msgs_py test/test_tf2_sensor_msgs.py)

Contributor

ahcorde Apr 8, 2021

add a TODO or remove

tf2_kdl/CMakeLists.txt Outdated

                 find_package(rclcpp REQUIRED)
                 find_package(tf2_msgs REQUIRED)
+                find_package(ament_cmake_cppcheck REQUIRED)

Contributor

ahcorde Apr 8, 2021

some here ? why only these 4 tests?

gleichdick and others added 2 commits

April 8, 2021 20:17


          Fix formatting nitpicks

7795ded

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>


          Add TODOs and comments

d3fb3bd

ahcorde requested changes

View reviewed changes

tf2_geometry_msgs/CMakeLists.txt Outdated

                 find_package(ament_cmake_gtest REQUIRED)
                 find_package(rclcpp REQUIRED)
+                find_package(ament_cmake_cppcheck REQUIRED)

Contributor

ahcorde Apr 12, 2021

You should be able to configure this before.

CMakeLists.txt example

Some ament links:

gleichdick added 3 commits

April 12, 2021 16:20


          Fix ament_lint_auto settings for cppcheck and uc

c565ee6


          [tf2_geometry_msgs] pep8 formatting


          [tf2_kdl] pep8 formatting

0dbe030

gleichdick force-pushed the ros2_new_toMsg_approach branch from 9646a40 to 0dbe030 Compare

April 12, 2021 17:02

gleichdick added 3 commits

April 12, 2021 17:26


          [tf2_kdl] fix pep257

59c66bd


          [tf2_geometry_msgs] fix copyright

ef72679


          [tf2_kdl] fix copyright

3e5fa25

Contributor Author

gleichdick commented Apr 12, 2021

Thanks for the hint for ament_lint_auto! I hope I did everything right while defeating ament_copyright...

Contributor Author

gleichdick commented Apr 21, 2021

Friendly ping for another CI shot 😁

Contributor

ahcorde commented May 12, 2021

Linux
Linux-aarch64
macOS
Windows


          move headers to .cpp

cc6d47a

Contributor Author

gleichdick commented May 13, 2021

Due to the latest changes on the ros2 branch, a lot of merge conflicts arised. As I don't see a lot of activity in this PR, I came to the conclusion that this PR won't be merged and that I can stop working on it. I guess someone else has to fix the underlying issues...

Contributor

clalancette commented May 25, 2021

Due to the latest changes on the ros2 branch, a lot of merge conflicts arised. As I don't see a lot of activity in this PR, I came to the conclusion that this PR won't be merged and that I can stop working on it. I guess someone else has to fix the underlying issues...

I was the cause of the conflicts, so apologies for that. I can go ahead and fix them, if you'd like.

However, it would be a lot easier if this PR was cleaned up a bit. In particular, it would be much better if we split parts of this out into smaller PRs (particularly things like the tests), and also consolidated the 51 commits into a more manageable number. @gleichdick are you willing to do that?

This was referenced May 26, 2021

Reenable sensor_msgs test #422

Merged

Conversion tests for toMsg() #423

Merged

Reenable Eigen tests #424

Closed

KDL python formatting and licenses #425

Merged

Geometry nitpicks #426

Merged

New approach for toMsg()/fromMsg() #427

Open

Contributor Author

gleichdick commented May 26, 2021

Closed in favor of #427

gleichdick closed this

taikitanaka3 mentioned this pull request

auto/fix_inline_expansion tier4/AutowareArchitectureProposal.iv#710

Merged

9 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet