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.

Sign up for GitHub

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

Jump to bottom

feat(default_ad_api): change to read topic by polling #7400

Closed
wants to merge 24 commits into from
Closed

feat(default_ad_api): change to read topic by polling #7400

wants to merge 24 commits into from

Conversation

shtokuda
Copy link
Contributor

@shtokuda shtokuda commented Jun 10, 2024 •
edited
Loading

Description

Some callback were replaced with polling subscriber.
I show a list of fixed node in default_ad_api in this PR.

Node Name Changes Made
autoware_state Yes
localization_conversion No
route_conversion No
diagnostics No
fail_safe No
heart_beat No
interface No
localization No
motion Yes
operation_mode No
perception No
planning Yes
routing Yes
vehicle_door No
vehicle_info No
vehicle Yes

Tests performed

The module works as before
I checked published data of following topics on the planning_simulator;

Topic Name Related Node Name
/autoware/state autoware_state
motion
/api/planning/velocity_factors planning
/api/planning/steering_factors
/api/routing/route routing
/api/routing/state
/api/vehicle/status vehicle
/api/vehicle/kinematics

Notes for reviewers

The motion node has two subscribers, which subscribe to the 'is_pause' and 'is_start' topics.
Is it sufficient to update only the on_timer function？

Interface changes

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.

shtokuda added 10 commits June 7, 2024 17:50
@shtokuda
add interface_subscriber.hpp in default_ad_api
0fc6d40 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
use polling subscriber
9364d39 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
Merge branch 'main' into feat/use-polling-subscriber-default_ad_api
d21d391
@shtokuda
remove unused callback function from vehicle in default_ad_api
c00a9ff 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
use polling subscriber in planning of default_ad_api
39b8e4e 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
use polling subscriber in routing of default_ad_api
aa8a63b 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
use polling subscriber in autoware_state of default_ad_api
e58c4a1 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
use polling subscriber in motion of default_ad_api
8325ec1 
Signed-off-by: shtokuda <[email protected]>
@shtokuda
Merge branch 'main' into feat/use-polling-subscriber-default_ad_api
402aee4
@shtokuda
Merge branch 'main' into feat/use-polling-subscriber-default_ad_api
8acb189
@shtokuda shtokuda requested review from zusizusi, shmpwk and N-Eiki June 10, 2024 07:13
@github-actions github-actions bot added the component:system System design and integration. (auto-assigned) label Jun 10, 2024
@shmpwk shmpwk added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 10, 2024
@shtokuda shtokuda marked this pull request as ready for review June 10, 2024 09:47
shmpwk
shmpwk reviewed Jun 10, 2024
View reviewed changes
system/default_ad_api/src/planning.cpp Outdated
Comment on lines 145 to 156
// Set the status if it is unknown.
const auto is_vehicle_stopped = stop_checker_->isVehicleStopped(stop_duration_);
for (auto & factor : velocity.factors) {
if ((factor.status == VelocityFactor::UNKNOWN) && (!std::isnan(factor.distance))) {
const auto is_stopped = is_vehicle_stopped && (factor.distance < stop_distance_);
factor.status = is_stopped ? VelocityFactor::STOPPED : VelocityFactor::APPROACHING;
}
}
}

pub_velocity_factors_->publish(velocity);
pub_steering_factors_->publish(steering);
pub_velocity_factors_->publish(velocity);
pub_steering_factors_->publish(steering);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you intend to execute them if (trajectory_ && kinematic_state_) which originally executed regardless of trajectory_ and kinematic_state_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for any confusion. I misunderstood that published topics in on_timer are related to subscribed topics directly. I will revert to the original implementation.

system/default_ad_api/src/routing.hpp Outdated Show resolved Hide resolved
system/default_ad_api/src/routing.hpp Outdated Show resolved Hide resolved
system/default_ad_api/src/vehicle.hpp Outdated Show resolved Hide resolved
@shmpwk
Copy link
Contributor

shmpwk commented Jun 10, 2024 •
edited
Loading

@shtokuda
By the way, how about using https://github.com/autowarefoundation/autoware.universe/blob/0e6528be2af66380b53956c78e75af246b21099c/common/tier4_autoware_utils/include/tier4_autoware_utils/ros/polling_subscriber.hpp instead of your interface_subscriber?

@shtokuda
Copy link
Contributor Author

Sorry, I didn't explain about create_polling_subscriber. create_polling_subscriber is a wrapper of InterProcessPollingSubscriber to set arguments from component_interface_specs. Here, we use the wrapper to improve readability.

shtokuda and others added 4 commits June 11, 2024 20:32
@shtokuda @shmpwk
update system/default_ad_api/src/routing.hpp
b9adb4c 
Signed-off-by: shtokuda <[email protected]>

Co-authored-by: Shumpei Wakabayashi <[email protected]>
@shtokuda @shmpwk
update system/default_ad_api/src/routing.hpp
36270e9 
Signed-off-by: shtokuda <[email protected]>

Co-authored-by: Shumpei Wakabayashi <[email protected]>
@shtokuda @shmpwk
update system/default_ad_api/src/vehicle.hpp
b56f589 
Signed-off-by: shtokuda <[email protected]>

Co-authored-by: Shumpei Wakabayashi <[email protected]>
@shtokuda
Revert "Merge branch 'feat/use-polling-subscriber-default_ad_api' of …
c2e6e5f 
…github.com:shtokuda/autoware.universe into feat/use-polling-subscriber-default_ad_api"

Signed-off-by: shtokuda <[email protected]>
@shmpwk
Copy link
Contributor

shmpwk commented Jun 11, 2024

The motion node has two subscribers, which subscribe to the 'is_pause' and 'is_start' topics.
Is it sufficient to update only the on_timer function？

I overlooked this 🙏
I agree to take is_pause and is_start in timer callback.

@shmpwk
Copy link
Contributor

shmpwk commented Jun 11, 2024

@isamu-takagi
Could you review this? I wonder we may break the way of default_ad_api.

@isamu-takagi
Copy link
Contributor

@shmpwk There are two subscriber wrappers that conflict. Can you wait while I work on a solution for component_interface_utils? It would also be useful to have an implementation of polling subscription using a free function.

@isamu-takagi isamu-takagi mentioned this pull request Jun 19, 2024
Open
7 tasks
@isamu-takagi
Copy link
Contributor

@shmpwk I created PR without polling subscriber wrapper. Could you review it?
#7588

@isamu-takagi
Copy link
Contributor

Close because #7588 has taken over.

@shtokuda shtokuda deleted the feat/use-polling-subscriber-default_ad_api branch July 19, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shmpwk shmpwk shmpwk left review comments

@zusizusi zusizusi Awaiting requested review from zusizusi

@N-Eiki N-Eiki Awaiting requested review from N-Eiki

@isamu-takagi isamu-takagi Awaiting requested review from isamu-takagi

@mitsudome-r mitsudome-r Awaiting requested review from mitsudome-r

@yukkysaito yukkysaito Awaiting requested review from yukkysaito

Assignees
No one assigned
Labels
component:system System design and integration. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shtokuda @shmpwk @isamu-takagi