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

Port to ROS 2 #42

Closed
jacobperron opened this issue Jul 19, 2019 · 11 comments
Closed

Port to ROS 2 #42

jacobperron opened this issue Jul 19, 2019 · 11 comments

Comments

@jacobperron
Copy link
Contributor

I've started a port to ROS 2.

Following a brief discussion in #41, I'll outline my plans here. Besides the obvious changes to switch to rclcpp, I plan to make the following changes:

  • Style refactor + enable common ament linter tests (cpplint, uncrustify, flake8, etc ...)
  • Use C++11/14 features where appropriate (e.g smart pointers).
  • Switch from tf to tf2
  • Add the interactive marker feedback publisher to InteractiveMarkerClient.
    • Previously handled by RViz.
  • Replace the "init" topic with a ROS service.
    • It seems more intuitive for clients to request the full state on demand rather than listening to a latched topic (which may have redundant messages queued).
    • We can avoid the logic for clients unsubscribing to the topic after receiving the state.
    • I will prototype a ROS 1 bridge shim to handle the new API.
  • Replace the use of the KEEP_ALIVE timer with the Liveliness QoS setting.
    • Seems like a natural use of Liveliness and should remove the existing timer-related logic.

I've completed a subset of the points above and will open PRs soon for visibility/review.

Unless there are objections, I will target my PR to a new ros2 branch of this repository.

@jacobperron
Copy link
Contributor Author

  • Replace the "init" topic with a ROS service.

See ros2/common_interfaces#70 for the proposed service interface

@dgossow
Copy link
Member

dgossow commented Jul 22, 2019

So, one question that comes to my mind: what if multiple interactive marker servers publish to the same topic? This was supported in the old architecture as far as I remember.

Perhaps its not a feature that is needed though and we can drop it

@jacobperron
Copy link
Contributor Author

So, one question that comes to my mind: what if multiple interactive marker servers publish to the same topic?

That's a good question. I guess the service call from a client would only be processed by one of the servers, and the client would filter the feedback topic by the server ID. If consecutive service calls were answered by a different server, then the response could be ignored (or an error reported). If we assume the relationship between clients and servers is many-to-one, then I think the implementation can be simplified a bit.

I don't know if one client to many servers is a common use-case. If we want to support this, we should probably continue to use a topic. IMO, users that want to support multiple servers should instantiate multiple clients (e.g. add multiple interactive marker displays in RVIz).

@dgossow
Copy link
Member

dgossow commented Jul 23, 2019

Yeah, I think I added support for this since it was easy to do with the init topic. But probably not a common use case, and you can just have multiple different topics as you said.

The client would then just need to detect when there are multiple servers on the same topic and refuse to connect.

What happens if multiple nodes advertise the same service, is that even possible?

@jacobperron
Copy link
Contributor Author

What happens if multiple nodes advertise the same service, is that even possible?

It is possible. Both servers will receive the request from a client, but only one of the responses is passed back to the client (I think it's whichever arrives first). It looks like this behavior comes from the RMW layer.

Also, I noticed that Fast-RTPS behaves differently than other DDS implementations (Connext and Opensplice); only one of the servers receives the request and it seems to alternate 🤷‍♂️


I've requested feedback on discourse.ros.org, so maybe we'll get some input from other community members

@IanTheEngineer
Copy link

@jacobperron MoveIt uses interactive markers for default GUI operation. Would you care to describe the changes you're proposing for interactive markers at the next MoveIt Maintainers Meeting (7/25)?
https://discourse.ros.org/t/moveit-maintainer-meeting-all-invited-july-25th/9899

@jacobperron
Copy link
Contributor Author

Would you care to describe the changes you're proposing for interactive markers at the next MoveIt Maintainers Meeting (7/25)?

@IanTheEngineer Sure, I can join the call.

@IanTheEngineer
Copy link

Just to recap the discussion on the MoveIt call revolving around interactive_markers:

Thanks for getting buy-in from the MoveIt community, @jacobperron.

One additional (perhaps naive) question - in transitioning to a service request / response model with an additional feedback topic, would an action interface make sense?

@jacobperron
Copy link
Contributor Author

One additional (perhaps naive) question - in transitioning to a service request / response model with an additional feedback topic, would an action interface make sense?

I'd considered it, but it seemed a little overkill for interactive markers, e.g.

  • There's no real sense of a "result" or "cancel" for interactive markers. When a client or server is done communicating they can simply stop / shutdown.
  • Similarly, we wouldn't get much from the action "status" topic.
  • There is two-way feedback between the interactive marker client and server. We could make use of the action's feedback topic from server to client, but we'd still need an additional publisher for feedback from client to server.

So, rather than using an action + an extra feedback topic (total 3 services, 3 topics) I've opted for 1 service and 2 topics.

@IanTheEngineer
Copy link

Thanks for the explanation. The reduction in interfaces alone makes the services path more appealing

@jacobperron
Copy link
Contributor Author

Done in #44

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

No branches or pull requests

3 participants