-
Notifications
You must be signed in to change notification settings - Fork 77
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
First step towards modernizing the rt publisher #210
First step towards modernizing the rt publisher #210
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #210 +/- ##
==========================================
+ Coverage 72.72% 73.16% +0.43%
==========================================
Files 7 7
Lines 429 436 +7
Branches 71 72 +1
==========================================
+ Hits 312 319 +7
Misses 73 73
Partials 44 44
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Makes sense to me
@christophfroehlich anything missing in order to get this merged? |
another review ;) |
And I just added a test for the tryPublish method :) @saikishor would you mind reviewing this PR ? |
I'll check it during the day. I have some pending review already |
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.
In general LGTM!
I left some minor comments
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
(cherry picked from commit e58c7ad)
I think the rt publisher is one of the most commonly used tools in this package.
When ever I was using it I stumbled upon its "weird" API.
This PR is a first step towards modernizing the RealtimePublisher class, providing a tiny bit of comfort by adding a
tryPublish
method and providing some typedefs similar to the rclcpp::Publisher.I plan to add a follow up PR where I refactor the NON_POLLING define into a template parameter.