More generic subscriber implementation using NodeInterfaces from rclcpp

[authaldo]

Fixes #112

To be more precise, these changes allow using message_filters::Subscriber in situations, where no full rclcpp::Node / rclcpp_lifecycle::LifecycleNode is available by requiring only the relevant interfaces.

[wodtko]

Depending on what the maintainers prefer, we could potentially add #98 to this PR.
Otherwise, a separate PR would have to be created.

[clalancette]

I haven't looked at this in much detail, but from a quick skim the big problem with this is the breakage of the API. We very much try to not break API, and if we have to, we always have a deprecation cycle to do it. So this needs to be reworked to keep the previous API, while adding in the new one.

[authaldo]

Thanks for the quick feedback. Regarding the API protection: How far does that go? Just adding the overloads of the subscribe functions I dropped in the base class, or even keeping the template parameter of the subscriber class although it is now completely unused?

[clalancette]

Thanks for the quick feedback. Regarding the API protection: How far does that go? Just adding the overloads of the subscribe functions I dropped in the base class, or even keeping the template parameter of the subscriber class although it is now completely unused?

I could be wrong, but I think we still need to keep the template parameter so we don't break the API of subscribe.

[wodtko]

Since the template parameter is required not only by the function but also by the whole class, the API might break as soon as there is no template parameter for SubscriberBase and Subscriber, respectively...

Adding a default template argument, which would allow to use the class and call functions without having to specify the obsolete Node Type, seems a little odd, doesn't it?
Is there another, perhaps cleaner way of getting rid of a template argument without an API break?

[authaldo]

Readding the NodeType Parameter with an unused default should be pretty straight forward. This should then also allow reverting the changes to the unit tests, thus, at least the tested part of the API should not be broken.

However, how would you deprecate the template parameter? I am aware of [[deprecated]], but as far as I can tell there is now way to apply it to template parameters. But I could be wrong on this.

Out of curiosity: How long is a deprecation cycle intended to be? A full LTS release?

[clalancette]

Out of curiosity: How long is a deprecation cycle intended to be? A full LTS release?

One release. For instance, if we want to get this in now, we can deprecate it for Jazzy, and then remove it for K-Turtle (which would be released in May 2025).

[authaldo]

I added another commit which aims at ensuring compatibility with the old API. This compatibility goes far enough to run all unit tests without any changes.

However, there are a few things which are still open:
I have no clue on how to issue a nice deprecation warning regarding the second template parameter. The code contains three TODOs within if constexpr blocks where a warning similar to a static_assert would be nice in my opinion. I assume that this is not the first time that a template parameter has to be deprecated within the ROS code base. Maybe you have some experience on how to solve this best?

I thought about using #warning, but it is likely compiler dependent (at least gcc warns me that it is an extension?). Furthermore, it seems to issue the warning even though the subscriber is not instantiated with the second template parameter. Other suggestions on stack overflow include a lot of boiler plate code, which seems ugly to me. Hence, I would be interested in your suggestions.

If none of them works, it the deprecation via a compiler warning strictly necessary?

Another open aspect:
Some of the functions within the SubscriberBase class cannot be readded since this would require pure virtual templated member functions (which C++ does not allow). I assume this is still critical regarding the API protection?

[authaldo]

I added another commit implementing the deprecation warnings for the second template parameter (thanks @ottojo for the suggestion!). It seems like these warnings are also the reason why the CI fails (?).
In my view the PR should now fulfill the API protection criteria discussed above, so I'll request another review.

[authaldo]

Any updates on when a review can be expected?

[ahcorde]

Friendly ping @clalancette

[ahcorde]

include <memory>

[ahcorde]

Do you mind to fix the conflicts?

[authaldo]

Sorry, but I currently do not have any spare time for working on this and it looks like this won't change in the near future...
However, if anyone else wants to tackle the conflicts please feel free to hijack this MR 😄

While I would still appreciate using the node interfaces class within message filters, I am also fine with closing this PR in order to keep the pull request overview clean (although the message filter repository has a rather small number of open PRs compared to other ROS repos).
With image_common - PR 304 the main motivation for this PR, namely the handling of lifecycle nodes, is solved. Hence, there is no eminent need for further pursuing this other than cleaner code.

[clalancette]

@authaldo Thanks for the response. Given that you don't have time to work on this in the near future, I'm going to go ahead and close it. If you do find you want to continue on this in the future, please feel free to reopen and we can continue.