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

Add QoS profile parameter to image bridge #335

Merged
merged 4 commits into from
Feb 8, 2023

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Dec 5, 2022

🎉 New feature

Closes #336

Summary

This PR modifies the image_bridge node in the ros_gz_image package to optionally specify a different (non-default) QoS profile. This was motivated by the fact that we (and presumably many others) want to use the "sensor data" QoS profile.

This exposes a new string parameter in the node named qos which defaults to default. This should be the same behavior as the original implementation of image_bridge.cpp.

Summary

Test it

To test this, you can run an image bridge:

ros2 run ros_gz_image image_bridge /topic1 /topic2
ros2 run ros_gz_image image_bridge /topic1 /topic2 --ros-args qos:=sensor_data

or in e.g. a Python launch file:

bridge_node = Node(
        package="ros_gz_image",
        executable="image_bridge",
        name="my_bridge",
        arguments=["/topic1", "/topic2"],
        parameters=[{"qos": "sensor_data"}],
    )

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
@sea-bass sea-bass force-pushed the image-bridge-qos-param branch from ebd3bf1 to d194dd6 Compare December 5, 2022 18:24
@azeey azeey requested a review from ahcorde December 5, 2022 19:22
@sea-bass sea-bass force-pushed the image-bridge-qos-param branch 2 times, most recently from 014197a to f111628 Compare December 5, 2022 20:06
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
@sea-bass sea-bass force-pushed the image-bridge-qos-param branch from f111628 to 72c2a03 Compare December 5, 2022 20:52
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

can you add some documentation in the README.md of this packages?

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

cpplint is failing too

3: /github/home/ws/src/ros_gz/ros_gz_image/src/image_bridge.cpp:21:  Found C system header after C++ system header. Should be: image_bridge.h, c system, c++ system, other.  [build/include_order] [4]

@sea-bass
Copy link
Contributor Author

sea-bass commented Dec 6, 2022

cpplint is failing too

3: /github/home/ws/src/ros_gz/ros_gz_image/src/image_bridge.cpp:21:  Found C system header after C++ system header. Should be: image_bridge.h, c system, c++ system, other.  [build/include_order] [4]

Somehow this issue was there for the code even without my modifications. Anyways, I've rearranged it and got rid of this check failing locally. Added doc as well.

Also sorry for the force-pushing. I keep forgetting to sign my commits :/

@sea-bass sea-bass force-pushed the image-bridge-qos-param branch from e7aa849 to b272c27 Compare December 6, 2022 14:43
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
@sea-bass sea-bass force-pushed the image-bridge-qos-param branch from b272c27 to 6afdea1 Compare December 6, 2022 14:45
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
@sea-bass sea-bass force-pushed the image-bridge-qos-param branch from 6afdea1 to 0e4f2ef Compare December 6, 2022 15:37
@sea-bass sea-bass requested a review from ahcorde December 6, 2022 15:39
@sea-bass
Copy link
Contributor Author

Anything else blocking kicking off CI? I think I got all the changes.

@livanov93
Copy link
Contributor

Anything else blocking kicking off CI? I think I got all the changes.

@ahcorde Let's merge this one as we are in the new year now. Anything that needs attention from your perspective?

@ahcorde ahcorde merged commit e7660f8 into gazebosim:ros2 Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consider supporting non-default QoS profile in Image Bridge
3 participants