Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a version of tf2 for ros2. #135

Merged
merged 12 commits into from
Dec 18, 2015
Merged

Create a version of tf2 for ros2. #135

merged 12 commits into from
Dec 18, 2015

Conversation

tfoote
Copy link
Member

@tfoote tfoote commented Dec 16, 2015

It is tested up to static_transform_publisher and tf2_echo and tf2_monitor are working.

It is tested up to static_transform_publisher and tf2_echo and tf2_monitor are working.
ament_lint_auto_find_test_dependencies()

ament_add_gtest(test_cache_unittest test/cache_unittest.cpp)
target_link_libraries(test_cache_unittest tf2 ${geometry_msgs_LIBRARIES} ${console_bridge_LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must check if the gtest target actually exists.

Same below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@dirk-thomas
Copy link
Member

The patch shows how invasive the types Time and Duration are being used. Instead of simply replacing the ROS 1 types with ROS 2 equivalents I would suggest to create at least typedefs for them in a single place and even better implement functions to operate on them. Then the whole code base only uses the custom defined types and the functions and allows to easily change this.

#include <builtin_interfaces/msg/time.hpp>

// TODO(tfoote)
#error This has not been converted to ROS2 and will not compile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for ros2 support or is this ok to overlook in this pr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently commented out of the build as the datatype conversions packages were not included. This is how you interact with them. You have to use either the message datatypes or the tf2 native datatypes.

@@ -61,7 +61,7 @@ int main(int argc, char** argv)
nh.param("buffer_size", buffer_size, 120.0);

// WIM: this works fine:
tf2_ros::Buffer buffer_core(ros::Duration(buffer_size+0)); // WTF??
tf2_ros::Buffer buffer_core(tf2::TempDuration(buffer_size+0)); // WTF??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@wjwwood
Copy link
Member

wjwwood commented Dec 18, 2015

Well, other than my comments I guess I'd +1 this for merge.

However, it still needs a lot of work and it is a bit discouraging that we don't have tests to verify that things behave as expected. There are a bunch of workarounds in this pr which just change the way tf works and I guess if the goal is to get something merged then that's ok, but realistically without some underlying features hashed out in ROS 2 this is pretty hack-y, e.g. latching, logging, ROS Time (sim time), etc.

@tfoote
Copy link
Member Author

tfoote commented Dec 18, 2015

I created the aliases for tf2::TimePoint and tf2::Duration and deployed them through the compiling code. I also cleaned up a few windowsisms necessary to build.

CI:
http://ci.ros2.org/job/ci_linux/803/
http://ci.ros2.org/job/ci_osx/671/ (expecting test failures due to ros2/ros2#174)
http://ci.ros2.org/job/ci_windows/857 (The bullet test fails)

tfoote added a commit that referenced this pull request Dec 18, 2015
Create a version of tf2 for ros2.
@tfoote tfoote merged commit a784664 into ros2_alpha Dec 18, 2015
@tfoote tfoote deleted the tf2_ament branch December 18, 2015 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants