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

feat(robosense): add Robosense M1 support #105

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Dec 5, 2023

PR Type

  • New Feature

Related Links

This PR depends on:

Description

⚠️ Needs #104 to be merged first!
After #104 is merged, perform rebase -i HEAD~8 --onto main on this branch to get rid of the duplicated commits in this PR.

This PR adds support for Robosense M1. Similarly to Helios and Bpearl this driver does not support setting return mode from config files (c.f. #77). This has to instead be done through the sensor's web interface.

_m1_azimuth
azimuth
_m1_elevation
elevation
_m1_channel
channel
_m1_return_type
return_type
_m1_distance
distance
_m1_intensity
intensity
_m1_timestamp
timestamp
m1_perf
performance*

*: Each M1 scan message has exactly 1 packet so decode performance looks much better here than it would be when aggregated for a full scan.

Review Procedure

Test all M1-specific functionality, especially pointcloud output and performance. C.f. the analysis in #77.

Remarks

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 force-pushed the add-robosense-m1-support branch from 2a069ad to 07ffa64 Compare December 5, 2023 09:50
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

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

Comparison is base (810849e) 6.43% compared to head (fbefba8) 8.00%.
Report is 1 commits behind head on main.

Files Patch % Lines
...decoders/nebula_decoders_robosense/decoders/m1.hpp 0.00% 88 Missing ⚠️
..._decoders_robosense/decoders/robosense_decoder.hpp 0.00% 85 Missing ⚠️
...robosense_hw_interfaces/robosense_hw_interface.cpp 0.00% 26 Missing ⚠️
.../nebula_decoders/nebula_decoders_common/sensor.hpp 0.00% 22 Missing ⚠️
...ders/nebula_decoders_robosense/decoders/helios.hpp 0.00% 22 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 ⚠️
...src/nebula_decoders_robosense/robosense_driver.cpp 0.00% 17 Missing ⚠️
...bula_decoders_robosense/decoders/bpearl_common.hpp 0.00% 16 Missing ⚠️
...rs/nebula_decoders_common/sensor_mixins/angles.hpp 0.00% 11 Missing ⚠️
... and 14 more

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

Additional details and impacted files
@@           Coverage Diff            @@
##            main    #105      +/-   ##
========================================
+ Coverage   6.43%   8.00%   +1.57%     
========================================
  Files        136      78      -58     
  Lines      10895    8308    -2587     
  Branches     854     844      -10     
========================================
- Hits         701     665      -36     
+ Misses      9618    7069    -2549     
+ Partials     576     574       -2     
Flag Coverage Δ
differential 8.00% <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 mojomex force-pushed the add-robosense-m1-support branch from 07ffa64 to 052e883 Compare December 6, 2023 08:17
Currently there is a lot of code duplication happening between sensor models, e.g. for accessing specific fields, determining return types, etc. Furthermore, exotic/non-standard sensors require extra branches or parameters in the decoder.

This commit introduces sensor mixins and generic field accessors, which together can be used to compose a wide range of sensor implementations.

Currently handled functionalities:
* Generic accessors for packet blocks and units
* Generic accessors for boost::endian and native struct members
* Mixin for getting point azimuth/elevation
* Mixin for getting channel IDs
* Mixin for getting distance units, distances
* Mixin for getting point intensity
* Mixin for getting sensor return mode
* Mixin for checking scan completion (not limited to angle-based checks)
* Mixin for getting packet and point timestamps
* Mixin for point pre-validation (e.g. distance !=0 etc.)

Sensors implemented using this system must inherit from SensorPase<PacketT>, as well as from all of the used generic/specific mixins they are composed of.
…ntation, rewrite decoder

In anticipation of the Robosense M1 PR, the `robosense_decoder.hpp` had to become more generic to support non-angle-based scan cutting along with completely different mechanisms for angle data, dual return and timestamps.

Thus, Robosense is the first vendor to be moved to the new composable sensor model, starting with Helios and Bpearl.

* Introduce decode groups: multiple packets can be grouped and decoded together, as is necessary for RSM1
* Packet/Block/Unit stride definition for generically handling different return group layouts
* Decoder now uses generic sensor member functions to access all point data

* Rewrite sensor classes to use SensorBase and mixins
* Add SensorInfo classes for the info decoder
* Currently, timing and return types are not fully re-implemented
* `angle_corrector.hpp` is deleted in favor of sensor mixins

* The driver now instantiates sensor objects passed to the decoder
@mojomex mojomex force-pushed the add-robosense-m1-support branch from 206c884 to 0ff52d8 Compare December 7, 2023 06:51
@mojomex
Copy link
Collaborator Author

mojomex commented Dec 8, 2023

🟡 gPTP Sync

gPTP seems to work with the settings from the M1 datasheet 👍
However, the DIFOP values for Time synchronization mode setting do not seem to be accurate: we are receiving 0x03 when the datasheet (2021-12-10) only documents 0x00, 0x01 and 0x02. The Helios datasheet does specify 0x03 as gPTP, and given that Wireshark shows gPTP working, I'll tentatively use Helios' values in the M1 decoder.
Time synchronization status seems to be correctly documented for M1.

@mojomex mojomex force-pushed the add-robosense-m1-support branch from 221ae59 to aaed2d6 Compare December 26, 2023 09:04
@mojomex mojomex marked this pull request as ready for review December 28, 2023 05:31
@mojomex mojomex requested review from amc-nu and drwnz December 28, 2023 05:35
@mojomex mojomex self-assigned this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants