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(log-messages): reduce excessive log messages in velodyne decoder #118

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

ahmeddesokyebrahim
Copy link
Contributor

@ahmeddesokyebrahim ahmeddesokyebrahim commented Feb 19, 2024

PR Type

  • Improvement

Related Links

More details in this autoware issue and this nebula issue.

Description

This PR is one of a group of PRs that aim to fix the Autoware logging system to reduce excessive error and warning logs on Autoware launch only on the Velodyne decoder.

Part of:

Review Procedure

  • Planning Simulation
    • Run Autoware planning simulator, and you can see no more excessive and recurring errors, warnings, and info messages when the system running as expected
  • Rosbag Replay Simulation
  • AWSIM
    • To be addressed in another PR - extended goal)

Remarks

Take into consideration relevant PRs.

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to a reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

@ahmeddesokyebrahim
Copy link
Contributor Author

@xmfcx @mitsudome-r @drwnz @amc-nu : This PR is ready for review

@drwnz drwnz requested a review from amc-nu March 6, 2024 02:15
@amc-nu
Copy link
Collaborator

amc-nu commented Mar 7, 2024

@ahmeddesokyebrahim
Thank you for your PR. Is there any reason why you only modify the Velodyne-related code?
Would it be possible also to include the rest of the decoders and hardware interfaces?

@ahmeddesokyebrahim
Copy link
Contributor Author

@amc-nu

Thank you for your PR. Is there any reason why you only modify the Velodyne-related code?
Would it be possible also to include the rest of the decoders and hardware interfaces?

The main reason behind is that the main target of my task is to work on improving log messages coming from the tutorials with default parameters and the velodyne is the one we have in our default in the rosbag replay simulation tutorial.

However, I`ll have a look to the effort needed for other decoders and HW interfaces to improve their logs messages and discuss that with @mitsudome-r and @xmfcx and let you know for sure.

@ahmeddesokyebrahim
Copy link
Contributor Author

@amc-nu
So, we have discussed the point of modifying the rest of the decoders and hardware interfaces, and we got to the conclusion to keep only velodyne related changes for now.
Hence, I guess there is no any expected further changes to be added in this PR as agreed. I hope we can review and merge it asap.
cc : @mitsudome-r - @xmfcx

@ahmeddesokyebrahim
Copy link
Contributor Author

ahmeddesokyebrahim commented Mar 13, 2024

Hi @amc-nu - @drwnz
This is currently the remaining open PR in the context of this issue. Is there any plan to review and merge it soon ?

@ahmeddesokyebrahim
Copy link
Contributor Author

@amc-nu @drwnz
I would like to get your attention to this PR. The PR is pretty straightforward and simple.

@amc-nu amc-nu changed the title fix(log-messages): reduce excessive log messages fix(log-messages): reduce excessive log messages in velodyne decoder Mar 29, 2024
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 9.14%. Comparing base (cb3c56a) to head (21c9e10).

Files Patch % Lines
..._ros/src/velodyne/velodyne_decoder_ros_wrapper.cpp 0.00% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##            main    #118      +/-   ##
========================================
+ Coverage   4.81%   9.14%   +4.33%     
========================================
  Files        249      78     -171     
  Lines      19207    9328    -9879     
  Branches    1079    1058      -21     
========================================
- Hits         924     853      -71     
+ Misses     17579    7778    -9801     
+ Partials     704     697       -7     
Flag Coverage Δ
differential 9.14% <0.00%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@amc-nu amc-nu merged commit dd31cc3 into tier4:main Mar 29, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants