search

ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
452 stars 288 forks source link

[parameter_bridge] add srv to the type to keep yaml description consistent #249

Closed mikaelarguedas closed 4 years ago

mikaelarguedas commented 4 years ago

Similar to https://github.com/ros2/ros1_bridge/pull/194

Before this PR the syntax for bridging services in both directions was the following:

```yaml --- # service requests going from 2 to 1 # server on ROS1 side, client on ROS2 side services_2_to_1: - service: /add_two_ints package: roscpp_tutorials type: TwoInts # service requests going from 1 to 2 # server on ROS2 side, client on ROS1 side services_1_to_2: - service: /add_two_ints package: example_interfaces type: srv/AddTwoInts ```

Having to specify the type as srv/AddTwoInts seems uncommon and counter intuitive

After this PR ```yaml --- # service requests going from 2 to 1 # server on ROS1 side, client on ROS2 side services_2_to_1: - service: /add_two_ints package: roscpp_tutorials type: TwoInts # service requests going from 1 to 2 # server on ROS2 side, client on ROS1 side services_1_to_2: - service: /add_two_ints package: example_interfaces type: AddTwoInts ```

This is still breaking pattern from what is used to specify topic bridging:

topics: 
  -
    topic: /clock
    type: rosgraph_msgs/msg/Clock

Not sure what the reason was in the first place for splitting the package and the type name only for services, but this PR keeps that behavior.

hidmic commented 4 years ago

Hmm, I'd be inclined towards keeping it consistent with how message bridges are described (i.e. using just type) instead of silently adding prefixes. Also, to not break existing code that's using the current syntax, accepting it still and printing a deprecation warning would be nice. WDYT @mikaelarguedas ? @dirk-thomas thoughts?

hidmic commented 4 years ago

@mikaelarguedas friendly ping.

mikaelarguedas commented 4 years ago

This is really just a fix because the "srv" was silently added in https://github.com/ros2/ros1_bridge/pull/194/files#diff-72198d9d0a32d2518cd7a4cedb8d26caR89 forcing people to add it by hand to their yaml file.

So it doesn't strike me as a case where this needs to preserve backward compatibility. If this is a must-have to get this merged I can try to find the bandwidth to look at it more closely.

Regarding describing services similarly to topics, it would be great. This could result in a significant refactor that I won't be able to work on by the foxy freeze so I'd prefer to target a minimum convenience change.

mikaelarguedas commented 4 years ago

@hidmic how do you feel about this ? is there anything that needs to be changed for this to get into Foxy ?

hidmic commented 4 years ago

Ideally, I'd (personally) prefer something closer to https://github.com/ros2/ros1_bridge/pull/249#issuecomment-611006124. But we can move forward with this as an incremental improvement. Do you have the bits to run CI?

hidmic commented 4 years ago

CI up to ros1_bridge:

After rebasing the branch

dirk-thomas commented 4 years ago

Support for services was added in #176 to the parameter bridge. That state has been released into Eloquent so I think it is desirable to keep backward compatibility with potentially existing parameters.

Please see #263 which deprecates the package key - but should keep it working. When no package key is defined the type values has the same pattern as for topics.