-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add lifecycle node compatibility to camera_info_manager #190
Add lifecycle node compatibility to camera_info_manager #190
Conversation
1ec6666
to
efac43b
Compare
Hey, can I get some feedback about this PR? |
node->get_node_services_interface(), node->get_node_logging_interface(), cname, url) | ||
{ | ||
} | ||
|
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.
Could you also add an overloaded constructor that takes in the rclcpp::LifecycleNode*
that calls the constructor in L88?
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've add the constructor. To do this, I had to add a dependency on rclcpp_lifecycle
8524ee9
to
146901f
Compare
@mjcarroll do you have time to take a look at this? |
dd188be
to
3097069
Compare
I've resolved the merge conflict |
Please fix the build errors and update your branch. When you do that, I will run CI again. Most likely your branch is out of date. |
3097069
to
ce01db1
Compare
We still have some build failures caused by this PR. Please take a look and fix the errors. |
Sorry that it's taking a bit of time, at my work we are still on ROS1 so I have to make some dedicated time free to figure out why it's not building. |
Seems to be fixed now |
CI looks good! Thanks for the work. |
This PR is similar to #167. It adds lifecycle node support only to the
camera_info_manager
. It is backwards compatible.