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.
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
Design document for Node Interface Definition Language (IDL) #266
base: gh-pages
Are you sure you want to change the base?
Design document for Node Interface Definition Language (IDL) #266
Changes from 13 commits
dccd234
e8e0247
b8cc64a
3c8028c
f8ebe30
ee4ee96
80ff88b
fce235c
45d4919
99a3e7c
ed7aedb
78f4d27
23aeab0
3fb7ab5
c97a7a9
da63aa6
19b9738
2da495a
ef5018b
3642623
83a4715
2a50c44
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.
I assume you are implicitly assuming non-runtime here which would be good to mention. At runtime it should absolutely be possible to introspect these information from a node.
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.
No we are not implicitly assuming non-runtime.
This is somewhat related to a point made in the
Security motivation
section were we consider theros2 security generate_policy
utility that only capturesSuch snapshot(s) may very well miss some dynamic aspects of the overall graph.
On an other hand we considered code introspection tools as far from ideal - e.g. for cpp.
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 would argue you do. All topics / service which are created during startup can perfectly be introspected at runtime. (Or do you see a case where it is not?) It would be technically possible to 1) start a (well written) component (which hopefully uses a lifecycle) and 2) query its "API" when it has finished initialization and using that information 3) generate the XML file representing the nodes interface.
Even with the "Node IDL" there is no limitation that a node can't change its interface later at runtime (add new topic, rename parameter, remove services). This kind of dynamic behavior can obviously not be captured by any static 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.
The "Node IDL" indeed does not enforce anything and could possibly go out-of-sync. It is one of the 'challenge' discussed in a comment/suggestion above.
The rational precisely covers the kind of dynamic behavior you are describing, assuming that the node author knows best the whole API and writes the (up to date) "Node IDL". Depending on the application, we foresee the development of some tools to catch the other dynamic behavior such as launch-time remapping.
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 suggest you find a catchy name. "IDL" is a known term in computer science, ROS 1 defines an IDL, and DDS uses "the" IDL (the one from OMG).
"NoDL"?
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 I see the potential for confusion, but what is proposed does seem like it could be called an IDL. For nodes. So Node IDL seems appropriate.
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 believe the terminology is appropriate, however I do agree that it could be confusing. As discussed in another thread with @dirk-thomas we leave the potential renaming to later on.
ps - I like 'NoDL', other acronymes include 'NAPI', 'NAPID' (Node API 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.
I didn't mean to imply the terminology is incorrect, it is perfectly fine. But it's an over-used term and a catchy name avoids 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.
Yeah, no strong feelings on the name. It is indeed an IDL, but we can name it whatever we like.
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 will be the impact of not having a complete description of node interfaces? Will you provide a way for a user to know if parts of their system are missing node descriptions? If not, will tools still be useful if the user does not know if they are impacting the entire system? Even if you do, will tools still be useful if applied to just part of the system?
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.
For clarification, just in case: partial here applies at the package level, not all node are required to have their interface described. However, if the interface of a given node is described, then it must be described fully.
The take here is that not every node in a package are meant to be part of a live graph, some may be meant for visualization, debugging and what not and therefore exporting their interface may not be useful.
It is assumed that a tool cannot be used for a node with no exported interface, nor for a node partially described (the later may be subject to technical difficulties).
This is a bit of a chicken & egg problem. The node IDL proposal addresses the issue that a system cannot be fully described with automatic tools since such tool may not capture the entire dynamic aspect of the graph. There could be tools such as the
ros nodl verify
you mentioned but we should not sell it as an exhaustive one. Mayberos nodl try_verify
would then be more appropriate.Related to this discussion.
From our perspective - automating security - the entire live system should be described.
But that may not be true for all tools, thus it would depend on the tool in use, assuming the aforementioned specifications on 'partial'.
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.
Makes me think of possible use-cases for partial NoDLs. For example, I write a LIDAR driver node that implements a common scan_nodl, but I also have an extra publisher. For security purposes, I want to describe all topics of my node, but for a system construction tool I only care that it implements scan_nodl. As the node author, it'd be nice to include scan_nodl into my custom NoDL and somehow communicate that my node implements scan_nodl.
On the other hand, maybe composable NoDLs are too fragile. If the upstream scan_nodl changes, my node may break my system unexpectedly at runtime. It might be easier to maintain a non-composed NoDL for my node, compared to relying on external descriptions.
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.
Well this isn't really an example of a partial NoDL file but rather a case for template/inheritable/composable one. The final NoDL of your driver would still export the scan_nodl + extra publisher and thus be complete.
I do see the potential for composition for NoDL, but it requires some more thinking - how to distribute template NoDL files ? How to prevent breaking from upstream ? Etc.
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 the wonderful world of interface design! ;) There's oodles of theory behind all this stuff.
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 possibility would be to specify that a node implements a version of a template (as if pointing to a git commit). If the upstream version changes, you are not obligated to implement newer versions. Of course, this might be too much clutter already.
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 have a use case for partial descriptions in static analysis, but I am kind of in a rush right now. I will write an elaborate post later on.
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.
Why is this problem not solved by upstream package authors providing template security policies?
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'd say that's a multi factor issue. I'll cite two at least that are related to one another,
This Node IDL proposal together with a following security-specific proposal aim at tackling both at the same time with a relatively simple solution (for the pkg developer).
Decoupling the Node IDL from security seemed like an opportunity to enable other possibilities and tools.
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.
Certainly.
I've posted a link to this PR in the ROS Driver Common Interface Standards discussion on ROS Discourse.
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 absolutely agree, but I think this needs to be done properly to ensure the full potential is realisable. I think you are currently too focused on the package concept and I'm not sure that is correct. I could be convinced otherwise, of course.
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 better if each of these use cases were written up in detail, at least as detailed as the security one. Otherwise you might miss details relevant to making the node IDL correct for those use cases.
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.
Our primary concern is security, that's how this proposal born. These other use cases are only speculation at this point. We could develop them a little but likely not as much as the sec one.
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 sure your primary concern is security, but if you are promoting this as something for use cases outside of security then I feel that those use cases should also be properly described and taken into account. Perhaps gather some feedback specifically on those use cases at Discourse?
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.
As I mentioned in another comment, I believe I have a use case for this in static analysis. I am still reading all of this for the first time though. I will write a more detailed comment once I go through all of 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.
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 those concerns belong to a design doc...
We therefore keep them as
suggestion
here until we figure that out.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 think it's worth adding potential issues and how we plan/could address them.
I'm not convinced that adoption is a challenge. I'd argue a node IDL is still useful to individuals, or organizations using it internally.
Regarding sync issues, we could at the very least have a tool that launches each node and does a best-effort sanity check against the IDL (maybe this is what you are describing, I couldn't tell). It could report any discrepancies between the declared and actual interfaces. I bet this check could be added as a unit test in a lot of cases. Beyond that, I think we just have to rely on authors and community reports of inconsistencies.
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.
Ok. I'll keep this as suggestion until updated with plan to address them.
The adoption challenge mentioned here is maybe a little too security (future plan/tools) centric and its tone could be voiced down a little.
As for your suggestion on the sync issue it will be added as potential plan to address 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.
Have a verb under
ros2 nodl
such asverify
. Make it really quick and people could potentially run it regularly at runtime, e.g. once a minute, as a sanity check.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.
Alright, updated suggestion with what was discussed here and pushed 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.
Why does it have to be tied to the package? Or the node, for that matter? I think it could be useful to distribute node interface descriptions separately and have several different nodes from different packages that support the same interface.
In computer science, this is one of the primary reasons for using interfaces: it allows substituteability. Particularly if you are going after static analysis tools and system constrution tools, such capability would be close to essential.
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.
This is a nice idea. Then there's the question: how to communicate that your node implements a particular NoDL?
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 could have common packages that just contain NoDL files, e.g.
sensor_nodls
,nav_nodls
, etc.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 how one would tied node and NoDL back together if they are distributed separately. Also I believe that people are somewhat familiar with the plugin export scheme, so a look alike scheme would ease adoption.
Finally separating NoDL files in a separate pkg(s) just increase the chances that the files go out-of-sync.
I do see and appreciate @gbiggs' point on having some kind of template NoDL files tho.
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.
@artivis I think you are looking at this too much from the point of view of describing existing nodes, and not enough from the point of view of describing types. Describing types is ultimately what you are doing, and I think the correct approach is to find a way to state that a node implements a particular NoDL interface rather than saying that every node is a completely unique, unsubstitutable type that needs its own NoDL file.
In some ways this is a philosophical question about where the substitution level is in ROS. There are probably those who say it is at the level of topic, and those who say it is at the level of node. There are definite benefits to doing the later, but it does require more careful engineering and/or tools to support and ROS users may not have an appetite for that. I would argue, though, that if they are at the level of caring about proper security they are probably at the level of caring about good engineering practices and so wouldn't mind some extra complexity if it gives useful benefits, like powerful static analysis tools.
However I think the most important is that the reality is that people treat many nodes in this way already, and the state publisher node discussion on Discourse is a currently-being-discussed example of 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.
In this context, I think that describing nodes versus types is a distinction without a difference. In the end, node A has interface X, regardless of where X comes from. As @ruffsl points out in another thread, we can xinclude files from other packages if we want to declare X in one place and have multiple packages/nodes use it. I don't see a solid use-case for designing much more than that at this 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.
I'm not asking for designing too much, but I think that we need to make sure what we design now does not close off future potential. Having a way to define node interfaces, parameters a node can take, etc. outside of the source code opens up huge possibilities for better engineering practices for ROS users, but it needs careful thought for what exactly is defined and how to ensure that we don't make it impossible to do, for example, easy substitution of equivalent nodes, or nodes with optional extensions to a defined interface, or verifying the compatibility of two nodes at launch time or in some pre-launch system check.
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 might also be worth mentioning that there could be one or more instances of this tag.
I could imagine a package author wanting to describe their nodes in separate files.
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.
Can you elaborate on this? I tend to lean toward the simplest system that will work instead of immediately trying to enable everything a developer would possibly want to do. Having multiple interface files is a complexity that I'm not sure is currently warranted, particularly given the ability to xinclude. We can of course add it, I just want to learn more about what's in your head 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.
I was thinking that we could define nodes interfaces in one or more files. This comment thread is more relevant: #266 (comment).
In that case, we might expect there to be one or more occurrences of this tag in the package.xml.
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 was not aware that ROS 2 had such a tight coupling between packages and nodes.
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.
Why should multiple independent nodes be described in the same file? I would think a one node-per-file mapping would be much more natural since they can evolve independently.
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 it's more natural to have one node per file. I think both option should be possible.
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 our mind this mechanism is very similar to that of plugins and therefore both options are possible. I'll make this explicit in the text.
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 think that allowing multiple nodes in a file is something you would come to regret.
I also think that if you are allowing mutliple nodes in a file, then you are describing a Package IDL, not a Node IDL. Although this comment applies generally to the tone throughout this 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.
@gbiggs Do you have an example to illustrate this? As long as one can parse the info on a per-node basis I don't see any problem.
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.
With XML, one could always xinclude from external paths, such that you could break your "package" IDL into separate node "IDL" files, if one wanted to.
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.
ROS 2 introduced read only parameters. Should the parameters attributes includes 'ro/rw' ?
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.
IMO, it's okay to not include the read only status. We can already query this constraint on parameters (and other constraints) at runtime. Unless there's a strong case for static analysis, I feel like describing it in the node IDL just adds another point where information can go out-of-sync.
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 might be good if the parameter description could be separated. I could see it being read independently by a node which doesn't have a "Node IDL" but wants to read the declared parameters from a file. That would imply that the parameter would need several additional arguments though (all which are possible to pass using the parameter declaration API: ranges, read-only, ...).
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.
@jacobperron Not that one of the point of this proposal is to access the API info in a 'pre-runtime' fashion - e.g. at launch-parsing time. So querying for ro/rw and other constraints at runtime is kinda too late already.
@dirk-thomas I agree that it might be interesting, but it does feel a little out of scope here. Do you mean something a la ROS 1 rosparam_handler?
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.
Since the QoS settings are deciding of two endpoints on the same topic name are being matched wouldn't it make sense to also specify those? Otherwise any "connectivity analysis" on these metadata might be incorrect.
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.
Fair point, going to update 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.
I added the QoS settings for actions, services and topics (da63aa6). I'm not familiar with the later two so please let me know your take on this.
I also went ahead assuming we would like those to defaults to the same values they do in code, hopefully I'm correctly linking to the defaults.
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.
Forgot to mention, I don't know what type should the
qos
attribute be. I'm not sure of to translate it to a xml tag attribute here...If anyone has a suggestion?
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.
Given that they are objects in code, it may not be suited to being an attribute at all, but an object nested under the respective tags.
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.
You mean something similar to
<rosparam>
inroslaunch
?E.g.
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.
That's the right idea, yeah, although I'd make those QoS attributes, maybe something like:
I think this makes sense. Given that the rationale for the defaults is "keep behavior like ROS 1" I doubt they will change and get out of sync.
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.
Updated
qos
so that it is a nested object inaction
,service
andtopic
.Updated the example to highlight
qos
. The example is not fully finished.