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

Parametrize Quality of Service in parameter_bridge. #331

Merged
merged 14 commits into from
Jun 22, 2022

Conversation

LoyVanBeek
Copy link
Contributor

@LoyVanBeek LoyVanBeek commented Oct 14, 2021

Had some trouble using the parameter_bridge with /tf_static Turns out, I/m not the only one: https://answers.ros.org/question/349230/problem-passing-tf_static-through-ros1_bridge-to-ros2/ . The dynamic_bridge has an special case for this topic introduced in #282, but not in the parameter_bridge.

To tackle that and be the most general I could be, I extended the parameter_bridge to set specific QoS settings per topic, allowing one the implement that special case if required.

I've used this config for testing (also with different and illegal values for the settings):

bridge.yaml.

topics:
  - 
    topic: /chatter  # ROS1 topic name
    type: std_msgs/msg/String  # ROS2 type name
    queue_size: 1  # For the publisher back to ROS1
  - 
    topic: /joint_states
    type: sensor_msgs/msg/JointState
    queue_size: 1
  - 
    topic: /message_to_ros
    type: std_msgs/msg/String
    queue_size: 1
  - 
    topic: /message_from_ros
    type: std_msgs/msg/String
    queue_size: 1
    qos:
      history: keep_last
      depth: 10
      reliability: reliable
      durability: transient_local
      deadline: 
          secs: 10
          nsecs: 2345
      lifespan: 
          secs: 20
          nsecs: 3456
      liveliness: LIVELINESS_AUTOMATIC
      liveliness_lease_duration: 
          secs: 40
          nsecs: 5678
  - 
    topic: /tf
    type: tf2_msgs/msg/TFMessage
    queue_size: 1
  - 
    topic: /tf_static
    type: tf2_msgs/msg/TFMessage
    queue_size: 1
    qos:
      history: keep_all
      durability: transient_local

Usage of the parameter_bridge is explained in #330, this bridge.yaml is can be used in the same way.

@LoyVanBeek LoyVanBeek force-pushed the feature-parametrize-qos branch from f405e65 to 9150c48 Compare October 14, 2021 14:03
@LoyVanBeek LoyVanBeek marked this pull request as ready for review October 18, 2021 11:38
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
@LoyVanBeek LoyVanBeek force-pushed the feature-parametrize-qos branch from fd9d12f to a7b4cb2 Compare October 18, 2021 14:52
…representation of liveliness enum values

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
@LoyVanBeek LoyVanBeek force-pushed the feature-parametrize-qos branch from a7b4cb2 to b777cbf Compare October 18, 2021 14:55
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
@LoyVanBeek LoyVanBeek force-pushed the feature-parametrize-qos branch from fe2621d to 38d0538 Compare October 19, 2021 07:46
In 2 cases the formatting conflicts with what uncrustify wants; // NOLINT got rid of the complaints of cpplint, favouring uncrustify

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
@LoyVanBeek LoyVanBeek force-pushed the feature-parametrize-qos branch from 38d0538 to 4796dcf Compare October 19, 2021 09:35
@LoyVanBeek
Copy link
Contributor Author

Anything I can do to improve this PR and maybe get it merged eventually?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
src/parameter_bridge.cpp Outdated Show resolved Hide resolved
LoyVanBeek and others added 2 commits November 23, 2021 08:00
Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>
@LoyVanBeek
Copy link
Contributor Author

Anything I can do to get this merged?

@gbiggs
Copy link
Member

gbiggs commented May 5, 2022

CI: Build Status

@gbiggs
Copy link
Member

gbiggs commented May 6, 2022

Let's try that again.

Build Status

@methylDragon
Copy link
Contributor

@gbiggs
Copy link
Member

gbiggs commented Jun 22, 2022

Indeed, the failures look unrelated, so I'm going to go ahead and merge this.

@jacobperron
Copy link
Member

Because of the added dependency on xmlrpcpp, this PR breaks building ros1_bridge on a system with only ROS 2 installed. Previously, I was able to build this package without ROS 1 installed (well, it kind of just skipped the build).

We might want to make xmlrpcpp a conditional dependency, like roscpp, emit a warning and skip the build, e.g.

if(NOT ros1_roscpp_FOUND)

Maybe this is not super important since we've removed ros1_bridge from the default ros2.repos file.

@gbiggs
Copy link
Member

gbiggs commented Jul 18, 2022

In the interest of not having to deal with bug reports about that problem, I've gone ahead and made a PR that moves the xmlrpcpp search to after searching for ROS 1.

#371

@LoyVanBeek
Copy link
Contributor Author

Ah, my bad. I had assumed (the mother of ...) that xmlrpcpp would always be available on a system where ros1_bridge would be built, given the ROS 1 dependencies.

hsd-dev pushed a commit to hsd-dev/ros1_bridge that referenced this pull request Mar 27, 2023
* Extend create_bidirectional_bridge to take qos param for ROS2 publisher

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Busy setting up a way to read QoS parameters from ROS1 params

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Parse history qos params

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Call qos_from_params when setting up topics

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Configure deadline, lifespan, liveliness_lease_durations

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Configure liveliness

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Add some basic debug text

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Print the QoS settings to stdout when setting them up

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Catch XmlRpc::XmlRpcExceptions when constructing QoS from parameters

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Parse liveliness as either int enum value or upper/lower case string representation of liveliness enum values

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Fix formatting with uncrustify

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Fix cpplint formatting

In 2 cases the formatting conflicts with what uncrustify wants; // NOLINT got rid of the complaints of cpplint, favouring uncrustify

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Clearer logging as suggested by code review

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Clarify keep_last vs keep_all setting for history

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
lucyannofrota added a commit to lucyannofrota/ros1_bridge that referenced this pull request May 4, 2023
Just reproduces the changes made in ros2#331 in ros foxy.

Parametrize Quality of Service in `parameter_bridge`

Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>
sgermanserrano pushed a commit to sgermanserrano/ros1_bridge that referenced this pull request May 15, 2023
* Extend create_bidirectional_bridge to take qos param for ROS2 publisher

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Busy setting up a way to read QoS parameters from ROS1 params

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Parse history qos params

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Call qos_from_params when setting up topics

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Configure deadline, lifespan, liveliness_lease_durations

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Configure liveliness

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Add some basic debug text

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Print the QoS settings to stdout when setting them up

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Catch XmlRpc::XmlRpcExceptions when constructing QoS from parameters

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Parse liveliness as either int enum value or upper/lower case string representation of liveliness enum values

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Fix formatting with uncrustify

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Fix cpplint formatting

In 2 cases the formatting conflicts with what uncrustify wants; // NOLINT got rid of the complaints of cpplint, favouring uncrustify

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Clearer logging as suggested by code review

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

* Clarify keep_last vs keep_all setting for history

Signed-off-by: Loy van Beek <loy.vanbeek@mojin-robotics.de>

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
lucyannofrota added a commit to lucyannofrota/ros1_bridge that referenced this pull request May 23, 2023
Just reproduces the changes made in ros2#331 in ros foxy.

Parametrize Quality of Service in `parameter_bridge`

Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>
quarkytale pushed a commit that referenced this pull request May 24, 2023
… commits (ec44770) and (86b4245) to foxy branch] (#401)

* Port of the commit (ec44770) to foxy branch

Just reproduces the changes made in #331 in ros foxy.

Parametrize Quality of Service in `parameter_bridge`

Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>

* Port of the commit (86b4245) to foxy branch

Reproduces the changes made in #371 in ros foxy.

Move xmlrpcpp find_package so it only searches if ROS 1 is found

Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>

* CI fix

White spaces removed
Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>

---------

Signed-off-by: Lucyanno Frota <lucyannofrota@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants