fix(hesai_hw_interface): add error handling #131

mojomex · 2024-03-28T11:26:14Z

PR Type

Improvement
Bug Fix

Description

This PR introduces error checking and handling in hesai_hw_interface.cpp. The following features have been added:

check PTC command error codes and pass them to the calling function instead of the payload in case of error
throw std::runtime_errors in case an error occurs
perform size checks before parsing TCP responses
emit errors on too-large payload size
remove all fire-and-forget type requests, ALWAYS check the error code

The following other things have changed as a result of the fix:

structs in hesai_cmd_response.hpp are now parsed via memcpy, and use boost::endian buffers in the sizes specified by the datasheet(s)
a utility class expected modeled after C++23 std::expected was added to make error handling code more compact
added FIXMEs in places that are wrong but not high priority (do not cause crashes etc.)

Behavior for TCP commands is now as follows:

if any error occurs, a std::runtime_error is thrown with a detailed list of error messages
for some structs that are wrongly parsed in some cases (e.g. HesaiPtpConfig):
- a std::runtime_error is thrown if the payload is too short to memcpy into the struct
- an error is PrintErrored if the payload is too large. This is done because the first few fields of the struct are typically correct, even if the format of later fields differs

Review Procedure

Test TCP with real sensors (at least AT128, OT128 and an older sensor).

Remarks

We should refactor TCP communication in a similar way that was done with mixins in #104. This would make the sensor definition much more readable: one could read the sensor definition alongside the TCP documentation, without boilerplate code. This would provide much better maintainability, and, for new sensors, forces the developer to check every TCP command for changes when implementing.

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.

* check PTC command error codes, throw exception if necessary * perform size checks before parsing responses * emit errors on too-large payload size

mojomex · 2024-03-28T11:26:31Z

(I still have to test this on real sensors)

amc-nu · 2024-04-02T06:30:21Z

Test Result Pandar128E4X

@mojomex @drwnz

The driver works with an actual sensor. Point cloud looks normal

Issues:

I see some output marked as error:
- [hesai_hw_monitor_hesai]: HesaiInventory from Sensor has unknown format. Will parse anyway.
- sn: XXXX, date_of_manufacture: XXXX-XX-XX, mac: ec:9f:XX:XX:XX:XX, sw_ver: 1.3.19, hw_ver: , control_fw_ver: 1.3.10, sensor_fw_ver: 1.3.13, angle_offset: 8169, model: *, motor_type: , num_of_lines: �, 4f 54 31 32 38 2d 42 30 31 00 00
  (SN and MAC hidden on purpose)
The diagnostics page reports 1800V input, might need some check.

[hesai_hw_interface_ros_wrapper_node-2] Starting UDP server on: SensorModel: Pandar128_E4X_OT, FrameID: hesai, HostIP: 255.255.255.255, SensorIP: 192.168.1.201, DataPort: 940, ReturnMode: Dual, Frequency: 0, MTU: 5dc, Use sensor time: 0, GnssPort: 941, ScanPhase:0, RotationSpeed:258, FOV(Start):0, FOV(End):168, DualReturnDistanceThreshold:0.1, PtpProfile:IEEE_1588v2, PtpDomain:0, PtpTransportType:UDP/IP, PtpSwitchType:TSN
[hesai_hw_monitor_ros_wrapper_node-3] 192.168.1.201:9347
[hesai_hw_monitor_ros_wrapper_node-3] [INFO 1712038423.862968919] [TcpSocket::open]: connected (open() at /home/ne0/nebula_ws/src/transport_drivers/tcp_driver/src/tcp_socket.cpp:531)
[hesai_hw_monitor_ros_wrapper_node-3] [ERROR 1712038423.863656262] [hesai_hw_monitor_hesai]: HesaiInventory from Sensor has unknown format. Will parse anyway. (PrintError() at /home/ne0/nebula_ws/src/nebula/nebula_hw_interfaces/src/nebula_hesai_hw_interfaces/hesai_hw_interface.cpp:1331)
[hesai_hw_monitor_ros_wrapper_node-3] HesaiInventory

Parameter	Screenshot
Voltage
Temperature
RPM
No PTP
PTP

amc-nu · 2024-04-02T06:45:25Z

Test PandarXT32

The driver works with an actual sensor. Pointcloud looks normal.

