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(ndt_scan_matcher): fix map_update_module structure #5915

Conversation

SakodaShintaro
Copy link
Contributor

Description

In this pull request, the structure of map_update_module in ndt_scan_matcher has been changed to optimize processing.

Context

(1) Currently, map_update_module continuously outputs ERROR logs due to its inability to subscribe to ekf odometry until ndt_scan_matcher estimates the initial pose. Outputting ERROR in this normal sequence is not ideal. To avoid this, a structural change was required because map_update_module needs to check the state of ndt_scan_matcher.

(2) Upon reviewing the code, I identified another issue: map_update_module subscribes to ekf odometry independently of ndt_scan_matcher. This redundancy is inefficient. Given that the ekf odometry topic is published at 50Hz, subscribing to it twice could negatively impact CPU usage.

(3) Also, because map_update_module creates callback groups in its constructor, it is difficult to understand how many callback groups there were in the entire ndt_scan_matcher.

Changes

  • Moved ekf subscriber and timer callbacks to ndt_scan_matcher_core.cpp.
  • Changed the map_update_module to no longer manage the current location and focus only on updating the map.
  • Changed the name from main_callback_group to sensor_callback_group.
  • Some minor refactoring, such as removing unused arguments.

Tests performed

The logging_simulator has been verified to run with the same accuracy as before on AWSIM data with GT. It also performs well when dynamic_map_loading is used for divided maps.

Effects on system behavior

There are no large effects on system behavior. There is a possibility that the CPU load has improved slightly because EKF's double subscription is eliminated.

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
@SakodaShintaro SakodaShintaro added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Dec 19, 2023
@SakodaShintaro SakodaShintaro self-assigned this Dec 19, 2023
@github-actions github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Dec 19, 2023
Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Please check my following comments. 🙏

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

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

Comparison is base (acf7e51) 15.28% compared to head (fa3c0b5) 15.32%.
Report is 10 commits behind head on main.

Files Patch % Lines
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp 0.00% 12 Missing and 10 partials ⚠️
...ization/ndt_scan_matcher/src/map_update_module.cpp 0.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5915      +/-   ##
==========================================
+ Coverage   15.28%   15.32%   +0.03%     
==========================================
  Files        1749     1747       -2     
  Lines      120356   119816     -540     
  Branches    36708    36305     -403     
==========================================
- Hits        18401    18356      -45     
+ Misses      81305    81092     -213     
+ Partials    20650    20368     -282     
Flag Coverage Δ *Carryforward flag
differential 3.07% <0.00%> (?)
total 15.32% <ø> (+0.03%) ⬆️ Carriedforward from 6a3a3a5

*This pull request uses carry forward flags. Click here to find out more.

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

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
@SakodaShintaro
Copy link
Contributor Author

SakodaShintaro commented Dec 21, 2023

Also, the is_activated_ variable is accessed from multiple threads as shown below.

  • Read/write by sensor_callback_group thread
  • Read in initial_pose_callback_group and timer_callback_group_ threads

I made it atomic. Please let me know if there is any misunderstanding.

(When I made it an atomic variable, I got a warning from clang-tidy if I didn't initialize it in the member initializer lists, so I changed the initialization location.)

Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

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

LGTM
I think the two additional exclusivity controls make sense. 👍 (for is_activated_ and latest_ekf_position_ )

@SakodaShintaro SakodaShintaro merged commit 553f1f2 into autowarefoundation:main Dec 21, 2023
26 of 29 checks passed
@SakodaShintaro SakodaShintaro deleted the fix/map_update_module_sub branch December 21, 2023 23:57
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 26, 2024
…ation#5915)

* Refactored map_update_module

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Rename a variable and add comments

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Change the order of if statements

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Fixed to constexpr

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Added latest_ekf_position_mtx_

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Changed is_activated_ to atomic

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

---------

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…ation#5915)

* Refactored map_update_module

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Rename a variable and add comments

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Change the order of if statements

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Fixed to constexpr

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Added latest_ekf_position_mtx_

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Changed is_activated_ to atomic

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

---------

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…ation#5915)

* Refactored map_update_module

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Rename a variable and add comments

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Change the order of if statements

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Fixed to constexpr

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Added latest_ekf_position_mtx_

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Changed is_activated_ to atomic

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

---------

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…ation#5915)

* Refactored map_update_module

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Rename a variable and add comments

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Change the order of if statements

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Fixed to constexpr

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Added latest_ekf_position_mtx_

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* Changed is_activated_ to atomic

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

---------

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
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