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

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?

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.

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?

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?

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).

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?

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.

Any updates on when a review can be expected?

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.

@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.

@clalancette Could you please reopen this pull request? It seems like I am not able to do this on my own.

I had almost forgotten about this pull request as its initial commit has been pushed over a year ago. However, recently, @ahcorde latest comment on ros-perception/image_common#304 reminded me that I had already taken first steps along the now requested way (relying on rclcpp::NodeInterfaces) a while ago. I even commented this on the respective PR, but dropped the unfinished efforts as the templated alternative seemed to be closer to a finalized state...

As the problem of a missing lifecycle supports now additionally reappeared within pointcloud transport, I think it's worth tackling this again. At least the image_common package is built upon message filters, hence, this repository has to migrate first from the template parameter to rclcpp::NodeInterfaces. Thus, I've already started taking a look at the conflicts which accumulated over the year and will push the rebased branch once the pull request is open again (if I am not wrong, GitHub will otherwise not allow to reopen the PR if I force push first).

awesome @authaldo happy to review your new changes

I unfortunately forgot to sign-off the last commit and had to re-add this through a second force push.

The code feels quite bloated given so many deprecation warnings. I already tried to group the constructors and methods into relevant and deprecated ones. However, one could also think about moving the deprecated constructors down below the non-deprecated methods so that anyone taking a look into the code first sees the relevant stuff.

Pulls: #113
Gist: https://gist.githubusercontent.com/ahcorde/628c4a8a38363c55c31ae71bc69690fe/raw/e9c7ed0f4e0b59e8b440b7cc337b0e504b9bcfe1/ros2.repos
BUILD args: --packages-above-and-dependencies message_filters
TEST args: --packages-above message_filters
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15487

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

@ahcorde I'm guessing this can't be backported to Jazzy? Some recent refactoring in Nav2 makes this break support for rolling's branch for jazzy (https://github.com/ros-navigation/navigation2/actions/runs/15833497811/job/44631594864?pr=5288) since we now override all pub/sub/action/client/service with our own Nav2 types. So if using node->create_*, it doesn't give an rclcpp type anymore. If this could be backported to Jazzy, that would let Nav2's main be usable with Jazzy

Edit: I was able to get around it by casting the new node type to a rclcpp_lifecycle node, so its lower priority, but still a nice to have backport

Hmm, the tick-tock deprecation apparently hasn't been done properly. Changing the signatures of the pure-virtual functions breaks descending classes.

Also, with this PR (and after removing the deprecated functions), this package will lose ability to correctly resolve sub-node namespaces (because they are not reflected in any way in NodeInterfaces). (It doesn't handle them correctly even now, but once Node interface is removed, there will be no trace of the sub-namespaces).

Create a sub-node:

rclcpp::Node::SharedPtr subNh = this->create_sub_node("sub");

Compare:

this->fixSub = subNh->create_subscription<sensor_msgs::msg::NavSatFix>("fix", rclcpp::SensorDataQoS(),
[this](const sensor_msgs::msg::NavSatFix& msg) { this->fixCb(msg); });

and

this->magSub = std::make_unique<message_filters::Subscriber<sensor_msgs::msg::MagneticField>>(subNh, "mag", 100);

fixSub will correctly subscribe to /sub/fix, while magSub will counter-intuitively subscribe /mag.

I'm not sure what's the best way to solve this.

Node::create_subscription() calls extend_name_with_sub_namespace() here.

One way would be to keep the Node constructors and un-deprecate them. But then the result would be different based on whether you pass *node or node as argument, which would be another confusing pattern.

It seems to me the more correct way would be adding sub-namespace-related functionality to NodeTopicsInterface or NodeBaseInterface.

What do you think?

I'm sorry, I figured the latter report will be better as a standalone issue, so let's please continue in #227.

Hmm, the tick-tock deprecation apparently hasn't been done properly. Changing the signatures of the pure-virtual functions breaks descending classes.

Sorry to read that there have been issues missed in the review which now cause problems in downstream packages. However, I am not yet fully sure which downstream package you mean. Could you be more specific about that?