-
-
Notifications
You must be signed in to change notification settings - Fork 78
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?
Conversation
…ort and is corporate internal legally cleared for OSS contribution.
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
fmvilas
left a comment
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.
Left a round of review. Thanks for putting it together, Florian 🙏
Achllle
left a comment
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.
My 2c as a ROS dev: ROS 2 parameters are a must-have for this to represent a ROS 2 interface.
@Achllle while we think that could be really interesting to include the parameters, we don't think that it is a hard requirement to have them to be able to have the first version of the ROS2 bindings. |
|
Considering the ROS 2 version of this effort, node_idl / NoDL includes them in their spec, I would argue this is a must-have for a complete spec. It's fine by me if it happens in two phases, but the point of a spec is that it's complete. |
yes. I agree that parameters need to be added to the ros2 bindings. In our use cases, we did not use them so far. That is why they slipped our implementation. I would like to add them in a second step. From what I've gathered, parameters are interfaced through fixed services that all nodes have, unless pro-actively deactivated. What can be transported is also fixed in terms of data / message types. So like you put it, it might only be necessary to add to a node==application what parameters it has and probably not list all the additional We need to investigate this more. I still am having trouble how it embeds into an asyncAPI binding.. Is somebody in the ros-infrastructure team already familiar with asyncAPI @Achllle ? |
|
The best way to get involvement from the ROS community would be via a post in ROS discourse! |
https://discourse.ros.org/t/invitation-to-collaborate-on-asyncapi-specification-for-ros2/42915 @fmvilas if you have the time, it would be a pleasure to have you join such a meeting. |
1. Include in the server binding links and change to none instead of localhost. 2. Include the link to the non-negative less than 232. 3. Explain better the message binding with links. 4. Put at the end a whole example of a ROS application with its 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.
ROS2 -> ROS 2
Co-authored-by: Christophe Bedard <[email protected]>
|
@gramss @amparo-siemens @fmvilas I'm hoping this doesn't get stuck in limbo - what needs to be done to get this over the finish line? |
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
|
Thanks for bringing this up, @Achllle. I somehow missed the notifications. My recommendation is to tidy up this PR a bit and mark all the conversations as resolved when they are. This way it will be clearer what's still missing. That said, in general, my role here is to be a facilitator. You folks are the ROS 2 experts. I can only suggest stuff and ask questions but ultimately it's up to you to decide how to structure the binding. Sure, let's not misuse stuff like Let's make sure we give this a final push 👍 If I ever miss another notification feel free to ping me on Slack or LinkedIn. |
…mw implementations
|
Thank you all! @fmvilas I have tidied up all open conversations. Agree that we should have the I hope we can finally merge this now 👍 Again, thank you all for your contributions! Enjoy the summer everybody. |
|
LGTM as it is 👍 We should now proceed with the PRs in the github.com/asyncapi/spec and github.com/asyncapi/spec-json-schemas repos. I don't think we have to add anything to the github.com/asyncapi/parser-js repo. In the spec repo, we should add the In the spec-json-schemas repo, we should add the And that's it, with that in place we can finally proceed to merge everything have have the cc @derberg @dalelane @char0n @GreenRover please have a look and let them know if I missed something 🙏 |
@gramss Write to me if you will stuck |
@Pakisan could you tell us where exactly we need to add our contribution? Is it into the definitions folder? Or in the bindings folder? Should we add them in the bindings and the definitions will be automatically updated? Thank you for your help 😄 |
|
Put your bindings under https://github.com/asyncapi/spec-json-schemas/tree/master/bindings And create PR. Under it I'll review your changes and will provide further instructions upd: Bind them with each needed object, like this https://github.com/asyncapi/spec-json-schemas/blob/master/definitions/3.0.0/messageBindingsObject.json Server, Message, Channel etc. |
|
Is there anything pending here @fmvilas? 😄 |
|
@fmvilas schemas were merged today asyncapi/spec-json-schemas#582 What's next steps are? fyi: @derberg |
|
well schemas should not be released as bindings and spec is not released 😃 for that we need new release cycle to start, and volunteer release coordinator: https://github.com/asyncapi/spec/blob/master/RELEASE_PROCESS.md |
|
I volunteer to become release coordinator for 3.1.0. Unless someone else wants to try that ride 😄 Or want to pair with me. @amparo-siemens to answer your question, what's still pending is that we should release a new version of the spec to accommodate the ROS2 binding. Once that's done, everything else will follow. You know, this binding, the spec-json-schema PR that was reverted, and I think that's it because the parser-js will pick up changes from the spec-json-schema repo automatically IIRC. This is a minor release so it shouldn't take long. What's making it to take longer is the holiday season. I was out the whole July, partially active in August as I'm writing a book, Lukasz will be out some days in August, and I bet other spec code owners will be out too. So, in short, I can't promise this will be done in August but if everyone is available, I'll try my best. |
|
@fmvilas there might be another repo waiting for this to become official.. 😇 |
|
haha! love the name 🌹 |
|
@fmvilas any update on the V3.1 minor release? 🙂 |
Release on me, my bad. I'll prepare needed things in a few days |
|
in case you missed asyncapi/spec-json-schemas#582 (comment) @amparo-siemens @gramss I'm available for onboarding call. Please reach out to me at [email protected] to sync on the exact day and time. It's purely bindings maintainance onboarding but I'll also share overall info about how the AsyncAPI Initiative operates. |
Sorry, we missed it. We will reach you now 🙂 |
|
Hey folks, fyi we scheduled our Spec Triage meeting: asyncapi/spec#1131 (comment) First time in a very, very long time 😄 I think v3-trauma has passed 😄 |
Description
Maintaining will be done through @amparo-siemens and me.
Further maintainers and improvements suggested in #254 are highly welcome.
This binding is tested already in a few internal tools and generators as seen in this presentation: ROSCon DE
cc: @renzo-sie (thank you for the preparations!) and @fmvilas