Update graph API documentation. #272

hidmic · 2020-09-11T22:33:19Z

Precisely what the title says. First pass.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-09-11T22:35:07Z

rmw/include/rmw/get_node_info_and_types.h

+ *   the effects of starvation due to OS scheduling).
+ *
+ * \par Thread-safety
+ *   Nodes are thread-safe objects, and so are all operations on them except for finalization.


We need to come up with a way to document object thread-safety and not just that of functions. This is way more duplication I'm comfortable with.

We need to come up with a way to document object thread-safety and not just that of functions.

This is something of a philosophical debate, honestly. In some sense, there is no such thing as a thread-safe object; thread safety only comes into play when accessing data, and that can only be done through functions or methods.

I think your main concern here is the duplication of this paragraph down below. It seems to me that that can mostly be resolved by pointing the reader to the documentation about the node API, which presumably talks about that.

This is something of a philosophical debate, honestly. In some sense, there is no such thing as a thread-safe object; thread safety only comes into play when accessing data, and that can only be done through functions or methods.

Sure, but what is an object if not state (data) coupled with behavior (functions)? At any rate, I think ros2/ros2#1004 would benefit from your POV.

It seems to me that that can mostly be resolved by pointing the reader to the documentation about the node API, which presumably talks about that.

Do we have node API documentation? Or are you talking about something that we should have at some point?

We can have higher level sections in doxygen (using the file for example), or we can document the struct, like I do with context:

https://github.com/ros2/rcl/blob/8bcada7b884693644d7ba2e3bd2c22fd9541eba7/rcl/include/rcl/context.h#L43-L143

Any objections to proceed without relocating documentation? For the sake of speed, and to keep it consistent with previous API spec updates that do things this way.

Fine with me.

hidmic · 2020-09-11T22:36:08Z

rmw/include/rmw/get_node_info_and_types.h

+ * <i>[1] rmw implementation defined, check the implementation documentation</i>
+ *
+ * \par Runtime behavior
+ *   To query the ROS graph is a synchronous operation.


We need to come up with a way to document whole features and abstractions/concepts like the ROS graph.

Agreed. Two places I can think to put it:

In a doc subdirectory at the top-level of this repository. That's nice in that it keeps it close to where it is "defined", but it also buries the concept down in an essentially "detail" package.

At index.ros.org or docs.ros2.org .

I'd be inclined towards having it here, in this repository, close to the API specifications that rely on it. External web sites can then consume that content.

I think here is fine, but it depends on the purpose. If it is to make this API clear, then here, but it's goal is to explain the concept to users, then higher up the stack is more appropriate, but some details will not, like the thread-safety of this API, as an example. Because higher level APIs may give different thread-safety guarantees and the users are more likely to use those interfaces.

If it is to make this API clear, then here, but it's goal is to explain the concept to users, then higher up the stack is more appropriate

Hmm, higher up the stack where? IMHO we cannot use terms and concepts in API documentation without an explanation or a link to an explanation. Besides, isn't the ROS middleware interface the best place to introduce ROS middleware concepts (barring tutorials and conceptual overviews that we can have elsewhere for educational purposes)?

Yes, I was just saying that tutorials and conceptual guides (things actual users will need) shouldn't depend on this except obliquely.

rmw/include/rmw/get_topic_names_and_types.h

rmw/include/rmw/rmw.h

hidmic · 2020-09-11T22:46:11Z

rmw/include/rmw/topic_endpoint_info.h

@@ -24,8 +24,9 @@ extern "C"
 #include "rmw/types.h"
 #include "rmw/visibility_control.h"



These APIs expect the same allocator to be passed around to not hit UB, presumably to spare storage. I wonder if these shouldn't be explicitly tagged for internal implementation use only.

Also, documentation duplication makes me cry.

They really should store the allocator given during init and reuse it during fini rather than having the user pass it...

That is to say, I agree with you.

Signed-off-by: Michel Hidalgo <[email protected]>

rmw/include/rmw/topic_endpoint_info_array.h

clalancette

I've got a bunch of comments in here.

I think there are 2 major things that this has pointed out.

The first is that there is a lot of duplication in the comments. Partly this is because the API could be improved. One example is rmw_get_node_names() and rmw_get_node_names_with_enclaves(). In my opinion, there is no reason to have both of those APIs. Instead we could have rmw_get_node_names take an enclave argument, and don't attempt to fill in enclave information if it is NULL. Partly the duplication is because we are missing some place to point to that has higher-level documentation about concepts.

The second major point is that we need a place to put higher-level documentation about concepts. My first inclination would be to start putting some conceptual documents at https://index.ros.org/doc/ros2/Tutorials/ or http://docs.ros2.org/foxy/ , but I'm not sure.

I honestly think neither is blocking; I would go with duplication for now, and we can review both of them later. But if we do that, then we should open up issues and/or add roadmap items to do so.

clalancette · 2020-09-14T12:27:48Z

rmw/include/rmw/get_node_info_and_types.h

+ * <i>[1] rmw implementation defined, check the implementation documentation</i>
+ *
+ * \par Runtime behavior
+ *   To query the ROS graph is a synchronous operation.


Agreed. Two places I can think to put it:

In a doc subdirectory at the top-level of this repository. That's nice in that it keeps it close to where it is "defined", but it also buries the concept down in an essentially "detail" package.

At index.ros.org or docs.ros2.org .

clalancette · 2020-09-14T12:35:40Z

rmw/include/rmw/get_node_info_and_types.h

+ *   the effects of starvation due to OS scheduling).
+ *
+ * \par Thread-safety
+ *   Nodes are thread-safe objects, and so are all operations on them except for finalization.


We need to come up with a way to document object thread-safety and not just that of functions.

This is something of a philosophical debate, honestly. In some sense, there is no such thing as a thread-safe object; thread safety only comes into play when accessing data, and that can only be done through functions or methods.

I think your main concern here is the duplication of this paragraph down below. It seems to me that that can mostly be resolved by pointing the reader to the documentation about the node API, which presumably talks about that.

rmw/include/rmw/get_node_info_and_types.h

rmw/include/rmw/rmw.h

rmw/include/rmw/topic_endpoint_info.h

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-09-14T18:16:18Z

Comments addressed, PTAL!

gbiggs · 2020-09-17T07:59:16Z

@clalancette Regarding writing a higher level concepts document, "write a tutorial on how to write an RMW implementation" is one of the outputs we planned for the Zenoh work. I'm happy to help out with writing the higher-level concepts document, provided someone with more experience with the API is willing to review it in detail.

hidmic · 2020-09-18T16:42:16Z

FYI I'll start writing tests and adapting implementation to comply with this. We can sort out where to put documentation later on.

wjwwood

lgtm, it's a great improvement!

I commented on most of the conversations, but you guys picked out everything I noticed already, so no extra stuff from me I think.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-09-24T18:25:26Z

Alright, going in!

Signed-off-by: Michel Hidalgo <[email protected]>

Update graph API documentation.

0034946

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic requested review from wjwwood, clalancette and ivanpauno September 11, 2020 22:33

hidmic commented Sep 11, 2020

View reviewed changes

rmw/include/rmw/get_topic_names_and_types.h Outdated Show resolved Hide resolved

hidmic commented Sep 11, 2020

View reviewed changes

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved

hidmic commented Sep 11, 2020

View reviewed changes

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved

hidmic commented Sep 11, 2020

View reviewed changes

Put curly bracket back in place.

97de605

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic commented Sep 11, 2020

View reviewed changes

rmw/include/rmw/topic_endpoint_info_array.h Show resolved Hide resolved

clalancette reviewed Sep 14, 2020

View reviewed changes

Address peer review comments.

cbabbac

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic requested a review from clalancette September 14, 2020 18:16

This was referenced Sep 18, 2020

Update graph API return codes. ros2/rmw_cyclonedds#243

Merged

Update graph API return codes. ros2/rmw_connext#459

Merged

Update graph API return codes. ros2/rmw_fastrtps#436

Merged

Bump graph API test coverage. ros2/rmw_implementation#132

Merged

wjwwood reviewed Sep 22, 2020

View reviewed changes

hidmic mentioned this pull request Sep 23, 2020

Update rmw_server_is_available() API documentation. #277

Merged

Address more peer review comments.

c92a73d

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic mentioned this pull request Sep 23, 2020

Push notification on publisher/subscription match #264

Open

hidmic requested a review from wjwwood September 23, 2020 20:32

clalancette approved these changes Sep 23, 2020

View reviewed changes

wjwwood approved these changes Sep 23, 2020

View reviewed changes

hidmic merged commit 7b82413 into master Sep 24, 2020

delete-merged-branch bot deleted the hidmic/update-graph-api-docs branch September 24, 2020 18:25

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update graph API documentation. (#272)

b03ef73

Signed-off-by: Michel Hidalgo <[email protected]>

ahcorde pushed a commit that referenced this pull request Oct 13, 2020

Update graph API documentation. (#272)

0c23600

Signed-off-by: Michel Hidalgo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update graph API documentation. #272

Update graph API documentation. #272

hidmic commented Sep 11, 2020

hidmic Sep 11, 2020

clalancette Sep 14, 2020

hidmic Sep 14, 2020

wjwwood Sep 22, 2020

hidmic Sep 23, 2020

wjwwood Sep 23, 2020

hidmic Sep 11, 2020 •

edited

Loading

clalancette Sep 14, 2020

hidmic Sep 14, 2020

wjwwood Sep 22, 2020

hidmic Sep 23, 2020

wjwwood Sep 23, 2020

hidmic Sep 11, 2020

hidmic Sep 11, 2020

wjwwood Sep 22, 2020

wjwwood Sep 22, 2020

clalancette left a comment

clalancette Sep 14, 2020

clalancette Sep 14, 2020

hidmic commented Sep 14, 2020

gbiggs commented Sep 17, 2020

hidmic commented Sep 18, 2020

wjwwood left a comment

hidmic commented Sep 24, 2020

		@@ -24,8 +24,9 @@ extern "C"
		#include "rmw/types.h"
		#include "rmw/visibility_control.h"

Update graph API documentation. #272

Update graph API documentation. #272

Conversation

hidmic commented Sep 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Sep 14, 2020

gbiggs commented Sep 17, 2020

hidmic commented Sep 18, 2020

wjwwood left a comment

Choose a reason for hiding this comment

hidmic commented Sep 24, 2020

hidmic Sep 11, 2020 •

edited

Loading