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(pose_instability_detector): fix a rare error #7681

Conversation

SakodaShintaro
Copy link
Contributor

@SakodaShintaro SakodaShintaro commented Jun 25, 2024

Description

In pose_instability_detector, a bug has been fixed that in rare abnormal cases, it may access to outside the queue size and crash, or it may show abnormal values.

I have added test cases that cause this, and I have confirmed that it fails probabilistically if it is not fixed.
Since accessing outside the range is undefined behavior, it does not necessarily always fail.

By making a modification of pose_instability_detector.cpp, the test will no longer fail even after 100 consecutive attempts.

Related links

None.

How was this PR tested?

The following script was used.

./test_loop.sh pose_instability_detector
#!/bin/bash
set -eux

TEST_TARGET_PKG=$1
LOOP_NUM=${2:-100}

colcon build --symlink-install \
  --cmake-args \
    -DCMAKE_BUILD_TYPE=Release \
    -DBUILD_TESTING=ON \
  --packages-select $TEST_TARGET_PKG

for ((i=1; i<=LOOP_NUM; i++))
do
  colcon test --packages-select ${TEST_TARGET_PKG} --event-handlers console_cohesion+ --return-code-on-test-failure
  echo "Finish test $i"
done

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@SakodaShintaro SakodaShintaro added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) tag:run-clang-tidy-differential labels Jun 25, 2024
@SakodaShintaro SakodaShintaro self-assigned this Jun 25, 2024
@github-actions github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Jun 25, 2024
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 12.82%. Comparing base (507e3f4) to head (d8e27de).
Report is 171 commits behind head on main.

Files Patch % Lines
...ability_detector/src/pose_instability_detector.cpp 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7681       +/-   ##
===========================================
- Coverage   14.84%   12.82%    -2.03%     
===========================================
  Files        1999       34     -1965     
  Lines      139163     2043   -137120     
  Branches    43716      201    -43515     
===========================================
- Hits        20661      262    -20399     
+ Misses      95731     1768    -93963     
+ Partials    22771       13    -22758     
Flag Coverage Δ
differential 12.82% <88.46%> (?)
total ?

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

@xmfcx
Copy link
Contributor

xmfcx commented Jun 25, 2024

@SakodaShintaro could you rebase to the latest main?

I want to see if the fix on comment-on-pr workflow issue is fixed.

Copy link

github-actions bot commented Jun 25, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

Copy link
Contributor

@TaikiYamada4 TaikiYamada4 left a comment

Choose a reason for hiding this comment

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

After all, I think the dead reckon process should be skipped when the time of latest and prev is same, and publish a WARN diagnostics. (And then I believe the size of result_deque will be always be > 2.)

But I will mark that as a TODO for me, and I can approve the cpp part.
Can you take a look to my comment in the test part? 🙏

localization/pose_instability_detector/test/test.cpp Outdated Show resolved Hide resolved
Signed-off-by: Shintaro Sakoda <[email protected]>
Signed-off-by: Shintaro Sakoda <[email protected]>
Copy link
Contributor

@TaikiYamada4 TaikiYamada4 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@SakodaShintaro SakodaShintaro merged commit a81e1f7 into autowarefoundation:main Jul 1, 2024
29 of 31 checks passed
@SakodaShintaro SakodaShintaro deleted the fix/fix_rare_error_in_pose_instability_detector branch July 1, 2024 03:45
SakodaShintaro added a commit to tier4/autoware.universe that referenced this pull request Jul 2, 2024
…7681)

* Added an error test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed to avoid out-of-size access

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed the test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed test

Signed-off-by: Shintaro Sakoda <[email protected]>

---------

Signed-off-by: Shintaro Sakoda <[email protected]>
mitukou1109 pushed a commit to mitukou1109/autoware.universe that referenced this pull request Jul 2, 2024
…7681)

* Added an error test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed to avoid out-of-size access

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed the test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed test

Signed-off-by: Shintaro Sakoda <[email protected]>

---------

Signed-off-by: Shintaro Sakoda <[email protected]>
palas21 pushed a commit to palas21/autoware.universe that referenced this pull request Jul 12, 2024
…7681)

* Added an error test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed to avoid out-of-size access

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed the test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed test

Signed-off-by: Shintaro Sakoda <[email protected]>

---------

Signed-off-by: Shintaro Sakoda <[email protected]>
Signed-off-by: palas21 <[email protected]>
tby-udel pushed a commit to tby-udel/autoware.universe that referenced this pull request Jul 14, 2024
…7681)

* Added an error test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed to avoid out-of-size access

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed the test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed test

Signed-off-by: Shintaro Sakoda <[email protected]>

---------

Signed-off-by: Shintaro Sakoda <[email protected]>
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
* Added an error test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed to avoid out-of-size access

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed the test case

Signed-off-by: Shintaro Sakoda <[email protected]>

* Fixed test

Signed-off-by: Shintaro Sakoda <[email protected]>

---------

Signed-off-by: Shintaro Sakoda <[email protected]>
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) tag: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.

3 participants