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

clearly state and check assumptions on input data of pointcloud_preprocessor filters #3318

Closed
3 tasks done
VRichardJP opened this issue Apr 8, 2023 · 3 comments
Closed
3 tasks done
Assignees
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) type:new-feature New functionalities or additions, feature requests.

Comments

@VRichardJP
Copy link
Contributor

VRichardJP commented Apr 8, 2023

Checklist

  • I've read the contribution guidelines.
  • I've searched other issues and no duplicate issues were found.
  • I've agreed with the maintainers that I can plan this task.

Description

As a way to improve filters reliability, and after #3306 is merged, I would like to add proper assumption checks in pointcloud_preprocessor filters.

As explained in #3306 and #2618, several pointcloud_preprocessor filters make implicit assumptions on the input data. In most cases these assumptions are correct but sometimes it is not the case. And when it is not, it is actually really difficult to find it out.

For example ring_outlier_filter intensity output has been wrong for at least a year without anyone noticing.

Purpose

Improve filters reliability.
Later, the information could be used to optimize the code further (e.g. use constexpr field offset when input data layout is compatible with known types.)

Possible approaches

A simple solution is to add a simple check in each filter function that makes implicit assumptions on the input data, like this:

void RingOutlierFilterComponent::filter(
  const PointCloud2ConstPtr & input, [[maybe_unused]] const IndicesPtr & indices,
  PointCloud2 & output)
{
  // make sure we can memcpy from input->data to PointXYZI type
  if (!utils::is_data_layout_compatible_with_PointXYZI(*input)) {
    RCLCPP_ERROR(get_logger(), "Invalid input data layout. Output may be incorrect");
  }

Definition of done

  1. list functions which make strong assumption on the data layout (e.g. reinterpret_cast<PointXYZI*>)
  2. for each, add small check and report an error if the assumptions is wrong (the code is most likely buggy)
@VRichardJP VRichardJP added type:new-feature New functionalities or additions, feature requests. component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) labels Apr 8, 2023
@VRichardJP VRichardJP self-assigned this Apr 8, 2023
@stale
Copy link

stale bot commented Jun 9, 2023

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Jun 9, 2023
@amadeuszsz
Copy link
Contributor

Hi @VRichardJP
Does recent point types migration covers your idea? #6996
I see multiple check for point types are now performed in filters nodes.

@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Jul 31, 2024
@VRichardJP
Copy link
Contributor Author

I think so.
The other day I updated autoware a saw a few pointcloud format error messages popping at runtime. This is very convenient as I could quickly find the root cause of in my sensor kit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) type:new-feature New functionalities or additions, feature requests.
Projects
None yet
Development

No branches or pull requests

2 participants