-
Notifications
You must be signed in to change notification settings - Fork 51
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(hesai)!: combine Hesai ROS wrappers into a single node #127
Conversation
e27acbf
to
dd1bbdc
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #127 +/- ##
==========================================
Coverage ? 11.73%
==========================================
Files ? 85
Lines ? 8882
Branches ? 1190
==========================================
Hits ? 1042
Misses ? 6995
Partials ? 845
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cb2f0c8
to
fdbeb45
Compare
600bc84
to
7fd450b
Compare
🟢 Evaluation🟢 LatencyThis section evaluates latency of this PR (ref. 1bfba05) compared to Nebula's current main branch (ref. 8fe029e). The same data in table form:
The PR has a speedup of 75% (avg-avg) and 154% (max-max) for 🟢 TCP CommunicationBecause the same HWInterface is shared by the decoder, monitor and the HW interface itself, communication is done in one TCP stream.
🟢 Error Logging🟢 This PR includes #131, so parsing errors and TCP errors are printed or result in exceptions thrown with pretty-printed error messages.
🟢 Warnings about pointclouds not being decoded
This is implemented using a watchdog timer that checks every 🟢 Parameter Handling🟢 All parameters are correctly handled on startup. Parameters specific to a sub-wrapper are only defined and accessed if that sub-wrapper is instantiated (e.g. 🟢 Updating parameters via When changing calibration/correction file path during runtime, it will override the configuration downloaded from the sensor, even when the sensor is connected. |
67225bd
to
7a27587
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the new files, please run pre-commit to limit the syntax discrepancies (for example, using braces on newlines for if
statements).
All the other comments are pedantic/can be considered notes for future development (I don't think the larger ones need addressing in this PR - will leave to your discretion).
nebula_examples/src/hesai/hesai_ros_offline_extract_bag_pcd.cpp
Outdated
Show resolved
Hide resolved
nebula_examples/src/hesai/hesai_ros_offline_extract_bag_pcd.cpp
Outdated
Show resolved
Hide resolved
nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp
Outdated
Show resolved
Hide resolved
nebula_examples/src/hesai/hesai_ros_offline_extract_bag_pcd.cpp
Outdated
Show resolved
Hide resolved
65be49b
to
28b42fc
Compare
This is step one of the single node refactoring of Nebula. In this step, only the three Hesai wrapper nodes were combined into one, and no optimizations have been done that are made possible by this refactoring. The next step is to do those optimizations (e.g. get rid of unnecessary pointcloud copying).
…type more readable
1f00fa6
to
18f193a
Compare
…ut colliding filenames
…d improve formatting
PR Type
Related Links
TIER IV INTERNAL LINK -- corresponding issue
TIER IV INTERNAL LINK -- detailed article
This PR depends on the following PR being merged first:
This PR fixes the following issues (for Hesai only):
set_parameter
will ignore everything but the first parameter #108Description
This PR combines the three ROS wrapper nodes (Driver Wrapper, HW Interface Wrapper, HW Monitor Wrapper) into one.
Doing this has multiple maintainability, reliability and performance benefits:
This PR only addresses Hesai. Once this PR has been accepted, others will follow for the other vendors.
Changes Made
transport_drivers
const
qualifier from returned byte vector inasyncReceive
to allowstd::move
ing packet dataHesaiCalibrationConfiguration
andHesaiCorrection
HesaiCalibrationConfigurationBase
type providing a unified interface for parsing/loading/storingHesaiDecoder
std::vector<uint8_t>
insteadlast_phase
correctly as otherwise, sometimes an empty pointcloud is published on startupHesaiDriver
HesaiCalibrationConfigurationBase
instead ofHesaiCalibrationConfiguration
+HesaiCorrection
as the two are used mutually exclusively. The passed-in pointer is type-cast to the correct subtypenebula_ros/hesai
HesaiRosWrapper
containing aHesaiDecoderWrapper
,HesaiHwInterfaceWrapper
andHesaiHwMonitorWrapper
which are not nodesHesaiDecoderWrapper
receives raw packet data fromHesaiRosWrapper
, decodes and publishes it. it also uses the HW interface provided byHesaiHwInterfaceWrapper
to get calibration dataHesaiHwInterfaceWrapper
manages the one and only HW interface, and all sensor configuration getting/settingHesaiHwMonitorWrapper
uses the HW interface provided byHesaiHwInterfaceWrapper
to make diagnostics requests and published diagnostic dataHesaiRosWrapper
registers a callback for incoming UDP packets with the HW interface, maintains a thread-safe packet queue, and calls the decoder in a second thread when packets are in the queueHesaiSensorConfiguration
still exists and is populated inHesaiDecoderWrapper
mt_queue
pop()
andpush()
, as well as a non-blockingtry_push()
capacity
argument, after whichmt_queue::push()
blocks andmt_queue::try_push()
returns instantlyhesai_launch_all_hw.xml
delay_hw_ms
anddelay_monitor_ms
as their communication is blocking and sequential nowhesai_launch_component.launch.xml
nebula_launch.py
hesai_launch_all_hw.xml
should be used insteadHesaiDecoderWrapper
launch_hw:=false
, so HW interface is not instantiated at all in that modeOther changes:
HesaiSensorConfiguration
const
, swapping pointers to update the config if necessary.Review Procedure
I structured the changelog above roughly in the order the files appear in
Files changed
on GitHub for easier back-referencing.The sensor driver should behave as it did before, so testing sensor setup and pointcloud output with real sensors or PCAPs is advised. Also, testing with
ros2 bag record /pandar_packets
andros2 bag play
should be performed.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