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

Expanding support for rosbridge dependent software #708

Merged
merged 6 commits into from Feb 4, 2022

Conversation

kedus42
Copy link
Contributor

@kedus42 kedus42 commented Jan 30, 2022

Public API Changes
None

Description
Added a service that responds with the ros version and distro being used. This helps software dependent on rosbridge recognize which ros messages to expect.

@defunctzombie
Copy link
Contributor

@kedus42 Thanks for taking a first pass at this. It seems sensible that rosbridge, which works across ROS versions, report this information since ROS1 and ROS2 are quite different - and event ROS2 has some differences between versions IIRC.

Before any code is changed, we should agree on updates to the spec

If you could update this PR (or make a new PR) with what you think the spec should look like to support this feature we can start there.

@kedus42
Copy link
Contributor Author

kedus42 commented Feb 4, 2022

Hi @defunctzombie, I've moved some of the changes into a future pull request should this one go through. This pull request only adds a service that gets the ros version and distro and should not require any updates to the specs.

@defunctzombie
Copy link
Contributor

@kedus42 Who are the intended users of this new service call? If the end goal is to add an API to rosbridge to expose this information - we should start with the spec and work backwards from there. By updating the spec we can get a sense of how this functionality will appear to users and then make an implementation that supports that functionality.

@kedus42
Copy link
Contributor Author

kedus42 commented Feb 4, 2022

@defunctzombie The intended users are packages that depend on rosbridge suite (specifically zethus) that need to get which version of ros is running. The way I propose they get this information is by calling a ros service which is started up by this pull request. If I am not mistaken, ros services are already supported by the specs.

@defunctzombie
Copy link
Contributor

The way I propose they get this information is by calling a ros service which is started up by this pull request. If I am not mistaken, ros services are already supported by the specs.

Gotcha - so the idea is there's a ROS service for the request rather than a rosbridge protocol layer op?

@jtbandes thoughts on this approach? Seems like having the protocol layer respond with the version would be more natural to users but if this works as a starting point for @kedus42 then adding this service to rosapi seems 👍 with me.

Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems fine and a bit simpler than adding an op to the protocol. Is it a problem if a client requests the service and it's not available (e.g. if they are using an older version of the bridge or talking to a ROS 1 bridge)?

@jtbandes jtbandes merged commit 1342a50 into RobotWebTools:ros2 Feb 4, 2022
@kedus42
Copy link
Contributor Author

kedus42 commented Feb 4, 2022

@jtbandes Yes, but ros allows you to check if a service is up before calling it, I think most clients make use of this anyway

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

3 participants