Skip to content

Add qos option to override qos#208

Merged
gbiggs merged 6 commits into
ros-perception:ros2from
wep21:feature/qos-override
May 5, 2022
Merged

Add qos option to override qos#208
gbiggs merged 6 commits into
ros-perception:ros2from
wep21:feature/qos-override

Conversation

@wep21
Copy link
Copy Markdown
Contributor

@wep21 wep21 commented Sep 7, 2021

I added qos option to override qos via ros parameter.

@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Sep 7, 2021

@mjcarroll could you review this?

@abstanton
Copy link
Copy Markdown

abstanton commented Feb 17, 2022

any update on this?

@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Feb 17, 2022

@gbiggs friendly ping

@gbiggs
Copy link
Copy Markdown
Contributor

gbiggs commented Feb 19, 2022

Can you please add a test that verifies:

  1. That when the parameter is not set, the original QoS is preserved.
  2. That when the parameter is set, the specified QoS is correctly used.

@abstanton
Copy link
Copy Markdown

abstanton commented Feb 21, 2022

@gbiggs my team needs this quite badly, how could I take over the patch to add these tests?

@wep21 wep21 changed the base branch from galactic to ros2 February 21, 2022 18:51
@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Feb 21, 2022

I found out that subsriber plugin with options has been already implemented in #186.
I will take another look at publisher implementation and try to make some tests.

@wep21 wep21 force-pushed the feature/qos-override branch from cd0ed13 to 5be5095 Compare February 21, 2022 20:02
@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Feb 21, 2022 •
edited
Loading

@gibiggs I added basic tests at 9086994.

gbiggs
gbiggs reviewed Feb 22, 2022
View reviewed changes
Comment thread image_transport/test/test_publisher.cpp Outdated
Comment thread image_transport/test/test_subscriber.cpp Outdated
Comment thread image_transport/include/image_transport/raw_publisher.hpp Outdated
Comment thread image_transport/include/image_transport/publisher_plugin.hpp
rclcpp::Node * nh, const std::string & base_topic,
rmw_qos_profile_t custom_qos) = 0;

virtual void advertiseImpl(
Copy link
Copy Markdown
Contributor

@gbiggs gbiggs Feb 22, 2022

Choose a reason for hiding this comment

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

I think it would be better to add rclcpp::PublisherOptions options = rclcpp::PublisherOptions() to the existing abstract advertiseImpl function. Then plugins can provide the same default options or customised options as they see fit and pass them along. That would avoid the need to overload the advertiseImpl function.

Copy link
Copy Markdown
Contributor Author

@wep21 wep21 Mar 21, 2022

Choose a reason for hiding this comment

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

I implement this in accordance with subscriber impl. I guess this is to keep the interface for the inherited classes in another project such as https://github.com/ros-perception/image_transport_plugins.
@audrow Please correct me if I'm wrong.

Comment thread image_transport/include/image_transport/publisher_plugin.hpp
(void) options;
RCLCPP_ERROR(
node->get_logger(),
"PublisherPlugin::advertiseImpl with four arguments has not been overridden");
Copy link
Copy Markdown
Contributor

@gbiggs gbiggs Feb 22, 2022

Choose a reason for hiding this comment

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

If it hasn't been overridden, why is it not pure abstract?

@wep21 wep21 force-pushed the feature/qos-override branch from 9086994 to 5f26f2c Compare March 21, 2022 15:31
@wep21 wep21 requested a review from gbiggs March 21, 2022 19:49
gbiggs
gbiggs reviewed Mar 24, 2022
View reviewed changes
Comment thread image_transport/test/test_publisher.cpp Outdated
Comment thread image_transport/test/test_publisher.cpp Outdated
wep21 added 4 commits March 28, 2022 11:10
feat: add PublisherOptions arg
ef90f27 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
test: add qos override test
6ce1063 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
test: add without options test
b6cd134 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
remove unused using
1bea8c0 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 force-pushed the feature/qos-override branch from df25b23 to 1bea8c0 Compare March 28, 2022 02:37
separate tests
16b9d87 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 force-pushed the feature/qos-override branch from f11de98 to fb89ded Compare March 28, 2022 03:27
separate qos override test
4eda95d 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 force-pushed the feature/qos-override branch from fb89ded to 4eda95d Compare March 28, 2022 03:33
@wep21 wep21 requested a review from gbiggs March 30, 2022 04:17
@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Mar 30, 2022

@audrow Could you comment about @gbiggs 's review?

@gbiggs
Copy link
Copy Markdown
Contributor

gbiggs commented May 5, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@gbiggs
Copy link
Copy Markdown
Contributor

gbiggs commented May 5, 2022

CI has passed so merging this. Thank you for the contribution!

@gbiggs gbiggs merged commit 9a9dd64 into ros-perception:ros2 May 5, 2022
@wep21 wep21 deleted the feature/qos-override branch May 5, 2022 07:18
@gecastro
Copy link
Copy Markdown

gecastro commented Jun 8, 2022

Is there a plan to backport this feature to galactic?

@gbiggs
Copy link
Copy Markdown
Contributor

gbiggs commented Jun 8, 2022

Unfortunately not, as this would break API in Galactic.

tonynajjar pushed a commit to angsa-robotics/image_common that referenced this pull request May 15, 2024
@wep21 @tonynajjar
Add qos option to override qos (ros-perception#208)
4148029 
* feat: add PublisherOptions arg

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* test: add qos override test

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* test: add without options test

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* remove unused using

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* separate tests

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* separate qos override test

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gbiggs gbiggs Awaiting requested review from gbiggs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wep21 @abstanton @gbiggs @gecastro