Rclpy minimal pub sub #139

mikaelarguedas · 2016-11-28T17:59:45Z

This PR adds several simple pub/sub examples.
This not sure if the "local function" and the "lambda" are necessary, maybe we should just pick one and discard the other.

codebot · 2016-11-28T18:16:50Z

rclpy/topics/minimal_publisher/README.md

+This package contains a few different strategies for creating short nodes that blast out messages.
+The `publisher_old_school` recipe creates a talker node very similar to how it would be done in ROS1 using rospy.
+The `publisher_lambda` and `publisher_local_function` allow to leverage the timers provided by ROS2 to trigger message publication.
+The `publisher_member_function` created a class MinimalPublisher that sends messages periodically


codebot · 2016-11-28T18:18:50Z

rclpy/topics/minimal_publisher/publisher_lambda.py

+        publisher_.publish(msg)
+        rclpy.spin_once(node)
+
+    node.destroy_timer(timer)


do these timers and nodes get destroyed automatically on shutdown, or is manual destruction necessary?

they do get destroyed automatically, I added the destruction to avoid that linters complain about unused variables. I'm unsure about the amount of comments that should be placed in these examples. But this one definitely deserves deserves one.

done in a55f5b1

codebot

my comments are very minor. these simple examples and setup.py examples are gr8

codebot · 2016-11-28T18:20:24Z

rclpy/topics/minimal_publisher/publisher_lambda.py

+        msg.data = 'Hello World: %d' % i
+        i += 1
+        print('Publishing: "%s"' % msg.data)
+        publisher_.publish(msg)


I'm probably just not understanding this correctly, but it seems like the timer with lambda will also be publishing, so the messages will be sent twice (one from the lambda, one from this publish() call ? And will this loop run at a particular rate, or will it run as quickly as possible?

_< indeed this publish shouldn't be here.
Regarding the loop, filling up the message etc cannot be done in the lambda because of how lambdas work in python (no variable assignment, has to be single line etc). I would be in favor of removing the publisher_lambda and the subscriber_local_function but I'm not a fan of not having symmetry in the minimalist publisher/subscriber examples

codebot · 2016-11-28T18:21:13Z

rclpy/topics/minimal_publisher/publisher_local_function.py

+    timer = node.create_timer(0.5, timer_callback)
+
+    while rclpy.ok():
+        rclpy.spin_once(node)


