fuse -> ROS 2 fuse_publishers: Port fuse_publishers #299

methylDragon · 2022-12-15T18:23:11Z

Description

This PR ports the entirety of fuse_publishers, including:

CMakeLists and packages
Source code
Tests
Other stuff like docs are in other PRs.

Notably, the AsyncPublisher class in fuse_core was updated (together with its tests)
I also changed the signature of its constructor to make use of initialize to set executor threads as well as pass it the appropriate NodeInterfaces.

In order to avoid specifying all manner of node interfaces in the AsyncPublisher class, I made it so that the derived publisher classes shadow their internal interface variable (and also call the parent's initialize method).

TODOs were left in downstream packages (models and tutorials) to update the usage of AsyncPublisher just like how it was done in fuse_publishers.

Furthermore, there's a missing signature in tf2_ros::Buffer that allows for buffer construction with node-like types. I left a TODO. I don't think it's too important, since it's only for logging/debugging with service calls.

Also, I realized I could just declare parameters instead of loading them in via yaml files, so I changed around some of the tests to reflect that...

Notes

Pinging @svwilliams for visibility.

methylDragon · 2022-12-20T08:30:07Z

@ros-pull-request-builder run tests please!

methylDragon · 2022-12-20T09:05:35Z

@ros-pull-request-builder run tests please!

Only instabilities are linters!

fuse_core/include/fuse_core/publisher.hpp

fuse_publishers/fuse_plugins.xml

fuse_publishers/src/pose_2d_publisher.cpp

fuse_publishers/test/test_pose_2d_publisher.cpp

fuse_publishers/test/test_path_2d_publisher.cpp

methylDragon · 2022-12-21T21:53:38Z

Likewise, only linters

fuse_core/include/fuse_core/async_publisher.hpp

sloretz · 2022-12-29T01:26:11Z

fuse_core/include/fuse_core/async_publisher.hpp

   * @param[in] name A unique name to give this plugin instance
+   * @param[in] name The number of threads to use with this publisher


Nit, remove this line

sloretz · 2022-12-29T01:27:49Z

fuse_core/src/async_publisher.cpp

-void AsyncPublisher::initialize(const std::string & name)
+void AsyncPublisher::initialize(
+  node_interfaces::NodeInterfaces<
+    node_interfaces::Base,


Nit, I think this could use ALL_RCLCPP_NODE_INTERFACES macro. The macro says fuse_core::..., but I think that will still work inside the fuse_core namespace.

Ah I missed this one

sloretz · 2022-12-29T01:29:16Z

fuse_core/test/test_async_publisher.cpp

@@ -45,7 +45,7 @@ class MyPublisher : public fuse_core::AsyncPublisher
 {
 public:
  MyPublisher()
-  : fuse_core::AsyncPublisher(0),
+  : fuse_core::AsyncPublisher(),


nit, 1 to match the ROS 1 test:

fuse/fuse_core/test/test_async_publisher.cpp

Line 49 in 0461f77

fuse_core::AsyncPublisher(1),

sloretz · 2022-12-29T01:31:32Z

fuse_publishers/include/fuse_publishers/pose_2d_publisher.h

  std::string base_frame_;  //!< The name of the robot's base_link frame
  fuse_core::UUID device_id_;  //!< The UUID of the device to be published
+  rclcpp::Clock::SharedPtr clock_;  //!< The publisher's clock, for timestamping and logging
+  rclcpp::Logger logger_;  //!< The publisher's logger, shared_ptr for deferred init


nit, update comment to remove shared_ptr

sloretz · 2022-12-29T01:40:46Z

fuse_publishers/test/test_pose_2d_publisher.cpp


  // Send the graph to the Publisher to trigger message publishing
  publisher.notify(transaction_, graph_);

  // Verify the subscriber received the expected pose
-  rclcpp::Time timeout = node_->now() + rclcpp::Duration::from_seconds(10.0);
-  while ((!received_pose_msg_) && (node_->now() < timeout))
+  rclcpp::Time timeout = node->now() + rclcpp::Duration::from_seconds(20.0);


why was the timeout increased from 10s to 20s?

I found that it sometimes times out if left at 10 seconds, same with the ones below

Interesting! I wonder if the timeout was happening because of the extra 5 second sleep that was removed #299 (comment)

I changed the timeouts back to 10 seconds and didn't see any failures in 100 runs using:

colcon test --event-handlers console_direct+ --packages-select fuse_publishers --ctest-args -R _2d_publisher --retest-until-fail 100

methylDragon#4

sloretz · 2022-12-29T01:41:11Z

fuse_publishers/test/test_pose_2d_publisher.cpp


  // Send the graph to the Publisher to trigger message publishing
  publisher.notify(transaction_, graph_);

  // Verify the subscriber received the expected pose
-  rclcpp::Time timeout = node_->now() + rclcpp::Duration::from_seconds(10.0);
-  while ((!received_pose_with_covariance_msg_) && (node_->now() < timeout))
+  rclcpp::Time timeout = node->now() + rclcpp::Duration::from_seconds(20.0);


same question about the timeout increase

sloretz · 2022-12-29T01:41:27Z

fuse_publishers/test/test_pose_2d_publisher.cpp

  // Send the graph to the Publisher to trigger message publishing
  publisher.notify(transaction_, graph_);

  // Verify the subscriber received the expected pose
-  rclcpp::Time timeout = node_->now() + rclcpp::Duration::from_seconds(10.0);
-  while ((!received_tf_msg_) && (node_->now() < timeout))
+  rclcpp::Time timeout = node->now() + rclcpp::Duration::from_seconds(20.0);


same question about the timeout increase

fuse_publishers/test/test_pose_2d_publisher.cpp

fuse_publishers/test/test_path_2d_publisher.cpp

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon requested a review from sloretz December 15, 2022 18:23

methylDragon mentioned this pull request Dec 15, 2022

Fuse -> ROS 2 Rolling Porting Progress #276

Closed

54 tasks

methylDragon force-pushed the rolling-publishers branch 5 times, most recently from 96bfbd8 to d95e745 Compare December 20, 2022 08:17

methylDragon changed the title ~~fuse -> ROS 2 fuse_publishers: Port fuse_publishers (WIP)~~ fuse -> ROS 2 fuse_publishers: Port fuse_publishers Dec 20, 2022

methylDragon force-pushed the rolling-publishers branch from d95e745 to cb6b42e Compare December 20, 2022 08:29

methylDragon marked this pull request as ready for review December 20, 2022 08:29

methylDragon force-pushed the rolling-publishers branch 3 times, most recently from 3e45013 to f9d587a Compare December 20, 2022 09:04

sloretz reviewed Dec 21, 2022

View reviewed changes

methylDragon force-pushed the rolling-publishers branch from f9d587a to 6ee89e8 Compare December 21, 2022 21:51

methylDragon requested a review from sloretz December 21, 2022 21:52

sloretz reviewed Dec 28, 2022

View reviewed changes

fuse_core/include/fuse_core/async_publisher.hpp Show resolved Hide resolved

methylDragon requested a review from sloretz December 29, 2022 01:21

sloretz reviewed Dec 29, 2022

View reviewed changes

sloretz mentioned this pull request Dec 29, 2022

Reduce timeouts to 10s methylDragon/fuse#4

Closed

methylDragon force-pushed the rolling-publishers branch from 2f0e84d to 14c26a6 Compare December 29, 2022 02:39

methylDragon requested a review from sloretz December 29, 2022 02:39

methylDragon force-pushed the rolling-publishers branch from dd4875f to 0a890d4 Compare December 29, 2022 18:15

sloretz approved these changes Dec 29, 2022

View reviewed changes

methylDragon and others added 3 commits December 29, 2022 16:23

Port messages

57e83f5

Signed-off-by: methylDragon <methylDragon@gmail.com>

Port fuse_publishers and some upstream tests

648f89d

Signed-off-by: methylDragon <methylDragon@gmail.com>

Make thread_count an AsyncPublisher constructor argument (#3)

f86f7b5

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon added 2 commits December 29, 2022 16:25

Implement ALL_RCLCPP_NODE_INTERFACES macro

63dd697

Signed-off-by: methylDragon <methylDragon@gmail.com>

Revert timeout change for publisher tests

1ee1885

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon force-pushed the rolling-publishers branch from 0a890d4 to 1ee1885 Compare December 30, 2022 00:25

methylDragon merged commit 5f4c5ed into locusrobotics:rolling Dec 30, 2022

methylDragon deleted the rolling-publishers branch December 30, 2022 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuse -> ROS 2 fuse_publishers: Port fuse_publishers #299

fuse -> ROS 2 fuse_publishers: Port fuse_publishers #299

methylDragon commented Dec 15, 2022 •

edited

Loading

methylDragon commented Dec 20, 2022 •

edited

Loading

methylDragon commented Dec 20, 2022 •

edited

Loading

methylDragon commented Dec 21, 2022 •

edited

Loading

sloretz Dec 29, 2022

sloretz Dec 29, 2022

methylDragon Dec 29, 2022

sloretz Dec 29, 2022

sloretz Dec 29, 2022

sloretz Dec 29, 2022

methylDragon Dec 29, 2022

sloretz Dec 29, 2022

sloretz Dec 29, 2022

sloretz Dec 29, 2022

		* @param[in] name A unique name to give this plugin instance
		* @param[in] name The number of threads to use with this publisher

fuse -> ROS 2 fuse_publishers: Port fuse_publishers #299

fuse -> ROS 2 fuse_publishers: Port fuse_publishers #299

Conversation

methylDragon commented Dec 15, 2022 • edited Loading

Description

Notes

methylDragon commented Dec 20, 2022 • edited Loading

methylDragon commented Dec 20, 2022 • edited Loading

methylDragon commented Dec 21, 2022 • 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

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

methylDragon commented Dec 15, 2022 •

edited

Loading

methylDragon commented Dec 20, 2022 •

edited

Loading

methylDragon commented Dec 20, 2022 •

edited

Loading

methylDragon commented Dec 21, 2022 •

edited

Loading