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 6 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.
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.
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.
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.