Support bridging services #211

ivanpauno · 2022-01-19T17:07:23Z

🎉 New feature

Closes #70

Summary

Add support for bridging services.
At the moment you can only bridge the world control service.
Add some message definitions that can be useful for bridging other ignition services.

I plan to clean up the code a bit and support to bridge more services in a follow up PR.

Test it

ros ign /path/to/any/world/with/ign/gui/world/control/enabled
ros2 run ros_ign_bridge parameter_bridge /world/wheel_slip/control@ros_ign_interfaces/srv/ControlWorld
ros2 service call /world/<world_name>/control ros_ign_interfaces/srv/ControlWorld "world_control:
  pause: false
  step: false
  multi_step: 0
  reset:
    all: false
    time_only: false
    model_only: false
  seed: 0
  run_to_sim_time:
    sec: 0
    nanosec: 0"

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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.

ahcorde

I can't see any tests here, is there any chance to add then ?

ivanpauno · 2022-01-19T20:21:17Z

I can't see any tests here, is there any chance to add then ?

Yes, I will add tests.
I opened a PR to get some early feedback.

ivanpauno · 2022-01-26T21:42:53Z

I can't see any tests here, is there any chance to add then ?

See ab63ae6.

chapulina

This is great! The overall approach looks good to me! I mainly have suggestions on how to split up the PR, so that it's easier to check that all new code is tested and documented.

ros_ign_bridge/include/ros_ign_bridge/convert/geometry_msgs.hpp

ros_ign_bridge/include/ros_ign_bridge/convert/builtin_interfaces.hpp

ros_ign_bridge/src/parameter_bridge.cpp

chapulina · 2022-01-27T21:32:34Z

ros_ign_bridge/src/parameter_bridge.cpp

+  // skip the process name in argument procesing
+  ++argv;
+  --argc;
+  auto filteredArgs = rclcpp::init_and_remove_ros_arguments(argc, argv);


This is a nice cleanup! Also a good candidate for its own PR

ros_ign_bridge/src/service_factory.hpp

ros_ign_bridge/test/launch/test_ign_server.launch.py

ros_ign_bridge/test/serverclient/ign_server.cpp

ros_ign_interfaces/msg/VideoRecord.msg

ivanpauno · 2022-01-31T20:39:23Z

I mainly have suggestions on how to split up the PR, so that it's easier to check that all new code is tested and documented.

Sounds good!
Yeah, I ended up putting a lot of different things together.

mjcarroll · 2022-02-07T21:52:04Z

Alright @ivanpauno this is probably ready for a rebase. Sorry for causing so much chaos with the other fixes, but now we are getting debs again on galactic and rolling.

ivanpauno · 2022-02-07T22:27:54Z

Alright @ivanpauno this is probably ready for a rebase. Sorry for causing so much chaos with the other fixes, but now we are getting debs again on galactic and rolling.

No problem, that's easy to fix, I only need to discard one commit 😉 .
I will probably be opening some PRs split from this one (similar to #214), before rebasing.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Louise Poubel <louise@openrobotics.org>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ros_ign_bridge/test/serverclient/ros_client.cpp

ros_ign_bridge/test/serverclient/ign_server.cpp

ros_ign_bridge/src/parameter_bridge.cpp

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

jacobperron

LGTM, but I think we should get someone from the Ignition team to approve ultimately.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2022-02-28T20:27:07Z

CI failures seem to be unrelated ...

chapulina

Great work! 👍

ivanpauno · 2022-03-02T21:09:04Z

@chapulina should I wait for the CI (infrastructure?) issue to be fixed or can this be merged?

chapulina · 2022-03-02T22:46:20Z

It would be good to have CI passing to merge this, since it's such a big change. I believe everything here should be backportable, right? How about targeting foxy and we merge it forward? That's usually our preferred pattern anyway, and will allow us to run CI for this while #219 is being worked on.

ivanpauno · 2022-03-03T18:58:14Z

Applying the same changes to the foxy branch is not that easy after #192 and some other PRs.
I will check what I can do

ivanpauno · 2022-03-03T19:18:43Z

Applying the same changes to the foxy branch is not that easy after #192 and some other PRs.
I will check what I can do

Well, either way it's going to be hard to rebase 😰.
@chapulina do you prefer to also have #192 and the related PRs applied to foxy/galactic/etc or should I try to apply this change directly to foxy?

In the later case, I probably want to apply #214 and #216 before.

chapulina · 2022-03-03T19:27:51Z

do you prefer to also have #192 and the related PRs applied to foxy/galactic/etc

Those aren't easily backportable either because they change public headers 😕

or should I try to apply this change directly to foxy?

If you're not in a rush to get this in, it may be better to wait until we have CI working on Jammy. It's probably not worth the effort trying to backport this right now.

ivanpauno · 2022-03-03T19:33:28Z

If you're not in a rush to get this in, it may be better to wait until we have CI working on Jammy. It's probably not worth the effort trying to backport this right now.

Sounds good, I think we're not in a rush, and worst case we could use a fork temporally (or complete the rebasing).

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina · 2022-04-22T22:11:54Z

No new test failures, merging!

tests were added

ivanpauno added the enhancement New feature or request label Jan 19, 2022

ivanpauno requested a review from chapulina as a code owner January 19, 2022 17:07

ivanpauno self-assigned this Jan 19, 2022

ahcorde previously requested changes Jan 19, 2022

View reviewed changes

ivanpauno requested a review from ahcorde January 26, 2022 21:42

chapulina reviewed Jan 27, 2022

View reviewed changes

ivanpauno mentioned this pull request Feb 1, 2022

[ros_ign_interfaces] Add GuiCamera, StringVec, TrackVisual, VideoRecord #214

Merged

8 tasks

ivanpauno mentioned this pull request Feb 8, 2022

Add conversions for ros_ign_interfaces/WorldControl and builtin_interfaces/Time #216

Merged

8 tasks

ivanpauno added 8 commits February 11, 2022 12:00

Add service factory

eef122a

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Update ros_ign_interfaces service factory

e93b281

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Use init_and_remove_ros_arguments

24101f6

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Update parameter bridge to support services

2b71faa

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Add tests

cbc1f80

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Remove debug logs

000d899

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Fix other linters errors

c8c5e21

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Fix linters

5779057

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/services-support branch from 6304410 to 5779057 Compare February 11, 2022 17:08

ivanpauno and others added 2 commits February 11, 2022 14:12

nit

f0681e5

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Louise Poubel <louise@openrobotics.org>

Address peer review comments

1b4c935

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

jacobperron reviewed Feb 24, 2022

View reviewed changes

ivanpauno added 2 commits February 28, 2022 15:50

Address peer review comments

b2a37da

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ignition -> Ignition

7961125

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno requested a review from jacobperron February 28, 2022 18:52

jacobperron approved these changes Feb 28, 2022

View reviewed changes

fix

fb65f62

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

chapulina self-requested a review February 28, 2022 19:33

chapulina approved these changes Mar 1, 2022

View reviewed changes

chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 12, 2022

Merge branch 'ros2' into ivanpauno/services-support

5cb8274

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina removed the needs upstream release Blocked by a release of an upstream library label Apr 22, 2022

chapulina merged commit 699a195 into ros2 Apr 22, 2022

chapulina deleted the ivanpauno/services-support branch April 22, 2022 22:12

jacobperron mentioned this pull request Apr 26, 2022

Bridge services #70

Closed

chapulina mentioned this pull request Jul 7, 2022

Bridge for service calls on galactic SetEntityPose #266

Closed

jwdinius mentioned this pull request Apr 30, 2023

Service bridging for ignition services that are not WorldControl? #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bridging services #211

Support bridging services #211

ivanpauno commented Jan 19, 2022

ahcorde left a comment

ivanpauno commented Jan 19, 2022

ivanpauno commented Jan 26, 2022

chapulina left a comment

chapulina Jan 27, 2022

ivanpauno commented Jan 31, 2022

mjcarroll commented Feb 7, 2022

ivanpauno commented Feb 7, 2022

jacobperron left a comment

ivanpauno commented Feb 28, 2022

chapulina left a comment

ivanpauno commented Mar 2, 2022

chapulina commented Mar 2, 2022

ivanpauno commented Mar 3, 2022

ivanpauno commented Mar 3, 2022

chapulina commented Mar 3, 2022

ivanpauno commented Mar 3, 2022

chapulina commented Apr 22, 2022

Support bridging services #211

Support bridging services #211

Conversation

ivanpauno commented Jan 19, 2022

🎉 New feature

Summary

Test it

Checklist

ahcorde left a comment

Choose a reason for hiding this comment

ivanpauno commented Jan 19, 2022

ivanpauno commented Jan 26, 2022

chapulina left a comment

Choose a reason for hiding this comment

chapulina Jan 27, 2022

Choose a reason for hiding this comment

ivanpauno commented Jan 31, 2022

mjcarroll commented Feb 7, 2022

ivanpauno commented Feb 7, 2022

jacobperron left a comment

Choose a reason for hiding this comment

ivanpauno commented Feb 28, 2022

chapulina left a comment

Choose a reason for hiding this comment

ivanpauno commented Mar 2, 2022

chapulina commented Mar 2, 2022

ivanpauno commented Mar 3, 2022

ivanpauno commented Mar 3, 2022

chapulina commented Mar 3, 2022

ivanpauno commented Mar 3, 2022

chapulina commented Apr 22, 2022