fix(nebula_decoders_hesai) unify pointcloud order and filtering of all sensors #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The XT32 decoder, in dual return mode, employs a check whether the two returned points are identical. The way the point for comparison was indexed was erroneous and ended up always being the 1st or 2nd block of the packet, although there are a total of 8 blocks.
This field was missing earlier, leading to avoidable troubleshooting.
The XT32 decoder did not classify First+Last, First+Strongest as dual return in its check. Added the return mode constants to the header and to the check
The Pandar40P decoder decoded points in firing time order. Since all other sensors use logical ordering (by ascending index), Pandar40P is adapted to this convention.
LASER_RETURN_DISTANCE was defined as float, is now double. This brings Pandar40P distance accuracy up to the level of all other sensors, which calculate distance on the fly in higher precision.
Other sensors output in `return_group > channel > block` order, the XT32M decoder was outputting `block > channel`. The XT32M scan completion check involved timing checks. These are currently unused and complicate the code. Further, they prevent a generic implementation. Thus, the check has been streamlined.
Fixes tier4#49, fixes tier4#48. Multiple decoders (QT64, QT128, 64, XT32, 128) had distance checks using comparison operators different to the other sensors (e.g. `>` instead of `>=`). Pandar40P had no min_range / max_range checks. AT128, XT32, XT32M had no dual_return_threshold checks. Identical points were not removed when dual_return_distance_threshold = 0. All of these checks are corrected/added with this commit.
The sin/cos azimuth lookup tables did not include the channel-wise azimuth offset in their calculation, leading to up to 0.166 deg of azimuth error in the output X/Y values. The tables have been replaced with sinf/cosf calls.
Unit tests for Hesai decoders were not setting `dual_return_distance_threshold` at all, causing it to be `0` implicitly. This caused a large number of duplicate points to be output by the decoders. The distance is set to `0.1 [m]` for all Hesai sensors now, which is the default in the Hesai launch file.
mojomex
force-pushed
the
unified_distance_checks
branch
from
9bd1284
to
4ee86a5
Compare
Codecov ReportPatch coverage:
❗ 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 #56 +/- ##
==========================================
+ Coverage 13.35% 16.17% +2.81%
==========================================
Files 111 54 -57
Lines 10989 8706 -2283
Branches 1725 1735 +10
==========================================
- Hits 1468 1408 -60
+ Misses 8345 6104 -2241
- Partials 1176 1194 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
This PR will be made obsolete by #42 but it still serves as documentation on how unit test reference data was changed and how the new, generic decoder handles range checks etc. |
|
Superseeded by #38 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unified output and filtering behavior for all Hesai decoders, fixes #48 (duplicate points in output), fixes #49 (incoherent distance checks).
PR Type
Related Links
#48 -- duplicate points being output
#49 -- range checks not being consistent
Description
This PR unifies the behavior of all Hesai decoders to produce outputs with
return_group > channel > blockpoint.distance != 0point.distance in [min_range, max_range]dist(point1, point2) >= dual_return_distance_thresholdfor dual returnThe unit tests are regenerated to match the new conventions.
Detailed changes
Below are the detailed changes made, grouped by sensor.
For details on which comparison operators have been changed, please refer to the table in #49.
For details on how this PR affects output point counts (in the unit test data), see the table in #48.
XT32M
return_group > channel > blockfromblock > channelto match the other sensors40P
LASER_RETURN_TO_DISTANCE_RATEis changed todoubleto match the precision of the calculations of other sensorsmin_rangeandmax_rangedistance checks are addedXT32
AT128
Review Procedure
Remarks
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