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

feat(pointcloud_preprocessor): runtime configurable output topic qos #6658

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

ralwing
Copy link
Contributor

@ralwing ralwing commented Mar 20, 2024

Description

This change would allow runtime qos configuration for filter output topic. Currently, it is not even possible to use filters with nodes which have qos configured as reliable.
According to ROS2 documentation it would be possible to configure qos at runtime:

Node(
                package="pointcloud_preprocessor",
                executable="crop_box_filter_node",
                name="crop_box_filter",
                parameters=[
                    {"input_frame": "base_link"},
                    {"output_frame": "base_link"},
                    {"qos_overrides.output.reliability": "reliable"},
                ],
            ),

The output topic must be in fact fully qualified topic name, and cannot be changed after the node is constructed, however this feature is still extending filter nodes usability.

Tests performed

Feature: I've tested crop-box-filter with different qos settings.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Mar 20, 2024
@ralwing ralwing force-pushed the feat-configurable-qos branch from cdb0ccf to e851e2f Compare March 20, 2024 09:35
@kminoda
Copy link
Contributor

kminoda commented Mar 29, 2024

@ralwing Could you elaborate more on the background of this PR, e.g. what issue you want to solve?

@ralwing
Copy link
Contributor Author

ralwing commented May 6, 2024

@kminoda
In this pull request, I'd like to fix missing node ability to work with different qos subscriber configurations. Currently, when a subscriber has a reliable qos reliability configured, it can't retrieve data from this node.
And primarly, IMHO QoS settings should be easily configurable without the need to change source code.

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

@ralwing
Sorry for taking so long in checking this PR
I talked to the relevant people at TierIV and there were no opinions against this scheme (in fact, we will introduce to several of our packages outside universe).
However, for this particular PR, could you apply the same idea to all the relevant pub/subs in the package? I don't think it is a good idea to implement this only half way

@ralwing ralwing force-pushed the feat-configurable-qos branch 2 times, most recently from 5e45715 to a0a3f59 Compare May 22, 2024 12:33
@ralwing
Copy link
Contributor Author

ralwing commented May 22, 2024

@knzo25
I've added it to publishers in pointcloud_processor.

@knzo25
Copy link
Contributor

knzo25 commented May 23, 2024

@ralwing
Thanks for the changes. Doing a Ctrl+F search, I think I saw concatenate_and_time_sync and ring_outlier_filter with PointCloud2 publishers without the runtime configurable options (not the main pointcloud but still releveant)

@knzo25
Copy link
Contributor

knzo25 commented Jun 21, 2024

@ralwing
It is been around a month since the last activity in this PR.
If you do not think you will be able to continue working on this PR, please let us know so that we can try to arrange resources to continue working on this ourselves, or close the PR.

Copy link

github-actions bot commented Jun 25, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@ralwing
Copy link
Contributor Author

ralwing commented Jun 25, 2024

I've added missing policies configuration in pointcloud preprocessor and additionally in livox_tag_filter and radar_scan_to_pointcloud2, which concludes these changes in sensing subfolder.

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for your contribution !

@knzo25
Copy link
Contributor

knzo25 commented Jul 1, 2024

@scepter914 @technolojin @YoshiRi
It seems we need an approval from one of you guys !

Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

@knzo25 Thanks for your support!
LGTM

@knzo25 knzo25 added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jul 3, 2024
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 17.85714% with 23 lines in your changes missing coverage. Please review.

Project coverage is 28.54%. Comparing base (335ed8b) to head (cbde5fa).
Report is 4 commits behind head on main.

Files Patch % Lines
...atenate_data/concatenate_and_time_sync_nodelet.cpp 0.00% 4 Missing ⚠️
...are_livox_tag_filter/src/livox_tag_filter_node.cpp 0.00% 3 Missing ⚠️
...lier_filter/dual_return_outlier_filter_nodelet.cpp 0.00% 3 Missing ⚠️
...ointcloud2_node/radar_scan_to_pointcloud2_node.cpp 0.00% 3 Missing ⚠️
...r/src/concatenate_data/concatenate_pointclouds.cpp 0.00% 2 Missing ⚠️
...or/src/crop_box_filter/crop_box_filter_nodelet.cpp 0.00% 2 Missing ⚠️
...src/outlier_filter/ring_outlier_filter_nodelet.cpp 0.00% 2 Missing ⚠️
...rc/time_synchronizer/time_synchronizer_nodelet.cpp 0.00% 2 Missing ⚠️
.../vector_map_filter/lanelet2_map_filter_nodelet.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6658      +/-   ##
==========================================
+ Coverage   28.37%   28.54%   +0.16%     
==========================================
  Files        1583     1588       +5     
  Lines      115588   115852     +264     
  Branches    49278    49351      +73     
==========================================
+ Hits        32798    33066     +268     
+ Misses      73819    73773      -46     
- Partials     8971     9013      +42     
Flag Coverage Δ *Carryforward flag
differential 9.64% <17.85%> (?)
total 28.37% <ø> (+<0.01%) ⬆️ Carriedforward from 335ed8b

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ralwing added 3 commits July 3, 2024 07:40
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
@ralwing ralwing force-pushed the feat-configurable-qos branch from ccd3955 to cbde5fa Compare July 3, 2024 05:41
@ralwing
Copy link
Contributor Author

ralwing commented Jul 3, 2024

@knzo25 The clang-tidy reports hundreds of warnings, which are unrelated with this PR.

@knzo25
Copy link
Contributor

knzo25 commented Jul 3, 2024

@ralwing
Yes, but they are not required as of now. Can you Squash and merge ?

@ralwing
Copy link
Contributor Author

ralwing commented Jul 3, 2024

Nope:
image

@knzo25
Copy link
Contributor

knzo25 commented Jul 3, 2024

Ok, I was waiting for you to merge this. I will do it instead

@knzo25 knzo25 merged commit f1e4a9e into autowarefoundation:main Jul 3, 2024
28 of 30 checks passed
palas21 pushed a commit to palas21/autoware.universe that referenced this pull request Jul 12, 2024
…utowarefoundation#6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: palas21 <palas21@itu.edu.tr>
tby-udel pushed a commit to tby-udel/autoware.universe that referenced this pull request Jul 14, 2024
…utowarefoundation#6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
…6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Ariiees pushed a commit to Ariiees/autoware.universe that referenced this pull request Jul 22, 2024
…utowarefoundation#6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
TomohitoAndo pushed a commit to tier4/autoware_universe that referenced this pull request Sep 10, 2024
…utowarefoundation#6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
TetsuKawa pushed a commit to tier4/autoware_universe that referenced this pull request Jan 23, 2025
…utowarefoundation#6658)

* feat(pointcloud_preprocessor): runtime configurable output topic qos

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in livox_tag_filter_node

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

* configurable qos in radar_scan_to_pointcloud2

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>

---------

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
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) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants