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(default_ad_api_helpers): use polling subscriber #7416

Conversation

zusizusi
Copy link
Contributor

@zusizusi zusizusi commented Jun 10, 2024

Description

The same as #6997 based on the discussion, the polling subscriber is used in the default_ad_api_helpers.

Tests performed

psim test were performed

Effects on system behavior

Nothing but more efficient CPU usage

Interface changes

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.

  • There are no open discussions or they are tracked via tickets.

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

zusizusi added 5 commits June 10, 2024 17:35
Signed-off-by: Sho Iwasawa <sho.iwasawa.2@tier4.jp>
Signed-off-by: Sho Iwasawa <sho.iwasawa.2@tier4.jp>
Signed-off-by: Sho Iwasawa <sho.iwasawa.2@tier4.jp>
Signed-off-by: Sho Iwasawa <sho.iwasawa.2@tier4.jp>
Signed-off-by: Sho Iwasawa <sho.iwasawa.2@tier4.jp>
@zusizusi zusizusi requested review from shmpwk, N-Eiki and shtokuda June 10, 2024 10:30
@github-actions github-actions bot added the component:system System design and integration. (auto-assigned) label Jun 10, 2024
@zusizusi zusizusi changed the title Feat/replace subscriber in default ad api helpers feat(default_ad_api_helpers): use polling subscriber Jun 10, 2024
@shtokuda
Copy link
Contributor

https://github.com/autowarefoundation/autoware.universe/blob/db6da56c616620e34447c3ff2118ee178f1b13de/system/default_ad_api_helpers/ad_api_adaptors/src/routing_adaptor.cpp#L51-L55

IMO
Repeatedly calling the same function such as rclcpp::Time may decrease readability. Storing the result of the function in a variable can simplify the code and make it more understandable for other developers. Additionally, reducing the number of function calls may improve performance.

@zusizusi zusizusi marked this pull request as ready for review June 10, 2024 11:31
const rclcpp::Time current_time = this->get_clock()->now();
auto fixed_goal_msg = sub_fixed_goal_.takeData();
auto rough_goal_msg = sub_rough_goal_.takeData();
auto waypoint_msg = sub_waypoint_.takeData();
Copy link
Contributor

Choose a reason for hiding this comment

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

This change reduces waypoint throughput. Only one waypoint can be added per timer period. Also, information about the order of arrival of waypoints and goals is lost. Waypoints and goal must be processed in the order in which they arrive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for taking the time to review my PR.
I understand your concerns regarding the usage of take().
Therefore, I will close this PR.

@zusizusi zusizusi closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:system System design and integration. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants