feature: components support

[jcarlosgm30]

Hello,

I think that it can be very interesting to transform the "filter_chains" into ROS2 components. This is crucial to facilitate the creation of more flexible and efficient laser pipelines.

This pull request fits this feature.

Key Changes:

• Componentization of filter_chains: Transforming "filter_chains" into ROS2 components greatly enhances the flexibility and efficiency in configuring laser pipelines.

• Code Restructuring: Classes have been reorganized into private headers and their corresponding source files.

• Executable Updates: Executables have been modified to directly run the relevant node, simplifying the deployment and execution process.

• Launch Files Update: Example launch files have been updated to reflect these changes.

Benefits:

• Improved Flexibility: The ability to easily configure and modify laser pipelines is important in several contexts.

• Increased Efficiency: The new component structure facilitates more efficient execution, crucial in high-performance robotics environments.

Functionality Status:

• Preservation of Existing Features: No changes have been made to the functionality beyond the mentioned restructuring, maintaining the system's integrity and performance.

I eagerly await feedback and am willing to make any necessary adaptations to align even more closely with the project's needs and expectations. These changes are designed to not only improve the current development experience but also to open up new possibilities for future enhancements and features.

[jonbinney]

Wow, this is cool! Thanks @jcarlosgm30 for implementing it! This seems like a good change, but since it is a big change it'll take some time to understand, review and test, especially to ensure backwards compatibility.

A few questions:

• how backwards compatible is this? Will people who depend on laser_filters be able to use their existing launchfiles and yaml files without modification?
• could you give some more detail about how this is more efficient? and more flexible? than the current filters implementation?
• have you been using this on your own robots? How much testing has it seen so far?

[jcarlosgm30]

Hi @jonbinney,

Thank you for the reply! I will try to answer the questions as best I can:

how backwards compatible is this? Will people who depend on laser_filters be able to use their existing launchfiles and yaml files without modification? Currently, it is backward compatible with all its functionalities, except for the name of the executables. This is an item of discussion, as I have added *_node to the executables to maintain a naming convention concerning the libraries that generate the component. To maintain full backwards compatibility, we can keep the same name in the executables as before (without *_node) and assign the name you deem appropriate to the libraries.

could you give some more detail about how this is more efficient? and more flexible? than the current filters implementation? With composition, it is possible to:

• run multiple nodes in separate processes with the benefits of process/fault isolation as well as easier debugging of individual nodes
• run multiple nodes in a single process with the lower overhead and optionally more efficient communication through intra-process communication.

Therefore, this allows us to create pipelines (e.g. driver+filters+other functionalities) that run in the same process improving efficiency and overhead. Or run the system, as it has been done so far, on a separate node.

have you been using this on your own robots? How much testing has it seen so far? The changes have been tested on simulation and also on a TIAGo robot from PAL Robotics. The system has been running while navigating the robot for one hour, without encountering any problems. But, I encourage you to test it anyway.

If you have any further questions or I can do anything to further align the solution with the project, please let me know

[jonbinney]

When I initially read the title of this PR, I thought it was turning each individual filter into a ros2 component.... but now I see that it is much simpler than that. The big scary diff is due to a few things:

1. Adding header files and moving class declarations into them
2. Adding a laser_filters namespace
3. Appending _node the filter chain executable names

(1) and (2) are good changes overall, but usually I'd like to have them in a separate PR that doesn't change any functionality. IMixing them into a PR that adds components makes it very hard to see the actually important changes. Also they make it harder to look back through git history, so in general I tend to leave it as is. (3) breaks all launchfiles not hosted in this repo, so we'd need a very strong reason to allow it, and do at least one ROS distro with a deprecation notice where we have both executables, etc. etc.

@jcarlosgm30 can you undo the changes I describe in (1), (2), and (3) and just leave the changes that makes the filter chain nodes into components? I think that should be easier to review and merge. Or is there something about these other changes that is necessary?

[jcarlosgm30]

Hi @jonbinney ,

The changes (2) and (3) are neither necessary nor significant. However, in my opinion, change (1) from a functionality and expressiveness point of view is recommendable. The component has to be compiled as a shared library, and we want to keep backward compatibility by having an executable node for each filter chain.

So in this case, from a software point of view, it makes sense to keep the library separate from the executable. The library can be loaded by the component container that needs it, while the executable simply makes use of the library.

[jonbinney]

The component has to be compiled as a shared library, and we want to keep backward compatibility by having an executable node for each filter chain.

Yes we want the executable - but that doesn't require doing (1), (2), or (3), right? Or am i misunderstanding?

[jcarlosgm30]

Yes, it doesn't require doing (1), (2) or (3). But (1) is highly recommended from a functionality and expressiveness point of view. Otherwise, we must compile the same .cpp as a shared library and an executable. This is something I don't like because it violates many of the best practices in software development.

[jonbinney]

Adding new functionality and doing code reorganization in one commit also violates many of the best practices of software development :-)

Could you undo the renaming of the nodes and undo the adding of the laser_filters namespace, as well as split the file reorganization and adding of the components into two commits so that I can diff them separately? Then I'll do a detailed review to make sure everything looks good. Thanks @jcarlosgm30 !

[jcarlosgm30]

Hi @jonbinney! Sorry for taking so long to reply, I've been away for a while.

Adding new functionality and doing code reorganization in one commit also violates many of the best practices of software development :-)

You are absolutely right! putting all the code in a commit is also a bad practice

Could you undo the renaming of the nodes and undo the adding of the laser_filters namespace, as well as split the file reorganization and adding of the components into two commits so that I can diff them separately?

Sure! once I get this done, let me know anything I can help with!

Thanks!

[jonbinney]

Looks like you update this PR - thanks for working on this! Is it ready for me to take another look?

[jcarlosgm30]

@jonbinney Yes, I undid the renaming of the nodes and the laser_filters namespace. Also, I've split the file reorganization and the components into two different commits

[jonbinney]

Looks good. @jcarlosgm30 thanks for this PR!