-
Notifications
You must be signed in to change notification settings - Fork 668
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(pid_longitudinal_controller): use Integrated error when vehicle is stopped #5549
feat(pid_longitudinal_controller): use Integrated error when vehicle is stopped #5549
Conversation
44872a0
to
c9efeba
Compare
c9efeba
to
48dfaca
Compare
LGTM. Horibe-san will review as well. |
@danielsanchezaran Thank you for the PR! I agree with enabling integration during stops. However, there are several considerations we need to address. Even in AUTONOMOUS mode, it takes time for a vehicle to start moving. Therefore, the integral term will inevitably accumulate unintentionally during the start. This could have a bad effect on the system, making parameter tuning difficult. When stopped, I think we need a method such as enabling integration only if the vehicle doesn't start moving after a certain period (e.g. 2 seconds). Additionally, vehicle speed sensors often perform poorly at low speeds, frequently returning zero speed. To avoid unintended integral processing in such vehicles, it remains necessary to have the option to disable integration at low speeds. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5549 +/- ##
===========================================
+ Coverage 15.32% 41.15% +25.83%
===========================================
Files 1721 18 -1703
Lines 118559 1572 -116987
Branches 37995 851 -37144
===========================================
- Hits 18169 647 -17522
+ Misses 79657 248 -79409
+ Partials 20733 677 -20056
☔ View full report in Codecov by Sentry. |
9a88100
to
ec6b1a0
Compare
Signed-off-by: Daniel Sanchez <[email protected]>
Not sure why the issue was closed, I did not do it. |
Signed-off-by: Daniel Sanchez <[email protected]>
Signed-off-by: Daniel Sanchez <[email protected]>
With these new changes, the error integration does not kick off until a threshold time limit is surpassed Test: Modified the slope gravity so the vehicle can ONLY move if the Integrative part of the PID kicks in. In this test, the time threshold is set at 5 secs. cap-.2023-11-21-15-03-23.mp4There is a parameter to set the threshold limit and a parameter to enable or disable the feature altogether @TakaHoribe @takayuki5168 Note: the changes to autoware_launch are necessary: autowarefoundation/autoware_launch#698 |
I believe the new changes satisfy these requirements. Could you please take a look again? @TakaHoribe |
Signed-off-by: Daniel Sanchez <[email protected]>
I leave this PR's review to @TakaHoribe -san. |
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.
@danielsanchezaran LGTM. Two minor comments:
- Would you write a background of this feature on the README? Maybe new users don't know why the time_threshold is necessary.
- One suggestion below
control/pid_longitudinal_controller/src/pid_longitudinal_controller.cpp
Outdated
Show resolved
Hide resolved
…oller.cpp Co-authored-by: Takamasa Horibe <[email protected]> Signed-off-by: Daniel Sanchez <[email protected]>
f02b710
to
dc4b351
Compare
Signed-off-by: Daniel Sanchez <[email protected]>
Note: | ||
|
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.
?? Should it be removed?
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! (but one odd part in README?)
Signed-off-by: Daniel Sanchez <[email protected]>
0496b19
into
autowarefoundation:main
…is stopped (autowarefoundation#5549) * Add time threshold to activate error integration Signed-off-by: Daniel Sanchez <[email protected]> * add pre-commit changes Signed-off-by: Daniel Sanchez <[email protected]> * add param to enable or disable low speed error integration Signed-off-by: Daniel Sanchez <[email protected]> * eliminate unused include Signed-off-by: Daniel Sanchez <[email protected]> * Update control/pid_longitudinal_controller/src/pid_longitudinal_controller.cpp Co-authored-by: Takamasa Horibe <[email protected]> Signed-off-by: Daniel Sanchez <[email protected]> * Update README and formatting Signed-off-by: Daniel Sanchez <[email protected]> * delete extra NOTE comment Signed-off-by: Daniel Sanchez <[email protected]> --------- Signed-off-by: Daniel Sanchez <[email protected]> Signed-off-by: Daniel Sanchez <[email protected]> Co-authored-by: Takamasa Horibe <[email protected]>
Description
Related to this Issue
The goal is to make the PID use the integrative gain to offset possible slope compensation overshoots. The reason this is not currently done is shown on the documentation:
"Also, the integral term is not accumulated when the vehicle is stopped. This is to prevent unintended accumulation of the integral term in cases such as Autoware assumes that the vehicle is engaged, but an external system has locked the vehicle to start."
But I believe that, by tuning the I effort parameters:
The maximum acceleration caused by the I controller can be tuned to be confortable & safe. So, the velocity limit is not necessary.
Related links
Tests performed
evaluator tests: TIER4 internal link No degradation when compared to today’s baseline TIER4 internal link
Notes for reviewers
Notes for reviewers
Requires launch changes: autowarefoundation/autoware_launch#698
Interface changes
Effects on system behavior
The use of the integrative gain is no longer limited by the vehicle speed for the PID controller, and the ego can now start on downward slopes.
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.