perf: unpack velodyne packets in parallel

[mebasoglu]

PR Type

• Improvement

Related Links

Description

The issue: #61

Review Procedure

On the nebula_decoders package, I have created another interface called ParallelUnpack. The decoder classes for each Velodyne model inherits ParallelUnpack alongside VelodyneScanDecoder.

On the velodyne_driver.cpp, I converted the for loop to a parallel for_each. The unpack method is called in parallel and every Velodyne packet unpacks into a seperate point cloud. Then, at the end, when get_pointcloud() is called, al the clouds are merged into a single one. Overall, the unpack process was speeded up.

Remarks

VLS-128 produces a lot of packets and processing it takes too much time. This improvement tends to speed up this process.

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.

[drwnz]

@mebasoglu thank you for the PR.
One request, is it possible to exclude the formatting improvements for review, so that we can more easily see the changes you have made?
I am aware that code style refactoring is required for the Velodyne drivers generally, but it would be easier to do that in a separate PR.

[mebasoglu]

Hello @drwnz , I undid the code formatting.

[codecov-commenter]

Codecov Report

Patch coverage: 69.56% and project coverage change: +8.41% 🎉

Comparison is base (e0d05a7) 13.35% compared to head (bd7d9f4) 21.76%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   13.35%   21.76%   +8.41%     
==========================================
  Files         111       49      -62     
  Lines       10989     6330    -4659     
  Branches     1725     1670      -55     
==========================================
- Hits         1468     1378      -90     
+ Misses       8345     3805    -4540     
+ Partials     1176     1147      -29     

Flag	Coverage Δ
differential	21.76% <69.56%> (?)
total	?

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

Files Changed	Coverage Δ
...coders_velodyne/decoders/velodyne_scan_decoder.hpp	50.00% <ø> (ø)
...ebula_decoders_velodyne/decoders/vlp32_decoder.cpp	0.00% <0.00%> (ø)
...s/src/nebula_decoders_velodyne/velodyne_driver.cpp	25.00% <40.00%> (+0.75%)	⬆️
...ula_decoders_velodyne/decoders/parallel_unpack.hpp	87.50% <87.50%> (ø)
...ebula_decoders_velodyne/decoders/vlp16_decoder.cpp	78.76% <100.00%> (+1.52%)	⬆️
...bula_decoders_velodyne/decoders/vls128_decoder.cpp	64.93% <100.00%> (+3.49%)	⬆️

... and 63 files with indirect coverage changes

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

[amc-nu]

We see two issues with the PR

1. Parallel encoding is always enforced. Can you please make this parallel decoding optional?
2. This PR does not consider whether the scan phase differs from 0. Can you please check the scan phase before resetting and generating the point cloud?

[amc-nu]

#62 (comment)

[mebasoglu]

Hello, thank you for your feedbacks. We prioritized adding Robosense support at Leo Drive side, after that I will have a look at other issues. Thank you.

[xmfcx]

If the following issue:

• Enhancement: allow dynamic scan message size to reduce latency #94

is resolved, then the parallelization wouldn't be necessary anymore since we wouldn't be waiting for the entire revolution to finish before starting the unpacking process.

[mojomex]

Hi @mebasoglu, thank you for your work.
We have decided to close this PR in favor of

• refactor(velodyne)!: combine Velodyne ROS wrappers into a single node #148

Our reasoning and details can be found in this comment on

• Performance Enhancement: Parallelized Unpacking of Velodyne Packets #61

Thank you for your understanding and your continued support!