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: velodyne lidar azimuth correction #180

Merged
merged 10 commits into from
Aug 15, 2024

Conversation

ike-kazu
Copy link
Contributor

@ike-kazu ike-kazu commented Jul 31, 2024

PR Type

  • Improvement

Related Links

Description

I fixed #112 azimuth correction function pr. It was reverted.
This pr makes nabula velodyne detecoder lighter.

Review Procedure

Remarks

Pre-Review Checklist for the PR Author

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

  • Assign PR to 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.

@ike-kazu ike-kazu changed the title velodyne lidar azimuth correction feat: velodyne lidar azimuth correction Jul 31, 2024
@mojomex mojomex self-requested a review July 31, 2024 06:58
Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

@ike-kazu thank you for the PR, great work!
There's a few minor things I'd like you to change, mostly removing debug comments.

I still have to test the decoder with a real sensor.

@mojomex mojomex requested review from mojomex and vividf July 31, 2024 09:23
Copy link
Collaborator

@vividf vividf 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 azimuth value with atan2(y, x) and LGTM!

Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

Everything looks good, but could you quickly remove the two submodules that were erroneously added?
Thank you!

src/ros2_socketcan Outdated Show resolved Hide resolved
src/transport_drivers Outdated Show resolved Hide resolved
@mojomex
Copy link
Collaborator

mojomex commented Aug 14, 2024

Hi, I just merged the current main branch into your PR but it seems that this PR:

Could you regenerate the VLS128 unit test .pcd file once more?
Sorry for the extra work this causes 🙇‍♂️

@ike-kazu ike-kazu force-pushed the feat/add_fixed_corrected_azimuth branch from f92849a to 789f80d Compare August 14, 2024 09:34
Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

I took the liberty to remove transport_drivers again 😉
LGTM!

@mojomex mojomex merged commit 6d55141 into tier4:main Aug 15, 2024
6 of 7 checks passed
mojomex added a commit that referenced this pull request Sep 5, 2024
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.

3 participants