-
Notifications
You must be signed in to change notification settings - Fork 682
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(mission_planner): add the interface to reroute while driving #3184
feat(mission_planner): add the interface to reroute while driving #3184
Conversation
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #3184 +/- ##
==========================================
- Coverage 12.31% 12.31% -0.01%
==========================================
Files 1340 1340
Lines 93513 93526 +13
Branches 26777 26777
==========================================
Hits 11519 11519
- Misses 69832 69845 +13
Partials 12162 12162
*This pull request uses carry forward flags. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
Signed-off-by: Takagi, Isamu <isamu.takagi@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.
thanks! LGTM
@yabuta @tkhmy @kenji-miyake |
struct ChangeRoute | ||
{ | ||
using Service = autoware_planning_msgs::srv::SetRoute; | ||
static constexpr char name[] = "/planning/mission_planning/change_route"; |
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.
@isamu-takagi cc @mitsudome-r @yukkysaito
Could you please clarify why we need the change
APIs in addition to the existing set
APIs? If it's already documented, just writing the link is fine!
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.
This PR was created for MRM pull over (#3221). Since the route change by MRM pull over is done while driving, it is necessary to keep the route for some distance. The change route
interface diverts this so that it can be used in normal operations as well. Therefore, the interface is separated to call a different process than the set route
.
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.
Thank you, I understand.
But I feel it's a little different from what I guess from the word reroute
.
Also, I think it can be completed within the Planning component, so I'm not sure whether we need to add new component interfaces for this.
I'll wait for comments from other architects.
(I forgot to mention @xmfcx, sorry.)
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.
@kenji-miyake I made a block diagram. This is the interface between ADAPI and planning.
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.
@isamu-takagi Thank you, but when and why do users call this change
API? 🤔
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.
@kenji-miyake For example, when users want to change the goal while driving, or when splitting a route to create a circular route.
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.
Hmm, I'm sorry I don't understand the use cases well... 🥺
I'll leave the review to @mitsudome-r and @yukkysaito.
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
struct SetMrm | ||
{ | ||
using Service = autoware_planning_msgs::srv::SetPoseWithUuidStamped; | ||
static constexpr char name[] = "~/srv/set_mrm"; |
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.
MRM is abbreviation of "minimum risk maneuver" so I prefer the name to be "~/srv/set_mrm_goal" or "~/srv/set_emergency_goal"
Description
Add interfaces for rerouting while driving autowarefoundation/autoware#3406.
Related links
#3221
TIER IV INTERNAL LINK.
Tests performed
Tested that the vehicle can reach the road shoulder goal in planning simulator.
Notes for reviewers
Need following PRs to build.
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.