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

chore(autoware_pointcloud_preprocessor): change unnecessary warning message to debug #8525

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Aug 19, 2024

Description

This PR provides a quick fix for changing unnecessary warning messages to debug to reduce the console log.
This PR #8300 fixes the issue but will take time to review.

According to TIER4_INTERNAL_LINK
[component_container_mt-1] [WARN 1724036039.656393647] [sensing.lidar.concatenate_data] computeTransformToAdjustForOldTimestamp(): "old_stamp is newer than new_stamp," at (/home/autoware/pilot-auto/src/autoware/universe/sensing/pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_nodelet.cpp:L255)

Warning messages keep showing up due to the algorithm design.

Current algorithm:

  1. Sort the timestamp from large to small in a vector.
  2. Pointclouds will find the timestamp smaller than its timestamp (in the vector) to calculate the transformation. But since it is finding the timestamp from large to small, if the pointcloud is not the newest one, it will always encounter the situation that the current pointcloud timestamp is smaller than the largest (or other larger) timestamp in the vector. When this happens the warning will show up.

Related links

Parent Issue:

  • Link

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@vividf vividf self-assigned this Aug 19, 2024
@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Aug 19, 2024
Copy link

github-actions bot commented Aug 19, 2024

Thank you for contributing to the Autoware project!

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

Please ensure:

@vividf vividf added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 19, 2024
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

@vividf Thank you. I believe this should ideally be issued as a warning, so could you explain why it was categorized as debug information instead? The root cause should be addressed as a warning, but by downgrading it to debug, there’s a risk of missing the underlying issue.

That said, if this process has little to no impact on the system’s operation and doesn’t require correction, then treating it as debug information is acceptable.

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 23.68%. Comparing base (1bfb4c0) to head (0fade6c).
Report is 2 commits behind head on main.

Files Patch % Lines
...atenate_data/concatenate_and_time_sync_nodelet.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8525      +/-   ##
==========================================
- Coverage   24.06%   23.68%   -0.38%     
==========================================
  Files        1397     1406       +9     
  Lines      102639   102355     -284     
  Branches    39035    39099      +64     
==========================================
- Hits        24697    24244     -453     
- Misses      75465    75601     +136     
- Partials     2477     2510      +33     
Flag Coverage Δ *Carryforward flag
differential 18.77% <0.00%> (?)
total 23.95% <ø> (-0.11%) ⬇️ Carriedforward from e958c63

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

@vividf
Copy link
Contributor Author

vividf commented Aug 20, 2024

@yukkysaito
Thanks for the comments.
From the algorithm described in the description above, all of the pointcloud will show this warning message instead of the pointcloud that has the latest timestamp.
For instance, if we have 4 pointcloud ready to concatenate, 3 of them satisfy the if condition.
if (old_stamp > new_stamp) { RCLCPP_DEBUG_STREAM_THROTTLE( get_logger(), *get_clock(), std::chrono::milliseconds(10000).count(), "old_stamp is newer than new_stamp,");

if condition is true
1st (earliest) pointlcoud: 3 time, 2nd pointcloud: 2 times, 3rd pointcloud: 1 times

Since almost all of the pointcloud will satisfy the if condition and this has no impact on system operation, I don't think a warning is a good logging message

@vividf vividf requested a review from yukkysaito August 20, 2024 06:15
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

@vividf I think this PR is temporary because new design for concatenation in #8300 will solve this problem, right?
If so, saito-san's concern will not really affect products so much. LGTM.

@vividf vividf enabled auto-merge (squash) August 26, 2024 02:55
@vividf vividf merged commit 87cfc7f into autowarefoundation:main Aug 26, 2024
25 checks passed
h-ohta pushed a commit to tier4/autoware.universe that referenced this pull request Aug 27, 2024
h-ohta added a commit to tier4/autoware.universe that referenced this pull request Aug 27, 2024
kyoichi-sugahara pushed a commit to kyoichi-sugahara/autoware.universe that referenced this pull request Aug 27, 2024
a-maumau pushed a commit to a-maumau/autoware.universe that referenced this pull request Sep 2, 2024
ktro2828 pushed a commit to ktro2828/autoware.universe that referenced this pull request Sep 18, 2024
h-ohta pushed a commit to tier4/autoware.universe that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) tag:require-cuda-build-and-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants