Message filters rework port in progress for review by tfoote · Pull Request #5 · ros2/message_filters

tfoote · 2018-07-30T21:46:17Z

This is a rework of #4 for better traceability of code origin as well as some cleanup.

I've been splitting it into many different commits for easier auditing. I've separated out copying in files from different locations from converting them to the new syntax.

After that restructuring I've then gone through and been doing reviews of the changes.

There's now more commits in the history than I plan to have at the end. Several of the later ones are reverting non-essential changes earlier. I plan to find where they were changed and squash the revert of the changes down.

Outstanding TODOs

implement correct solutions for

ROS_WARN -> cout
ROS_ASSERT -> assert
ROS_BREAK -> std::abort
Commented asserts such as in test_subscriber.cpp
time_sequencer_unittest.cpp:135
ros::Time::setNow(ros::Time::now() + ros::Duration(2));
removed from test.

@j-rivero @mjcarroll @gaoethan If you could take a look and give any feedback/comments suggestions I'll pull them in as i finish my audit and cleanup before a final review.

@mjcarroll Please look closely at c7f8d24 and abe143a It's a slight change in the API that I made based on talking with @wjwwood

dirk-thomas · 2018-07-30T22:54:48Z

Should ROS 1 specific concepts like connection headers, caller id, etc. be removed?

j-rivero

Two small comments on building and python tests, I was not able to find any more errors. Build and tests are fine on my Ubuntu Bionic system.

j-rivero · 2018-07-30T23:02:50Z

CMakeLists.txt

+find_package(rclcpp REQUIRED)
+find_package(rcutils)
+find_package(builtin_interfaces REQUIRED)
+find_package(std_msgs REQUIRED)


I can only find std_mgs in test/test_message_filters_cache.py. If this is the case, I think that we can apply this patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt index 81fcb1e..c982b47 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,7 +16,6 @@ find_package(ament_cmake REQUIRED) find_package(rclcpp REQUIRED) find_package(rcutils) find_package(builtin_interfaces REQUIRED) -find_package(std_msgs REQUIRED) find_package(sensor_msgs REQUIRED) include_directories(include) @@ -24,7 +23,6 @@ add_library(${PROJECT_NAME} SHARED src/connection.cpp) ament_target_dependencies(${PROJECT_NAME} "rclcpp" "rcutils" - "std_msgs" "builtin_interfaces" "sensor_msgs") @@ -53,6 +51,8 @@ ament_export_include_directories(include) ament_export_libraries(${PROJECT_NAME}) if(BUILD_TESTING) + find_package(std_msgs REQUIRED) + find_package(ament_lint_auto REQUIRED) ament_lint_auto_find_test_dependencies() diff --git a/package.xml b/package.xml index 0d159d1..fc490be 100644 --- a/package.xml +++ b/package.xml @@ -19,9 +19,9 @@ <build_depend>builtin_interfaces</build_depend> <build_depend>rclcpp</build_depend> <build_depend>rclpy</build_depend> - <build_depend>std_msgs</build_depend> <build_depend>sensor_msgs</build_depend> + <test_depend>std_msgs</test_depend> <test_depend>ament_lint_auto</test_depend> <test_depend>ament_cmake_gtest</test_depend> <test_depend>ament_cmake_pytest</test_depend>

j-rivero · 2018-07-30T23:36:58Z

CMakeLists.txt

+    target_link_libraries(${PROJECT_NAME}-test_fuzz ${PROJECT_NAME})
  endif()

-  # Unit test of the approximate synchronizer


Looks like the python tests were not ported or I can not find them. Not sure if this was on propose.

yes, @tfoote focuses on the CPP part now, the python part hasn't been involved in this PR.

gaoethan

From my simple checking, the following remains to do:

missing python refactoring, maybe it's another PR.
readme update
The rest APIs doc update accordingly with ros2

gaoethan · 2018-07-31T06:31:34Z

include/message_filters/message_traits.h

+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef ROSLIB_MESSAGE_TRAITS_H


Maybe the prefix of ROSLIB is not very suitable now in ROS2

gaoethan · 2018-07-31T06:37:21Z

include/message_filters/parameter_adapter.h

+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef ROSCPP_PARAMETER_ADAPTER_H


ROSCPP needs to be replaced accordingly now.

