feat(hesai): add filtered pointcloud counter function #247

ike-kazu · 2024-12-20T06:38:56Z

PR Type

Improvement

Description

This pr will be able to watch filtered pointcloud's count by each filtering such as distance, fov and so on. It is usefull for finding filtering error while seeking causes of lidar pointclouds error.

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.

codecov · 2024-12-20T06:50:48Z

Codecov Report

Attention: Patch coverage is 98.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 26.13%. Comparing base (97959dd) to head (d83d65c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/nebula_decoders_hesai/decoders/hesai_decoder.hpp	98.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
+ Coverage   26.07%   26.13%   +0.05%     
==========================================
  Files         101      104       +3     
  Lines        9232     9429     +197     
  Branches     2213     2242      +29     
==========================================
+ Hits         2407     2464      +57     
- Misses       6436     6578     +142     
+ Partials      389      387       -2

Flag	Coverage Δ
differential	`26.13% <98.33%> (?)`
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

Thanks for the PR! Here is the review so far. Performance looks good but there are some more counters and naming changes I'd like to request 🙇

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

mojomex · 2024-12-23T08:19:51Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

@@ -36,6 +37,52 @@
 namespace nebula::drivers
 {

+struct HesaiDecodeFilteredInfo


Please also add counters for

invalid points (distance == 0), line 198

points filtered due to dual_return_threshold (line 238)

points filtered due to identical return type (line 217)

points kept in total (= not filtered) (shall be equal to the final pointcloud size)

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

mojomex · 2024-12-23T08:22:09Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  NebulaPointCloud point_timestamp_start;
+  NebulaPointCloud point_timestamp_end;
+
+  void clear()


Please make sure that all fields are reset (e.g. timestamp_counter is missing)

mojomex · 2024-12-23T08:27:38Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  float distance_start = 0;
+  float distance_end = 0;
+  float raw_azimuth_start = 0;
+  float raw_azimuth_end = 0;
+  std::uint32_t packet_timestamp_start = 0;
+  std::uint32_t packet_timestamp_end = 0;
+  NebulaPointCloud point_azimuth_start;
+  NebulaPointCloud point_azimuth_end;
+  NebulaPointCloud point_timestamp_start;
+  NebulaPointCloud point_timestamp_end;


Please rename from start/end to min/max.

Please also add unit suffixes like _ns for nanoseconds, _rad for radians, _m for meters etc.

packet_timestamp_min/max should probably have type uint64_t (uint32_t cannot represent absolute timestamps in nanoseconds)

Instead of point_, please rename to cloud_ so that it is clear that those values are among the points that were not filtered.
I would suggest replacing raw_ with packet_ as well, so we have packet_ (before filtering) vs. cloud_ (after filtering).

mojomex · 2024-12-23T08:30:00Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+    j["distance_counter"] = distance_counter;
+    j["fov_counter"] = fov_counter;
+    j["timestamp_counter"] = timestamp_counter;


For filters, let's output them as a JSON array instead:

"filter_pipeline": [ { "filter": "distance", "count": 50 }, { "filter": "fov", "count": 120 } ]

mojomex

@ike-kazu Thanks for the changes, I have a few more small requests to polish everything!

After implementing the changes, could you also provide a self-evaluation (you running Nebula with different parameters for min_range/max_range, cloud_min_angle, cloud_max_angle, dual_return_distance_threshold and showing the output JSON diagnostics?

Thank you!

mojomex · 2025-01-06T05:17:23Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

@@ -26,6 +26,8 @@
 #include <rclcpp/logging.hpp>
 #include <rclcpp/rclcpp.hpp>

+#include <sys/types.h>


This should be #include <cstdint> probably

mojomex · 2025-01-06T05:18:09Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  uint16_t timestamp_filtered_count = 0;
+  uint16_t invalid_point_count = 0;
+  uint16_t multiple_return_point_count = 0;
+  uint16_t mutliple_return_point_count = 0;


Please remove this misspelled line

Ah, I guess this should be the counter for filtering identical points? Please rename to identical_filtered_count

mojomex · 2025-01-06T05:21:46Z

ros2_socketcan

Please remove this submodule

mojomex · 2025-01-06T05:21:57Z

transport_drivers

Please remove this submodule

mojomex · 2025-01-06T05:26:11Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  uint16_t distance_filtered_count = 0;
+  uint16_t fov_filtered_count = 0;
+  uint16_t timestamp_filtered_count = 0;
+  uint16_t invalid_point_count = 0;
+  uint16_t multiple_return_point_count = 0;
+  uint16_t mutliple_return_point_count = 0;
+  uint16_t total_kept_point_count = 0;
+  uint16_t invalid_packet_count = 0;


Given that there can be a few 100k points in a pointcloud, uint16_t (only going up to 65535) could easily overflow.

Generally try to use uint64_t for counters.

mojomex · 2025-01-06T05:44:53Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+{
+  uint16_t distance_filtered_count = 0;
+  uint16_t fov_filtered_count = 0;
+  uint16_t timestamp_filtered_count = 0;


Nebula does not filter by timestamp, so this counter can be removed.

mojomex · 2025-01-06T05:45:31Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  uint16_t fov_filtered_count = 0;
+  uint16_t timestamp_filtered_count = 0;
+  uint16_t invalid_point_count = 0;
+  uint16_t multiple_return_point_count = 0;


Please rename to multiple_return_filtered_count

mojomex · 2025-01-06T06:07:31Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+    nlohmann::json distance_j;
+    distance_j["filter"] = "distance";
+    distance_j["distance_filtered_count"] = distance_filtered_count;
+    distance_j["cloud_distance_min_m"] = cloud_distance_min_m;
+    distance_j["cloud_distance_max_m"] = cloud_distance_max_m;
+    nlohmann::json fov_j;
+    fov_j["filter"] = "fov";
+    fov_j["fov_filtered_count"] = fov_filtered_count;
+    fov_j["cloud_azimuth_min_rad"] = cloud_azimuth_min_rad;
+    fov_j["cloud_azimuth_max_rad"] = cloud_azimuth_max_rad;
+    nlohmann::json timestamp_j;
+    timestamp_j["filter"] = "timestamp";
+    timestamp_j["timestamp_filtered_count"] = timestamp_filtered_count;
+    timestamp_j["packet_timestamp_min_ns"] = packet_timestamp_min_ns;
+    timestamp_j["packet_timestamp_max_ns"] = packet_timestamp_max_ns;
+    nlohmann::json invalid_j;
+    invalid_j["filter"] = "invalid";
+    invalid_j["invalid_point_count"] = invalid_point_count;
+    invalid_j["invalid_packet_count"] = invalid_packet_count;
+    nlohmann::json identical_j;
+    identical_j["filter"] = "identical";
+    identical_j["multiple_return_point_count"] = multiple_return_point_count;
+    nlohmann::json multiple_j;
+    multiple_j["filter"] = "multiple";
+    multiple_j["mutliple_return_point_count"] = mutliple_return_point_count;


Please separate the measured pointcloud bounds (distance/azimuth/time) from the filter pipeline information.
Also make sure that the keys for each filter are called "name" and "filtered_count", regardless of filter type:

"filter_pipeline": [ {"name": "distance", "filtered_count": 300}, ... ], "pointcloud_bounds": { "azimuth_deg": { "min": 0.0, "max": 270.0 }, // same with timestamp and distance }

invalid_packet_count has very different causes than invalid_point_count, so I'd like to keep them separate.
Make it a top-level JSON property instead (same level as total_kept_point_count.

mojomex · 2025-01-06T06:07:56Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

    return j;
  }
+
+  void get_minmax_info(const NebulaPoint & point)


Please rename to update_pointcloud_bounds for more clarity.

mojomex · 2025-01-06T06:09:10Z

nebula_decoders/include/nebula_decoders/nebula_decoders_hesai/decoders/hesai_decoder.hpp

+  float cloud_distance_max_m = 0;
+  float cloud_azimuth_min_rad = 0;
+  float cloud_azimuth_max_rad = 0;
+  uint64_t packet_timestamp_min_ns = 0;


For easier reaability, please make these deg instead of rad and convert accordingly in the get_minmax_info function below.

ike-kazu and others added 2 commits December 20, 2024 15:15

add filterd pointcloud counter function

52eb0a2

ci(pre-commit): autofix

4379451

ike-kazu added 2 commits December 20, 2024 15:51

del submodule

1e366d6

fix type

c20d6bf

mojomex requested changes Dec 23, 2024

View reviewed changes

ike-kazu and others added 5 commits December 27, 2024 15:27

fix reviewed points

5cd9da2

ci(pre-commit): autofix

aaa3495

fix reviewed points

ff8b7f5

ci(pre-commit): autofix

2706c08

fix typo

d83d65c

mojomex requested changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hesai): add filtered pointcloud counter function #247

feat(hesai): add filtered pointcloud counter function #247

ike-kazu commented Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

mojomex left a comment

mojomex Dec 23, 2024

mojomex Dec 23, 2024

mojomex Dec 23, 2024

mojomex Dec 23, 2024

mojomex left a comment

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

mojomex Jan 6, 2025

feat(hesai): add filtered pointcloud counter function #247

Are you sure you want to change the base?

feat(hesai): add filtered pointcloud counter function #247

Conversation

ike-kazu commented Dec 20, 2024

PR Type

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

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

mojomex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojomex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2024 •

edited

Loading