Input current/power 0.0

[hesai_hw_interface_ros_wrapper_node-2] Starting UDP server on: SensorModel: PandarXT32, FrameID: hesai, HostIP: 255.255.255.255, SensorIP: 192.168.1.201, DataPort: 940, ReturnMode: Dual, Frequency: 0, MTU: 5dc, Use sensor time: 0, GnssPort: 941, ScanPhase:0, RotationSpeed:258, FOV(Start):0, FOV(End):168, DualReturnDistanceThreshold:0.1, PtpProfile:IEEE_1588v2, PtpDomain:0, PtpTransportType:UDP/IP, PtpSwitchType:TSN
[hesai_hw_monitor_ros_wrapper_node-3] 192.168.1.201:9347
[hesai_hw_monitor_ros_wrapper_node-3] [INFO 1712039923.485050914] [TcpSocket::open]: connected (open() at /home/ne0/nebula_ws/src/transport_drivers/tcp_driver/src/tcp_socket.cpp:531)
[hesai_hw_monitor_ros_wrapper_node-3] HesaiInventory
[hesai_hw_monitor_ros_wrapper_node-3] sn: XT3XXXXXXXX, date_of_manufacture: XXXX-XX-XX, mac: ec:9f:XX:XX:XX:XX, sw_ver: 0.1.25, hw_ver: 1.0.0, control_fw_ver: 1.1.13, sensor_fw_ver:   1.2.25, angle_offset: 1048, model: , motor_type: , num_of_lines:  , 00 00 00 00 00 00 00 00 00 00 00

Parameter	Screenshot
No PTP
PTP
RPM
Temperature
Voltage

mojomex · 2024-04-08T06:19:01Z

@amc-nu

I see some output marked as error:
[hesai_hw_monitor_hesai]: HesaiInventory from Sensor has unknown format. Will parse anyway.
sn: XXXX, date_of_manufacture: XXXX-XX-XX, mac: ec:9f:XX:XX:XX:XX, sw_ver: 1.3.19, hw_ver: , control_fw_ver: 1.3.10, sensor_fw_ver: 1.3.13, angle_offset: 8169, model: *, motor_type: , num_of_lines: �, 4f 54 31 32 38 2d 42 30 31 00 00
(SN and MAC hidden on purpose)

The actual parsing is the same in this PR as it was before. However, OT128 (and to some extent AT128, possibly others) have slightly different struct definitions from e.g. XT32. As it was not an issue until now, I opted to print an error that we are parsing the wrong data for some fields, rather than doing a proper refactoring at this point.

What has changed however, is that payload size is now checked before parsing into the structs is attempted.

mojomex · 2024-04-08T06:28:17Z

@amc-nu

The diagnostics page reports 1800V input, might need some check.

OT128's TCP API doc lists the input_voltage and input_current fields in a different order than the HesaiLidarMonitor struct at hesai_cmd_response.hpp:480. I think this issue already existed, as I did not change that struct. I do not have access to all TCP API docs of Hesai sensors so I cannot confirm at this point if it is in fact only different for OT128.

I have added FIXMEs to all structs where I found the API docs I do have (OT128 and AT128) to differ from the implementation.

Do you think it is best to add an exception if (sensor_model == OT128) to the parser?

mojomex · 2024-04-08T07:28:57Z

sn: XXXX, date_of_manufacture: XXXX-XX-XX, mac: ec:9f:XX:XX:XX:XX, sw_ver: 1.3.19, hw_ver: , control_fw_ver: 1.3.10, sensor_fw_ver: 1.3.13, angle_offset: 8169, model: *, motor_type: , num_of_lines: �, 4f 54 31 32 38 2d 42 30 31 00 00

Indeed, the output had some other errors as well: I forgot to reset the output modifiers (std::hex etc.) after printing MAC addresses etc. and printed some uint8 numbers as characters instead of numbers. Fixed in e5d0a9c.

Output now:

sn: XXXX, date_of_manufacture: 2023-11-23, mac: ec:xx:xx:xx:xx, sw_ver: 1.4.01, hw_ver: , control_fw_ver: 1.4.1, sensor_fw_ver: 1.4.1, angle_offset: 19817, model: 42, motor_type: 1, num_of_lines: 128, 4f 54 31 32 38 2d 42 30 31 00 00

…type more readable

nebula_common/include/nebula_common/util/expected.hpp

drwnz

Great work!

fix(hesai_hw_interface): add error handling

0d252f3

* check PTC command error codes, throw exception if necessary * perform size checks before parsing responses * emit errors on too-large payload size

mojomex self-assigned this Mar 28, 2024

mojomex marked this pull request as ready for review March 28, 2024 11:37

mojomex requested review from amc-nu and drwnz March 28, 2024 11:37

mojomex mentioned this pull request Mar 28, 2024

AT128 HW interface crashes on GetLidarRange #129

Closed

Merge branch 'main' into hesai-hw-interface-error-handling

3468d96

mojomex mentioned this pull request Apr 2, 2024

fix(hesai_hw_monitor): fix 2s drops issue - again #135

Merged

5 tasks

mojomex and others added 2 commits April 8, 2024 15:06

Merge branch 'main' into hesai-hw-interface-error-handling

e69e313

fix(hesai_hw_monitor_ros_wrapper): fixed wrong range given for S/N copy

6713b0d

fix(hesai): print uint8, uint16 as numbers

decf1ac

mojomex force-pushed the hesai-hw-interface-error-handling branch from e5d0a9c to decf1ac Compare April 8, 2024 07:32

mojomex added 2 commits April 16, 2024 17:57

chore: update cspell ignore

a857d50

chore(velodyne_calibration_decoder): fix spelling

4f022cd

mojomex force-pushed the hesai-hw-interface-error-handling branch from 19e8101 to 4f022cd Compare April 18, 2024 07:50

chore(velodyne_calibration_decoder): fix spelling once and for all

187ad04

mojomex mentioned this pull request Apr 18, 2024

refactor(hesai)!: combine Hesai ROS wrappers into a single node #127

Merged

5 tasks

mojomex and others added 2 commits April 18, 2024 21:06

Merge branch 'main' into hesai-hw-interface-error-handling

02bb644

fix(hesai_hw_interface): add missing check for PTC error, make error …

4d3bf5b

…type more readable

mojomex mentioned this pull request Apr 24, 2024

feat(hesai): add high resolution mode setting for Pandar128 and OT128 #142

Open

5 tasks

Merge branch 'main' into hesai-hw-interface-error-handling

bb97daa

drwnz changed the base branch from main to develop May 17, 2024 04:01

drwnz reviewed May 21, 2024

View reviewed changes

nebula_common/include/nebula_common/util/expected.hpp Outdated Show resolved Hide resolved

drwnz reviewed May 21, 2024

View reviewed changes

nebula_common/include/nebula_common/util/expected.hpp Outdated Show resolved Hide resolved

drwnz approved these changes May 21, 2024

View reviewed changes

fix(expected): make error behavior clearer, add doc comments

14ab32d

mojomex merged commit b8b25a6 into tier4:develop May 21, 2024
6 checks passed

mojomex deleted the hesai-hw-interface-error-handling branch May 21, 2024 07:52

mojomex mentioned this pull request Jul 18, 2024

chore: merge nebula/develop #176

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hesai_hw_interface): add error handling #131

fix(hesai_hw_interface): add error handling #131

mojomex commented Mar 28, 2024 •

edited

Loading

mojomex commented Mar 28, 2024

amc-nu commented Apr 2, 2024

amc-nu commented Apr 2, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading

drwnz left a comment

fix(hesai_hw_interface): add error handling #131

fix(hesai_hw_interface): add error handling #131

Conversation

mojomex commented Mar 28, 2024 • edited Loading

PR Type

Related Links

Description

Review Procedure

Remarks

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-Review Checklist for the PR Author

CI Checks

mojomex commented Mar 28, 2024

amc-nu commented Apr 2, 2024

Test Result Pandar128E4X

amc-nu commented Apr 2, 2024 • edited Loading

Test PandarXT32

mojomex commented Apr 8, 2024 • edited Loading

mojomex commented Apr 8, 2024 • edited Loading

mojomex commented Apr 8, 2024 • edited Loading

drwnz left a comment

Choose a reason for hiding this comment

mojomex commented Mar 28, 2024 •

edited

Loading

amc-nu commented Apr 2, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading

mojomex commented Apr 8, 2024 •

edited

Loading