Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter #121

jacobperron · 2019-06-21T23:53:59Z

This work is towards enabling the use of MessageFilter in RViz (see ros2/rviz#375 (comment)).
The idea is to make RViz transformer plugins extend the new tf2::FrameTransformerInterface so that we can take advantage of tf2::MessageFilter with different transformer plugins (not just tf2::BufferCore).

This PR does three things:

Propose the new interface tf2::FrameTransformerInterface (533ccd6)
Make tf2::BufferCore implement the interface (f4c2c98)
Make tf2::MessageFilter use the interface (9550875)

jacobperron · 2019-06-24T23:56:53Z

After an offline discussion with @wjwwood and @tfoote, I've done some refactoring.
Now, the new base class for BufferCore is named BufferCoreInterface for consistency.
The waitForTransform API has been moved to tf2_ros::BufferInterface, which is the ROS-aware (ie. time aware) interface on top of BufferCore.
I've also done a few other small things to make this PR slightly less intrusive to the existing code.

I still need to implement the timeout for waitForTransform.

jacobperron · 2019-06-28T00:39:02Z

In order to implement the asynchronous timeout I've introduced the interface CreateTimerInterface that is registered with tf2_ros::Buffer after construction (@wjwwood as per our offline discussion).

I realized that tf2::TransformListener does not take a tf2::Buffer as an argument so, rather than changing it's constructor, I've left it alone and set the CreateTimerInterface in the test:

geometry2/tf2_ros/test/message_filter_test.cpp

Lines 62 to 64 in d751313

    
           tf2_ros::Buffer buffer(clock); 
        
           buffer.setCreateTimerInterface(create_timer_interface); 
        
           tf2_ros::TransformListener tfl(buffer);

wjwwood · 2019-06-28T11:15:07Z

I realized that tf2::TransformListener does not take a tf2::Buffer as an argument so, rather than changing it's constructor, I've left it alone and set the CreateTimerInterface in the test:

What? the tf2_ros::TransformListener takes the tf2_ros::Buffer as the first argument in the constructor. I don't entirely follow why the tf2_ros::TransformListener cannot call this method on the buffer? You said specifically tf2 and not tf2_ros in your message, is that the difference?

jacobperron · 2019-06-28T16:18:56Z

What? the tf2_ros::TransformListener takes the tf2_ros::Buffer as the first argument in the constructor. I don't entirely follow why the tf2_ros::TransformListener cannot call this method on the buffer? You said specifically tf2 and not tf2_ros in your message, is that the difference?

I meant tf2_ros::Buffer. tf2_ros::TransformerListener accepts a tf2::BufferCore (not a tf2_ros::Buffer), so we can't register the new timer interface from inside the transform listener (unless we change the signature).

Edit: We can pass a tf2_ros::Buffer because it is extending tf2::BufferCore

wjwwood · 2019-06-28T17:30:15Z

I see what you mean now. Oof. Does it make sense to move all the stuff (timeout based waitFor and the timer creation interface) into tf2::Buffer? I guess not huh?

I suppose we could add a new signature for tf2_ros::TransformListener that takes a tf2_ros::Buffer instead and only sets up the interface if that signature is used. I think most people will be passing an instance of the tf2_ros::Buffer anyways, and if not the new waitFor will not work unless they do so.

jacobperron · 2019-06-28T17:36:20Z

Yeah, I don't think it makes sense to add the new async API to tf2::BufferCore since it is time agnostic.

Unless there's a use-case for passing a tf2::BufferCore, I would change the type passed to tf2_ros::TransformListener to tf2_ros::Buffer. But maybe it's safer to add a new signature as you suggest.

wjwwood · 2019-06-28T17:46:49Z

Only if you wanted to backport it and not break API. Otherwise, unless @tfoote sees a different reason, I think changing it would be ok.

jacobperron · 2019-06-28T17:51:40Z

I've already broken API by changing the signature of tf2_ros::MessageFilter, which I don't think is as straightforward to make backwards compatible; I'd have to think about it.

jacobperron · 2019-07-02T18:55:03Z

I think this is ready for review. Here's a description of the changes:

Add pure virtual interfacetf2::BufferCoreInterface and have tf2::BufferCore implement it.
It is required that the object passed to tf2_ros::MessageFilter implements this interface.
The interface signature is very similar to the methods already in tf2::BufferCore to keep the diff smaller.
Add pure virtual interface tf2_ros::AsyncBufferInterface and have tf2_ros::Buffer implement it.
The new interface declares a single method waitForTransform.
It is required that the object passed to tf2_ros::MessageFilter implements this interface.
Since tf2 doesn't have a sense of time, tf2_ros seems to be the appropriate place to put the interface. An alternative approach is to add the async method to the existing tf2_ros::BufferInterface, but since tf2_ros::MessageFilter doesn't need any of the other interface methods, I chose to create a new interface. This cuts down on the number of methods that need to be implemented by third-parties (e.g. RViz).
Add pure virtual interface tf2_ros::CreateTimerInterface and concrete class tf2_ros::CreateTimerROS.
Added a method tf2_ros::Buffer::setCreateTimerInterface() for registering the interface, which is required for computing timeouts for the new asynchronous method (AsyncBufferInterface::waitForTransform()).
tf2_ros::MessageFilter signature templated on the Buffer type.
Static assertions are added to ensure the template type implements tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface.

Pending review and approval, I'll curate the commits so that they reflect the above description.

jacobperron · 2019-07-02T19:06:54Z

See proposed usage in RViz: ros2/rviz#422

tfoote

Overall this looks good to me. One small nitpick/suggestion on the exception type.

tf2_ros/src/buffer.cpp

jacobperron · 2019-07-10T18:19:07Z

Packages above tf2 and tf2_ros:

Linux
Linux-aarch64
macOS
Windows

Edit: Also need to build up to tf2_ros

tf2/include/tf2/buffer_core.h

wjwwood · 2019-07-10T21:56:25Z

tf2/include/tf2/buffer_core.h

@@ -108,7 +109,7 @@ class BufferCore

  /** \brief Clear all data */
  TF2_PUBLIC
-  void clear();
+  void clear() override;


I'm also mildly concerned about making these functions virtual (though I don't see an alternative), because it will now require a vtable lookup, and these functions are called very frequently. Just bringing it up in case anyone else agrees, but I don't see an alternative at the moment as the standard way to avoid a vtable is CRTP and the way we're using this with plugins I think makes that option not viable.

