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

[WIP] Rough port of interactive marker server to Dashing #41

Closed

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jun 4, 2019

Opening PR for visibility. This is a rough port of the interactive marker server to ROS 2 Dashing. I was testing if the server could be used on a ROS 2 node to communicate with a ROS 1 client (rviz interactive marker display) over the bridge. The answer appears to be no; at least not without modifying the ROS 1 bridge to use QoS settings for topics that were latched in ROS 1.

@wjwwood I'm not sure if I'll be porting any more of this, or coming back to it at all. Should I leave the PR open or closed?

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@wjwwood
Copy link
Member

wjwwood commented Jun 4, 2019

I don't have a preference. I guess you could close it but not delete the branch? Seems like some bot might complain in the future if we leave the branch. I dunno.

@david-vay
Copy link

So there are no latched topics in ROS2, or does this just not translate well over the bridge?

@sloretz
Copy link
Contributor Author

sloretz commented Jun 5, 2019

So there are no latched topics in ROS2, or does this just not translate well over the bridge?

ROS 2 has more nuanced QoS settings than just latched and queue size. I think the problem is ros1_bridge always creates publishers and subscribers with the same QoS settings. I would need a way to change those QoS settings to make this work.

@wjwwood
Copy link
Member

wjwwood commented Jun 5, 2019

So there are no latched topics in ROS2, (?)

Short answer is, no ROS 2 does have something like latching, but it's not always just a single message (the latest message) that is "latched". In DDS and in ROS 2 this setting is called Durability and is influenced by the History setting.

or does this just not translate well over the bridge?

Therefore, no, it doesn't translate perfectly over the bridge, but the bridge doesn't even make an attempt to forward this behavior at them moment. As @sloretz said.

@jacobperron
Copy link
Contributor

I'm picking this up as part of migrating the InteractiveMarkerDisplay (ros2/rviz#98). I want to ask a couple questions.

Do we want to do a direct port to ROS 2 (ie. minimal changes to get it working)?
Or, go to the drawing board and do some refactoring? This seems like an ideal time to improve on the original design (if there are improvements to be made). Perhaps others more familiar with this package can chime in. We could go so far as updating the various message definitions involved; though this could make ROS 1 bridge support more difficult.

I don't have anything specific in mind right now, but figured I'd ask before going forward. Thoughts?

@david-vay
Copy link

I'm personally quite happy with the design and wouldn't see a need to change the API or how it's implemented. Maybe ROS2 has new features that I don't know about which you could use though.

@david-vay
Copy link

Well, so the latched "init" topic would perhaps be something that you could improve, i.e. use a service to request the full state instead.

@jacobperron
Copy link
Contributor

Well, so the latched "init" topic would perhaps be something that you could improve, i.e. use a service to request the full state instead.

Yeah, I was also thinking the same.

Some other thoughts for refactoring:

  • Add the feedback publisher to the client (right now RViz is managing the feedback publisher).
  • Instead of the KEEP_ALIVE timer sent by the server, we could use Liveliness QoS setting on the update topic.

@jacobperron jacobperron mentioned this pull request Jul 19, 2019
@sloretz
Copy link
Contributor Author

sloretz commented Aug 14, 2019

Closing in favor of #44

@sloretz sloretz closed this Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants