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

fix(ekf_localizer): correct the calculation of delay_step in updateMeasurementPose/Twist #5691

Merged

Conversation

TaikiYamada4
Copy link
Contributor

@TaikiYamada4 TaikiYamada4 commented Nov 27, 2023

Description

In the measurementUpdatePose() and measurementUpdateTwist() of ekf_module.cpp, a variable delay_step, which is used for extracting previous states in the EKF, have been computed by the following.

const int delay_step = std::roundf(delay_time / dt);

Currently, the devisor dt is the measured lap time of the timer_callback, but since ekf_localizer is a single thread node, the lap time of the timer callback may be different than expected. Generally, the lap time gets once longer when other callbacks provoke, and then gets shorter to catch up. This results to incorrect extracting of previous states. Thanks to the damping strategy in the EKF, this bug hasn't been exposed as a critical outcome, but yet it should be solved.

To solve this, I added a variable to accumulated_delay_times_ to store the time how long the previous states in the EKF are old.
With this, we can search the state index that has the closest delay time to that of the measurement, which is the correct delay_step we want.

(Added on 2023/12/04)
I also added a feature to output a WARNING message, if the timer callback period (ekf_dt_) gets too long for some reason.

Related links

To debug these commits, I locally made a debug commit by my self.
https://github.com/TaikiYamada4/autoware.universe/tree/feat/debug_y_ekf_with_accumulated_delay_time

The PR below has the same objective, but since this PR's approach is more simple than the previous one, I'd rather like to merge this PR and the previous PR should be closed if so.
#5587

Tests performed

These changes have been tested through the rosbag replay simulation in the tutorial.
Plus, I have monitored the first element of variable y_ekf in measurementUpdatePose() which suffers from the mentioned bug. This is monitored in the same rosbag replay simulation.
(Extracting values of y_ekf is not included in this PR and done by my local branch)

Despite this value should behave in a continuous manner, there are spikes due to the bug mentioned here (Upper figure).
After applying the changes in this PR, they will disappear (Lower figure).

compared_y_ekf

Notes for reviewers

Interface changes

None

Effects on system behavior

The predicted observation in the EKF should not look up to wrong previous states.
However, since the smoothing strategy in the EKF have been relieving the effects of the bug, it hardly makes any changes to the outputs in a macro manner.

Maybe this bug fix benefits the system when the timer callback in the ekf_localizer is unstable.

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.

… callback.

Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
…ap time of timer callback.

The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
@github-actions github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Nov 27, 2023
@TaikiYamada4 TaikiYamada4 changed the title fix(ekf_localizer): Correct the calculation of delay_step in updateMeasurementPose/Twist fix(ekf_localizer): correct the calculation of delay_step in updateMeasurementPose/Twist Nov 27, 2023
@TaikiYamada4 TaikiYamada4 self-assigned this Nov 27, 2023
@TaikiYamada4 TaikiYamada4 added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Nov 27, 2023
TaikiYamada4 and others added 4 commits November 27, 2023 18:00
Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
…autoware.universe into fix/correct_ekf_delay_step
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

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

Comparison is base (765a596) 15.32% compared to head (89b85b7) 44.10%.
Report is 165 commits behind head on main.

Files Patch % Lines
localization/ekf_localizer/src/ekf_module.cpp 40.00% 6 Missing and 6 partials ⚠️
localization/ekf_localizer/src/ekf_localizer.cpp 20.00% 3 Missing and 5 partials ⚠️
localization/ekf_localizer/src/warning_message.cpp 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5691       +/-   ##
===========================================
+ Coverage   15.32%   44.10%   +28.78%     
===========================================
  Files        1721       25     -1696     
  Lines      118559      993   -117566     
  Branches    37995      531    -37464     
===========================================
- Hits        18169      438    -17731     
+ Misses      79657      123    -79534     
+ Partials    20733      432    -20301     
Flag Coverage Δ
differential 44.10% <38.88%> (?)
total ?

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

@TaikiYamada4 TaikiYamada4 marked this pull request as ready for review November 28, 2023 04:30
@KYabuuchi KYabuuchi self-requested a review November 28, 2023 08:24
TaikiYamada4 and others added 2 commits December 4, 2023 10:22
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
@TaikiYamada4
Copy link
Contributor Author

I also added a feature to output a WARNING message, if the timer callback period (ekf_dt_) gets too long for some reason.

TaikiYamada4 and others added 3 commits December 5, 2023 16:16
…gest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
…gest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
@TaikiYamada4 TaikiYamada4 merged commit a1f354d into autowarefoundation:main Dec 6, 2023
20 of 23 checks passed
@TaikiYamada4 TaikiYamada4 deleted the fix/correct_ekf_delay_step branch December 6, 2023 06:26
danielsanchezaran pushed a commit to tier4/autoware.universe that referenced this pull request Dec 15, 2023
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Dec 19, 2023
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: karishma <karishma@interpl.ai>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Dec 19, 2023
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: karishma <karishma@interpl.ai>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 26, 2024
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…asurementPose/Twist (autowarefoundation#5691)

* Added X_delay_times_ to obtain the accumulated lap times of the timer callback.
Added find_closest_index function to easily use the X_delay_times_.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Added a concept/variable of accumulated_delay_time_ and store every lap time of timer callback.
The delay_step will be calculated from it, and the bug of calculating delay_step should be gone.
Besides, removed dt in measurementUpdatePose/Twist since it is not needed.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Fixed dt to ekf_dt_ in predictUpdateFrequency()

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed temporary debug stuff

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed code style pointed out from pre-commit.ci

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Fixed typo

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Removed variable ekf_rate_ which is currently unused.
Added warnings when the ekf_dt_ is too large.

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

* Changed threshold of delay time so that to look the most last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* Correct the warning messages of diag_info to look up the last (or largest) value of accumulated_delay_times_

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>

* style(pre-commit): autofix

---------

Signed-off-by: TaikiYamada4 <taiki.yamada@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:localization Vehicle's position determination in its environment. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants