refactor(velodyne): implement generic velodyne decoder #138
Conversation
…c-velodyne-decoder
Progress Update:First draft of generic decoder is finished! (It still needs some refactoring to get rid of compiler warnings and make the code more readable, but functionality is complete, so I think it can be reviewed). All tests are broken, but for explainable reasons: 1. VLP16
2. VLP32The number of points differ because ROI selection is limited by both Other point differences appear to be small offsets based on precision difference or comparison of incomparable points arising from the difference in number of points (all floats were made doubles in 0195ef0, and azimuth calculation uses a different method than before (following page 68 in the VLP32 User Manual. This is the method that was used for the current VLP16 and VLS128 decoders)). 3. VLS128Point differences are small offsets coming from changing floats to doubles in 0195ef0. Visual results of current decoder (red) and new generic decoder (white) on test data:vlp16_before_and_after.webm |
There was a problem hiding this comment.
Hey, thank you for your great and very fast work!
There are still a few points I'd like to address before merging, most of which are just refactorings, which by the looks of it, you still want to do anyway (it's not yet marked ✅ in your table above).
There are a few relics from before your refactoring that I would like you to address while you're at it, mainly having to do with parsing and type safety. I left comments in all the relevant places.
I will do a functional test once the comments are addressed.
Thanks!
There was a problem hiding this comment.
This file is currently empty. Please either refactor code into here or delete this file.
| 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 |
There was a problem hiding this comment.
Please address these TODOs
There was a problem hiding this comment.
Please move all sensor-specific constants to the respective sensor's file. For constants common to all sensors, move them to a VelodyneSensor class that all sensors intherit from. Ideally the decoder does not contain any constants that do not inherently apply to all sensors.
There was a problem hiding this comment.
All the structs (raw_block etc.) would ideally be moved to velodyne_packet.hpp.
Also, make sure all structs that are parsed using memcpy or (not recommended: reinterpret_cast and C-style casts) are defined between
#pragma pack(push, 1)
// define all structs here
#pragma pack(pop)so that memory layout without padding is guaranteed.
|
|
||
| constexpr static double single_firing_s = 2.304 * 1e-6; | ||
|
|
||
| constexpr static double offset_packet_time = 0; |
There was a problem hiding this comment.
These constants should ideally be implemented as arguments to the parent class (VelodyneSensor) constructor or template.
As it is, there is no guarantee that each child class defines all the constants accessed by the decoder (a new developer implementing a new sensor might not realize they have to define those constants for the decoder to work).
If these constants need to be passed to a constructor, there is no way that anyone forgets to put them here.
|
|
||
| constexpr static double single_firing_s = 2.304 * 1e-6; | ||
|
|
||
| constexpr static double offset_packet_time = 0; |
| default: | ||
| // Do not flood the log with messages, only issue at most one | ||
| // of these warnings per minute. | ||
| return; // bad packet: skip the rest |
There was a problem hiding this comment.
Please do some error handling here, at least RCLCPP_ERROR_THROTTLE() or something to let the user know something is wrong.
| } | ||
|
|
||
| // Condition added to avoid calculating points which are not in the interesting defined area | ||
| // (cloud_min_angle < area < cloud_max_angle). |
There was a problem hiding this comment.
Please change this comment to <= area <=
| azimuth >= sensor_configuration_->cloud_min_angle * 100))) { | ||
| for (int firing_seq = 0, k = 0; | ||
| firing_seq < | ||
| std::max(static_cast<long>(SensorT::firing_sequences_per_block), static_cast<long>(1)); |
There was a problem hiding this comment.
You are casting to long but firing_seq is an int, so passing int is fine.
| azimuth <= sensor_configuration_->cloud_max_angle * 100) || | ||
| (sensor_configuration_->cloud_min_angle > sensor_configuration_->cloud_max_angle && | ||
| (azimuth <= sensor_configuration_->cloud_max_angle * 100 || | ||
| azimuth >= sensor_configuration_->cloud_min_angle * 100))) { |
There was a problem hiding this comment.
For all if-statements that filter out points, please convert them to guard clauses:
Instead of
if (conditionA) {
// ...
if (conditionB) {
// decode point
}
}invert the conditions and skip the point, resulting in much less nested code:
if (!conditionA) {
continue;
}
// ...
if (!conditionB) {
continue;
}
// decode point|
|
||
| void unpack(const velodyne_msgs::msg::VelodynePacket & velodyne_packet) | ||
| { | ||
| const raw_packet_t * raw = (const raw_packet_t *)&velodyne_packet.data[0]; |
There was a problem hiding this comment.
Never use C-style casts for converting byte arrays to structs. This is undefined behavior.
Use memcpy instead: please refer to the Hesai decoder for a safe implementation.
Please also move parsing to the existing parsePacket member function for readability.
|
Thank you @bgilby59 for refactoring velodyne drivers! Pointcloud Output
🟢Pointcloud Output is looking same as before! It is great refactoring!! Decoder Performance
|
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_commonRemarks
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