Skip to content
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

Merged
merged 2 commits into from
May 17, 2024

Conversation

soblin
Copy link
Contributor

@soblin soblin commented May 13, 2024

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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added component:control Vehicle control algorithms and mechanisms. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels May 13, 2024
@soblin soblin force-pushed the feat/aeb-use-no-exec-callback branch 3 times, most recently from 9b5eb8e to ed1569a Compare May 14, 2024 04:54
@soblin soblin marked this pull request as ready for review May 14, 2024 05:53
Comment on lines 57 to 62
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) {
Copy link
Contributor

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?

Comment on lines 46 to 51
RCLCPP_WARN(
node->get_logger(),
"InterProcessPollingSubscriber will be used with depth > 1, which may cause inefficient "
"serialization while updateLatestData()");
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@soblin soblin force-pushed the feat/aeb-use-no-exec-callback branch from d9518d5 to 8c70042 Compare May 16, 2024 09:43
@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label May 16, 2024
Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
@soblin soblin force-pushed the feat/aeb-use-no-exec-callback branch from 8c70042 to 1977b31 Compare May 16, 2024 22:02
@github-actions github-actions bot removed component:planning Route planning, decision-making, and navigation. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels May 16, 2024
Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
@soblin soblin added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label May 17, 2024
@soblin soblin enabled auto-merge (squash) May 17, 2024 01:55
Copy link
Contributor

@tkimura4 tkimura4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@soblin soblin merged commit 74da4c1 into autowarefoundation:main May 17, 2024
29 of 33 checks passed
@soblin soblin deleted the feat/aeb-use-no-exec-callback branch May 17, 2024 10:29
mitsudome-r added a commit to mitsudome-r/autoware.universe that referenced this pull request May 27, 2024
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…6997)

Signed-off-by: Mamoru Sobue <mamoru.sobue@tier4.jp>
beyzanurkaya pushed a commit to beyzanurkaya/autoware.universe that referenced this pull request Jun 4, 2024
beyzanurkaya pushed a commit to beyzanurkaya/autoware.universe that referenced this pull request Jun 4, 2024
beyzanurkaya pushed a commit to beyzanurkaya/autoware.universe that referenced this pull request Jun 5, 2024
beyzanurkaya pushed a commit to beyzanurkaya/autoware.universe that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:control Vehicle control algorithms and mechanisms. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants