[ros2topic] pub: add --repeat #66

mikaelarguedas · 2017-12-03T22:37:40Z

I kept the default behavior (publishing every second) the same for now as we dont have latching.

Not sure what the right place is to put the float_or_int utility for reuse. I'm not convinced it belongs in the Base class but I can imagine it being reused in many places. Maybe a set of argparse utilities in ros2cli would make sense.
@dirk-thomas thoughts?

dirk-thomas · 2017-12-03T22:46:09Z

Instead of adding --repeat I would suggest to add options similar to the ones available in ROS 1.

mikaelarguedas · 2017-12-03T23:46:57Z

thanks I'll do that. I guess I'll adapt ros2 service call accordingly.

Not sure what the right place is to put the float_or_int utility for reuse. I'm not convinced it belongs in the Base class but I can imagine it being reused in many places. Maybe a set of argparse utilities in ros2cli would make sense.

Do you have any feedback on this?

dirk-thomas · 2017-12-03T23:52:43Z

Not sure what the right place is to put the float_or_int utility for reuse. I'm not convinced it belongs in the Base class but I can imagine it being reused in many places. Maybe a set of argparse utilities in ros2cli would make sense.

In general: the function seems to be so trivial that I wouldn't bother to declare it as a public API (which needs to be documented and tested at some point).

In this specific case it might be possible to simply use type=float since that is what this custom function is doing anyway. I haven't tried that though...

dirk-thomas · 2017-12-04T00:46:15Z

ros2service/ros2service/verb/call.py

+        try:
+            rate = float(rate)
+        except ValueError:
+            raise argparse.ArgumentTypeError('rate must be a number')


This is unnecessary. Since the argument is of type=float the variable args.rate is already of type float (or None).

dirk-thomas · 2017-12-04T00:47:29Z

ros2service/ros2service/verb/call.py

+def requester(service_type, service_name, values, rate, once):
+    if rate is not None:
+        if once:
+            raise RuntimeError('You cannot select both -r and -1 (--once)')


This argument checking should happen within argparse. You can use a mutually exclusive group for this.

dirk-thomas · 2017-12-04T00:50:03Z

ros2service/ros2service/verb/call.py

+    if rate is not None:
+        period = 1. / rate
+    else:
+        period = 1


I would suggest to use a default value for the argument instead. That way the code doesn't have to contain some not-documented constants.

That might affect the choice to group the arguments...

Yeah I went back and forth on this.
I'm fine with adding the default vaIue for the rate and keeping the logic for the mutual exclusion. That logic can be in a main fuction before calling the actual requester like it's done for ros2topic.

TBH I think that they dont have to be mutually exclusive. If you specify a rate you override a default of 1Hz, but if you specify --once, the rate (user specified or not) will not matter as we exit after the first call. I'm not convinced the executable should raise, it could just honor once

dirk-thomas · 2017-12-04T01:51:05Z

ros2topic/ros2topic/verb/pub.py


    def main(self, *, args):
        return main(args)


 def main(args):
-    return publisher(args.message_type, args.topic_name, args.values)
+    if args.rate <= 0:
+        raise ValueError('rate must be greater than zero')


I would suggest ti move this into PubVerb.main. That way the entity defining the command line arguments is also responsible of checking the sanity of the values. (The same way as it is for the CallVerb above.)

Does the ValueError result in a stack trace for the user? If so I would recommend to either use argparse to signal the error or raise a RuntimeError (both with the goal to not show a stack trace to the user).

Good point, I changed it to a RuntimeError

I would suggest ti move this into PubVerb.main. That way the entity defining the command line arguments is also responsible of checking the sanity of the values. (The same way as it is for the CallVerb above.)

Good point, moved in 041b411

dirk-thomas · 2017-12-04T01:51:29Z

ros2service/ros2service/verb/call.py

+def requester(service_type, service_name, values, rate, once):
+    if rate is not None:
+        period = 1. / rate
+    else:


This is now never the case.

forgot to commit local changes :/
29015d6

dirk-thomas

lgtm (I haven't tried it though).

mikaelarguedas · 2017-12-04T02:36:04Z

Thanks for the various fixups.
I tried the various scenarios it seems to work fine.
One thing that's a bit weird is that if the use passes an invalid rate and also pass once it raises. I could make it perform the check on the rate being only if once is not passed if we wanted to be more correct.

dirk-thomas · 2017-12-04T02:38:09Z

One thing that's a bit weird is that if the use passes an invalid rate and also pass once it raises. I could make it perform the check on the rate being only if once is not passed if we wanted to be more correct.

If we would check the argument directly within argparse that would also fail for invalid values even when they are later not used. So I am ok with it raising in that case.

mikaelarguedas · 2017-12-04T02:54:46Z

If we would check the argument directly within argparse that would also fail for invalid values even when they are later not used. So I am ok with it raising in that case.

Fair point. Merging then.

dhood · 2017-12-06T18:00:10Z

what's the motivation for having repeated service calls the default? just to match ros2 topic? Repeated service calls wasn't the case in ros1 and I don't think it's appropriate as the default for ros2

mikaelarguedas · 2017-12-06T18:06:19Z

It was not the case for rostopic in ROS 1 either, we did it for consistency but I agree the default should likely be to send only once. I'm fine to change it if everybody agrees

codebot · 2017-12-06T18:31:37Z

👍 to sending it only once. Sometimes services have nontrivial downstream effects, like starting a motion plan or something. Of course, that argument can also be said for (some) topics, but somehow it feels like this happens more often for services.

mikaelarguedas · 2017-12-07T16:46:30Z

#67

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

first shot at passing -r argument

b98caf3

mikaelarguedas added the in progress Actively being worked on (Kanban column) label Dec 3, 2017

mikaelarguedas self-assigned this Dec 3, 2017

mikaelarguedas added 2 commits December 3, 2017 16:30

[ros2topic] add once and rate parameters

20f054b

[ros2service] add once and rate parameters

643f23b

dirk-thomas reviewed Dec 4, 2017

View reviewed changes

mikaelarguedas and others added 2 commits December 3, 2017 17:41

simplify logic, add sleepd for once publisher and remove argparse

75988c1

fix spelling

5dc0e79

dirk-thomas reviewed Dec 4, 2017

View reviewed changes

dirk-thomas and others added 4 commits December 3, 2017 17:52

format default the same as argparse does

ce69c41

format default the same as argparse does

c1801d6

move logic to the right function

041b411

mimic ros2topic and remove extra logic

29015d6

mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 4, 2017

consistent with services

c0dfcec

dirk-thomas approved these changes Dec 4, 2017

View reviewed changes

mikaelarguedas merged commit 10f4147 into master Dec 4, 2017

mikaelarguedas deleted the pub_add_repeat branch December 4, 2017 02:55

mikaelarguedas removed the in review Waiting for review (Kanban column) label Dec 4, 2017

esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022

Unindent setup.cfg options. (ros2#66)

13e7bf6

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ros2topic] pub: add --repeat #66

[ros2topic] pub: add --repeat #66

mikaelarguedas commented Dec 3, 2017

dirk-thomas commented Dec 3, 2017

mikaelarguedas commented Dec 3, 2017

dirk-thomas commented Dec 3, 2017

dirk-thomas Dec 4, 2017

mikaelarguedas Dec 4, 2017

dirk-thomas Dec 4, 2017

dirk-thomas Dec 4, 2017

mikaelarguedas Dec 4, 2017

dirk-thomas Dec 4, 2017

dirk-thomas Dec 4, 2017

mikaelarguedas Dec 4, 2017

mikaelarguedas Dec 4, 2017

dirk-thomas Dec 4, 2017

mikaelarguedas Dec 4, 2017

dirk-thomas left a comment

mikaelarguedas commented Dec 4, 2017

dirk-thomas commented Dec 4, 2017

mikaelarguedas commented Dec 4, 2017

dhood commented Dec 6, 2017

mikaelarguedas commented Dec 6, 2017

codebot commented Dec 6, 2017

mikaelarguedas commented Dec 7, 2017

[ros2topic] pub: add --repeat #66

[ros2topic] pub: add --repeat #66

Conversation

mikaelarguedas commented Dec 3, 2017

dirk-thomas commented Dec 3, 2017

mikaelarguedas commented Dec 3, 2017

dirk-thomas commented Dec 3, 2017

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

dirk-thomas left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Dec 4, 2017

dirk-thomas commented Dec 4, 2017

mikaelarguedas commented Dec 4, 2017

dhood commented Dec 6, 2017

mikaelarguedas commented Dec 6, 2017

codebot commented Dec 6, 2017

mikaelarguedas commented Dec 7, 2017