Question: port ros1 functionality to ros2

[reinzor]

Hi,

We are in the process of migrating our ros1 system to ros2. We are currently using the ros1 driver that holds some additional diagnostics features compared to this ros2 driver. We would like to port these features to the ros2 repo, e.g.:

• feat: Add firmware version to diagnostics sick_safetyscanners#128
• feat: Add checksums to diagnostics sick_safetyscanners#126
• feat(diagnostics): Summary for application error, contamination error and contamination warning sick_safetyscanners#125
• feat(diagnostics): Sensor state diagnostics sick_safetyscanners#50
• feat: diagnosed publisher sick_safetyscanners#30
• etc.

What would be the best way for us to address this? I also see that this repo contains quite some duplicated code:

• https://github.com/SICKAG/sick_safetyscanners2/blob/master/src/SickSafetyscannersLifeCycle.cpp
• and https://github.com/SICKAG/sick_safetyscanners2/blob/master/src/SickSafetyscannersRos2.cpp

Should we target both implementations or maybe do a refactor first?

What do you think?

Thanks!

[lenpuc]

Hi,

if you are willing to do a refactor first, i would be happy to have a look at that PR.
And yes i know that are currently two implementations with a lot of similar code.

Afterwards, feel free to port your needed feature, I am happy to integrate them once again in the ROS2 version

[lenpuc]

#26 this PR might be relevant for this

[reinzor]

Great; we will look into it!

[reinzor]

Due to holidays we did not have that much develop cycles this month. Just wanted to let you know that we will start working on this in the coming weeks.

[reinzor]

Hi @puck-fzi ; when thinking about this a bit more. We think it would be an easier process to create ros2 branch in the https://github.com/SICKAG/sick_safetyscanners repository and do the ros2 migration based on the current main branch. Of course we could additionally implement the lifecycle node afterwards. What do you think?

This way the two repositories would be more aligned.

[lenpuc]

Dear Reinzor, long term, it is planned to have the node separation of the CPP core lib and the driver on top of it. So, I would instead integrate the functionality into this lib here rather than in the ROS1 lib as a separate branch. Long-term all features of the ROS1 node should be included in the ROS2 driver.

The CPP Lib already contains additional features, which are not in the ROS1 driver included anymore.

[reinzor]

OK, clear. Then we will take this repository as a base and start with a refactor that eliminates the duplicated code. After that, we can start porting ROS1 features to ROS2.

[reinzor]

Hi @puck-fzi ; I am somewhat stuck in the refactor of combining the lifecycle and normal node since rclcpp::Node and rclcpp_lifecycle::LifecycleNode have different implementations. Also see ros2/rclcpp#898

The problem is that I cannot properly combine two implementations when I am going to add diagnostics, declare parameters, diagnostics etc. We would like to add a diagnostic updater and a diagnosed publisher.

What do you think what is the best plan here? We internally have no need for the lifecycle nodes and adding the ros1 features to the normal node would be sufficient. However, this results in both node implemenations going out of sync. Any ideas, suggestions appreciated.

[reinzor]

@puck-fzi any thoughts?

[lenpuc]

Just off the top of my head, it might be worth refactoring the functionality into a separate helper class, which is then used by both the lifecycle and the normal nodes. Therefore, the code duplication is reduced.

Then maybe both nodes can have their separate interface and just use the same helper classes, then you could add to the normal node your desired diagnostics. Then the code would not be that out of sync and just slightly different functionality in each node.
I think there are cases where either node might be beneficial.

[lenpuc]

PR #33 merged