Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
- Coverage 31.77% 31.12% -0.66%
==========================================
Files 113 112 -1
Lines 9702 9562 -140
Branches 4478 4048 -430
==========================================
- Hits 3083 2976 -107
- Misses 6040 6041 +1
+ Partials 579 545 -34
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
mojomex
left a comment
mojomex
left a comment
There was a problem hiding this comment.
Thank you for the PR!
I have left a few comments about simplifying the code, please have a look.
Also, could you add a self-evaluation section where you use a real VLP16 with different settings?
Ones I'm interested in are:
- 0, 360 -- no gaps should be observed
- 90, 90 -- should be the same output as 0, 360
- 0, 180 -- no weird timestamp0s at edges
- 180, 0 -- same but the other half of the pointcloud is shown
- 355, 5 -- only 10deg of the pointcloud are shown
- 5, 355 -- a 10deg gap is shown
- 3, 357 -- a 6deg gap is shown
| if (setting_cloud_min_angle < 5) { | ||
| setting_cloud_min_angle = (setting_cloud_min_angle + 355) % 360; | ||
| } else { | ||
| setting_cloud_min_angle -= 5; | ||
| } |
There was a problem hiding this comment.
This if/else can be simplified into one generic formula (-5 + 360 might be more understandable than +355):
| if (setting_cloud_min_angle < 5) { | |
| setting_cloud_min_angle = (setting_cloud_min_angle + 355) % 360; | |
| } else { | |
| setting_cloud_min_angle -= 5; | |
| } | |
| setting_cloud_min_angle = (setting_cloud_min_angle - 5 + 360) % 360; |
| if (setting_cloud_max_angle > 354) { | ||
| setting_cloud_max_angle = 359; | ||
| } else { | ||
| setting_cloud_max_angle += 5; | ||
| if (setting_cloud_max_angle >= 360) { | ||
| setting_cloud_max_angle = 359; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can be simplified to:
| if (setting_cloud_max_angle > 354) { | |
| setting_cloud_max_angle = 359; | |
| } else { | |
| setting_cloud_max_angle += 5; | |
| if (setting_cloud_max_angle >= 360) { | |
| setting_cloud_max_angle = 359; | |
| } | |
| } | |
| setting_cloud_max_angle = (setting_cloud_max_angle + 5) % 360; |
| setting_cloud_min_angle = 359; | ||
| int setting_cloud_max_angle = sensor_configuration->cloud_max_angle; | ||
|
|
||
| // FIXME: VLP16 has problems for timestamp. Whatch github issue # |
There was a problem hiding this comment.
Please explain in more detail (combination of hardware and software scan cutting causes timestamping issues at the FoV borders, so the hardware FoV has to be increased for scan cutting to work.
|
|
||
| // FIXME: VLP16 has problems for timestamp. Whatch github issue # | ||
| if (sensor_configuration->sensor_model == SensorModel::VELODYNE_VLP16) { | ||
| int angle_diff = (setting_cloud_max_angle - setting_cloud_min_angle + 360) % 360; |
There was a problem hiding this comment.
Please define the angle we oversize by as a const int32_t fov_tolerance_deg = 5; and replace all 5s below by that constant.
| if (sensor_configuration->sensor_model == SensorModel::VELODYNE_VLP16) { | ||
| int angle_diff = (setting_cloud_max_angle - setting_cloud_min_angle + 360) % 360; | ||
|
|
||
| if (angle_diff >= 360 || angle_diff == 0) { |
There was a problem hiding this comment.
Angle diff cannot be >= 360 (it was % 360ed), so the below condition is sufficient:
| if (angle_diff >= 360 || angle_diff == 0) { | |
| if (angle_diff == 0) { |
2 participants
PR Type
Only vlp16 has problems for timestamp. In this PR, hot fix is created for this.
It's reported in #292
Related Links
VLP16 timestamp problem report
Description
Only VLP16 has problems for timestamps in poincloud from nebula, Last Filtered Pointcloud timestamp problem.
When vlp16 pointcloud is filtered through nebula, last pointcloud timestamp is weird sometime. We research about this problem but we can't discover the cause of this clearly.
But we Discover that VLP16 pointcloud timestamps are weird when it is filtered by hardware.
It's fine when VLP16 pointcloud are filtered by only nebula.
From this discovery, we decide that we create hardware filter offset as hot fix for this problem.
Fixes #292.
Review Procedure
Remarks
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks