[ros2] make transient local reliable

[Karsten1987]

When using the robot state publisher topic (robot_description) to spawn an entity into Gazebo, the topics don't match because the subscription reliability QoS don't match the publisher.

I am wondering whether there is a mismatch between Python and C++ that by default a Python subscription is best effort whereas in C++ it's reliable.

Both, in Dashing as well as Eloquent, we weren't able to spawn objects correctly with the spawn_entity.py script.

/cc @clalancette @relffok

Signed-off-by: Karsten Knese [email protected]

[chapulina]

Interesting that this hasn't come up before. Do you think you'd be able to add a test for spawning from a topic? If a test would be too much, maybe at least add an example to spawn_entity_demo.launch.py?

[Karsten1987]

@chapulina as mentioned offline, 00bf7ff fixes the actual compilation/tests for master.

regarding the tests, I would propose to utilize launch_test to provide a node which publishes some xml model with the same QOS as the robot state publisher and run the entity spawner script after that and see if the scripts returns successfully. However, I don't have a clue about launch testing yet. So any help would be appreciated here.

[Karsten1987]

b9a2ba2 adds a launch test which starts a mock robot state publisher, gzserver and finally checks if the spawn entity script exits correctly; meaning the entity is spawned correctly.

@hidmic could you please have a look at the provided launch test? I've ran into quite some issues when changing launch.actions.OpaqueFunction(function=lambda context: ready_fn()) to the actual advised launch_testing.actions.ReadyToTest()

@chapulina Can you please run CI on this?

[Karsten1987]

It took a bit to figure out what's happening, but I believe I've understood now that the entity_spawner never used an actual latching QoS.
d45abff fixes that. The default values are correctly set, so there is no need to additionally set the reliability to reliable.

[chapulina]

Can you please run CI on this?

Triggered here, hopefully I didn't mess it up:

Also documented the CI triggering process here: https://github.com/ros-simulation/gazebo_ros_pkgs/wiki/ROS-2-CI

[Karsten1987]

@chapulina your CI branch has to be rebased on the latest master.

Specifically you're missing this commit: ros2/ci#364

[chapulina]

Rebased and retriggered: (after I triggered I noticed that CI_TEST_ARGS will test everything. I can't cancel the build, so it will just take a while...)

[Karsten1987]

Re-triggered testing only the related gazebo_ros pkg:

[Karsten1987]

and once more:

[Karsten1987]

not giving up on this one:

[Karsten1987]

boy, I start to love this PR:

[Karsten1987]

Exciting times. Build has passed.

Test ran successfully:

15:02:55 19: [INFO] [ros2-3]: process started with pid [1701]
15:02:55 19: [ros2-3] [INFO] [1579734175.644794957] [spawn_entity]: Spawn Entity started
15:02:55 19: [ros2-3] [INFO] [1579734175.645249507] [spawn_entity]: Loading entity published on topic /mock_robot_description
15:02:55 19: [ros2-3] [INFO] [1579734175.646847036] [spawn_entity]: Waiting for entity xml on /mock_robot_description
15:02:55 19: [ros2-3] [INFO] [1579734175.744446788] [spawn_entity]: Waiting for service /spawn_entity
15:02:55 19: [ros2-3] [INFO] [1579734175.748032487] [spawn_entity]: Calling service /spawn_entity
15:02:55 19: [ros2-3] [INFO] [1579734175.878373861] [spawn_entity]: Spawn status: SpawnEntity: Successfully spawned entity [mock_robot_state_entity]
15:02:55 19: [INFO] [ros2-3]: process has finished cleanly [pid 1701]

So this is now up for review and should definitely be backported to eloquent.

[chapulina]

Thank you for the tests and for addressing all those other issues 💪

[chapulina]

Can the comment be removed?

[Karsten1987]

I am waiting on @hidmic to comment on this. ReadyToTest is apparently the preferred way of handling this, but it yields to problems on my machine. So I hope to get this solved.

[hidmic]

It is the preferred way indeed, and thus I'm curious why it doesn't work for you.

[Karsten1987]

The tests fails with output similar to this:

19: [INFO] [sh-1]: process started with pid [74368]
19: Timed out waiting for processes to start up
19: [INFO] [sh-1]: sending signal 'SIGINT' to process[sh-1]

19: [INFO] [gzserver-2]: process has finished cleanly [pid 73832]
19: Processes under test stopped before tests completed
19: -- run_test.py: return code 1

The test run correctly when using the old method.

[hidmic]

@Karsten1987 did you remove ready_fn from the arguments' list, as in examples/ready_action_test.py when you tried launch_testing.actions.ReadyToTest? On a probably unpleasant note, trying to use both old and new methods simultaneously may leave you in a state where neither works (see here).

[Karsten1987]

That seems to work. See 880cf8b

Is there a way to warn the user of this UB? Something like issuing a warning if ready_fn is available but the OpaqueFunction isn't used? I don't know the implications of this in the first place, just thinking out loud.

[Karsten1987]

@chapulina fee09f6 should address your concerns about removing multiple instances.

CI:

[Karsten1987]

what a time to be alive. Next round of CI hopefully sorting out the launch testing behavior:

[chapulina]

Thank you for all the extra fixes. It all looks good to me.

I tested the changes on gazebo_ros_force_system by using #1035 like this:

• Launch the demo world: ros2 launch gazebo_ros gazebo.launch.py world:="src/gazebo_ros_pkgs/gazebo_ros/worlds/gazebo_ros_force_system_demo.world"
• Insert a box from the top toolbar
• Apply 1st wrench: ros2 service call /apply_link_wrench gazebo_msgs/srv/ApplyLinkWrench '{link_name: "unit_box::link", wrench: { force: { x: 10 } }, duration: {sec: -1, nanosec: 0} }'
• Apply 2nd wrench: ros2 service call /apply_link_wrench gazebo_msgs/srv/ApplyLinkWrench '{link_name: "unit_box::link", wrench: { force: { y: 10 } }, duration: {sec: -1, nanosec: 0} }'
• Clear wrenches: ros2 service call /clear_link_wrenches 'gazebo_msgs/srv/LinkRequest' '{link_name: "unit_box::link"}'
• Reset world (Ctrl+R) and check that the box doesn't move, because both wrenches have been correctly deleted.

(While testing this I noticed that the wrenches are not actually accumulating and only the last wrench is applied. I think we may want to use AddForce instead of SetForce but I can also see a use case for SetForce, so I won't touch the plugin for now.)