feat: improve script for decoding packets into a new rosbag #236

Owen-Liuyuxuan · 2024-11-26T06:24:12Z

PR Type

New Feature

Description

Improve the decoding script to add a function of transferring a packet topic from ROSBag, decode that, and write the output point clouds into a new rosbag.

Review Procedure

I tested that with ROSBags containing OT128 LiDAR points obtained from internal projects.

Modification from `extract_bag_pcd`

Update CMakeLists.txt
Fixed a bug on the usage of out_num_. The original script does not correctly break from the loop, it only breaks from inner loop.
Adding a list of sensor configuration parameters; Support using LiDAR parameter file to fill the default value.
After driver_ptr_->parse_cloud_packet, we transform the PCL data into point cloud messages and write to a new ROSBag.

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.

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

codecov · 2024-11-26T06:33:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (3d42cd0) to head (662275b).

❗ There is a different number of reports uploaded between BASE (3d42cd0) and HEAD (662275b). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (3d42cd0) HEAD (662275b)

total 3 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #236       +/-   ##
==========================================
- Coverage   26.11%   0.00%   -26.12%     
==========================================
  Files         101      88       -13     
  Lines        9212    7646     -1566     
  Branches     2211     938     -1273     
==========================================
- Hits         2406       0     -2406     
- Misses       6417    7646     +1229     
+ Partials      389       0      -389

Flag	Coverage Δ
differential	`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.

knzo25 · 2024-11-26T06:35:40Z

nebula_examples/CMakeLists.txt

@@ -45,7 +45,7 @@ target_link_libraries(hesai_ros_offline_extract_pcd PUBLIC
    nebula_decoders::nebula_decoders_hesai
 )



Is this needed? We were avoiding ament_auto on purpose for reduce building time

actually, when I first build this package without ament_auto, there is no output in the built output directory.

(please remind me to delete these two images when this is somehow resolved)

I built again with my current PR and I can see the contents correctly.

The solution is to take the steps that ament auto does manually. That requires some cmake knowledge, but there should be examples in nebula for you to follow.

@knzo25
I added list(APPEND ${PROJECT_NAME}_EXECUTABLES "${target}") and the build works, but the build time is similar to what we got with ament_auto_add_executable

Thank you, I instead add the install command in CMakeList to make it work.

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

mojomex · 2024-11-26T08:31:52Z

GitHub's review endpoint seems to be down, so I'll post a comment instead:

Hi @Owen-Liuyuxuan, thanks for the PR!

I notice that you created the fix in a separate copy of the already existing hesai_ros_offline_extract_bag_pcd o I would like to ask you to instead integrate your changes there.

For context:
We will probably also get rid of hesai_ros_offline_extract_pcd and rename the script so something like hesai_offline_decode in the future to reduce the duplicated code.

Owen-Liuyuxuan · 2024-11-26T08:50:57Z

I was trying to fix the cspell CI then I find out GitHub system failed.

@mojomex
Ok, I will try to merge them and make the action configurable. I didn't do that at first because I think their output target changes so much.

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan · 2024-11-27T00:21:53Z

Should we tackle the codecov CI here..?

mojomex · 2024-11-27T05:53:21Z

Nope, not needed 🙌
Is this good for another review round?

Owen-Liuyuxuan · 2024-11-27T05:59:27Z

@mojomex
Yes Please

mojomex

Code looks almost ready-to-go!
I found a few things here and there, if you could take a look! thanks!

mojomex · 2024-11-27T06:26:12Z

nebula_examples/CMakeLists.txt

Please remove the list() commands again, since they were only for debugging purposes.

Sorry if I remove them, there is no executable output...

OK, now this one looks good. The install command works

mojomex · 2024-11-27T06:32:24Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

+    <arg name="is_write_to_pcd" default="false"/>
+    <arg name="is_write_to_rosbag" default="true"/>
+    <arg name="is_write_packets_to_rosbag" default="false"/>
+


I think renaming these to output_pcd, output_rosbag would be more readable.

For packets, none of the options is particularly concise, but forward_packets_to_rosbag or copy_packets_to_rosbag would work I think.

mojomex · 2024-11-27T06:33:22Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

+    <arg name="sensor_model" default="Pandar128E4X" description="Pandar64|Pandar40P|PandarXT32|PandarXT32M|PandarAT128|PandarQT64"/>
+    <arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/Pandar128E4X.params.yaml"/>
+    <arg name="bag_path" default="rosbags/default_2024-10-24-10-50-48_p0900"/>
+    <arg name="target_topic" default="/sensing/lidar/top/pandar_packets"/>


Calling this input_topic would be clearer

mojomex · 2024-11-27T06:34:11Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

+
+    <!-- sensor related configuration -->
+    <arg name="sensor_model" default="Pandar128E4X" description="Pandar64|Pandar40P|PandarXT32|PandarXT32M|PandarAT128|PandarQT64"/>
+    <arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/Pandar128E4X.params.yaml"/>


Use $(var sensor_model) here instead of hard-coding LiDAR name

Suggested change

<arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/Pandar128E4X.params.yaml"/>

<arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/$(var sensor_model).param.yaml"/>

mojomex · 2024-11-27T06:34:26Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

+    <arg name="is_write_packets_to_rosbag" default="false"/>
+
+    <!-- sensor related configuration -->
+    <arg name="sensor_model" default="Pandar128E4X" description="Pandar64|Pandar40P|PandarXT32|PandarXT32M|PandarAT128|PandarQT64"/>


Please remove the default value.

it seems that sensor_model is also inside the parameter file, I will also remove sensor_model in the launch file.

oh no... The default parameter file requires the sensor model arguments to be pre-defined as a ROS parameter.. which is rather weird..

So I keep this sensor_model parameters in the end.

mojomex · 2024-11-27T06:47:05Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

        <param name="return_mode" value="$(var return_mode)"/>
        <param name="frame_id" value="$(var frame_id)"/>


Remove the LiDAR params here as well.

Thank you, I cleaned up the default parameters.

mojomex · 2024-11-27T06:47:21Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

@@ -31,8 +47,8 @@
        <param name="out_path" value="$(var out_path)"/>
        <param name="format" value="$(var format)"/>
        <param name="target_topic" value="$(var target_topic)"/>


rename to input_topic here as well

Thanks for the review!

I believe this one has also been resolved in the latest PR.

mojomex · 2024-11-27T06:48:56Z

nebula_examples/src/hesai/hesai_ros_offline_extract_bag_pcd.cpp

@@ -197,6 +220,7 @@ Status HesaiRosOfflineExtractBag::read_bag()
  reader.open(storage_options, converter_options);
  int cnt = 0;
  int out_cnt = 0;
+  bool pointcloud_enough = false;


Please rename to output_limit_reached or some other more self-explaining name.

Thanks for the review!
I believe this one has been resolved in the latest PR.

mojomex · 2024-11-27T06:53:04Z

nebula_examples/src/hesai/hesai_ros_offline_extract_bag_pcd.cpp

+      if (write_rosbag_packets_flag_) {
+        bag_writer->write(bag_message);
      }



There is no hard guarantee that one packets message == one pointcloud output (in usual cases the correspondence is 1-to-1 but in the past, when we made updates to scan cutting, this approach broke).

It would be best to move everything from L260-L281 (if (!bag_writer) ... bag_writer->write(...); }) up into the outer loop, so it runs even if there are no pointclouds output for a given packets message.

Thanks for the review!
I believe this one has been resolved in the latest PR.

mojomex · 2024-11-27T07:03:43Z

nebula_examples/launch/hesai_offline_bag_pcd.xml

    <arg name="only_xyz" default="true"/>


    <node pkg="nebula_examples" exec="hesai_ros_offline_extract_bag_pcd_node"
          name="hesai_cloud" output="screen">
+        <param from="$(var lidar_parameter_file)"/>
+        <param name="is_write_to_pcd" value="$(var is_write_to_pcd)"/>


Allow substitutions so that variables in the param file get resolved (e.g. calibration_file)

Suggested change

<param name="is_write_to_pcd" value="$(var is_write_to_pcd)"/>

<param from="$(var lidar_parameter_file)" allow_substs="true" />

I believe this one have been resolved.

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan · 2024-11-27T08:39:51Z

@mojomex
Thank you for your review.

I push three commits to conduct changes separately.

The First Commit

Update variable names as pointed out in your review, making them more readable.
Move the bag writer definition and packet write action to the outer loop. So that each message correctly corresponds to one forwarded message.

The Second Commit

Clean up most of the default values in the launch file.

The Third Commit

Restore the default lidar_parameter_file for easier usage.
Clean up default values for skip_num & out_num

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

…/nebula into feat/rosbag_decode_script

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan · 2024-11-29T09:38:32Z

If you have suggestion on parameter handling please inform here.

feat: create a new script for decoding packets into a new rosbag

8828069

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan requested a review from mojomex November 26, 2024 06:24

knzo25 reviewed Nov 26, 2024

View reviewed changes

Owen-Liuyuxuan added 3 commits November 26, 2024 16:27

do not use ament_auto instead

cb633d2

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

restore

fe3df92

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

fix spell check

80f7b26

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan added 5 commits November 26, 2024 18:34

merge modifications

322fe29

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

fix bug

af77670

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

fix bug

04f2e7b

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

reset output number to a small one so that we do not break things

645052f

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Merge branch 'main' into feat/rosbag_decode_script

ba92cae

mojomex requested changes Nov 27, 2024

View reviewed changes

Owen-Liuyuxuan and others added 3 commits November 27, 2024 17:34

update variable names

f5280ad

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

clean up default variables

8c1fa5b

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

ci(pre-commit): autofix

c18f4ca

Owen-Liuyuxuan added 3 commits November 27, 2024 17:44

fix typo

f52eb41

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Merge branch 'feat/rosbag_decode_script' of github.com:Owen-Liuyuxuan…

7322f34

…/nebula into feat/rosbag_decode_script

update configuration file

580b417

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

Owen-Liuyuxuan changed the title ~~feat: create a new script for decoding packets into a new rosbag~~ feat: improve script for decoding packets into a new rosbag Nov 28, 2024

Owen-Liuyuxuan added 2 commits November 28, 2024 09:29

fix CMakeList

6835e0f

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

remove extra empty line

662275b

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>

mojomex mentioned this pull request Nov 28, 2024

Hesai XT16 LiDAR #137

Open

		@@ -45,7 +45,7 @@ target_link_libraries(hesai_ros_offline_extract_pcd PUBLIC
		nebula_decoders::nebula_decoders_hesai
		)

	<arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/Pandar128E4X.params.yaml"/>
	<arg name="lidar_parameter_file" default="$(find-pkg-share nebula_ros)/config/lidar/hesai/$(var sensor_model).param.yaml"/>

		<param name="return_mode" value="$(var return_mode)"/>
		<param name="frame_id" value="$(var frame_id)"/>

	<param name="is_write_to_pcd" value="$(var is_write_to_pcd)"/>
	<param from="$(var lidar_parameter_file)" allow_substs="true" />

feat: improve script for decoding packets into a new rosbag #236

Are you sure you want to change the base?

feat: improve script for decoding packets into a new rosbag #236

Conversation

Owen-Liuyuxuan commented Nov 26, 2024 • edited Loading

PR Type

Related Links

Description

Review Procedure

Modification from extract_bag_pcd

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 Nov 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Owen-Liuyuxuan Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Owen-Liuyuxuan Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojomex commented Nov 26, 2024

Owen-Liuyuxuan commented Nov 26, 2024

Owen-Liuyuxuan commented Nov 27, 2024

mojomex commented Nov 27, 2024

Owen-Liuyuxuan commented Nov 27, 2024

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

Owen-Liuyuxuan Nov 27, 2024 • edited Loading

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

Owen-Liuyuxuan commented Nov 27, 2024 • edited Loading

The First Commit

The Second Commit

The Third Commit

Owen-Liuyuxuan commented Nov 29, 2024

Owen-Liuyuxuan commented Nov 26, 2024 •

edited

Loading

Modification from `extract_bag_pcd`

codecov bot commented Nov 26, 2024 •

edited

Loading

Owen-Liuyuxuan Nov 26, 2024 •

edited

Loading

Owen-Liuyuxuan Nov 26, 2024 •

edited

Loading

Owen-Liuyuxuan Nov 27, 2024 •

edited

Loading

Owen-Liuyuxuan commented Nov 27, 2024 •

edited

Loading