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

enhance parameter bridge enable options for topics #250

Closed
wants to merge 6 commits into from

Conversation

daisukes
Copy link

@daisukes daisukes commented Apr 2, 2020

This enhancement enables parameter_bridge to control the bridge precisely with the following keys. Please see the detail in doc/parameter_bridge.rst

  • direction
  • transient_local
  • reliable
  • latch

I was struggling with tf_static which is a latched topic in ROS1 but cannot be seen in ROS2.
With specifying transient_local and reliable to true, it can be seen in ROS2 like a latched topic (of course need to use transient_local and reliable for the subscriber).

Signed-off-by: Daisuke Sato [email protected]

Copy link
Contributor

@gonzodepedro gonzodepedro left a comment

Choose a reason for hiding this comment

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

It looks good to me. just a single nit comment. It needs a rebase tough, after running CI.

@daisukes daisukes force-pushed the enhance-parameter-bridge branch from 2401db5 to ce54891 Compare May 4, 2020 18:05
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

mostly lgtm, but someone else needs to review.

src/bridge.cpp Outdated
if (reliable) { qos.reliable(); }

return qos.get_rmw_qos_profile();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

@gonzodepedro
Copy link
Contributor

Build Status

Copy link
Contributor

@gonzodepedro gonzodepedro left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Please run the unit tests of the package which include the linters and address style issues.

@dirk-thomas dirk-thomas added enhancement New feature or request requires-changes labels May 20, 2020
@dirk-thomas
Copy link
Member

@daisukes Friendly ping.

@daisukes
Copy link
Author

daisukes commented Jun 4, 2020

@dirk-thomas Thank you for the reminder. I will work on this today.

@daisukes
Copy link
Author

daisukes commented Jun 4, 2020

Do I need to fix the conflict too?

@daisukes daisukes force-pushed the enhance-parameter-bridge branch from 51ceace to 2ab5c92 Compare June 5, 2020 00:14
@dirk-thomas
Copy link
Member

ros2/design#296 aims to provide a way to configure the QoS from the outside in a generic way. With that approach being the preferred choice I don't think a custom way to configure the bridge is desired.

@dirk-thomas dirk-thomas added duplicate This issue or pull request already exists and removed requires-changes labels Aug 19, 2020
@sloretz
Copy link
Contributor

sloretz commented Nov 5, 2020

Linking updated configurable QoS proposal ros2/design#300

@hidmic
Copy link
Contributor

hidmic commented Dec 1, 2020

@daisukes does the added functionality satisfy your use case? If so, you may go ahead and close this.

@daisukes
Copy link
Author

daisukes commented Dec 2, 2020

@hidmic, yes for ROS2 galactic. I may want to use direction and latch if I keep using the bridge, but I'm planning to port all my packages to ROS2 next year. Anyway, I will close this.

@daisukes daisukes closed this Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants