-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More generic subscriber implementation using NodeInterfaces from rclcpp #113
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Dominik Authaler <[email protected]>
Depending on what the maintainers prefer, we could potentially add #98 to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
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 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? |
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 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). |
Signed-off-by: Dominik Authaler <[email protected]>
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 thought about using If none of them works, it the deprecation via a compiler warning strictly necessary?
|
…I compatibility Signed-off-by: Dominik Authaler <[email protected]>
f3b8967
to
0a98ee9
Compare
Co-authored-by: Jonas Otto <[email protected]> Signed-off-by: Dominik Authaler <[email protected]>
8f2f66d
to
d7cff5b
Compare
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 (?). |
Signed-off-by: Dominik Authaler <[email protected]>
Any updates on when a review can be expected? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly ping @clalancette
class SubscriberBase | ||
{ | ||
public: | ||
typedef std::shared_ptr<NodeType> NodePtr; | ||
[[deprecated]] typedef std::shared_ptr<NodeType> NodePtr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include <memory>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind to fix the conflicts?
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.