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

ROS-268-min-valid-cols-lidar-scan #428

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Samahu
Copy link
Contributor

@Samahu Samahu commented Jan 29, 2025

Related Issues & PRs

Summary of Changes

  • Skip the lidar_scan based on a predefined percentage of the minimum number of valid columns in scan

Validation

  • Launch a node as usual but enable the new min_scan_valid_columns_ratio parameter (set a high value)
roslaunch ouster_ros driver.launch sensor_hostname:=... min_scan_valid_columns_ratio:=0.9
  • Observe the warning messages whenever the number of valid columns drops below the set amount:
[ WARN] [1738283732.021510354]: number of valid columns per scan 160 which is below the ratio 90%% SKIPPING SCAN
[ WARN] [1738283732.080962740]: number of valid columns per scan 304 which is below the ratio 90%% SKIPPING SCAN

@Samahu Samahu added the enhancement New feature or request label Jan 29, 2025
@Samahu Samahu self-assigned this Jan 29, 2025
@ovaag
Copy link
Contributor

ovaag commented Jan 29, 2025

Yep, that's cleaner! Thanks for taking the time to look into this.

If we now wanted to publish this on a rostopic, I guess we could return a std::optional<size_t> from the lidar_packet_handler? Since valid_cols will only be relevant if result = true in lidar_packet_accumlator.

A /ouster/statistics topic sounds good.

Is this something you would be interested in having upstream?

Another options is of course to have the publisher directly in the LidarPacketHandler, but that might differ a bit from how the rest of ros pubs/subs are done?

@Samahu
Copy link
Contributor Author

Samahu commented Jan 29, 2025

If we now wanted to publish this on a rostopic, I guess we could return a std::optional<size_t> from the lidar_packet_handler? Since valid_cols will only be relevant if result = true in lidar_packet_accumlator.

I assume you are talking about publishing valid_cols value to a separate topic

A /ouster/statistics topic sounds good.

If we are moving in this direction, I would say probably /ouster/scan_stats seems more appropriate (unless we can think of something else).

Is this something you would be interested in having upstream?

yeah, I think it makes sense to me to add the param to filter LidarScan based on this PR, I am not sure yet about adding a scan stats or statistics topic yet.

An alternative approach would be to publish the LidarScan to a topic as is, and the let subscribed nodes do the PointCloud2 scan decoding on their own, this would be a major work that I can not take on in the near term.

@Samahu Samahu marked this pull request as ready for review January 31, 2025 00:36
Copy link
Collaborator

@matthew-lidar matthew-lidar left a comment

Choose a reason for hiding this comment

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

One small thing about the warning print, otherwise looks good!

src/lidar_packet_handler.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants