Fix up errors in doxygen documentation. #311

clalancette · 2021-08-10T19:43:15Z

While running the rosdoc2 tool against this repository, I
came across these documentation errors. This PR fixes them
all up so that doxygen runs clean. We still can't successfully
run rosdoc2 on this package due to one other error, but at
least this will get us ready once we figure that issue out.

Signed-off-by: Chris Lalancette [email protected]

While running the rosdoc2 tool against this repository, I came across these documentation errors. This PR fixes them all up so that doxygen runs clean. We still can't successfully run rosdoc2 on this package due to one other error, but at least this will get us ready once we figure that issue out. Signed-off-by: Chris Lalancette <[email protected]>

aprotyas

Looks good overall. I couldn't find \param[inout] in the Doxygen commands manual, so I suggested the batch of changes.

rmw/include/rmw/event.h

rmw/include/rmw/security_options.h

wjwwood

I don't feel strongly about [inout] vs [in,out], but I think we use [inout] elsewhere like in rclcpp.

wjwwood · 2021-08-11T16:59:46Z

rmw/include/rmw/types.h

-   *
-   * The definition of local is somewhat vague at the moment.
-   * Right now it means local to the node, and that definition works best, but
-   * may become more complicated when/if participants map to a context instead.


Is this correct to remove?

The original thing that caused me to look at this was the \TODO, which apparently doxygen doesn't understand as a tag.

That said, I believe that the situation now is different from what this comment says. We definitely did all of the one-participant-per-context stuff back in Foxy (and finished it in Galactic), so the comment is outdated. I didn't know what to replace it with, so I figured nothing was better than the wrong thing.

That said, if you have additional context, I'm happy to update this to something else. Maybe @ivanpauno can weigh in as well.

Oh maybe it just needs to be \todo, because that's definitely a thing:

https://www.doxygen.nl/manual/commands.html#cmdtodo

I agree it's out-of-date and needs to be updated, but just dropping the whole section without putting something new isn't right in my opinion. I could see dropping the the text but leaving the TODO...

clalancette · 2021-08-11T19:11:14Z

I don't feel strongly about [inout] vs [in,out], but I think we use [inout] elsewhere like in rclcpp.

Yeah, that's true. I was following that pattern here, though @aprotyas is correct about Doxygen suggesting [in,out]. Is it better to be consistent with our style elsewhere, or correct in terms of the Doxygen documentation? I'm not sure. My inclination is to be consistent, and leave this as [inout]. Maybe in the future we can do a pass where we update all of them (but I also don't feel strongly about this point).

christophebedard · 2021-08-11T19:19:24Z

rmw/include/rmw/rmw.h

@@ -2041,8 +2041,8 @@ rmw_send_request(
 *   It will also be left unchanged if this function succeeds but `taken` is false.
 *
 * \param[in] client Service client to take response from.
- * \param[out] response_header Service response header to write to.
- * \param[out] ros_request Type erased ROS service response to write to.
+ * \param[out] request_header Service response header to write to.


I think this should be response_header (so the name in the signature needs to be updated). See the doc above and next to it.

So I looked into this a bit. I think I agree with you on principle. However, all implementors (rmw_cyclonedds, rmw_fastrtps_cpp, rmw_connextdds) call this request_header, and all callers (rcl) also call it request_header. That may all be down to the fact that this API named it that, but I'd rather not confuse the situation and change it here.

The request_header is the one passed originally when the request was made, so it's indicating which request this response that you're taking goes with. So I think the name is right.

The request_header is the one passed originally when the request was made, so it's indicating which request this response that you're taking goes with.

I don't think that's right unfortunately. It tripped me up at first when implementing rmw_take_response recently; I thought I could use the header's request ID/sequence number to get the corresponding response, but that's not it. By looking at the implementations, this header parameter is strictly an out param, so rmw_take_response takes any response and not a specific response for a specific request. That response is then matched with the original request elsewhere (like rclcpp) using the request ID from the header.

The documentation here and in rcl is kind of confusing/contradictory, which I why I commented when I saw this. request_header should be changed to response_header (because it's information about the response we are taking, and not the original request), but as @clalancette said, it's called request_header pretty much everywhere.

Sorry, I didn't mean to say the request_header is an input for this take function, but that it is an output, as you said.

I see why you're confused, but I don't think that request_header is wrong, because it really contains information about the originating request. This information is about the request, as in which request is associated with this response and as you said it is used later to associate the response with an original request by either the user or rclcpp.

That being said, it does contain information about the response specifically, like the source and destination timestamps. So maybe the name should be response_header or response_info (I think info is a better term, to match rcl_take_with_info for example, but that's not really important).

And rmw_service_info_t could more likely be named rmw_service_response_info_t, but for all of this the question is if changing the names is worth the churn. I'd lean towards yes (clearer is always better, also helps us feel proud in knowing what we make/maintain makes sense), but also it could be disruptive and work.

Yeah, renaming would make all of this confusion go away. If you think it's worth it, then I can probably do it at some point in the future. Otherwise, I'll just stop here 😝

Changing the parameter name is not a big deal, but changing the type name for the struct is a larger change, though a valuable one I think.

I'm fine with changing the variable name, but we should do that in another pr, since this one is about fixing the documentation for what's already there. @clalancette thoughts?

Yes, I'm fine with changing the variable name (this will have no material API/ABI impact, but will make things clearer).

If we do that, I think we should change the upstream and downstreams of this as well, so everything is consistent.

But all of that should be in a separate set of PRs.

I opened an issue to track this: #312

wjwwood · 2021-08-11T19:32:39Z

Yeah, I don't mind about the inout thing. I would say just leave it inout for consistency.

Signed-off-by: Chris Lalancette <[email protected]>

wjwwood

lgtm

wjwwood · 2021-08-17T01:47:33Z

rmw/include/rmw/rmw.h

@@ -2041,8 +2041,8 @@ rmw_send_request(
 *   It will also be left unchanged if this function succeeds but `taken` is false.
 *
 * \param[in] client Service client to take response from.
- * \param[out] response_header Service response header to write to.
- * \param[out] ros_request Type erased ROS service response to write to.
+ * \param[out] request_header Service response header to write to.


The request_header is the one passed originally when the request was made, so it's indicating which request this response that you're taking goes with. So I think the name is right.

clalancette · 2021-08-19T14:22:05Z

I believe the conversation in #311 (comment) is wrapped up (with further work to come, but in a different PR).

With that said, this now has approvals. It also has green PR job. I'm going to skip CI since this is a documentation change only. Thanks for the reviews!

ros-discourse · 2021-08-23T23:34:35Z

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-2021-08-19/22008/1

ros-discourse · 2021-09-20T20:52:43Z

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-2021-9-16/22372/1

clalancette requested review from wjwwood and aprotyas August 10, 2021 19:43

aprotyas reviewed Aug 11, 2021

View reviewed changes

wjwwood reviewed Aug 11, 2021

View reviewed changes

christophebedard reviewed Aug 11, 2021

View reviewed changes

ivanpauno approved these changes Aug 12, 2021

View reviewed changes

Fixes from review.

9dc61ba

Signed-off-by: Chris Lalancette <[email protected]>

wjwwood approved these changes Aug 17, 2021

View reviewed changes

clalancette merged commit 936a667 into master Aug 19, 2021

delete-merged-branch bot deleted the clalancette/update-doxygen branch August 19, 2021 14:22

christophebedard mentioned this pull request Aug 19, 2021

Change rmw_take_response() request_header param/variable name #312

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix up errors in doxygen documentation. #311

Fix up errors in doxygen documentation. #311

clalancette commented Aug 10, 2021

aprotyas left a comment

wjwwood left a comment

wjwwood Aug 11, 2021

clalancette Aug 11, 2021

wjwwood Aug 11, 2021

clalancette commented Aug 11, 2021

christophebedard Aug 11, 2021 •

edited

Loading

clalancette Aug 13, 2021

wjwwood Aug 17, 2021

christophebedard Aug 17, 2021 •

edited

Loading

wjwwood Aug 17, 2021

wjwwood Aug 17, 2021

christophebedard Aug 18, 2021

wjwwood Aug 19, 2021

clalancette Aug 19, 2021

christophebedard Aug 19, 2021

wjwwood commented Aug 11, 2021

wjwwood left a comment

wjwwood Aug 17, 2021

clalancette commented Aug 19, 2021

ros-discourse commented Aug 23, 2021

ros-discourse commented Sep 20, 2021

Fix up errors in doxygen documentation. #311

Fix up errors in doxygen documentation. #311

Conversation

clalancette commented Aug 10, 2021

aprotyas left a comment

Choose a reason for hiding this comment

wjwwood left a 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 commented Aug 11, 2021

christophebedard Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophebedard Aug 17, 2021 • 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

wjwwood commented Aug 11, 2021

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Aug 19, 2021

ros-discourse commented Aug 23, 2021

ros-discourse commented Sep 20, 2021

christophebedard Aug 11, 2021 •

edited

Loading

christophebedard Aug 17, 2021 •

edited

Loading