fix(velodyne_decoder): overflow handling in vls128

[knzo25]

The timestamps in the vls128 were still relative to the last pointcloud, which was producing some errors Additionally, when entire scans were dropped, the overflow was not being handled correctly (this bug was also present in the awf velodyne driver)

PR Type

• Bug Fix

Related Links

TIER IV INTERNAL LINK (discussion)
TIER IV INTERNAL LINK (footage)

Description

2 bugs were discovered and addressed related to the handling of time stamps during pointcloud overflow between scans.
Context:

• The lidar scan has an azimuth value (the origin is defined by the sensor) and usually a pointcloud would start at 0 degrees and end in 360 degrees.
• However, to synchronize multiple lidars and cameras we can actually make pointclouds starting from a different angle.
• Since the communication with the sensor is via "small" packets instead of a single packet per physical scan (360 degrees) we can decide how to assemble the packets into a pointcloud (e.g, start making a pointcloud starting from sensor angle = 30 degrees).
• However, since each packet contains several points with different azimuth values, we need to handle one packet in two poinclouds (we call this concept overflow). Example: a certain packet contains points both lower and higher than a desired azimuth threshold. Lower values are added to the current pointcloud, whereas the higher ones need to be handled by the next one.

Nebula-only problem:

• When a scan message arrives at the decoder, the absolute time stamp gets taken from the first block of the first package in the scan message. In nebula (velodyne_points), each individual point has a relative time stamp, and they are relative to the previously stated absolute time stamp.
• However, the points that were determined to be part of the next pointcloud (overflow) currently retain their relative timestamp. Since they are the last points of the scan, they actually have relative times close to 100ms (the highest value, since physical scans run at 10hz).
• When the next scan message arrives, the process is repeated once again, but this time, the first points of the pointcloud will have a very high relative time, followed by points with relative time close to 0 and increasing to almost 100ms (there is a discontinuity).

Although only the non-overflow points are OK, if we use algorithms that integrate timestamps from all the points starting from the beginning (overflow points), it will cause big errors. This was particularly observed in the distortion correction algorithm (see the provided videos)

The solution simply consists of using as global timestamp the stamp of the overflow points (some conversions were required).

Nebula & old vls driver problem:

• In the vehicle, due to how pointcloud containers work, "there are no dropped packages". All packets from the lidar end up being converted into pointclouds.
• However, when recording and replaying rosbags, some entire scans (aggregation of sensor packets corresponding to one physical 360 degree scan) can be missing.
• In this case, even using the previews fix, errors still occur, since the overflow packets are used to determine the global time stamp, but since the points following the overflow are missing, the points after the overflow corresponds to the ones of the next physical scan, which makes a total time difference between the first and last point of the pointcloud of about 200ms (two scans).

For this problem, the proposed solution is that we first detect dropped packages using a rough stamp criteria, and then just ignore the last pointcloud's overflow when necessary (since there are abut 600 packages per scan, we are only loosing 360 * 1 / 600 degrees of information)

Review Procedure

A good way to check that the timestamps are being computed correctly, is comparing the raw_pointcloud with the output of the distortion corrector of autoware.

For example, it would be convenient to compare the behavior of the output of the distortion correction before/after this PR
(additionally, the fix for the original vls implementation also serves as a good baseline https://github.com/knzo25/awf_velodyne/tree/fix/drop_packages_fix )

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.

[codecov-commenter]

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (73a1e1b) 6.43% compared to head (9d7768c) 11.37%.

Files	Patch %	Lines
...bula_decoders_velodyne/decoders/vls128_decoder.cpp	47.22%	16 Missing and 3 partials ⚠️
...ebula_decoders_velodyne/decoders/vlp32_decoder.cpp	0.00%	17 Missing ⚠️
...ebula_decoders_velodyne/decoders/vlp16_decoder.cpp	77.27%	2 Missing and 3 partials ⚠️
...s/src/nebula_decoders_velodyne/velodyne_driver.cpp	50.00%	0 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff            @@
##            main     #111      +/-   ##
=========================================
+ Coverage   6.43%   11.37%   +4.94%     
=========================================
  Files        136       60      -76     
  Lines      10901     5563    -5338     
  Branches     854      802      -52     
=========================================
- Hits         701      633      -68     
+ Misses      9624     4376    -5248     
+ Partials     576      554      -22     

Flag	Coverage Δ
differential	11.37% <45.56%> (?)
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.

[knzo25]

@bgilby59
Regarding the additional problems we experienced the other day, 9091b2f should fix those.

[bgilby59]

Looks good. Tested on my end with 3 rosbags with XX1 data. The rosbags used and videos of results can be found at the below link:

TIER IV INTERNAL LINK