fix robot state publisher #14

[Karsten1987]

fixes ros2/robot_state_publisher#14

As far as I could tell there were two problems which are both addressed in this PR.

• wrong QoS settings: The robot state publisher sends out a latched message. So RViz has to subscribe to it with the same settings.
• filepath vs xml content: In the current implementation, RViz does not expect a full formatted xml string but rather interprets the incoming message as a file path.

Signed-off-by: Karsten Knese karsten@openrobotics.org

[clalancette]

* wrong QoS settings: The robot state publisher sends out a latched message. So RViz has to subscribe to it with the same settings.

Just to be clear, it is actually fine to subscribe to a TRANSIENT_LOCAL topic with a VOLATILE subscription. You just won't get historical data; you'll only get data that is published after you connect.

The reason that this matters is that robot_state_publisher only ever publishes the robot description once, the very first time it publishes transforms.

With that said, I'm wondering what we should do here. I do think it makes sense to have rviz2 subscribe with TRANSIENT_LOCAL so that it gets the historical data. But it might also make sense to change robot_state_publisher to re-publish the description on some fixed interval so that other subscribers (which aren't TRANSIENT_LOCAL) will get the data. After all, without reading the robot_state_publisher code, it's not currently easy to tell that the QoS is wrong.

Thoughts?

[wjwwood]

But it might also make sense to change robot_state_publisher to re-publish the description on some fixed interval so that other subscribers (which aren't TRANSIENT_LOCAL) will get the data.

I mean, that's the use case for transient local, so I don't know why we'd avoid using it in this case. I think it should just be documented that it is transient local.

After all, without reading the robot_state_publisher code, it's not currently easy to tell that the QoS is wrong.

Yeah, it should definitely be better documented. We also probably need better warnings when QoS don't match, though that has to be balanced with cases where that's intentional (something along the same lines as SFINAE, except QoS mismatch rather than substitution failure).

Also, this case brings up the point against that DDS does not have a "best available" option for subscriptions, which would take transient local if it were available but will communicate with volatile if not. AFAIK such a setting does not exist.

[wjwwood]

* filepath vs xml content: In the current implementation, RViz does not expect a full formatted xml string but rather interprets the incoming message as a file path.

This seems like that actual fix. Without this it would never load a urdf from robot_state_publisher.

[clalancette]

I mean, that's the use case for transient local, so I don't know why we'd avoid using it in this case. I think it should just be documented that it is transient local.

Don't get me wrong, I think it should be TRANSIENT_LOCAL. What I'm essentially arguing for is that it should be TRANSIENT_LOCAL with a depth of 1 and a republication of once a second (or whatever). Because we don't have good QoS introspection tools, this will alleviate some of the pain that downstream consumers feel, since they'll get the data whether they use TRANSIENT_LOCAL or not.

[Karsten1987]

forgot CI - testing only rviz_default_plugins:

• Linux
• macOS
• Windows