gaoethan · 2018-07-31T06:43:06Z

include/message_filters/message_event.h

+ROS_DEPRECATED inline std::shared_ptr<M> defaultMessageCreateFunction()
+{
+  return DefaultMessageCreator<M>()();
+}


It's already been marked as ROS_DEPRECATED and better clean it if no use now.

gaoethan · 2018-07-31T06:49:17Z

include/message_filters/subscriber.h

@@ -88,30 +84,27 @@ typedef boost::shared_ptr<SubscriberBase> SubscriberBasePtr;
 *
 * The output connection for the Subscriber object is the same signature as for roscpp subscription callbacks, ie.


doc update to rclcpp

gaoethan · 2018-07-31T06:53:20Z

include/message_filters/subscriber.h

  }

  /**
   * \brief Returns the internal ros::Subscriber object


gaoethan · 2018-07-31T06:56:59Z

include/message_filters/time_sequencer.h

@@ -67,31 +68,31 @@ namespace message_filters
 *
 * TimeSequencer's input and output connections are both of the same signature as roscpp subscription callbacks, ie.


doc update like ros::Duration, roscpp ...

gaoethan · 2018-07-31T06:58:36Z

include/message_filters/time_sequencer.h

   * \param update_rate The rate at which to check for messages which have passed "delay"
   * \param queue_size The number of messages to store
-   * \param nh (optional) The NodeHandle to use to create the ros::SteadyTimer that runs at update_rate
+   * \param node The Node to use to create the ros::SteadyTimer that runs at update_rate


doc update for ros::SteadyTimer

gaoethan · 2018-07-31T06:58:49Z

include/message_filters/time_sequencer.h

   * \param update_rate The rate at which to check for messages which have passed "delay"
   * \param queue_size The number of messages to store
-   * \param nh (optional) The NodeHandle to use to create the ros::SteadyTimer that runs at update_rate
+   * \param node The Node to use to create the ros::SteadyTimer that runs at update_rate


doc update for ros::SteadyTimer

gaoethan · 2018-07-31T07:00:04Z

include/message_filters/cache.h

@@ -59,15 +60,15 @@ namespace message_filters
 *
 * Cache's input and output connections are both of the same signature as roscpp subscription callbacks, ie.


doc update for roscpp

gaoethan · 2018-07-31T07:02:49Z

include/message_filters/cache.h

@@ -127,11 +128,11 @@ class Cache : public SimpleFilter<M>
   */


better clean it for ROS_ERROR like //ROS_ERROR("Cannot set max_size to 0") ; at line 109

gaoethan · 2018-07-31T07:16:52Z

@tfoote it works within my Ubuntu bionic, thanks for your effort 👍

tfoote · 2018-08-06T07:40:54Z

Thanks for the reviews. I believe I've addressed all comments and have now finished cleaning up all items I flagged as well. It's ready for a review.

mikaelarguedas · 2018-08-09T22:43:16Z

include/message_filters/message_traits.h

