-
Notifications
You must be signed in to change notification settings - Fork 299
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
fix(obstacle_cruise_planner): add separate parameters for cruise and stop margins #1089
Conversation
…stop safe margins Signed-off-by: Ahmed Ebrahim <[email protected]>
@@ -12,8 +12,9 @@ | |||
idling_time: 2.0 # idling time to detect front vehicle starting deceleration [s] | |||
min_ego_accel_for_rss: -1.0 # ego's acceleration to calculate RSS distance [m/ss] | |||
min_object_accel_for_rss: -1.0 # front obstacle's acceleration to calculate RSS distance [m/ss] | |||
safe_distance_margin : 6.0 # This is also used as a stop margin [m] | |||
terminal_safe_distance_margin : 3.0 # Stop margin at the goal. This value cannot exceed safe distance margin. [m] | |||
cruise_safe_distance_margin : 0.5 # This is used as a cruise margin [m] |
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.
Due to the discontinuous distance when switching from CRUISE to STOP or STOP to CRUISE, we came across the issue where the velocity is sometimes not comfortable in the experiment.
With the current logic, It would be better to set parameters to the same values by default. Is it okay for you?
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 for your review @takayuki5168 -san.
It would be better to set parameters to the same values by default.
Do you mean by this comment to set the default value of cruise_safe_distance_margin
with 6.0 meters by default ?
if the answer is yes, so if this cruise_safe_distance_margin
a bit bigger value than 0.5, (2.0 for example) that would solve the mentioned problem ?
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.
What I meant is that safe_distance of the STOP and CRUISE should be totally the same (both 6.0) due to the discontinous distance explained above.
2.0 does not solve my problem.
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 again @takayuki5168 -san for your comment.
Actually having the value of cruise_safe_distance_margin
with 6.0 will turn into that the intended Dense Urban ODD scenarios for which we are proposing this change, they will still be failing with main Autoware !!
In addition, what will be then the significance of the parameters separation in obstacle_cruise_planner
package implemented using autoware.universe PR if we are setting both parameters with the same value ?
The purpose of separating these two parameters is having the ability to assign different values for them tackling different driving situation.
If we confirm that the discontinuous distance when switching from CRUISE to STOP or STOP to CRUISE
is leading to some behavior degradation and failing of other scenarios, then it would be better that you share more details about this problem and maybe we can work on adding a solution for it.
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.
@ahmeddesokyebrahim
Sorry, but I don't have a detailed scenario.
Theoretically, during the CRUISE state, the stable target distance is cruise_safe_distance_margin
, and after switching from CRUISE to STOP, the target distance becomes stop_safe_distance_margin
. Therefore, if cruise_safe_distance_margin
is smaller than stop_safe_distance_margin
, the ego cannot keep the distance stop_safe_distance_margin
when the ego stops.
That's why these distances have to be the same.
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 @takayuki5168 -san for your reply.
The development of this PR will be on-hold until further changes to be done in the scenario itself.
Afterwords, we can re-test again and check if we still need the parameters separation or not, and based on that we can check the discontinuity problem.
Closing this PR as the intended issue and scenario/s will be covered by this PR |
Description
Closes autowarefoundation/autoware.universe#7922
Related links
autowarefoundation/autoware.universe#7485
Tests performed
Using scenario simulation
❗ Note ❗
In this previous scenario, the safe distance is updated to be 12.5 m following the 3-seconds inter-vehicle time-distance rule in performance requirements here
Before this PR:
2024-07-23.15-28-31.mp4
After this PR:
2024-07-23.15-02-16.mp4
Notes for reviewers
This PR is used by an autoware.universe PR
Interface changes
ROS Topic Changes
N.A.
ROS Parameter Changes
common.cruise_safe_distance_method
common.stop_safe_distance_method
safe_distance_method
for stop purposescommon.terminal_stop_safe_distance_margin
terminal_safe_distance_margin
Effects on system behavior
Improving
obstacle_cruise_planner
behavior in cruising scenarios, mitigating any mitigate any unnecessary long safe distance ego and npc and satisfying performance requirementsPre-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.