jacobperron · 2019-07-12T00:16:28Z

Linux
Linux-aarch64
macOS
Windows

One of the new tests exposed an existing bug. A PR with a fix is open: #127

I also made a couple other small fixes with this PR. @wjwwood PTAL

jacobperron · 2019-07-12T16:43:10Z

I've rebased to get the fix from #127. The last three commits are new since the last review.

Linux
Linux-aarch64
macOS
Windows

tf2::BufferCore implements this interface. The plan is to use the interface in place of tf2::BufferCore (for example, in tf2_ros::MessageFilter). This gives applications a way to provide custom implementations of the core TF interface. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Add concrete class tf2_ros::CreateTimerROS that implements the new interface. The interface will be used by tf2_ros::Buffer to implement asynchronous operations. Providing a separate interface for interacting with timers lets us keep a separation of concerns when it comes to executing user-defined callbacks. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

The new interface declares a single method waitForTransform. The interface is implemented by tf2_ros::Buffer, which makes use of tf2_ros::CreateTimerInterface. This makes it easier for users to make asynchronous transform requests, rather than using the methods provided by the base class tf2::BufferCore. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

This change applications to use the message filter with custom buffer implementations. The buffer type provided must implement tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface, which is enforced with static assertions. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jdlangs · 2019-08-26T19:34:46Z

@jacobperron this code is not compiling for me and I'm unable to figure out why. The trouble seems to be with the added static_asserts where my compiler can't find the tf2::BufferCoreInterface name, even though there's a clear header chain that includes tf2/buffer_core_interface.h:

[Processing: tf2_ros]
--- stderr: tf2_ros
In file included from /home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/test/message_filter_test.cpp:39:0:
/home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/include/tf2_ros/message_filter.h: In constructor ‘tf2_ros::MessageFilter<M, BufferT>::MessageFilter(BufferT&, const string&, uint32_t, const SharedPtr&, std::chrono::duration<_Rep, _Period>)’:
/home/jlangsfeld/Workspaces/ros2_wssrc/geometry2/tf2_ros/include/tf2_ros/message_filter.h:135:40: error: ‘BufferCoreInterface’ is not a member of ‘tf2’
     static_assert(std::is_base_of<tf2::BufferCoreInterface, BufferT>::value,
                                        ^~~~~~~~~~~~~~~~~~~
/home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/include/tf2_ros/message_filter.h:135:40: note: suggested alternative: ‘TimeCacheInterface’
     static_assert(std::is_base_of<tf2::BufferCoreInterface, BufferT>::value,
                                        ^~~~~~~~~~~~~~~~~~~
                                        TimeCacheInterface

If I explicitly add #include <tf2/buffer_core_interface.h> to message_filter.h, this error goes away, but the static_assert fails even though there's a clear inheritance chain from tf2_ros::Buffer to tf2::BufferCoreInterface:

--- stderr: tf2_ros
In file included from /home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/test/message_filter_test.cpp:39:0:
/home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/include/tf2_ros/message_filter.h: In instantiation of ‘tf2_ros::MessageFilter<M, BufferT>::MessageFilter(BufferT&, const string&, uint32_t, const SharedPtr&, std::chrono::duration<_Rep, _Period>) [with TimeRepT = long int; TimeT = std::ratio<1, 1000000000>; M = geometry_msgs::msg::PointStamped_<std::allocator<void> >; BufferT = tf2_ros::Buffer; std::__cxx11::string = std::__cxx11::basic_string<char>; uint32_t = unsigned int; rclcpp::Node::SharedPtr = std::shared_ptr<rclcpp::Node>]’:
/home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/test/message_filter_test.cpp:59:92:   required from here
/home/jlangsfeld/Workspaces/ros2_ws/src/geometry2/tf2_ros/include/tf2_ros/message_filter.h:136:5: error: static assertion failed: Buffer type must implement tf2::BufferCoreInterface
     static_assert(std::is_base_of<tf2::BufferCoreInterface, BufferT>::value,

Any suggestions?

jacobperron · 2019-08-26T21:20:38Z

@jdlangs I can reproduce the error if I source a binary installation of Dashing and then try to compile geometry2. If I build ROS 2 from source, then I do not run into this issue. I suspect there's a mix-up happening with the Dashing installation, which doesn't contain the changes introduced in this PR, where tf2_ros is linking against the binary installation of tf2. Unfortunately, I believe in order to get the tf2_ros library to link against the latest version of tf2, we also need to have source builds of its other direct dependencies (rclcpp, message_filters, and geometry_msgs). Because there have been API/ABI breaking changes to rclcpp and its dependencies since Dashing, I suggest building ROS 2 from source if you want to take advantage of this change.

Edit: fixed in #156

Since the message filter is referencing tf2::BufferCoreInterface, we better include the appropriate header in any case.

jacobperron · 2019-08-26T22:11:44Z

@jdlangs Nevermind, this is a bug that can be resolved by changing the order of linked dependencies. See #156 for a fix.

jacobperron added the in review Waiting for review (Kanban column) label Jun 21, 2019

jacobperron self-assigned this Jun 21, 2019

jacobperron added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jun 24, 2019

jacobperron changed the title ~~Add FrameTransformerInterface as base class for BufferCore~~ Add async method 'waitForTransform' to BufferInterface and use BufferInterface in MessageFilter Jun 28, 2019

jacobperron mentioned this pull request Jun 28, 2019

Add tf2_ros::async_wait_for_transform() #117

Closed

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 2, 2019

jacobperron mentioned this pull request Jul 2, 2019

FrameTransformer implements tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface ros2/rviz#422

Merged

jacobperron changed the title ~~Add async method 'waitForTransform' to BufferInterface and use BufferInterface in MessageFilter~~ Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter Jul 2, 2019

jacobperron mentioned this pull request Jul 2, 2019

Re-enable use of tf message filter ros2/rviz#375

Merged

tfoote approved these changes Jul 10, 2019

View reviewed changes

tf2_ros/src/buffer.cpp Outdated Show resolved Hide resolved

wjwwood reviewed Jul 10, 2019

View reviewed changes

tf2/include/tf2/buffer_core.h Outdated Show resolved Hide resolved

wjwwood reviewed Jul 10, 2019

View reviewed changes

jacobperron mentioned this pull request Jul 12, 2019

Guard against invalid iterator #127

Merged

jacobperron force-pushed the jacob/frame_transformer branch from a4e45f5 to 60b0758 Compare July 12, 2019 16:41

wjwwood approved these changes Jul 12, 2019

View reviewed changes

jacobperron added 4 commits July 12, 2019 15:12

jacobperron force-pushed the jacob/frame_transformer branch from 60b0758 to 1d097dc Compare July 12, 2019 22:44

jacobperron merged commit 046df60 into ros2 Jul 12, 2019

delete-merged-branch bot deleted the jacob/frame_transformer branch July 12, 2019 22:44

ghost mentioned this pull request Jul 17, 2019

Nav2 system tests broken ros-navigation/navigation2#941

Closed

jacobperron mentioned this pull request Aug 6, 2019

Raise std::future_error: Promise already satisfied #141

Closed

ghost mentioned this pull request Aug 6, 2019

Default waitForTransform timeout of 0ns in message filter results in 100% transform timeout #145

Closed

jacobperron mentioned this pull request Aug 26, 2019

Use ament_target_dependencies to ensure correct dependency order #156

Merged

jacobperron mentioned this pull request Sep 30, 2019

Plans for release to dashing / ros2? ros-visualization/interactive_markers#52

Closed

ghost mentioned this pull request Oct 21, 2019

Should we provide a node to TF listeners? ros-navigation/navigation2#1182

Closed

cottsay mentioned this pull request Jan 10, 2020

Segmentation fault on laser scan #213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter #121

Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter #121

jacobperron commented Jun 21, 2019

jacobperron commented Jun 24, 2019

jacobperron commented Jun 28, 2019

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019 •

edited

Loading

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019 •

edited

Loading

jacobperron commented Jul 2, 2019

jacobperron commented Jul 2, 2019

tfoote left a comment

jacobperron commented Jul 10, 2019 •

edited

Loading

wjwwood Jul 10, 2019

jacobperron commented Jul 12, 2019

jacobperron commented Jul 12, 2019

jdlangs commented Aug 26, 2019

jacobperron commented Aug 26, 2019 •

edited

Loading

jacobperron commented Aug 26, 2019

Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter #121

Add tf2::BufferCoreInterface and tf2_ros::AsyncBufferInterface to use in tf2_ros::MessageFilter #121

Conversation

jacobperron commented Jun 21, 2019

jacobperron commented Jun 24, 2019

jacobperron commented Jun 28, 2019

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019 • edited Loading

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019

wjwwood commented Jun 28, 2019

jacobperron commented Jun 28, 2019 • edited Loading

jacobperron commented Jul 2, 2019

jacobperron commented Jul 2, 2019

tfoote left a comment

Choose a reason for hiding this comment

jacobperron commented Jul 10, 2019 • edited Loading

wjwwood Jul 10, 2019

Choose a reason for hiding this comment

jacobperron commented Jul 12, 2019

jacobperron commented Jul 12, 2019

jdlangs commented Aug 26, 2019

jacobperron commented Aug 26, 2019 • edited Loading

jacobperron commented Aug 26, 2019

jacobperron commented Jun 28, 2019 •

edited

Loading

jacobperron commented Jun 28, 2019 •

edited

Loading

jacobperron commented Jul 10, 2019 •

edited

Loading

jacobperron commented Aug 26, 2019 •

edited

Loading