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

Minimal service and client #138

Merged
merged 5 commits into from Dec 9, 2016

Conversation

Projects
None yet
4 participants
@codebot
Copy link
Member

commented Nov 23, 2016

builds on directory reorganization of #137

adds minimal examples of service and client packages

@codebot codebot self-assigned this Nov 23, 2016

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

Do you plan on providing the same set of strategies as for the pub sub in these packages (lambda, member_functions etc)?

@codebot

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

Yeah I'll try to add a few variants if I can figure out the C++11 magic. And I need to bring this PR up to the same level as the pub/sub PR now (style fixes, etc.).

@wjwwood

wjwwood approved these changes Dec 9, 2016

Copy link
Member

left a comment

I had some stylistic comments, but the code looks correct to me. +1

* `not_composable.cpp` uses a global function callback without a Node subclass

Note that `not_composable.cpp` instantiates a `rclcpp::Node` _without_
subclassing it. This was the typical usage model in "classic" ROS, but

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

I've seen "classic" ROS used a lot, but it's always in combination with ROS 2, so I think it would be better if we try to be consistent and use ROS 1 and ROS 2 everywhere. In the distant future it may not be clear if "classic" ROS refers to ROS 1 or some older version of ROS 2.

Note that `not_composable.cpp` instantiates a `rclcpp::Node` _without_
subclassing it. This was the typical usage model in "classic" ROS, but
unfortunately this style of coding is not compatible with composing multiple
nodes into a single process. Thus, it is no longer the recommended style for

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

I'd recommend doing one sentence per line and not wrap the lines manually. Anything else causes diffs to be hard to read.

#include "rclcpp/rclcpp.hpp"
#include "examples_rclcpp_minimal_service/srv/add_two_ints.hpp"

using Service = examples_rclcpp_minimal_service::srv::AddTwoInts;

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

I'd recommend two things here:

  • use example_interfaces/srv/AddTwoInts
  • do using example_interfaces::srv::AddTwoInts; and replace Service with AddTwoInts throughout

I don't know if there was a reason for creating the service locally, but every message and/or service we have to build on the farm is lots of extra time. As for illustrating how to make your own service, perhaps we could just leave the machinery for that commented out or just refer to the example_interfaces package, it is quite simple already:

https://github.com/ros2/examples/blob/master/example_interfaces/CMakeLists.txt

I'm less passionate about replacing Service with AddTwoInts, but I think it does give more meaning to each place it is used (in the callback and at the creation of the service). It gets really important if the user is going to create two services in the same file.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

I created the service locally on purpose here, to show the CMake pattern for doing so (since it's somewhat non-trivial to ensure the service is generated first before Make tries to build the code which uses it), but it's a fair point that this is no longer "minimal." I'll take your suggestion now and use example_interfaces, and in the future, if anyone cares, we can create a package which both generates its own service and uses it.

#include "examples_rclcpp_minimal_service/srv/add_two_ints.hpp"

using Service = examples_rclcpp_minimal_service::srv::AddTwoInts;
using Request = Service::Request;

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

Same comment as in the other file, I get what you're trying to do here with shorter lines and simpler names, but I think AddTwoInts::Request is so much more informative than Request or Service::Request, which require me to look up the translation.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

Thanks for the suggestion; mostly I was just going for fewer characters, and I agree your abbreviation is much better than mine.

int main(int argc, char * argv[])
{
rclcpp::init(argc, argv);
auto node = rclcpp::node::Node::make_shared("minimal_subscriber");

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

This looks like a copy-paste error, right?

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

doh 😞

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2016

Thanks @wjwwood for the comments; I believe they are all addressed now.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

lgtm, thanks!

@codebot codebot merged commit 60ce5f6 into master Dec 9, 2016

@codebot codebot deleted the minimal_service_and_client branch Dec 9, 2016

@codebot codebot removed the in review label Dec 9, 2016

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Dec 10, 2016

@codebot It looks like this PR has introduced new warning on the farm (http://ci.ros2.org/job/ci_osx/1554/warnings10Result/ http://ci.ros2.org/job/ci_windows/2006/warnings41Result/). Please address them.

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

Yeah. We just had a spirited discussion here about that 😃 in which much learning occurred. I'm converting all the printf() calls to use the C99 standard macros like PRId64 to fix this in a portable way.

It's amazing how tasks like printing numbers have become so complex over time! 😆

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

In case anybody cares: the PRI in macros like PRId64 is documented here and is an abbreviation for printf(). The corresponding cross-platform defined-width macros for scanf() begin with SCN.

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

And another thing to note for posterity (which perhaps everybody else already knows?): in C++11 you are required to have a space after a string literal before a macro name like PRId64 because otherwise it's ambiguous if you're using a C99 macro or a C++11 user-defined string literal. 😆 So printing a 64-bit number in a cross platform way is no longer "number: %ld\n", in the modern cross-platform world it has become "number: %" PRId64 "\n"

codebot added a commit that referenced this pull request Dec 10, 2016

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

I just kicked off CI on all platforms for 6d91bea which will hopefully fix this for all platforms.

codebot added a commit that referenced this pull request Dec 10, 2016

haueck added a commit to haueck/examples that referenced this pull request Dec 14, 2016

Minimal service and client (ros2#138)
* add examples_ prefix to package names to avoid future collisions
* change talker/listener to minimal_publisher/minimal_subscriber
* minimal example of service and client packages
* style fixes and cleanup
* improve names and use example_interfaces service

haueck added a commit to haueck/examples that referenced this pull request Dec 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.