refactor(robosense): composable sensor behavior

[mojomex]

PR Type

• New Feature
• Improvement

Related Links

This PR depends on:

• feat: add robosense support #77

Description

Currently there is a lot of code duplication happening between sensor models (and even more between vendors), e.g. for accessing specific fields, determining return types, etc. Furthermore, sensors with non-ring-based scan patterns (such as the Robosense M1) require extra logic and parameters in the decoder.

This PR introduces sensor mixins and generic field accessors, which together can be used to compose a wide range of sensor implementations. This is the first step towards the ultimate goal of having one decoder handle (almost) all vendors and sensor models.

Here is a before/after comparison of decoding functionality:

Type	Before	With this PR
Blocks, Units	Decoder dictated packet structure	Generic accessors, packet-structure-agnostic decoder
Endianness	Hardcoded boost::endian accessors	Generic accessors for boost::endian and native struct members
Point Angles	Supported only block/channel-id based angles	Mixin for getting point azimuth/elevation
Channel ID	Channel ID inferred from packet position	Mixin for getting channel IDs
Distance	Template mess	Mixin for getting distance units, distances
Intensity	Copied from unit field	Mixin for getting point intensity (allows for scaling/corrections)
Return Mode	Decoder had to support sensor-specific mechanisms	Mixin for getting sensor return mode
Scan Cutting	Angle-based scan cutting	Mixin for checking scan completion (not limited to angle-based checks)
Timestamps	Per-vendor implementation, template mess	Mixin for getting packet and point timestamps
Point Pre-Validation	unit.distance != 0	Mixin for point pre-validation

All mixins have default implementations (opt-in) that correspond to the behavior prior to this PR.

Sensors implemented using this system must inherit from SensorBase, as well as from all of the used generic/specific mixins they are composed of.

Review Procedure

• Compile and verify that Bpearl and Helios are still working
• Rate the design decisions made: if the code is not beautiful, it will create problems with maintainability later

Remarks

The sensor mixins introduced in this PR are a compromise given that we cannot use C++20's concepts. Thus, the code inherently cannot be as beautiful as in modern C++. Still, I feel like this is close to the best we can do in terms of

• readability
• tool support (sadly go to definition does not work in all IDEs even though the sensor inheritance hierarchy is pretty clear)
• performance (although there is heavy use of inheritance, this is compiled out since the decoder is instantiated on the subclass (e.g. Helios instead of SensorBase) and thus no VTables etc. remain)

Still, please let me know any feedback/criticism!!

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]

Performance is still degraded when compared to baseline (PR #77) but I will fix that shortly.

Helios	Bpearl

[codecov-commenter]

Codecov Report

Attention: 269 lines in your changes are missing coverage. Please review.

Comparison is base (810849e) 6.43% compared to head (2fcfef1) 8.11%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
..._decoders_robosense/decoders/robosense_decoder.hpp	0.00%	80 Missing ⚠️
...ders/nebula_decoders_robosense/decoders/helios.hpp	0.00%	22 Missing ⚠️
.../nebula_decoders/nebula_decoders_common/sensor.hpp	0.00%	21 Missing ⚠️
...s/nebula_decoders_robosense/decoders/bpearl_v3.hpp	0.00%	21 Missing ⚠️
...s/nebula_decoders_robosense/decoders/bpearl_v4.hpp	0.00%	21 Missing ⚠️
...bula_decoders_robosense/decoders/bpearl_common.hpp	0.00%	16 Missing ⚠️
...src/nebula_decoders_robosense/robosense_driver.cpp	0.00%	14 Missing ⚠️
..._decoders_common/sensor_mixins/scan_completion.hpp	0.00%	10 Missing ⚠️
...nse/decoders/angle_corrector_calibration_based.hpp	0.00%	10 Missing ⚠️
...nebula_decoders_common/sensor_mixins/timestamp.hpp	0.00%	8 Missing ⚠️
... and 13 more

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

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #104      +/-   ##
========================================
+ Coverage   6.43%   8.11%   +1.68%     
========================================
  Files        136      77      -59     
  Lines      10895    8192    -2703     
  Branches     854     844      -10     
========================================
- Hits         701     665      -36     
+ Misses      9618    6953    -2665     
+ Partials     576     574       -2     

Flag	Coverage Δ
differential	8.11% <0.00%> (?)
total	?

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

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

[mojomex]

🟢 Performance fixed

Performance now matches PR #77 within 2%.
The main culprit was the std::vector<const unit_t *> in convertReturnGroup. It was there long before, however, this PR calls this function once per point rather than once per block. Thus, dynamic allocation became quite costly. This is now solved by using boost::container::static_vector which uses static allocation, boost -ing performance back to pre-PR levels.

Why is Boost necessary (why not use std::array)?

The vector contains pointers to all units in a return group. Return groups are dynamically sized (1 for single return, 2 for dual, etc.). while they do have an upper size limit (the highest number of returns a sensor supports), the exact size is not known at compile time (which is a requirement for std::array). One could use an upper bound-sized array and keep track of the actual items stored manually, but given that boost::container::static_vector was created exactly for that purpose, I opted for that as it's cleaner.

The benchmarks

I so far only benchmarked Helios as the optimizations are generic and should apply to all sensors equally.
The rightmost benchmark is the newest commit.

	helios 931fc70	helios_pr105 d9b0746	helios_pr105_boost dc7f88c	helios_pr105_boost_timestamp 1fd80c4
d_total AVG	1.02077	4.61202	1.09336	1.04474
n_out AVG	47285.3	47285.8	47285.1	47286
d_total STD	0.0733364	0.149254	0.0582077	0.0612363
n_out STD	47.2906	46.7324	47.5576	47.7135
d_total AVG % rel to helios	100	451.816	107.111	102.348
n_out AVG % rel to helios	100	100.001	99.9995	100.001
d_total STD % rel to helios	100	203.52	79.3708	83.5006
n_out STD % rel to helios	100	98.8196	100.565	100.894