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(pid_longitudinal_controller): use Integrated error when vehicle is stopped #5549

Conversation

danielsanchezaran
Copy link
Contributor

@danielsanchezaran danielsanchezaran commented Nov 10, 2023

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:

max_i_effort: 0.3
min_i_effort: -0.3

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.

  • 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.

@github-actions github-actions bot added the component:control Vehicle control algorithms and mechanisms. (auto-assigned) label Nov 10, 2023
@danielsanchezaran danielsanchezaran changed the title feat(pid_longitudinal_controller): Take out velocity requirement to always use integrated error in PID feat(pid_longitudinal_controller): Use Integrated error when vehicle is stopped Nov 10, 2023
@danielsanchezaran danielsanchezaran changed the title feat(pid_longitudinal_controller): Use Integrated error when vehicle is stopped feat(pid_longitudinal_controller): use Integrated error when vehicle is stopped Nov 10, 2023
@danielsanchezaran danielsanchezaran force-pushed the feature/error-integration-on-slope-take-off branch 3 times, most recently from 44872a0 to c9efeba Compare November 17, 2023 05:49
@danielsanchezaran danielsanchezaran force-pushed the feature/error-integration-on-slope-take-off branch from c9efeba to 48dfaca Compare November 20, 2023 06:48
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Nov 20, 2023
@danielsanchezaran danielsanchezaran marked this pull request as ready for review November 20, 2023 06:52
@danielsanchezaran danielsanchezaran added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Nov 20, 2023
@takayuki5168
Copy link
Contributor

LGTM. Horibe-san will review as well.

@TakaHoribe
Copy link
Contributor

@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.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (765a596) 15.32% compared to head (9c0e169) 41.15%.
Report is 71 commits behind head on main.

Files Patch % Lines
...nal_controller/src/pid_longitudinal_controller.cpp 20.00% 4 Missing and 8 partials ⚠️
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     
Flag Coverage Δ
differential 41.15% <20.00%> (?)
total ?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielsanchezaran danielsanchezaran force-pushed the feature/error-integration-on-slope-take-off branch from 9a88100 to ec6b1a0 Compare November 21, 2023 07:02
@danielsanchezaran
Copy link
Contributor Author

Not sure why the issue was closed, I did not do it.

@danielsanchezaran
Copy link
Contributor Author

danielsanchezaran commented Nov 21, 2023

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.mp4

There 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

@danielsanchezaran
Copy link
Contributor Author

danielsanchezaran commented Nov 21, 2023

@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.

I believe the new changes satisfy these requirements. Could you please take a look again? @TakaHoribe

Signed-off-by: Daniel Sanchez <[email protected]>
@takayuki5168
Copy link
Contributor

takayuki5168 commented Nov 23, 2023

I leave this PR's review to @TakaHoribe -san.

Copy link
Contributor

@TakaHoribe TakaHoribe left a 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

…oller.cpp

Co-authored-by: Takamasa Horibe <[email protected]>
Signed-off-by: Daniel Sanchez <[email protected]>
@danielsanchezaran danielsanchezaran force-pushed the feature/error-integration-on-slope-take-off branch from f02b710 to dc4b351 Compare November 24, 2023 07:57
Signed-off-by: Daniel Sanchez <[email protected]>
Comment on lines 220 to 221
Note:

Copy link
Contributor

Choose a reason for hiding this comment

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

?? Should it be removed?

Copy link
Contributor

@TakaHoribe TakaHoribe left a 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]>
@danielsanchezaran danielsanchezaran merged commit 0496b19 into autowarefoundation:main Nov 27, 2023
22 of 24 checks passed
@danielsanchezaran danielsanchezaran deleted the feature/error-integration-on-slope-take-off branch November 27, 2023 00:08
danielsanchezaran added a commit to tier4/autoware.universe that referenced this pull request Dec 15, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:control Vehicle control algorithms and mechanisms. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants