Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: initial ROS2 AsyncAPI contribution by SIEMENS AG #270
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
base: master
Are you sure you want to change the base?
feat: initial ROS2 AsyncAPI contribution by SIEMENS AG #270
Changes from 2 commits
7872c4f
3d21380
f62254f
a1e9876
463fb3d
f6380c3
5e6bbc8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor perhaps, but while the main RMWs in use may be DDS-based, there are certainly others that aren't (
rmw_zenoh
being a recent addition, but also eclipse-ecal/rmw_ecal fi).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment!
As you know, there are a quite acouple of them with my favourite (for the sake of this exact argument) the christophebedard/rmw_email.
We also had this argument earlier with @fmvilas , that for the moment we only want to focus on DDS and Zenoh based systems. But I think that the majority of the RMW implementations should also be representable through this binding. But this current implication should be put out exactly in this sentence. Therefor, thank you for highlighting it!
Additionally, we are already indicating the vast options one can choose from with ROS2 here:
bindings/ros2/README.md
Line 27 in 463fb3d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're including a "ROS 2 Versions" column but this column always has the "all" value. Should we get rid of it or do you expect future properties to appear only for certain versions of ROS 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently this list is not comprehensive. This could be much larger but a better place to look for changes is here: e.g. for Jazzy: official ROS doc for release changelog
For an initial PR I would leave it that way and depending of the acceptance and usage of AsyncAPI in ROS this can be extended more and bring back the ROS 2 Version column that way.
Leaving this open to feedback from others for now, but the change is reflected in the latest commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no channel binding, I recommend leaving it out of the document. E.g., the way we're doing with the AMQP 1.0 channel binding: https://github.com/asyncapi/bindings/tree/master/amqp1#channel-binding-object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm.. Just had time to dig a little bit deeper into channels.
Would you describe this picture here as a channel?

Source from OSRF
Or another example here (circle = node == application and box = topic == message)

The message
/joint_states
is consumed by 2 nodes..For me,
topic
translates to an asyncAPImessage
and a rosnode
translates to an asyncAPIapplication
.There can be one-to-many communications in ROS between one node (pub) and many other nodes (subs). Same applies for the other ros2
roles
.But I am unsure how this can be represented as a channel here. The specifics how the message is transported to many clients is defined inside of the concrete rmw implementation(?). A node can only tune how to react to a new event but not influence the transport mode other than specifying the QoS level.
Does @Achllle maybe has an idea about that? Here is the asyncAPI definition: link
I am having a hard time especially with translating this sentence from the doc to ROS:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm 🤔 from what I'm reading here, it seems that AsyncAPI channels map to ROS2 topics and AsyncAPI messages map to ROS2 messages (or data types, data units). So, from what I'm learning so far:
Feel free to correct me as I'm looking at this for the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs here:
And I'm assuming, in
listener_callback
, themsg
variable will contain amsg.header
which maps directly to AsyncAPI's message headers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gavanderhoorn thank you for the proper fact checking and updating (at least) me on the current state of some ROS developments. 👍
We can bring the topic about a ROS2 release-versioned database of common-msgs in the mentioned discourse discussion.
I am open to discuss this topic. The binding should include a guideline how to deal with common messages regardless of the outcome of this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A specific (recent) distribution of ROS 2 + the package name + interface name is the only thing needed to fully define a message. It feels somewhat silly to redefine such messages in the asyncAPI. Without the message headers or import it's not possible (AFAIK) to create the payload like one would do with a json blob. To me the ideal experience would involve linking to the interface definition rather than redefining it. When someone intends to update to a newer distribution, they would have to manually validate all AsyncAPI messages to match the new distribution. Most ROS applications heavily depend on those interface definitions included in ros-base or other common packages, so this would mean a lot of duplication and room for user error.
I don't really see the point with 'black box code'. Such repos will have at least their interface descriptions made available, so whether the code is accessible doesn't really matter.
Perhaps this 'linking' is possible through a binding where if it's a standard message, you'd link to it with:
and we include some magic to render the definitions when they're found.
Looking at NoDL, it seems that this approach is happening there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Achllle First, thank you for your thoughtful feedback on this discussion. I appreciate your expertise in ROS2 and would like to address some of the points you raised.
Regarding References in AsyncAPI
I understand your suggestion about using direct URL references to ROS2 message definitions. While this approach seems intuitive from a ROS2 perspective, the AsyncAPI specification has a specific way of defining message payloads, which relies on the $ref keyword to reference reusable schema definitions. From my understanding, it's not possible to use a direct URL in this context
The $ref syntax you proposed would require custom extensions or resolvers that aren't part of the standard AsyncAPI functionality. This could create compatibility issues with existing AsyncAPI tools and validators.
On Message Definition Duplication
You make an excellent point that a ROS2 distribution + package name + interface name is sufficient to fully define a message. What might seem like redundant redefinition actually serves an important purpose in our approach.
By including complete message definitions within the AsyncAPI document, we enable:
Our Approach: Automation Over Manual Definition
I believe we might be aligned on the core issue but approaching it differently. Our goal isn't to have developers manually redefine ROS2 messages in AsyncAPI (which would indeed be tedious and error-prone). Instead, we envision automated tools that:
This way, developers don't need to manually maintain these definitions - the tooling handles the synchronization between ROS2 interfaces and AsyncAPI documents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmvilas can you please comment on
specifically whether this would be achievable today through e.g. bindings? Is there any precedent for this as well, such as including existing json/yaml schemas that are defined outside AsyncAPI? Are those re-defined within the AsyncAPI spec?
I understand it may be harder to do this, but it's hard to argue that it's also not superior. Automated generation doesn't solve the issue of duplicating information, thus leading to potential conflicts, especially since messages can vary between ROS 2 distros.
Documentation generation can take care of linking or even embedding the message definitions so that would address self-containment. That would also solves the issue for those unfamiliar with ROS 2. I'm also not sure why someone unfamiliar with ROS 2 would want to deeply interact with a ROS 2 API without first learning ROS 2 basics.
That being said, if it proves unattainable or would require updating the spec in a major way, I'm fine with the duplication approach, though I might consider using NoDL in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree on getting @fmvilas opinion on this.
I think we cannot alter it from this binding-spec but rather would need to make contributions to the larger asyncAPI body.
@Achllle we should also look at the problem from another angle. ROS already does a great job at defining its messages through a standard approach on the code level with the dedicated
.msg
/.srv
and.action
files. Are other supported protocols also doing that way? I was not able to gather such information about MQTT for instance.Regarding your concerns:
Can we dig deeper into this?
Are you concerned about the duplication itself or more about the missing reference to the original origin?
The datatypes need to be translated anyway from ROS into AsyncAPI at some point. Either at a rendering stage of HTML or before by holding the information in files.
We could influence this from the ROS UX so that those files are only generated on-demand and deleted afterwards.
If your only concern might be regarding different ros distros, this information is also available at the head asyncAPI file. But this is not pinned on the message level for mix-bistro-robots (that I would like to neglect if possible at a first stage?).
Reading a documentation of a robot and finding information (where and if e.g. a camera raw stream is made available) is a step before going into the details of a specific application. Could be that a robot offers such information only as a websocket, then one does not need to go deeper into the ROS universe.
Generally, I would love to bring this discussion on a use-case based requirement level. I totally respect your view on this subject but would love to bring it into use-case scenarios that we can either accept or reject (non-relevant) and with it the proposing requirements.
This is something we could focus on the meeting next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this. Could we improve this description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be put in the description field of the table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I'm nitpicking, but technically speaking:
send and receive refers to publisher.publish(), receive would point to a callback()/register_callback()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why we used send/receive is because those are the only options for the field
action
in AsyncAPI and we were tying to map it to the different ROS2 types, since we cannot change the valid options for the fieldaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understand correctly, when the
action
field issend
, this value can bepublisher
,action_client
, orservice_client
. And whenaction
isreceive
, this value can besubscriber
,action_server
, orservice_server
. Is that right? If so, this relationship should be expressed here instead of above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will put the explanation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to make this value the default so people don't have to specify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion does not make sense since in ROS2 there is no default value for roles. It depends if you are using topics, actions, services... and it depends if you are publishing, subscribing... But we are completely open to suggestions. @Achllle what is your opinion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not a strong opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand
role
. I can't find it in the adding bindings doc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have a ros2 binding named as "role" inside the operation bindings to define if a node is publisher/subscriber or service/action client/server. Right now you don't see it in the binding documentation because it does not exist yet. What we are trying here is to define the necessary bindings to put them as official bindings (in the same way that you see right know the mqtt ones) and then you will be able to see them. Right know all ros2 bindings that don't use the existing ones, are experimental.
Summarizing, in bindings we can get "anything" we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this table about? When should it be used and what for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should delete the table from here since it is not about the bindings but we should have this table somewhere. We think it is important that ros developers have this table to know how to express the ros2 messages types in asyncAPI format.
@Achllle do you think that we should put this information in the ros2 documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm all in with having it, I just think we have to explain what is for. Right now it's just there without any further explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe this belongs here, agreed with adding explanation. Suggest including a link to the design document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We included the design document suggested by @Achllle .
But @fmvilas question is not answered:
I think such an information could be needed to properly integrate/visualize different bindings in one tool. That is why you have your AsyncAPI message types and formats for.
Right now, this is used as our Rosetta Stone table for the siemens PoC AsyncAPI generator reading ros message file definitions and producing respecting AsyncAPI files.
I hope that the current wording reflects this already. If not, we can make it more distinctive.