Yet Another filters port to ROS 2 #32

sloretz · 2020-04-04T01:05:02Z

Alternative to #19 and #24. The parameter specification of filter1..filterN @swatifulzele came up with in #24 is used here, but implemented a bit differently.

I've been working on this a while. Here's a package where I was playing with porting laser filters with very similar code to this port: https://github.com/sloretz/ros2_laser_filters

TODO

~~@tfoote May I fast-forward merge the ros2 branch up to noetic-devel?~~ edit: done
~~There's some dead code removal (seed_rand and generate_rand_vectors()) that could be moved to a separate PR and done in noetic-devel too~~ edit opened [noetic] Delete unused code #33
~~There are some commented tests in test_chain.cpp that were deleted and could be done in a separate PR in noetic-devel too~~ edit opened [noetic] Delete unused code #33
*This PR moves headers from .h to .hpp, so it should not be squash merged because then the history of that move is lost. Instead it should be squashed to two commits where the first renames the files and the second is all the changes and then rebase and merged. edit opened [noetic] deprecate h for hpp #34
This PR does not address style; and does not enable the ROS 2 style tests. I figured that can be done in a follow up PR.

Differences from #24:

This builds on ROS Eloquent instead of ROS Bouncy
Use node logger and parameter interfaces instead of expecting rclcpp::Node so it works with lifecycle nodes
Keep getParam() functions so downstream filter changes are less
- This is implemented by declaring the parameter if not declared in getParam()
Keeps license the same
I tried to only change lines if I needed to or if they caused a compiler warning
The original tests all run and pass 🎉

Signed-off-by: Shane Loretz <[email protected]> include .h -> include .hpp Signed-off-by: Shane Loretz <[email protected]> Port FilterBase, MultiChannelFilterBase, and all filters to ROS 2 Signed-off-by: Shane Loretz <[email protected]> Fix compile warning Signed-off-by: Shane Loretz <[email protected]> don't concatenate if prefix is empty Signed-off-by: Shane Loretz <[email protected]> Prefix param only once Signed-off-by: Shane Loretz <[email protected]> Don't install test_param plugin Signed-off-by: Shane Loretz <[email protected]> Build type ament_cmake Signed-off-by: Shane Loretz <[email protected]> Port single channel filter chain Signed-off-by: Shane Loretz <[email protected]> Factor out parameter parsing Signed-off-by: Shane Loretz <[email protected]> move load_chain_config to free function Signed-off-by: Shane Loretz <[email protected]> Port MultiChannelFilterChain to ROS 2 Signed-off-by: Shane Loretz <[email protected]> Delete unused files from ROS 1 Signed-off-by: Shane Loretz <[email protected]>

clalancette

First pass, mostly small things. I'll do another pass later that is more thorough.

include/filters/filter_base.hpp

CMakeLists.txt

include/filters/filter_base.hpp

include/filters/filter_chain.hpp

Signed-off-by: Shane Loretz <[email protected]>

clalancette

There's a lot of legacy stuff in here. I've left specific comments inline, but some overall comments:

We should open an issue to follow-up and cleanup the style everywhere, including removing trailing whitespace.
We should mark all of the methods that are overriding the parent methods as override to give the compiler a hint.
The types in some of these classes are a bit loose. Comparing unsigned int/uint32_t to size_t, for instance, may be a problem on some architectures. Probably should make an issue to clean that up.

clalancette · 2020-04-08T13:52:29Z

package.xml

-  <run_depend>boost</run_depend>
+  <depend>pluginlib</depend>
+  <depend>rclcpp</depend>
+  <depend>boost</depend>


Bah. We could almost make the boost dependency a build_depend; we only use circular_buffer, which is header only. The only problem is that it is used in an exported header (include/filters/realtime_circular_buffer.hpp), so downstream consumers still need to know about it. I'll suggest that we either remove boost completely, or change this to a PIMPL pattern so we can do that. But feel free to punt on that and just open an issue to follow-up in the future.

(also nit: alphabetize)

I started the port by trying to eliminate the dependency on boost in ROS 1, but I wasn't able to find a good replacement for realtime_circular_buffer :( . #30

I'll punt on the pimpl pattern for now.