@@ -0,0 +1,73 @@
+/*


In the past when we copied code from other repos (without the file's history) we place a comment in the file pointing to where the file was copied from (e.g. https://github.com/ros2/turtlebot2_demo/blob/2017ddd2c5781434d9dbcb0c172d553e28f7ab02/depthimage_to_pointcloud2/include/depthimage_to_pointcloud2/depth_traits.hpp#L34)

Then adapting the header guards (as mentioned in https://github.com/ros2/message_filters/diffs#r206407588) to match the name of the new hosting package is IMO preferred but not a blocker for merging

@tfoote did you see this comment ?

Yeah I did, but I wanted to get this in since it was blocking our other steps. I'll open a new PR with the sources.

Yeah I did, but I wanted to get this in since it was blocking our other steps. I'll open a new PR with the sources.

👍 sounds good, just wanted to make sure it didnt get lost in the process.
Thanks for opening the follow-up PR!

gaoethan

lgtm

Use internal MessageEvent, traits, TODO needs noncopyable first. Should use '= delete' instead https://stackoverflow.com/questions/31940886/is-there-a-stdnoncopyable-or-equivalent TODO std::cout used instead of ROS_WARN_STREAM equivalent TODO assert migration TODO ROS_BREAK equivalent

TODO(tfoote) find why

This gets rid of the need for the accessor too.

And remove all references to NodeHandle and call everything nodes as they are.

Remove old roscpp references from docs and an include guard.

Provide migration path #define and TODO for resolving once provided upstream in ros2/rcutils#112

Import message_traits.h from roscpp

86ceaa6

tfoote requested review from j-rivero and mjcarroll July 30, 2018 21:46

j-rivero reviewed Jul 31, 2018

View reviewed changes

gaoethan suggested changes Jul 31, 2018

View reviewed changes

mjcarroll mentioned this pull request Jul 31, 2018

Image Transport rework port in progress for review ros-perception/image_common#84

Merged

dirk-thomas mentioned this pull request Aug 3, 2018

Make message_events build for ros2 (on windows 10) ros/roscpp_core#72

Closed

sloretz added the in review Waiting for review (Kanban column) label Aug 9, 2018

mikaelarguedas reviewed Aug 9, 2018

View reviewed changes

mjcarroll approved these changes Aug 14, 2018

View reviewed changes

gaoethan approved these changes Aug 15, 2018

View reviewed changes

tfoote added 16 commits August 14, 2018 20:30

Import ParameterAdapter from ROS1

6e805a6

Import MessageEvent class from roscpp

be328d1

Convert to ament_cmake

44562f2

Update visibility macros for rclcpp

45fc606

updating message_traits for ros2

039d093

copy parameter_adapater from roscpp

79c907d

update parameter_adapter.h for ros2

90101a8

updating message_event.h for ros2

8421da8

Add noncopyable base class

7158f13

deleted connection method

85cc96f

TODO(tfoote) find why

Import fuzz test

173c9bc

Make node required as an internal one is required to spin.

2986b82

This gets rid of the need for the accessor too.

Use Node::SharedPtr instead of Node*

26efc5b

And remove all references to NodeHandle and call everything nodes as they are.

Remove vestigial test launch file

54716f1

Update docs to remove nodehandle reference.

a9f2c45

tfoote added 8 commits August 14, 2018 20:45

Restore unit tests commented out in the porting

f0312e3

updating tests to be faster

de9c6dc

Remove using usage in a header.

c20a461

demote std_msgs to test dependency

213945f

Doc updates to remove old ros:: references.

9f6d10d

Remove old roscpp references from docs and an include guard.

Remove ROS1 specific connection header info from API.

6c70808

Remove references in pointer API calls.

62d8611

restore assert -> RCUTILS_ASSERT and abort -> RCUTILS_BREAK

b8519b6

Provide migration path #define and TODO for resolving once provided upstream in ros2/rcutils#112

tfoote force-pushed the refactor7 branch from bf7f37a to b8519b6 Compare August 15, 2018 03:49

tfoote merged commit 567ad41 into master Aug 15, 2018

tfoote deleted the refactor7 branch August 15, 2018 03:50

tfoote removed the in review Waiting for review (Kanban column) label Aug 15, 2018

mjcarroll mentioned this pull request Aug 16, 2018

Integrate changes from intel/ros2_message_filters repository #2

Closed

dirk-thomas mentioned this pull request Aug 16, 2018

most changes from ros/ros_comm#1301 #1

Closed

dirk-thomas mentioned this pull request Sep 4, 2018

Port image transport ros2/ros2#531

Closed

8 tasks

		@@ -88,30 +84,27 @@ typedef boost::shared_ptr<SubscriberBase> SubscriberBasePtr;
		*
		* The output connection for the Subscriber object is the same signature as for roscpp subscription callbacks, ie.

		@@ -67,31 +68,31 @@ namespace message_filters
		*
		* TimeSequencer's input and output connections are both of the same signature as roscpp subscription callbacks, ie.

		@@ -59,15 +60,15 @@ namespace message_filters
		*
		* Cache's input and output connections are both of the same signature as roscpp subscription callbacks, ie.

		@@ -127,11 +128,11 @@ class Cache : public SimpleFilter<M>
		*/

Conversation

tfoote commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Outstanding TODOs

Uh oh!

dirk-thomas commented Jul 30, 2018

Uh oh!

j-rivero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoethan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoethan commented Jul 31, 2018

Uh oh!

tfoote commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoethan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tfoote commented Jul 30, 2018 •

edited

Loading