refactor(velodyne): implement generic velodyne decoder #144
Conversation
…c-velodyne-decoder
|
Thank you @bgilby59 for refactoring velodyne drivers! Pointcloud Output
🟢Pointcloud Output is looking same as before! It is great refactoring!! Decoder Performance
|
Progress Update: Reason why all vlp tests are brokenIt has 2 main reasons
🔴 Pointcloud Shape is looking good. It has bag on just azimuth angle. Maybe getAzimuthCorrected in (each lidar).hpp file is broken |
|
Thank you @ike-kazu! The point cloud fields are looking good, and it's great the performance became better already. Once everything is The azimuth angle is interesting! It looks like it's just rotated, right? It might only be that case for VLP32 as its decoder was quite different from the other two. |
ike-kazu
force-pushed
the
feat/add-generic-velodyne-decoder
branch
from
eb653c4
to
c528884
Compare
Progress UpdateThank you @mojomex for your advises! I tried to change all
|
Progress UpdateI tried the test with changing |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
=========================================
+ Coverage 4.84% 11.52% +6.67%
=========================================
Files 249 72 -177
Lines 19210 7118 -12092
Branches 1075 1064 -11
=========================================
- Hits 931 820 -111
+ Misses 17579 5508 -12071
- Partials 700 790 +90
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@mojomex |
| * Raw Velodyne packet constants and structures. | ||
| */ | ||
| static const int SIZE_BLOCK = 100; | ||
| static const int RAW_SCAN_SIZE = 3; // TODO: remove |
| static const int SIZE_BLOCK = 100; | ||
| static const int RAW_SCAN_SIZE = 3; // TODO: remove | ||
| static const int RAW_CHANNEL_SIZE = 3; | ||
| static const int SCANS_PER_BLOCK = 32; // TODO: remove |
| static const int BLOCK_DATA_SIZE = (SCANS_PER_BLOCK * RAW_SCAN_SIZE); | ||
|
|
||
| static const double ROTATION_RESOLUTION = 0.01; // [deg] | ||
| static const uint16_t ROTATION_MAX_UNITS = 36000u; // [deg/100] |
There was a problem hiding this comment.
Please change to 360 * 100u to make it clearer to the reader
| static const uint16_t ROTATION_MAX_UNITS = 36000u; // [deg/100] | |
| static const uint16_t ROTATION_MAX_UNITS = 360 * 100u; // [deg/100] |
| static const uint16_t RETURN_MODE_LAST = 56; | ||
| static const uint16_t RETURN_MODE_DUAL = 57; | ||
|
|
||
| const int PACKET_SIZE = 1206; |
There was a problem hiding this comment.
Please rename to PACKET_SIZE_BYTES so the unit is clear
| { | ||
| uint16_t header; ///< UPPER_BANK or LOWER_BANK | ||
| uint16_t rotation; ///< 0-35999, divide by 100 to get degrees | ||
| uint8_t data[BLOCK_DATA_SIZE]; |
There was a problem hiding this comment.
- Please rename
datatounits(so it is easier to compare to Hesai'spandar_packet.hpp). - Please change the data type from
uint8_ttoraw_unitand create araw_unit struct:struct raw_unit { uint16_t distance; uint8_t reflectivity; };
| #include "nebula_decoders/nebula_decoders_velodyne/decoders/vlp_16.hpp" | ||
| #include "nebula_decoders/nebula_decoders_velodyne/decoders/vls_128.hpp" |
There was a problem hiding this comment.
The generic decoder should not use any sensor-specific headers. Please remove.
| uint bank_origin = 0; | ||
| // Used to detect which bank of 32 lasers is in this block. | ||
| switch (current_block.header) { | ||
| case VLS128::BANK_1: | ||
| bank_origin = 0; | ||
| break; | ||
| case VLS128::BANK_2: | ||
| bank_origin = 32; | ||
| break; | ||
| case VLS128::BANK_3: | ||
| bank_origin = 64; | ||
| break; | ||
| case VLS128::BANK_4: | ||
| bank_origin = 96; | ||
| break; | ||
| default: | ||
| RCLCPP_ERROR( | ||
| rclcpp::get_logger("VelodyneDecoder"), | ||
| "Invalid bank origin detected in packet. Skipping packet."); | ||
| return; // bad packet: skip the rest | ||
| } |
There was a problem hiding this comment.
If this is the same for all Velodyne sensors, move this code and the constants BANK_1 etc. to velodyne_sensor.hpp. If it is different for sensors, make a virtual function in velodyne_sensor.hpp and override it in the sensor .hpp files.
| for (int channel = 0; | ||
| channel < velodyne_packet::CHANNELS_PER_BLOCK && channel < SensorT::channels_per_firing_sequence; | ||
| channel++, k += velodyne_packet::RAW_CHANNEL_SIZE) { |
There was a problem hiding this comment.
Iterate over raw_unit_ts here, for example:
for (size_t unit_idx = 0; unit_idx < velodyne_packet::UNITS_PER_BLOCK; ++unit_idx) {
const auto & unit = current_block.units[unit_idx];
// ...
}| // Distance extraction. | ||
| current_return.bytes[0] = current_block.data[k]; | ||
| current_return.bytes[1] = current_block.data[k + 1]; | ||
| if (dual_return) { | ||
| other_return.bytes[0] = | ||
| block % 2 ? raw->blocks[block - 1].data[k] : raw->blocks[block + 1].data[k]; | ||
| other_return.bytes[1] = | ||
| block % 2 ? raw->blocks[block - 1].data[k + 1] : raw->blocks[block + 1].data[k + 1]; | ||
| } |
There was a problem hiding this comment.
These should all access unit.distance instead (which is already uint16_t so only one line will be needed instead of two)
|
|
||
| float last_azimuth_diff = 0; | ||
| uint16_t azimuth_next; | ||
| const uint8_t return_mode = velodyne_packet.data[velodyne_packet::RETURN_MODE_INDEX]; |
There was a problem hiding this comment.
This should now use the return_mode field introduced in velodyne_packet.hpp
src/transport_drivers
Outdated
There was a problem hiding this comment.
Please remove this as a git submodule
|
@mojomex |
|
|
Will be continued in #228. |
PR Type
Related Links
Description
Previously, there was a separate Velodyne decoder for each sensor. However, since the sensors function very similarly, this leads to large amounts of repeated code that is unnecessary and difficult to maintain.
This PR implements a generic decoder to handle all Velodyne sensors.
Design
VelodyneDecoder<SensorT>Generic decoder using templating to get sensor-specific information
VLP16Class holding VLP16 specific information
VLS128Class holding VLS128 specific information
Review Procedure
colcon test --event-handlers console_cohesion+ --packages-above nebula_commoncode reviewRemarks
Current Progress:
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