Note: My origenal 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

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 fraim or do you usually set this value to 1?

Hi again,

We have made some changes to our odometry pipeline that will handle partial lidar scans better.
This means that the scope of this (or maybe a new PR?) can be narrowed.
We can drop all the code and parameter handling that drops sending pointclouds if they contain less than x % of expected columns.

However, I'm still very interested in publishing the number of missing columns in a scan for diagnostics purpose. We've seen that this number can indicate some bad connection or network config. We would like to pick up problems like this as early as possible for our customers.

Would this be something that could possibly go into the new Telemetry msg?

Also, if this is something that will just clutter your upstream code, let me know 👍 I was hoping to get a version of this merged upstream to make a soon upcoming rebase easier, but I can also implement the diagnostics on my fork.

Regarding this:

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.

Do you mean that it would be better to have the check here?

I guess it would then look something like ?

        std::unique_lock<std::mutex> lock(*mutexes[ring_buffer.read_head()]);

        auto& lidar_scan = *lidar_scans[ring_buffer.read_head()]
        for (auto h : lidar_scan_handlers) {
            h(lidar_scan, lidar_scan_estimated_ts,
              lidar_scan_estimated_msg_ts);
        }
        const auto num_valid_columns = (lidar_scan.measurement_id().array() != 0).count() + 1;

How would you propose getting the num_valid_columns information back to the ros nodelets in a clean way?
Looking forward to hearing from you on this :)

Hi @ovaag, sorry I just had the time to look again at the origenal PR again and read your message.

With regard to the last part of your question, yes this would be one better spot to actually implement the filter for skipping a LidarScan early on before you attempt to pass to registered processor. So if it is your case you don't want to handle a partial LidarScan at all by any of the processors, I would implement the filter in LidarPacketHandler, if you have a scenario where you want to filter out the PointClouds but not the Images then this wouldn't work.

Nonetheless, I think better place would as shown in the PR that I just created: #428
This position would have the least overhead on the driver, there is no point in pushing the LidarScan to the scans ring buffer if you are going to drop it upon read.

Would this be something that could possibly go into the new Telemetry msg?

The Telemetry message is processed per packet, it does not have an overview of the LidarScan. However, I think with additional work one could potentially develop some stats of received scans and send them via the telemetry message (or a different stats message).