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

Param min packets in cloud #268

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

Conversation

ovaag
Copy link
Contributor

@ovaag ovaag commented Nov 23, 2023

Related Issues & PRs

#265

Summary of Changes

This adds a parameter the user can alter in order to discard clouds that consists of x amount of lidar packets.

Validation

I have an example bag where we experienced bad connectivity to lidar, resulting in dropped lidar packets.

Validated this PR by doing the following with replay of named bag:

Set param to 0, see that behaviour is as before:

2023-11-23.10-00-41.mp4

Set param to 500 (I'm in lidar_mode 512x64)

2023-11-23.13-33-05.mp4

corresponding terminal output:
image

No functional changes with default param settings, but this allows the user to discard incomplete
clouds that could lead to e.g. bad scan-matching results.
@ovaag
Copy link
Contributor Author

ovaag commented Nov 23, 2023

Note: My original implementation here was d41c11e and 0aab35a.

I changed my approach to checking the condition in the nodelet, which felt cleaner.

Think the latter approach also will scale nicely if a user want to publish lidar_scan metadata e.g. shot_limiting or thermal shutdown status as this can now just be added to the OutputType struct

lmk what you think @Samahu

@ovaag ovaag force-pushed the param-min-packets-in-cloud branch from 7abe64f to 3f4472c Compare November 29, 2023 13:30
@Samahu Samahu self-requested a review November 29, 2023 21:36
@Samahu Samahu self-assigned this Nov 29, 2023
@Samahu
Copy link
Contributor

Samahu commented Dec 4, 2023

I had a brief look at your PR 👀 but the way I see it, it would be much better to implement this filter at the LidarPacketHandler level rather than having to process this unwanted scan and pass it to all registered processors and then drop all the byproducts before publishing.

Also what is the use case to have this parameter as a number rather than a boolean? Do you have a certain threshold where you actually use to discard the frame or do you usually set this value to 1?

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