Merge this repository back with sick_scan?

[clalancette]

I'd like to discuss merging the code in this repository back into https://github.com/SICKAG/sick_scan . What it looks like happened here was that the code in the original sick_scan was copied to a new repository, then changes were made to port it to ROS 2. While this works, this is problematic for a couple of reasons:

1. This way of doing forks doesn't create any specific link between the original code and this one. Thus, it is hard to know that they are related.
2. Having it in a separate repository means that any updates to the ROS 1 driver will most likely not make it to the ROS 2 port either.

What I'd propose is to instead is to deprecate this repository and have a separate "ros2" branch on https://github.com/SICKAG/sick_scan. We can merge the code that has already been ported here onto that branch, and then continue doing the port incrementally as it makes sense. This fixes the first problem above, and at least makes it slightly more likely that any changes are propagated to both ROS 1 and ROS 2. This is also the workflow that we are recommending for porting ROS 2 packages in general.

Thoughts on this proposal?

[michael1309]

We will discuss this issue with the SICK management next week and give you an update after this discussion.

[clalancette]

We will discuss this issue with the SICK management next week and give you an update after this discussion.

All right, thanks for the update. If it helps, I'm willing to do some of the initial work to make this happen and improve the ROS 2 port (make it more composable, etc).

[clalancette]

I'm just wondering if you were able to discuss this, and how we should move forward.

[michael1309]

We discussed this issue and decided to go into the following direction:

1. Implementing a generic software structure in c++11 (like sick_scan_base but without ros wrapper)
2. ROS2-Support of tim5xx family in general
3. Using results of step 1 and 2 to migrate these results into sick_scan for ROS1 and ROS2.
4. Creating ROS2-implementation as a branch like proposed by you.

This is our plan for Q4/2019 and Q1/2020.

Thanks again for your hint. By doing these steps we appreciate your support and recommendations.

[michael1309]

@clalancette
For a first migration test we tried to start the bloom release process for the ROS2-dashing-release.
During the release run the release process gives messages like:
Failed to resolve dynamic_reconfigure on ubuntu:bionic with: Error running generator: Failed to resolve rosdep key 'dynamic_reconfigure', aborting. dynamic_reconfigure is depended on by these packages: ['sick_scan2'] <== Failed Could not resolve rosdep key 'message_generation'
This seems related to ros-infrastructure/rosdep#660

In the end we ignored the rosdep-warnings. Do you know whether we can solve this behavior?

[clalancette]

Failed to resolve dynamic_reconfigure on ubuntu:bionic with: Error running generator: Failed to resolve rosdep key 'dynamic_reconfigure', aborting. dynamic_reconfigure is depended on by these packages: ['sick_scan2'] <== Failed Could not resolve rosdep key 'message_generation'

The basic problem is that dynamic_reconfigure and message_generation are not actually ROS 2 packages; they are ROS 1 ones. In order to solve this, you need to update the package.xml to depend on the packages you actually depend upon here. I'll open a PR momentarily which has what you need to get started.

[clalancette]

See #5

[michael1309]

Thanks @clalancette for your support!
I integrated your pull request and trigged a new bloom run.

[michael1309]

bloom build completed without any problems. @clalancette thanks again!

[clalancette]

Happy to help.

[michael1309]

Hi @clalancette ,

we tried to build the ROS2 against the build farm.

Unfortunately, the error messages from the build farm are not so meaningful that we can easily isolate the problem. Could you support us here or forward this request? Thank you very much for your support.

Link: http://build.ros2.org/job/Ebin_uB64__sick_scan2__ubuntu_bionic_amd64__binary/23/display/redirect

sick_scan2_errorlog.txt

[clalancette]

See #8 , which fixes a few different issues I found in the CMakeLists.txt. Don't forget that once you merge that, you'll have to do a source release with catkin_prepare_release, a bloom-release, and it has to be merged at https://github.com/ros/rosdistro before it will be retried on the buildfarm.

[michael1309]

Thanks a lot for your hints. We integrated this into our master branch and will retry a bloom-release.

[Kaju-Bubanja]

What is the status on this issue? I don't see a ROS2 branch in the sick_scan repo. Was this idea scraped?

[michael1309]

In general, we think merging is an good idea. But due to resource considerations, this idea is currently on hold. We will close the ticket at this time and then reopen it when this idea is taken up.