-
Notifications
You must be signed in to change notification settings - Fork 90
Create Publisher in RealtimePublisher #469
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
base: master
Are you sure you want to change the base?
Create Publisher in RealtimePublisher #469
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #469 +/- ##
==========================================
- Coverage 85.33% 85.23% -0.10%
==========================================
Files 17 17
Lines 1398 1443 +45
Branches 132 137 +5
==========================================
+ Hits 1193 1230 +37
- Misses 119 122 +3
- Partials 86 91 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
a3bd565 to
fc1f537
Compare
christophfroehlich
left a comment
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.
Have you installed pre-commit? ;)
What would be a usecase for still passing a publisher to the constructor, and not deprecating the old API?
| } | ||
| } | ||
|
|
||
| explicit RealtimePublisher(std::shared_ptr<rclcpp::Node> node, const std::string & topic_name, |
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.
| explicit RealtimePublisher(std::shared_ptr<rclcpp::Node> node, const std::string & topic_name, | |
| template <typename NodeT> | |
| explicit RealtimePublisher(NodeT node, const std::string & topic_name, |
| publisher_= node->create_publisher<MessageT>( | ||
| topic_name, | ||
| qos); |
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.
What about a variadic template to parse these arguments directly. It would be cool?
This PR aims at creating the publisher inside the RealtimePublisher simplifying the creation of RealtimePublisher.
I chose to just add another constructor where you can pass the node and topic as well as qos on which we want to publish. I left the old constructor to give the user the flexibility to create or pass their own publisher if they want to.
If this approach is thought to simple let me know and i can change the implementation in a more desired direction.