clalancette · 2020-04-08T13:54:07Z

CMakeLists.txt

+)
+
+if(BUILD_TESTING)
+  # TODO(sloretz) this disables ROS 2 style tests; reenable and fix style


Please open an issue to follow-up on this.

CMakeLists.txt

package.xml

clalancette · 2020-04-08T14:03:15Z

include/filters/filter_base.hpp

+    return true;
+  }
+
+protected:


Can we collapse all of the protected sections into one? And then put the private section after that?

include/filters/filter_base.hpp

clalancette · 2020-04-08T14:06:34Z

include/filters/filter_chain.hpp

+      const rclcpp::node_interfaces::NodeParametersInterface::SharedPtr & node_params,
+      std::vector<struct FoundFilter> & found_filters)
+    {
+      // TODO(sloretz) error if someone tries to do filter0


Can you open an issue for this?

clalancette · 2020-04-08T14:07:24Z

include/filters/filter_chain.hpp

+      // Make prefix be an empty string or a string ending in '.'
+      std::string norm_param_prefix = param_prefix;
+      if (!norm_param_prefix.empty()) {
+        if ('.' != norm_param_prefix.back()) {
+          norm_param_prefix += '.';
+        }
+      }


Seems like common code, at least it is duplicated here with FilterBase. Maybe make a utility? Feel free to open an issue to followup.

include/filters/increment.hpp

Co-Authored-By: Chris Lalancette <[email protected]>

Signed-off-by: Shane Loretz <[email protected]>

clalancette · 2020-04-23T13:11:40Z

All right, with the latest changes this is getting closer. I think there are a few open problems that would be nice to fix now, and a few to punt until later. The ones I think we can do now without too much work:

Make a utility method to normalize the parameter prefix to be empty or a string and .
Mark methods with override as appropriate.
Collapse all of the FilterBase class protected/private sections into one of each.

And then here are ones I think we can punt on until a later PR:

Remove boost::circular_buffer, or at least PIMPL it so it is a build_depend only.
Cleanup style and whitespace everywhere.
Cleanup types, particularly around uint/size_t
Fix errors if someone does filter0.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-04-30T15:36:28Z

@sloretz @clalancette I've opened sloretz#1 against this PR's branch. I believe I've addressed most pain points, except:

Remove boost::circular_buffer, or at least PIMPL it so it is a build_depend only.

Which is somewhat tricky as filters:;RealtimeCircularBuffer is a template class itself and a type erased PIMPL seemed a bit too much to me, and

Fix errors if someone does filter0.

As I think that should be entirely handled by rclcpp itself. It currently bans setting undeclared parameters if allow_undeclared_parameters is false, but you can still provide an undeclared parameter override on start up and that will not produce any error nor warning. API to let rclcpp know that no further parameter declaration will occur may help with this.

Signed-off-by: Michel Hidalgo <[email protected]>

Clean up ROS 2 port

clalancette

I've reviewed this pretty thoroughly in both @sloretz's original incarnation and @hidmic's PR, so I'm comfortable with this now. I'll see about getting this merged and released.

These are also backwards compatible with Eloquent's uncrustify. Signed-off-by: Chris Lalancette <[email protected]>

Only shows up in Foxy, but it is definitely a bug. Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-05-14T18:40:03Z

All right, this is relatively happy on both Eloquent and Foxy now. With blessing from @tfoote , merging this in and doing some releases for Eloquent and Foxy. Thanks to @swatifulzele for the initial work, @sloretz for this port, and @hidmic for the updates.

sloretz requested a review from tfoote April 4, 2020 01:05

sloretz self-assigned this Apr 4, 2020

sloretz force-pushed the sloretz/port_ros2 branch from c457d5e to 6752d63 Compare April 4, 2020 01:09

sloretz changed the base branch from ros2 to noetic-devel April 4, 2020 01:20

sloretz changed the base branch from noetic-devel to ros2 April 4, 2020 01:20

This was referenced Apr 4, 2020

[noetic] Delete unused code #33

Merged

[noetic] deprecate h for hpp #34

Merged

sloretz force-pushed the sloretz/port_ros2 branch from 6752d63 to 49c133a Compare April 7, 2020 15:56

This was referenced Apr 7, 2020

Make filters build for ros2 (on windows 10) #19

Closed

Ros2 devel #24

Closed

clalancette reviewed Apr 7, 2020

View reviewed changes

Add ament_package()

2d0ab62

Signed-off-by: Shane Loretz <[email protected]>

clalancette reviewed Apr 8, 2020

View reviewed changes

sloretz and others added 10 commits April 22, 2020 18:37

Instal -> Install

a7b330c

Co-Authored-By: Chris Lalancette <[email protected]>

Use empty() instead of size() == 0

cd955ad

Co-Authored-By: Chris Lalancette <[email protected]>

remove unnecessary ';'

11ea912

Co-Authored-By: Chris Lalancette <[email protected]>

Include what is used

71a5347

Signed-off-by: Shane Loretz <[email protected]>

Get rid of unnecessary retval

8dba039

Signed-off-by: Shane Loretz <[email protected]>

Alphabetize package deps

6e7d2af

Signed-off-by: Shane Loretz <[email protected]>

Include what is used

7f2b109

Signed-off-by: Shane Loretz <[email protected]>

FilterBase<T> to avoid name hiding

f8550c7

Signed-off-by: Shane Loretz <[email protected]>

Fix wrong include guard

d17e990

Signed-off-by: Shane Loretz <[email protected]>

Misc boost fixes

5056e31

Signed-off-by: Shane Loretz <[email protected]>

hidmic added 4 commits April 29, 2020 16:09

Address pending review comments.

936e4a6

Signed-off-by: Michel Hidalgo <[email protected]>

Lint all sources.

41dfa04

Signed-off-by: Michel Hidalgo <[email protected]>

Cleanup types and literals.

da0545d

Signed-off-by: Michel Hidalgo <[email protected]>

Enforce proper code style.

168d8d4

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic mentioned this pull request Apr 30, 2020

Clean up ROS 2 port sloretz/filters#1

Merged

hidmic and others added 2 commits May 6, 2020 10:23

Address peer review comments.

143d7f1

Signed-off-by: Michel Hidalgo <[email protected]>

Merge pull request #1 from hidmic/hidmic/cleanup_ros2_port

98712b9

Clean up ROS 2 port

clalancette approved these changes May 14, 2020

View reviewed changes

clalancette added 2 commits May 14, 2020 18:28

Fixes for Foxy uncrustify.

4018d57

These are also backwards compatible with Eloquent's uncrustify. Signed-off-by: Chris Lalancette <[email protected]>

Fix a bug in the tests.

f6c8ac2

Only shows up in Foxy, but it is definitely a bug. Signed-off-by: Chris Lalancette <[email protected]>

clalancette merged commit 1b9ffbf into ros:ros2 May 14, 2020

Marwan99 mentioned this pull request Nov 10, 2020

Marking free functions inline #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yet Another filters port to ROS 2 #32

Yet Another filters port to ROS 2 #32

sloretz commented Apr 4, 2020 •

edited

Loading

clalancette left a comment

clalancette left a comment

clalancette Apr 8, 2020

sloretz Apr 23, 2020

clalancette Apr 8, 2020

clalancette Apr 8, 2020

clalancette Apr 8, 2020

clalancette Apr 8, 2020

clalancette commented Apr 23, 2020

hidmic commented Apr 30, 2020 •

edited

Loading

clalancette left a comment

clalancette commented May 14, 2020

Yet Another filters port to ROS 2 #32

Yet Another filters port to ROS 2 #32

Conversation

sloretz commented Apr 4, 2020 • edited Loading

TODO

Differences from #24:

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette Apr 8, 2020

Choose a reason for hiding this comment

sloretz Apr 23, 2020

Choose a reason for hiding this comment

clalancette Apr 8, 2020

Choose a reason for hiding this comment

clalancette Apr 8, 2020

Choose a reason for hiding this comment

clalancette Apr 8, 2020

Choose a reason for hiding this comment

clalancette Apr 8, 2020

Choose a reason for hiding this comment

clalancette commented Apr 23, 2020

hidmic commented Apr 30, 2020 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented May 14, 2020

sloretz commented Apr 4, 2020 •

edited

Loading

hidmic commented Apr 30, 2020 •

edited

Loading