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

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

Closed

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Mar 31, 2020

Similar to #194

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

---
# 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
---
# 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
Copy link
Contributor

hidmic commented Apr 8, 2020

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 hidmic added the enhancement New feature or request label Apr 8, 2020
@hidmic
Copy link
Contributor

hidmic commented Apr 16, 2020

@mikaelarguedas friendly ping.

@mikaelarguedas
Copy link
Member Author

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
Copy link
Member Author

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

@hidmic
Copy link
Contributor

hidmic commented May 4, 2020

Ideally, I'd (personally) prefer something closer to #249 (comment). But we can move forward with this as an incremental improvement. Do you have the bits to run CI?

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.

lgtm, i am good to go with this fix.

@hidmic
Copy link
Contributor

hidmic commented May 11, 2020

CI up to ros1_bridge:

  • Packaging Linux Build Status

After rebasing the branch

  • Packaging Linux Build Status

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@dirk-thomas
Copy link
Member

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.

@mikaelarguedas mikaelarguedas deleted the fixups_parameter_bridge branch May 27, 2020 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants