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

refactor(hesai): per-sensor diagnostic struct definitions #208

Merged
merged 30 commits into from
Nov 8, 2024

Conversation

ike-kazu
Copy link
Contributor

@ike-kazu ike-kazu commented Oct 4, 2024

PR Type

  • Bug Fix

Related Links

Description

Hesai LiDAR TCP errors are occurred. To fix it, TCP Structs and other processes are fixed in this pr.

Review Procedure

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 changed the title refactor(hesai): hesai TCP refactor(hesai): per-sensor diagnostic struct definitions Oct 10, 2024
@mojomex mojomex self-requested a review October 10, 2024 01:12
Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ike-kazu, thank you for your work!
There are a few changes, mainly for duplicated code, remaining debug statements, and code style. I will test the PR for functionality once the comments are addressed. Thank you :D

Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!
I left a few small comments, please address them!

@mojomex
Copy link
Collaborator

mojomex commented Oct 28, 2024

@ike-kazu Thank you!
I'll test with real sensors this week, after which we can merge.

@mojomex mojomex self-assigned this Oct 29, 2024
@mojomex
Copy link
Collaborator

mojomex commented Oct 30, 2024

Evaluation

I have reduced the remaining log spam to a minimum and implemented the missing QT128 support.
I also added sensor-specific parsing of HesaiInventory.
Due to the datasheets available to me not being quite up-to-date, some of the sensors still cannot be parsed correctly and some of the diagnostics (listed below the first table) are still incorrect.

The sensors we currently use in-house (Pandar40P, XT32, AT128, QT128, OT128) are all working as expected apart for some minor issues listed below.

No PandarQT64 was available for testing.

Summary

Criterion Pandar40P Pandar64 XT32 AT128 QT128 OT128
All structs parsed warning-free? 🟢 🔴 🔴 🟢 🟢 🔴
All diagnostics correct? 🟢 🔴 🔴 🟢 🟢 🟢
PTP diagnostics correct? 🟢 🔴 🟢 🟢 🟢 🟢
Nebula running without errors? 🟢 🟢 🟢 🟢 🟢 🟢
  • Pandar64: startup_times, total_operation_time, ptp_status always 0
  • XT32: input_current, input_power have wrong values
  • OT128: inventory returned from sensor has many more bytes than what the datasheet says, but all parsed information is correct.

Diagnostics Output

Model PTP off PTP on
Pandar40P Screenshot from 2024-10-30 15-16-43 Screenshot from 2024-10-30 15-17-18
Pandar64 Screenshot from 2024-10-30 15-12-02 Screenshot from 2024-10-30 15-15-19
XT32 Screenshot from 2024-10-30 15-08-40 Screenshot from 2024-10-30 15-09-15
AT128 Screenshot from 2024-10-30 15-18-41 Screenshot from 2024-10-30 15-19-13
QT128 Screenshot from 2024-10-30 16-36-25 Screenshot from 2024-10-30 16-37-05
OT128 Screenshot from 2024-10-30 15-20-50 Screenshot from 2024-10-30 15-21-11

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 305 lines in your changes missing coverage. Please review.

Project coverage is 26.12%. Comparing base (a908f61) to head (83a226a).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
.../nebula_hw_interfaces_hesai/hesai_cmd_response.hpp 0.00% 184 Missing ⚠️
nebula_ros/src/hesai/hw_monitor_wrapper.cpp 0.00% 63 Missing ⚠️
.../nebula_hesai_hw_interfaces/hesai_hw_interface.cpp 0.00% 54 Missing ⚠️
nebula_ros/src/hesai/hesai_ros_wrapper.cpp 0.00% 2 Missing ⚠️
.../include/nebula_common/util/string_conversions.hpp 0.00% 1 Missing ⚠️
nebula_ros/src/hesai/hw_interface_wrapper.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   26.16%   26.12%   -0.04%     
==========================================
  Files          99      100       +1     
  Lines        9198     9212      +14     
  Branches     2215     2216       +1     
==========================================
  Hits         2407     2407              
- Misses       6401     6416      +15     
+ Partials      390      389       -1     
Flag Coverage Δ
differential 26.12% <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.

Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed with @drwnz in person, I will merge this myself.

@mojomex mojomex merged commit a5fb803 into tier4:main Nov 8, 2024
10 of 12 checks passed
mojomex added a commit that referenced this pull request Nov 26, 2024
* fix hesaiconfig ptp

* chagnge struct name

* refactor lidaStatus

* fix lidarstatus tcp

* fix lidarmonitor and some issues

* fix lidarstatus binary issue

* refactor lidarConfig

* fix any codes

* fix as review

* fix reviewd points

* fix reviewd point

* chore(hesai): reduce log spam

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai_cmd_response): make all inheritances public

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): correct sensor get command return logic after merge

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): add missing diagnostics summary, display strings without quotes

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): fix diagnostics keys being output in the wrong categories

Signed-off-by: Max SCHMELLER <[email protected]>

* feat(hesai): change PTP status to ERROR when not synchronized

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai): remove duplicate config printing

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai): remove unnecessary print statements

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai): lessen log spam, error messages

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): add back support for  pandar64

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): add back support for QT128

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai): disable voltage monitor output for sensors that don't support it

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai_cmd_response): reduce compiler warnings

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(hesai_hw_interface): for sensors we couldn't test, fall back to using the simplest available structs to parse

Signed-off-by: Max SCHMELLER <[email protected]>

* chore(hesai_hw_interface): remove temporary cxxabi usage

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(nebula_common): properly add nlohmann_json dependency

Signed-off-by: Max SCHMELLER <[email protected]>

* fix(pandar64): add calibration_file to schema

Signed-off-by: Max SCHMELLER <[email protected]>

---------

Signed-off-by: Max SCHMELLER <[email protected]>
Co-authored-by: Max SCHMELLER <[email protected]>
Co-authored-by: Max Schmeller <[email protected]>
Signed-off-by: Max SCHMELLER <[email protected]>
@mojomex mojomex mentioned this pull request Nov 29, 2024
5 tasks
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