[hardware_interface::RobotHW] Extend documentation of read and write methods

[fjp]

This PR is related to ros-controls/ros_controllers/#474 and updates/extends the documentation for read and write methods.

The PR adds a \note to the documentation regarding the directional perspective of these two methods.

I've also added a named member group "Control Loop". Previously, read and write were grouped under "Hardware Interface Switching", which is not correct I guess.

[fjp]

Using rosdoc_lite locally seems to work except that I don't know why "Hardware Interface Switching" is grouped differently than "Control Loop" and "Resource Management":

[fjp]

Thanks a lot for this thorough review. I'll soon take care of your suggestions.

[fjp]

IMO the \brief on prepareSwitch and doSwitch doesn't add anything, and is too long to be a brief. I think it's fine without these changes, but if you want to give it a brief, then a shorter, less detailed summary might be better than just pulling the first sentence. Something like Check (in non-realtime) if the given controllers can be started and stopped., and then leave the rest of the body as it was before to add the necessary details.

I added some suggestions below, everything I wrote for read() can be translated to write() as well. The suggestions probably violate the line length limits, please ignore my bad formatting :)

Overall I think it's great but the language can be simplified a lot. In particular, I'd like to stay away from getting too close to implementation details, since we shouldn't try to force a particular style of implementation. I'm also not a fan of "raw data", so in my suggestions I used "robot state" and "command" instead.

Thanks again for the review and your suggestions @matthew-reynolds. I've switched back the \brief onf both prepareSwitch and doSwitch and changed a lot on the other docstrings as well (not only on read and write).

[fjp]

I have applied most of the suggestions so far but didn't put the example from the wiki page in the class description because I think it would overload the hardware_interface::RobotHW class. Instead, I've added a slightly modified version of it in a newly created README.md which is used as doxygen's mainpage and can also be used as a readme for the hardware_interface folder of the github repository itself. However, if the markdown formatting doesn't work it is also possible to create a new mainpage.dox, move the content of README.md there and just add a link to the mainpage.dox in the README.md and the delete the rest. One thing that is missing now is the \htmlinclude manifest.html which is usually found in a mainpage.dox of a ROS package. Note sure if this can be added inside the README.md. Please let me know what you think, any suggestions are welcome.

[matthew-reynolds]

Since PR this is growing larger, I just want to ping @ros-controls/ros_control for more opinions.

@fjp I will need a few days to read your newest changes :sweat_smile:

[matthew-reynolds]

@fjp Sorry to go back on you here, but I think this PR is losing focus a bit. Do you mind leaving the markdown group fixes, \briefs, and the read() and write() descriptions, but splitting the rest of this out into a new PR? I think those other changes need more discussion, particularly the inclusion of the big README.

The few changes I listed above are basically ready to be merged and address specific issues rather than general improvement of the documentation.

[fjp]

@matthew-reynolds no worries, splitting this up is a good idea. I'll leave the changes you mention and move the large changes of the documentation (README) to a new PR.

[fjp]

@matthew-reynolds I've reverted the large changes (remove README, revert rosdoc.yaml, remove details docstrings of RobotHW class and init method) of this PR in a new branch extend-read-write-docstrings in my fork. In this branch I've also squashed most of the commits as the commit messages were similar. Should I use this new branch for a new PR?

Or do you prefer using this PR with one additional commit to melodic-devel branch that removes the README.md, reverts rosdoc.yaml and removes the added details of the RobotHW class description and init method?

[matthew-reynolds]

I don't think it's a big deal either way. On one hand, if we stay here, we preserve the discussion. But on the other hand, it's getting hard to navigate this PR since there's so much discussion and back-and-forth. Plus we can (And should) link to this PR in the new one if we go that route.

Not sure if the higher-ups have an opinion one way or the other, but I'd say probably doesn't matter, up to you.

[fjp]

Thank for the feedback @matthew-reynolds. As this PR has quite a lot of comments and is rather confusing I'd like to make a new PR on a new branch (extend-read-write-docstrings). Also I think it is cleaner if I start to work on branches and not on melodic-devel, even if it's a fork.

Therefore, I created a new PR #444 which mainly takes care of the read and write method docstrings and the doxygen group names. For the rest of the changes in this PR (extend documentation of hardware_interface) I'll come up with a new PR once #444 get's sorted out. I hope this is ok?

[bmagyar]

I have merged #444 and also picked it for Noetic. I'd recommend opening a new PR with 100-200 lines of unchallenged changes from this PR so we can guarantee a somewhat smooth review process.

[fjp]

Great, thanks @bmagyar. I'll do my best to keep the changes in the upcoming PR small.

[fjp]

This PR should become obsolete once #457 is merged.

[bmagyar]

Closing on favour of #457 , thanks guys for the thorough discussion here