Skip to content
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

Rewrite of old rosidl tutorial #614

Merged
merged 9 commits into from
May 13, 2020
Merged

Rewrite of old rosidl tutorial #614

merged 9 commits into from
May 13, 2020

Conversation

maryaB-osr
Copy link
Contributor

The old rosidl tutorial had some concepts in it that I didn't cover in my new "simple" interface tutorial (because it's already too long) so instead of deleting the old one, I rewrote it and removed all the repeated information between the two.

The code works now but I'm not sure if it's "right".

This bit of code in the original:

get_default_rmw_implementation(rmw_implementation)
find_package("${rmw_implementation}" REQUIRED)
get_rmw_typesupport(typesupport_impls "${rmw_implementation}" LANGUAGE "cpp")

foreach(typesupport_impl ${typesupport_impls})
  rosidl_target_interfaces(publish_address_book
    ${PROJECT_NAME} ${typesupport_impl}
  )
endforeach()

works, but I replaced it with this because it's much shorter:

  rosidl_target_interfaces(publish_address_book
    ${PROJECT_NAME} "rosidl_typesupport_cpp")

Is there a different situation for each of these? If that longer block is meant for a specific purpose, I'd like to include that in the tutorial.

fixes #565

@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 April 16, 2020 19:06 Inactive
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 April 16, 2020 19:08 Inactive
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM but for a few details.

source/Tutorials/Rosidl-Tutorial.rst Outdated Show resolved Hide resolved
source/Tutorials/Rosidl-Tutorial.rst Outdated Show resolved Hide resolved
source/Tutorials/Rosidl-Tutorial.rst Outdated Show resolved Hide resolved
source/Tutorials/Rosidl-Tutorial.rst Outdated Show resolved Hide resolved
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 April 20, 2020 15:51 Inactive
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 May 5, 2020 20:47 Inactive
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 May 5, 2020 20:53 Inactive
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 May 6, 2020 19:17 Inactive
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like what you've done here. I think this is a good article to have.

One thing that I think we should change is the name of the article. "More-Interfaces" isn't very descriptive. I'd go with something like "Single-Package-Define-and-Use-Interface", but anything along those lines is OK with me. I've got a few other small things inline, though I didn't do a full review this time. Once things are renamed I can do another look over.

source/Tutorials.rst Outdated Show resolved Hide resolved
source/Tutorials/More-interfaces.rst Outdated Show resolved Hide resolved
source/Tutorials/More-interfaces.rst Outdated Show resolved Hide resolved
Signed-off-by: maryaB-osr <[email protected]>
@mjcarroll mjcarroll temporarily deployed to ros2-documentation-pr-614 May 12, 2020 14:50 Inactive
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the direction this one is going in. I've done a more thorough review now and have left a few more comments for you to think about.

source/Tutorials/Custom-ROS2-Interfaces.rst Outdated Show resolved Hide resolved
.. note::

You can use an existing interface definition in a new interface definition.
For example, the `rosidl_tutorials_msgs package <https://github.com/ros2/tutorials/blob/rosidl_tutorials/rosidl_tutorials/rosidl_tutorials_msgs/msg/Contact.msg>`_ on GitHub has a msg named ``Contact.msg``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, didn't we decide not to point to the tutorials repository as it doesn't really work right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it looks bad :/ but that's the msg we're using for the following example to show that you can use existing interfaces to define new ones.

Aside from having them create Contact.msg themselves in another package (meaning they'll have to go create another package just for that), or rewriting the example to use an existing msg from some up-to-date package in the ecosystem (which would also make the tutorial longer because a completely new publisher would have to be written to accommodate a different msg), I don't see a way around linking to Contact.

I rewrote the sentence to take the focus off the rosidl_tutorials package at least:

For example, let's use Contact.msg <https://github.com/ros2/tutorials/blob/rosidl_tutorials/rosidl_tutorials/rosidl_tutorials_msgs/msg/Contact.msg>_, which belongs to an existing ROS 2 package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I don't love it, but since that is the point of that section, so be it.

Signed-off-by: maryaB-osr <[email protected]>
@maryaB-osr maryaB-osr merged commit b7c6a48 into master May 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the maryaB/rosidl-redo branch May 13, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove old IDL tutorial in favor of new
4 participants