Is it recommended to have this while loop calling spin_once() or is it better to just have spin() (I don't know) 🐔

unfortunately there is no spin yet in python, only spin_once

codebot · 2016-11-28T18:23:59Z

rclpy/topics/minimal_publisher/publisher_member_function.py

+class MinimalPublisher:
+
+    def __init__(self):
+        self.node = rclpy.create_node('minimal_publisher')


I'm not sure about the python recommended style, but having the lines smashed together without whitespace seems nicer to me. But maybe there is a style guide for such things 🐔

codebot · 2016-11-28T18:29:31Z

rclpy/topics/minimal_subscriber/subscriber_local_function.py

+
+    subscription_ = node.create_subscription(
+        String, 'topic', listener_callback)
+    subscription_  # noqa


what does this line do? ELI5

This line tells the linter that this variable is not an unused variable. That's the counterpart of calling the destroy_* functions explicitely.

Comment changed from to avoid confusion

codebot · 2016-11-28T18:31:09Z

rclpy/topics/minimal_subscriber/subscriber_member_function.py

+    # Destroy the node explicitly
+    # (optional - otherwise it will be done automatically
+    # when the garbage collector destroys the node object)
+    minimal_subscriber.node.destroy_node()


can this be done in some sort of destructor for the MinimalSubscriber class?

dhood · 2016-11-28T18:11:07Z

rclpy/topics/minimal_publisher/README.md

+# Minimal "publisher" cookbook recipes
+
+This package contains a few different strategies for creating short nodes that blast out messages.
+The `publisher_old_school` recipe creates a talker node very similar to how it would be done in ROS1 using rospy.


can you use ROS 1 and ROS 2 (with spaces) for consistency please

dhood · 2016-11-28T18:21:15Z

rclpy/topics/minimal_publisher/publisher_old_school.py

+        i += 1
+        print('Publishing: "%s"' % msg.data)
+        publisher_.publish(msg)
+        # TODO(mikaelarguedas): explain why spin_once can't be used here


is it possible to add a comment to address this now?

that has been done in 1cf6702

dhood · 2016-11-28T18:22:54Z

rclpy/topics/minimal_publisher/package.xml

+  <test_depend>ament_copyright</test_depend>
+  <test_depend>ament_pep257</test_depend>
+  <test_depend>ament_pep8</test_depend>
+  <test_depend>ament_pyflakes</test_depend>


do you think the linters go against the minimalistic nature? there seems to be a few lines added that could confuse people just for linter sake

I though the conclusion was that examples should be tested and linted as opposed to the tutorials. Did I miss something?

I thought the conclusion was exactly the opposite, because testing would cause the CMake and potentially the code to be more complex, making it less copy-pastable. The tutorials, however, are not meant as minimal starting locations, so they can afford to have more complex CMake lists and code?

tutorial code tested, examples copy-paste friendly

Hmm I thought it was the other way around (examples are as short as possible, but tutorials are longer-form with narrative and automatically tested)? 🐔

Ok my bad I thought we wanted to make sure that the copy-pasted code compiles/run whereas it was not crucial for the ones supposed to show concepts on the wiki. Will remove all the tests then

We do want to make sure it compiles, and I don't think that linting it is a bad idea. If our code commonly make the sure do something that doesn't pass a linter, then we should consider changing the API.

So, I'd say leave the linters, but do not test it with automated tests.

I left the linters and added a comment in the package.xml to highlight the purpose of these dependencies

dhood · 2016-11-28T18:27:16Z

rclpy/topics/minimal_subscriber/subscriber_lambda.py

+    if args is None:
+        args = sys.argv
+
+    rclpy.init(args)


It would be more minimalistic to remove the args variable altogether (and sys import)

dhood · 2016-11-28T18:29:44Z

rclpy/topics/minimal_publisher/publisher_lambda.py

+
+    msg = String()
+    i = 0
+    timer = node.create_timer(0.5, lambda: publisher_.publish(msg))


I'd be in favour of removing/reworking this script since the lambda doesn't show that much (like you mentioned)

Yeah I agree, although it is a bit unfortunate that symmetry isn't possible, that's a language restriction and there isn't much we can do about it. I'm also in favor of removing this example 🐔

mikaelarguedas · 2016-11-28T19:26:56Z

Comments addressed

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

rclpy/topics/minimal_publisher/README.md

-The `publisher_local_function` allow to leverage the timers provided by ROS 2 to trigger message publication.
-The `publisher_member_function` created a class MinimalPublisher that sends messages periodically
+The `publisher_local_function` recipe shows how to leverage the timers provided by ROS 2 to trigger message publication.
+The `publisher_member_function` recipe creates a class MinimalPublisher that sends messages periodically.


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

Thanks everyone for reviewing.
Any other comments on this ?

codebot · 2016-11-28T20:01:37Z

👍 🐔

dhood · 2016-11-28T20:06:02Z

rclpy/topics/minimal_publisher/publisher_member_function.py

+    minimal_publisher = MinimalPublisher()
+
+    while rclpy.ok():
+        rclpy.spin_once(minimal_publisher.node)


since the cookbook examples are supposed to show best practice it might be best to not have the node attached to the class, so that multiple publishers can easily be attached to a node. it's not a blocker though, just a suggestion

good point, done eea27f3

dhood · 2016-11-28T21:58:54Z

rclpy/topics/minimal_publisher/publisher_local_function.py

+        print('Publishing: "%s"' % msg.data)
+        publisher_.publish(msg)
+
+    timer = node.create_timer(0.5, timer_callback)


I think it's worth highlighting the units here, since they're different to rclcpp. either commented or through a variable name

I don't think a comment is necessary given that we pass the same value to the python sleep function. It should be pretty obvious given that all scripts have equivalent behavior. If doubt there is, the variable name in the API is timer_period_sec.
I'm not opposed to it but don't think this is a source of confusion

ok I added a comment in c056f8f just in case

dirk-thomas · 2016-11-29T15:11:14Z

rclpy/topics/minimal_publisher/test/test_copyright.py

+
+
+def test_copyright():
+    rc = main(argv=['.', 'test'])


Why does this need to be . and test? Isn't test a subfolder of the package root .?

it was a copy paste from rclpy_examples, 'test' can be removed but '.' needs to stay because the linters don't provide a default path so removing it cause the nosetest invocation to fail. updated in e368944

* minimal pub examples * sub examples + linters * class examples * rename files and targets * add READMEs * add comment about object destruction * clarify use of spin_once * address comments * remove reference to deleted scripts * Readme fixup * pass node to pub/sub classes * variable name consistency * update old school publisher comment * remove unnecessary folder name * Clarify units * Update 'not recommended' comment

mikaelarguedas added 5 commits November 23, 2016 18:59

minimal pub examples

8022be4

sub examples + linters

2ca68d7

class examples

9d60e78

rename files and targets

1391f4d

add READMEs

1093673

mikaelarguedas added the in review Waiting for review (Kanban column) label Nov 28, 2016

mikaelarguedas self-assigned this Nov 28, 2016

codebot reviewed Nov 28, 2016

View reviewed changes

mikaelarguedas added 2 commits November 28, 2016 10:29

add comment about object destruction

a55f5b1

clarify use of spin_once

1cf6702

codebot approved these changes Nov 28, 2016

View reviewed changes

dhood reviewed Nov 28, 2016

View reviewed changes

mikaelarguedas added 2 commits November 28, 2016 11:18

address comments

cc6f8ac

remove reference to deleted scripts

475211c

Readme fixup

0de9ee2

mikaelarguedas commented Nov 28, 2016

View reviewed changes

dhood reviewed Nov 28, 2016

View reviewed changes

mikaelarguedas added 2 commits November 28, 2016 12:20

pass node to pub/sub classes

eea27f3

variable name consistency

82b14f3

dhood reviewed Nov 28, 2016

View reviewed changes

update old school publisher comment

ccc9b3e

dirk-thomas reviewed Nov 29, 2016

View reviewed changes

remove unnecessary folder name

e368944

mikaelarguedas mentioned this pull request Nov 29, 2016

Rclpy minimal services #140

Merged

dhood added 2 commits November 30, 2016 10:54

Clarify units

c056f8f

Update 'not recommended' comment

a80f6d3

dhood approved these changes Nov 30, 2016

View reviewed changes

mikaelarguedas merged commit cf6662f into master Nov 30, 2016

mikaelarguedas deleted the rclpy_minimal_pub_sub branch November 30, 2016 20:10

mikaelarguedas removed the in review Waiting for review (Kanban column) label Nov 30, 2016

Rclpy minimal pub sub #139

Rclpy minimal pub sub #139

Conversation

mikaelarguedas commented Nov 28, 2016

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 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

mikaelarguedas Nov 28, 2016 • 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

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

mikaelarguedas commented Nov 28, 2016

Choose a reason for hiding this comment

mikaelarguedas commented Nov 28, 2016

codebot commented Nov 28, 2016

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

mikaelarguedas Nov 28, 2016 •

edited

Loading