-
Notifications
You must be signed in to change notification settings - Fork 696
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
feat(aeb): replace InterProcessPollingSubscriber #6997
feat(aeb): replace InterProcessPollingSubscriber #6997
Conversation
9b5eb8e
to
ed1569a
Compare
bool is_latest_message_consumed = false; | ||
// pop the queue until latest data | ||
while (subscriber_->take(tmp, message_info)) { | ||
is_latest_message_consumed = true; | ||
} | ||
if (is_latest_message_consumed) { |
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.
Any reason not to update data_ inside the while loop?
RCLCPP_WARN( | ||
node->get_logger(), | ||
"InterProcessPollingSubscriber will be used with depth > 1, which may cause inefficient " | ||
"serialization while updateLatestData()"); | ||
} |
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 think this warning is not necessary since now the InterProcessPollingSubscriber can handle the topic whose depth is not 1.
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.
The user have the freedom to use this subscriber with depth > 1, but eventually he/she will not use the stacked obsolete data because getData
returns only the latest data.
The purpose of this subscriber is to minimize the repeated serialization overheads in subscriber_->take(tmp, message_info)
, so if this warning appears then he's not using this as expected. This warning is printed only once in the constructor.
d9518d5
to
8c70042
Compare
8c70042
to
1977b31
Compare
Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
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.
LGTM
…undation#6997)" This reverts commit 74da4c1.
…6997) Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
…ndation#6997)" This reverts commit 74da4c1.
…ndation#6997)" This reverts commit 74da4c1.
…ndation#6997)" This reverts commit 74da4c1.
…ndation#6997)" This reverts commit 74da4c1.
Description
replace the subscriber to InterProcessPollingSubscriber
Related links
depends #6976
https://tier4.atlassian.net/browse/RT1-5790
Tests performed
the module works as before
before
PR6997-2024-05-14_14.43.23.mp4
after
PR6997-2024-05-14_14.39.04.mp4
Notes for reviewers
Interface changes
none
ROS Topic Changes
ROS Parameter Changes
Effects on system behavior
none
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.