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

Make message_filters build for ros2 (on windows 10) #1301

Closed
wants to merge 2 commits into from

Conversation

bfjelds
Copy link

@bfjelds bfjelds commented Jan 9, 2018

Should go to a ROS2 branch, but I'm not sure how to create a PR for that :)

Working towards enabling laser_filters in ROS2. This provides ability to build and updates for ROS2 constructs (like Node vs NodeHandle).

@dirk-thomas
Copy link
Member

Thank you for starting this PR.

The patch depends on ros/roscpp_core#72 and I added a comment to that ticket since it is unclear to me how the "end goal" is supposed to look like. The generated C++ code for message is very different in ROS 1 and ROS 2. So e.g. the message traits from ROS 1 don't apply as-is in ROS 2.

Independent of that aspect the question arises where a potential ROS 2 version of message_filters should reside. The first thing which stands out is that only very few packages in this repository can be considered for being migrated / ported to ROS 2. Many packages are ROS 1 specific and have different equivalents in the ROS 2 domain.

Therefore I don't see a branch for ros2 or a forked repository for ROS 2 as suitable. Almost everything else must be removed from that branch / fork which makes synchronization more effort. Also the versioning of the package on the ROS 1 specific branches is subject to very different constraint than the ROS 2 branch.While this can be addressed by spacing out the version numbers "significantly" it is another indicator that this is a bit odd.

Imo the cleaner solution would be to split out message_filters from this repository (which contains too many packages in the first place). Than that repo can either contain a ros2 branch or be forked (usually we prefer the first option). The "only" downside with this approach is that it require quite some work to separate message_filters from this repo (preserve history, potentially update the branches for older ROS 1 distros to stay "in sync", the metapackage needs to be moved too in order to prevent a circular dependency between this repo and the newly created one, etc.). It also needs to be consider how this affects all users which have a working copy and/or fork of this repository.

I as the maintainer of this repo simply don't have the bandwidth to do all of this with the necessary thoroughness. I would welcome any contribution into this direction though.

@wjwwood FYI since you mentioned that rviz2 would like to use this too.

@dirk-thomas
Copy link
Member

@bfjelds While I think separating the package is the better long term direction I can create a ros2 branch for now. Please comment here and on the roscpp_core PR on how you would like to proceed.

@bfjelds
Copy link
Author

bfjelds commented Mar 20, 2018

My end goal was really to get laser_filters working, so I generally just chipped away at what needed porting without too much thought towards refactoring or bigger picture. I agree that refactoring is definitely important as rocpp* and ros_comm seem to be out of place in the ros2 world.

My general take would be to put this into a ros2 branch and if it is useful when the proper solution is being contemplated, great (if not, no big deal).

@testkit
Copy link

testkit commented Jul 9, 2018

Thanks for @mikaelarguedas 's reference.
@dirk-thomas, there was a discussion in discourse https://discourse.ros.org/t/syncronized-callbacks-in-ros2/2015/21 early of this year. Based on that discussion, we decided to have a separate ros2 message_filters repository and managed in github.com/intel/ organization at that time. Currently, we'd like to check if it's possible to create a repository ros2/message_filters to get this project cleaned with proper history commits and license per request in ros2/rosdistro#220.

@dirk-thomas
Copy link
Member

A new repository https://github.com/ros2/message_filters has been created which contains only the message_filters package (including its history up to 1.14.2 from the melodic-devel branch, filtered using this approach).

I will create a follow up ticket on the new repo for how to contribute the various changes applied in the repo under the intel org unit.

@dirk-thomas dirk-thomas closed this Jul 9, 2018
dirk-thomas added a commit to ros2/message_filters that referenced this pull request Jul 9, 2018
@dirk-thomas
Copy link
Member

I have committed the majority of the changes from this PR to the following branch: https://github.com/ros2/message_filters/tree/upstream1301

This will likely conflict with the changes form the intel repo. Also the patch seems to be incomplete - the tests have not been converted as well as the docblocks for various functions, etc.

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.

None yet

4 participants