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!: move diagnostics_module from localization_util to unverse_utils #9714

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Dec 23, 2024

Description

DiagnosticsModule in localization_util is useful. Thus I would like to move it from the package to universe_utils

Related links

None

How was this PR tested?

Added unit test for the module

Notes for reviewers

None.

Interface changes

Package include path would change.

Effects on system behavior

None.

Signed-off-by: kminoda <koji.minoda@tier4.jp>
@github-actions github-actions bot added component:localization Vehicle's position determination in its environment. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels Dec 23, 2024
Copy link

github-actions bot commented Dec 23, 2024

Thank you for contributing to the Autoware project!

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

Please ensure:

kminoda and others added 3 commits December 23, 2024 11:47
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Dec 23, 2024
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Comment on lines 59 to 60
template <>
void DiagnosticsModule::add_key_value(const std::string & key, const bool & value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these declarations because they do not make sense unless you declare them as "extern template" and define them in cpp files as specializations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this? c981570

@@ -22,7 +22,7 @@
#include <string>
#include <vector>

namespace autoware::localization_util
namespace autoware::universe_utils
{
class DiagnosticsModule
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO DiagnosticInterface is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: b7a90d8

Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Dec 24, 2024
Signed-off-by: kminoda <koji.minoda@tier4.jp>
pre-commit-ci bot and others added 2 commits December 25, 2024 00:00
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda
Copy link
Contributor Author

kminoda commented Dec 25, 2024

Thank you to all the reviewers. I've addressed all the comments.

Copy link
Contributor

@SakodaShintaro SakodaShintaro left a comment

Choose a reason for hiding this comment

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

I checked the following test items:

I also verified that the /diagnostics topic had the appropriate information.

LGTM, but please get soblin-san's approval before merging.

@SakodaShintaro
Copy link
Contributor

Sorry, pre-commit.ci is failing.

Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda
Copy link
Contributor Author

kminoda commented Dec 25, 2024

@SakodaShintaro Thanks, fixed the precommit.

Copy link

codecov bot commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 38.46154% with 24 lines in your changes missing coverage. Please review.

Project coverage is 29.75%. Comparing base (421ec7d) to head (cb5163f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ar_marker_localizer/src/lidar_marker_localizer.cpp 0.00% 21 Missing ⚠️
...n_error_monitor/src/localization_error_monitor.cpp 0.00% 1 Missing ⚠️
...are_ndt_scan_matcher/src/ndt_scan_matcher_core.cpp 83.33% 1 Missing ⚠️
...are_pose_initializer/src/pose_initializer_core.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9714      +/-   ##
==========================================
+ Coverage   29.72%   29.75%   +0.03%     
==========================================
  Files        1451     1453       +2     
  Lines      108839   108888      +49     
  Branches    42741    42761      +20     
==========================================
+ Hits        32348    32397      +49     
+ Misses      73313    73312       -1     
- Partials     3178     3179       +1     
Flag Coverage Δ *Carryforward flag
differential 32.01% <38.46%> (?)
total 29.72% <ø> (+<0.01%) ⬆️ Carriedforward from 421ec7d

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

@kminoda kminoda merged commit 16d5cb1 into autowarefoundation:main Dec 25, 2024
36 checks passed
@kminoda kminoda deleted the feat/diagnostics_module/move_to_universe_utils branch December 25, 2024 03:10
kyoichi-sugahara pushed a commit to kyoichi-sugahara/autoware.universe that referenced this pull request Dec 25, 2024
autowarefoundation#9714)

* feat!: move diagnostics_module from localization_util to unverse_utils

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* remove diagnostics module from localization_util

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* style(pre-commit): autofix

* minor fix in pose_initializer

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* add test

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* style(pre-commit): autofix

* remove unnecessary declaration

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* module -> interface

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* remove unnecessary equal expression

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* revert the remove of template function

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* style(pre-commit): autofix

* use overload instead

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* include what you use -- test_diagnostics_interface.cpp

Signed-off-by: kminoda <koji.minoda@tier4.jp>

---------

Signed-off-by: kminoda <koji.minoda@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:common Common packages from the autoware-common repository. (auto-assigned) 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) type:documentation Creating or refining documentation. (auto-assigned)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants