Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(nebula_decoders_hesai): implement generic Hesai decoder #42

Merged
merged 28 commits into from
Sep 11, 2023

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Jul 27, 2023

Implement a generic Hesai decoder that can handle all Hesai sensor models.
Fixes #48, fixes #49 and closes #38.

PR Type

  • Improvement
  • Refactoring

Related Links

#38 -- the issue for this PR
#48 -- the first issue that made the unit tests of this PR fail
#49 -- the second issue that made the unit tests of this PR fail

Description

Since sensors from the same vendor often follow similar conventions when it comes to packet structure and data processing steps, a generic decoder can be used for most of the decoding work.
This PR propses such a generic decoder for all Hesai sensors currently supported.

Potential for generalizaiton

Packet formats

For all handled Hesai sensors, the packet structure follows this rough format:

  1. (optional) header: static sensor info and feature flags
  2. body: point data
  3. tail and other appendices: timestamp, operation mode info

Decoding steps

For all handled Hesai sensors, decoding a packet follows these steps:

  1. parse (& validate) packet
  2. decode and append points to pointcloud, iterating over groups of blocks (corresponding to the same firing for multi-return) or single blocks (single-return)
    1. check if block (group) completes scan and swap output buffers
    2. iterate over units in block and decode
      1. get distance from packet
      2. filter by distance
      3. get and correct azimuth and elevation from packet *
      4. compute x, y, z using trigonometry (preferably lookup tables)
      5. compute and correct time offset of point *
      6. assign return type to point (handling possible duplicates from multi-return) *
      7. append point to pointcloud

The steps marked with * are model-specific:

  • angle correction
  • timing correction
  • return type assignment

Angle correction

There are two approaches between all the supported sensors:

  • Calibration file based
  • Correction file based (currently only used by AT128)

For both approaches, the same sin/cos lookup tables can be computed and used.
However, the resolution and calculation of these tables is different.

Calibration based

For each laser channel, a fixed elevation angle and azimuth angle offset are defined in the calibration file.
sin/cos lookup tables are computed at 0.001 deg resolution (the resolution observed in the calibration files) for the whole 360 deg range .

This yields a (360 deg / 0.001 deg) * sizeof(float) = 360000 * 4B = 1.4MB memory footprint per lookup table (of which there are two: sin, cos).

Correction based

While azimuth and elevation correction also have a per-channel component, an additional component depending on azimuth AND channel is present.

This leads to more individual azimuth/elevation values than the calibration-based approach, and for the case of the AT128, the lookup table resolution is around 0.00004 deg.

This yields a memory footprint of 36.9 MB per lookup table.

Timing correction

Timing correction for all sensors follows the same underlying formula:
Given a scan start timestamp $T_{s_i}$, packet start timestamp $T_{p_j}$, block offset $o_{b_k}$ within the packet and channel offset $o_{c_l}$ within the block, the point identified by $(i, j, k, l)$ has the relative scan timestamp $t_{i,j,k,l} = T_{p_j} - T_{s_i} + o_{b_k} + o_{c_l}$.

The block offset follows a formula linear in the block index for all sensor models which addidionally depends on the number of returns of the currently active return_mode.

The channel offset is given as a formula, table or set of tables for all sensors. A few sensors' formula is influenced by factors such as high resolution mode (128E3X, 128E4X), alternate firing sequences (QT128) and near/farfield firing (128E3X).

Return types

While there is a wide range of different supported return modes (e.g. single (first), single (strongest), dual (first, last), etc.) their handling is largely the same.
Differences only arise in multi-return (dual or triple) in the output order of the returns, and in the handling of some returns being duplicates (e.g. in dual(first, strongest), the first return coincides with the strongest one).

Here is an exhaustive list of differences:

  • For Dual (First, Last) 0x3B, 128E3X, 128E4X and XT32 reverse the output order (Last, First)
  • For Dual (Last, Strongest) 0x39, all sensors except XT32M place the second strongest return in the even block if last == strongest
  • For Dual (First, Strongest) 0x3c, the same as for 0x39 holds.

For all other return modes, duplicate points are output if the two returns coincide.

The implementation

HesaiPacket

Packets are defined as packed structs for effortless parsing.
The sensor-specific layout for sensor XYZ is defined in PacketXYZ and usually employs an own TailXYZ struct.
The header formats are largely shared between sensors.
The packet body (i.e. point data) is mainly parameterized by bytes per point, points per block, and blocks per body. Thus, parameterized generic structs are used. A few skews such as fine azimuth blocks and blocks with a start-of-block (SOB) header exist and are implemented as their own structs.

HesaiPacket diagram

HesaiSensor

Each sensor model has its own class PandarXYZ : HesaiSensor<...> that defines packet type and timing, and return mode handling logic. Angle correction is the same for 90% of sensors and thus outsourced into AngleCorrector and subclasses. These are template arguments for HesaiSensor.
Return mode handling has a default implementation that is supplemented by additional logic only in 3 sensors.

AngleCorrector

The angle corrector has three main tasks:

  • compute corrected azimuth/elevation for given azimuth and channel
  • implement hasScanCompleted() logic that decides where one scan ends and the next starts
  • compute and provide lookup tables for sin/cos/etc.

The two angle correction types are calibration-based and correction-based. In both approaches, a file from the sensor is used to extract the angle correction for each azimuth/channel.
For all approaches, cos/sin lookup tables in the appropriate size (360deg) and resolution (either 1/1000th deg or 1/25600th deg) are generated.
These resolutions come from the resolutions of the calibration file angle offsets (1/1000th deg) and the fine azimuth resolution of AT128 (1/15600th deg).

HesaiDecoder<SensorT>

The decoder is in charge of the control flow and shared decoding steps of all sensors.
It is a template class taking a sensor type SensorT from which packet type, angle correction etc. are deducted at compile time.
Thus, this unified decoder is an almost zero-cost abstraction.

Its tasks are:

  • parsing an incoming packet
  • managing decode/output point buffers
  • converting all points in the packet using the sensor-specific functions of SensorT where necessary

Review Procedure

Remarks

Current progress

Task Done? Notes
Visually correct output ✔️
Angle correction working ✔️ currently with slight deviations from the previous output, see comment below
Timing correction working ✔️ incl. hires-mode support for 128E3X/128E4X
Return type logic ✔️ incl. first/strongest becoming first_strongest/secondstrongest for some sensors
Documentation ✔️ API sufficiently documented (imho), design document integrated in nebula docs
Finalize design 🟡 discussion ongoing
Test on realworld data ✔️ validated 40P, 64, AT128, QT64, XT32 (others not available)
Unit tests passing ✔️ Adapted the original decoders to hotfix #48 and #49 and regenerated unit test reference data. With the new data, tests pass. For details, see comment below
Performance measurement ✔️ Faster or equal to before for all sensors

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.

@mojomex mojomex changed the title Implement generic Hesai decoder [WIP] refactor(nebular_decoders_hesai): implement generic Hesai decoder [WIP] Jul 27, 2023
@mojomex mojomex changed the title refactor(nebular_decoders_hesai): implement generic Hesai decoder [WIP] refactor(nebula_decoders_hesai): implement generic Hesai decoder [WIP] Aug 4, 2023
@mojomex
Copy link
Collaborator Author

mojomex commented Aug 7, 2023

@amc-nu @drwnz The implementation is feature complete and mostly documented, feel free to have a look. Design needs small cleanups in some spots but it should be fairly readable.

Unit tests now expect a `dual_return_distance_threshold == 0.1` and
the `<=` resp. `>=` operators for excluding points under/over a limit.
@mojomex
Copy link
Collaborator Author

mojomex commented Aug 17, 2023

Since all unit tests are passing now (*with some caveats, see below), I will mark this PR as ready and open the discussion!

✔️ Unit tests

The unit test reference point clouds had some issues that prevented this new decoder from passing tests, see #48 and #49.
To generate new, consistent reference data, I modified the original decoders in the following ways and then re-generated the unit test pointclouds using them:

  • Use the same comparison operators for min/max range check, dual return threshold in all decoders
  • Set dual_return_distance_threshold = 0.1 in the unit tests (was unset (=0) before) to match the hesai_launch_all_hw.xml launch file
  • Change output order for XT32M for multi-return to match the other sensors (was block-major, now is column-major within the return group, i.e. the two returns from one channel will be output next to each other now, whereas they had an offset of #channels before)
  • Remove weird logic for checking if a scan is complete in XT32M (involved timing for some reason)
  • Fix bug in XT32 where in dual return, points are always checked against the 1st block (instead of the 1/3/5th block for the 2/4/6th block respectively)

Further, I do not test for float equivalence between reference and decoder pointcloud, but allow for some error. Also, I do not test for x/y/z equivalence but rather compute distance, azimuth and elevaion, mainly for easier troubleshooting. This can be changed back once testing is complete.

Has been reverted back, now that unit tests pass.

✔️ Angle correction differences

There is currently a deviation between the angles my code computes and those in the unit test reference data.
I am very sure this is due to rounding errors either in the original or in my implementation but I am still investigating.

This has been due to float vs. double precision and multiplication order of floats in distance calculations. Fixed.

✔️ Performance

There has not yet been any performance tuning on the new implementation.
Performance has been optimized so decoding for every sensor is at least as fast as before, in some cases much faster. The current runtimes (:small_orange_diamond:) compare as follows to the old ones (:small_blue_diamond:):

40p 64
at128 qt64
xt32

Currently, only AT128 is slower, due to increased overhead in angle decoding, which I am currently optimizing. While AT128 has already benefitted from #37, no performance gain (but also no regression) is expected for this PR. For other sensors, similar gains to #37 can already be observed above.
AT128's old way of computing the current field (≈ currently active mirror) was slow, it is optimized now.

@mojomex mojomex marked this pull request as ready for review August 17, 2023 07:37
@mojomex mojomex changed the title refactor(nebula_decoders_hesai): implement generic Hesai decoder [WIP] refactor(nebula_decoders_hesai): implement generic Hesai decoder Aug 17, 2023
If a scan is completed mid-packet, the new scan's timestamp is neither that of the current, nor that of the next packet.
Rather, it is the current packet's timestamp plus the minimum time offset
of any point in the remainder of the packet.
Instead of separating azimuth/elevation correction and sin/cos lookups,
these values are now all returned by `getCorrectedAngleData`.

The `return_units` vector is now allocated once per return group,
instead of every unit.

`findField` for AT128 now contains not modulos or arithmetic and thus
is significantly faster.
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2023

Codecov Report

Patch coverage: 47.61% and project coverage change: -1.36% ⚠️

Comparison is base (918657b) 13.35% compared to head (9b917ec) 12.00%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   13.35%   12.00%   -1.36%     
==========================================
  Files         111       61      -50     
  Lines       10989     7830    -3159     
  Branches     1725     1517     -208     
==========================================
- Hits         1468      940     -528     
+ Misses       8345     5789    -2556     
+ Partials     1176     1101      -75     
Flag Coverage Δ
differential 12.00% <47.61%> (?)
total ?

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

Files Changed Coverage Δ
...ula_common/include/nebula_common/nebula_common.hpp 0.00% <ø> (ø)
...s/nebula_decoders_hesai/decoders/pandar_128e3x.hpp 0.00% <0.00%> (ø)
...s/nebula_decoders_hesai/decoders/pandar_128e4x.hpp 0.00% <0.00%> (ø)
...rs/nebula_decoders_hesai/decoders/pandar_qt128.hpp 0.00% <0.00%> (ø)
nebula_tests/hesai/hesai_ros_decoder_test_40p.cpp 25.62% <0.00%> (-1.30%) ⬇️
nebula_tests/hesai/hesai_ros_decoder_test_xt32.cpp 25.62% <0.00%> (-1.30%) ⬇️
...rs/nebula_decoders_hesai/decoders/pandar_at128.hpp 16.66% <16.66%> (ø)
...ecoders/src/nebula_decoders_hesai/hesai_driver.cpp 25.92% <25.92%> (ø)
...oders/nebula_decoders_hesai/decoders/pandar_64.hpp 40.00% <40.00%> (ø)
...ers/nebula_decoders_hesai/decoders/pandar_qt64.hpp 40.00% <40.00%> (ø)
... and 14 more

... and 58 files with indirect coverage changes

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

@drwnz drwnz requested review from amc-nu and drwnz August 21, 2023 08:02
Copy link
Collaborator

@drwnz drwnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

@drwnz drwnz merged commit ea482a8 into tier4:main Sep 11, 2023
6 checks passed
@mojomex mojomex deleted the generic-hesai-decoder branch November 6, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants