Minimal service and client #138

codebot · 2016-11-23T01:56:34Z

builds on directory reorganization of #137

adds minimal examples of service and client packages

mikaelarguedas · 2016-11-28T20:31:00Z

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

codebot · 2016-11-29T22:58:15Z

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

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

wjwwood · 2016-12-08T01:45:56Z

rclcpp/minimal_subscriber/README.md

+ * `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


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.

wjwwood · 2016-12-08T01:46:52Z

rclcpp/minimal_subscriber/README.md

+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


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

wjwwood · 2016-12-09T02:33:21Z

rclcpp/minimal_service/not_composable.cpp

+#include "rclcpp/rclcpp.hpp"
+#include "examples_rclcpp_minimal_service/srv/add_two_ints.hpp"
+
+using Service = examples_rclcpp_minimal_service::srv::AddTwoInts;


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.

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.

wjwwood · 2016-12-09T02:34:54Z

rclcpp/minimal_client/not_composable.cpp

+#include "examples_rclcpp_minimal_service/srv/add_two_ints.hpp"
+
+using Service = examples_rclcpp_minimal_service::srv::AddTwoInts;
+using Request = Service::Request;


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.

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

wjwwood · 2016-12-09T02:35:21Z

rclcpp/minimal_client/not_composable.cpp

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


This looks like a copy-paste error, right?

codebot · 2016-12-09T22:08:32Z

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

wjwwood · 2016-12-09T22:55:50Z

lgtm, thanks!

dirk-thomas · 2016-12-10T00:52:18Z

@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 · 2016-12-10T00:54:16Z

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 · 2016-12-10T00:56:09Z

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 · 2016-12-10T01:00:18Z

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"

…tform

codebot · 2016-12-10T01:09:03Z

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

…tform (#148)

* 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

…-platform (ros2#148)

codebot added 3 commits November 22, 2016 14:49

add examples_ prefix to package names to avoid future collisions

402932f

change talker/listener to minimal_publisher/minimal_subscriber

4fc7dd4

minimal example of service and client packages

6a13440

codebot added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 23, 2016

codebot self-assigned this Nov 23, 2016

mikaelarguedas mentioned this pull request Nov 28, 2016

Rclpy minimal services #140

Merged

style fixes and cleanup

3847ecd

wjwwood approved these changes Dec 9, 2016

View reviewed changes

improve names and use example_interfaces service

0d771c9

codebot merged commit 60ce5f6 into master Dec 9, 2016

codebot deleted the minimal_service_and_client branch December 9, 2016 22:57

codebot removed the in review Waiting for review (Kanban column) label Dec 9, 2016

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

fix #138 which caused problems on windows because it wasn't cross-pla…

6d91bea

…tform

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

fix #138 which caused problems on windows because it wasn't cross-pla…

d636e89

…tform (#148)

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

fix ros2#138 which caused problems on windows because it wasn't cross…

5ab5834

…-platform (ros2#148)

wjwwood mentioned this pull request Dec 17, 2016

use c++14 not c++11 in new packages #157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal service and client #138

Minimal service and client #138

codebot commented Nov 23, 2016

mikaelarguedas commented Nov 28, 2016

codebot commented Nov 29, 2016

wjwwood left a comment

wjwwood Dec 8, 2016

wjwwood Dec 8, 2016

wjwwood Dec 9, 2016

codebot Dec 9, 2016

wjwwood Dec 9, 2016

codebot Dec 9, 2016

wjwwood Dec 9, 2016

codebot Dec 9, 2016

codebot commented Dec 9, 2016

wjwwood commented Dec 9, 2016

dirk-thomas commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016

Minimal service and client #138

Minimal service and client #138

Conversation

codebot commented Nov 23, 2016

mikaelarguedas commented Nov 28, 2016

codebot commented Nov 29, 2016

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

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

codebot commented Dec 9, 2016

wjwwood commented Dec 9, 2016

dirk-thomas commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016

codebot commented Dec 10, 2016