Open artivis opened 5 years ago
@jacobperron Thanks for this initial review. I'll address it asap.
In addition to my comments below, it would also be nice to add an "alternatives" section where we document other options considered (if any) related to the node IDL.
I'm not sure of what alternatives we are talking about, alternative ideas to the concept presented here (explicitly exported node api) or alternative implementation design of the IDL or something else ?
Using IDL for this is imo a really confusing choice. ROS 2 already used the OMG IDL spec for defining message definitions. Therefore I strongly suggest a different abbreviation / terminology for this.
We were expecting this particular concern to be raised as we share it too. However we kept this terminology - 'Node IDL' - as it best describes the proposal. We're open to suggestions tho.
Also the "definition language" part of IDL doesn't make sense since the information is described in XML and not like message interfaces in an actual "definition language" (specific syntax).
We did tripped a little over the IDL
acronym. Depending on the source the D
letter stands for different things and we started this document with Description
in mind rather than Definition
.
Maybe "Node API definition / descriptor"? (We can figure that out later though to not derail the discussion with a bike shed topic like this.)
Agreed.
There is something I haven't seen made explicit yet, but which I think we should. This is about where the definitions are gathered and what they mean, semantically.
The current proposal says that a package can define the interfaces of the nodes it includes. Accepting that this can be done using xinclude
s to include multiple XML files that define individual nodes, this is what I think is semantically being created by the current proposal:
I may have missed some parts by I think it gets my point across.
At the source level, the "package namespace" concept does not really exist in current ROS. However it does exist at the tooling level, in tools such as ros2 run
and launch
.
What I want to be clear on is if we are all happy making the "package namespace" concept much more formal than it currently is and extend it throughout the run-time ROS system. Having run-time checks of "package interfaces" will do that because it makes the package a run-time entity.
Personally I am fine with that, I think it's a natural extension of where things are currently and there are benefits to doing so for identifying specific node implementations, even in cases where two nodes from two packages implement the exact same interface declared in the exact same Node IDL file.
Would the Node IDL make it possible to create some sort of rosbridge between nodes of different ROS 2 distributions?
It could be very liberating in large heterogeneous systems to allow nodes running on different distributions to communicate with each other. Right now if one node is stuck on an old distribution, for one reason or another like being proprietary code, it cannot participate in the system unless all other nodes are held or regressed back to that old distribution. Even if it was less efficient, a bridge could allow most nodes to upgrade at will while still letting old nodes participate in the system.
That's an interesting idea, @peterpolidoro. Off-hand I'd say it could help with it inasmuch as the node in question might change what it calls a given topic across distros, but the IDL doesn't dive into message contents, so it would be fairly limited. Specifically, if the message used by the given topic changed its structure, the IDL can't really help you there.
but the IDL doesn't dive into message contents
Perhaps the message contents are outside the scope of the Node IDL, but that information could be very valuable in a variety of situations. It could allow you to verify, for safety or consistency, that a message is defined as you expect. It might also be used to create some sort of plug-and-play interface that allows you to first ask a node to give you its IDL and use that to generate the messages necessary to communicate with that node so you do not need to access the original package that defined the node messages. It might be nice at times to be able to get everything you need to communicate with a node locally from the node itself, instead of relying on a connection to the outside world and finding the proper interface version for that node.
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/guidelines-on-how-to-architect-ros-based-systems/12641/17
The proposal has been updated to match the implementation in https://github.com/ubuntu-robotics/nodl .
Changes of note:
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/announcing-a-ros-package-generator/16317/3
@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?
@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?
Let me forward this to @Arnatious who has picked up this up.
@artivis What's the status of this design? I see there has been lots of discussion, though no approvals as of yet. Is there more work that needs to be done here?
There's been no comment since the last revision of the nodl schema, which added the role
attribute. I'm taking that as a good sign.
We have a reference implementation in https://github.com/ubuntu-robotics/nodl, and are nearly done with a simple launch_ros
integration pending some changes in SROS2. I think this proposal is ready to move ahead.
We have a reference implementation in https://github.com/ubuntu-robotics/nodl, and are nearly done with a simple
launch_ros
integration pending some changes in SROS2. I think this proposal is ready to move ahead.
All right, then we need some approvals. Once we have that, I'll go ahead and merge.
@Arnatious Have you thought of using yaml instead of xml? I just read some PRs and saw that for QoS settings first xml was discussed in #296 and then they decided to go the yaml way like for parameters in #300. Is it because of DDS or security interaction?
Have you thought of using yaml instead of xml?
We actually prefer YAML, but DDS and SROS2 use XML, and the package.xml (which the nodl would be alongside) is also XML. Using YAML just didn't seem consistent.
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/eprosima-presents-visual-ros/17632/3
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/eprosima-presents-visual-ros/17632/4
Hi @Arnatious, over the last few weeks - with the help of this design doc and the ubuntu-robotics/nodl
API - some preliminary work has been done in writing a nodl_to_policy
tool, that converts NoDL description files to a corresponding ROS 2 access control policy file. In doing so, we've come across some ambiguities with the NoDL description that I wanted to raise for discussion here:
<profile>
tag in an access control policy needs a node namespace, and insofar, the only option for namespace is to assume that all nodes are in the default "/
" namespace, which is an invalid assumption for many reasons. Having said that, are there/should there be plans to incorporate the namespace concept into NoDL? Possibly as an additional (optional) attribute of the <node>
tag?/[ns]/[node]
". Having said that, should namespacing get formalized as a concept in the NoDL description, enclave paths are probably a non-issue. Else, we should consider adding the concept of a node's enclave path to the NoDL description too.describe_parameters
, get_parameters
, etc.), default topics (rosout
, parameter_events
), and default actions in a NoDL description? I'm assuming these are implicit with every node, and hence considered redundant from the description's POV. However, when converting from a NoDL description to an access control policy, certain assumptions have to be made about the presence/absence of said default services/topics/actions. This is not ideal from a security POV, even if this simply grants "empty" permissions. A fallout from this discussion is that a node's "type" (is it a lifecycle node?) is currently not expressed in the NoDL description. The "type" of node changes the set of "default" permission tags to consider for it.FYI @iche033 @clalancette @wjwwood
I was wondering if this is still under active development and will get merged in?
Hi @danthony06, We do plan on picking up this work soon. Note that altho this PR is note yet merged, NoDL-related packages are already part of REP 2005 - ROS 2 Common Packages.
+1 to this. Merging it would give it more visibility and we need to encourage participation and contributors.
El El jue, 2 dic 2021 a las 17:22, Jeremie Deray @.***> escribió:
Hi @danthony06 https://github.com/danthony06, We do plan on picking up this work soon. Note that altho this PR is note yet merged, NoDL-related packages are already part of REP 2005 - ROS 2 Common Packages https://ros.org/reps/rep-2005.html.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ros2/design/pull/266#issuecomment-984783442, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKPYDUB7CUHIOUFK6ZHBGLUO6MMDANCNFSM4JRLER2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
I'm a little late to this discussion, but I am looking at this from the perspective of generating package documentation with a tool like rosdoc2. When we document interfaces in Python or C++, classes and parameters typically have a short description. It would not be difficult to extend rosdoc2 to read this NoDL file and extract information for documentation purposes - but nodes and parameters would need descriptions. Could you add a description attribute (type string) to the node and parameter elements for use by documentation utilities?
[update] I see that rosindex has some recommendations for node documentation in the package.xml file. Coming from a documentation perspective, their examples show a lot of text for nodes descriptions.
What is the relationship of this PR to the rosindex proposal? I don't think you really want to ask package maintainers to document their packages in two places. If this PR becomes the definitive source, then you'll need the same info (that is a description attribute or element) that rosindex wanted.
Design document for the 'Node Interface Definition Language (IDL)',
Initially discussed within the Security WG, this proposal is not security-related only. Security features and tooling building on top of this will be proposed in a subsequent document.
FYI @ruffsl @jacobperron @thomas-moulard @kyrofa