Enable ros2topic delay command by yechun1 · Pull Request #139 · ros2/ros2cli

yechun1 · 2018-08-24T06:13:58Z

Porting rostopic delay command from ROS to ROS2. Display delay of topic from timestamp in header.

Usage: ros2 topic delay /topic_name

Signed-off-by: Chris Ye chris.ye@intel.com

Connects to: #132

dirk-thomas · 2018-09-07T20:29:08Z

Thank you for working on this feature.

In the current form the patch is pretty difficult to review. Please split the patch into separate commit with roughly the following structure:

first commit containing the exact copy of the upstream file
additional commits for any changes you applied, e.g.
- style changes, comments, removing obsolete change in one commit
- ROS 2 API changes in a second commit
- functional changes in a third commit

yechun1 · 2018-09-10T00:39:08Z

Thanks for guide, I'll resubmit commits early this week.

yechun1 · 2018-09-10T11:23:25Z

patch splitted, be ready for review.

dirk-thomas · 2018-09-10T16:25:42Z

ros2topic/ros2topic/verb/delay.py

+            'topic',
+            help='Topic name to be calcurated the delay')
+        arg.completer = TopicNameCompleter(
+            include_hidden_topics_key='include_hidden_topics')


There is currently no include_hidden_topics argument here? I guess you need to add it in order to work.

Could work, same usage ref to pub.py and list.py
from ros2topic.api import TopicNameCompleter

ros2cli/ros2topic/ros2topic/api/__init__.py

Lines 45 to 46 in ce9077f

include_hidden_topics=getattr(

parsed_args, self.include_hidden_topics_key))

Never mind, the argument is added one level above in

ros2cli/ros2topic/ros2topic/command/topic.py

Lines 25 to 27 in ce9077f

parser.add_argument(

'--include-hidden-topics', action='store_true',

help='Consider hidden topics as well')

and will work for all subverbs under topic.

dirk-thomas · 2018-09-10T16:25:45Z

ros2topic/ros2topic/verb/delay.py

+        arg.completer = TopicNameCompleter(
+            include_hidden_topics_key='include_hidden_topics')
+        parser.add_argument(
+            '--window', '-w', default=-1,


Please define the type of the argument as int than argparse will already ensure the type for you. It would make sense if the default value would be a valid value so that users don't have to specify one.

fixed (5e7cc6a)

dirk-thomas · 2018-09-10T16:25:59Z

ros2topic/ros2topic/verb/delay.py


        # can't have infinite window size due to memory restrictions
-        if window_size < 0:
+        if window_size < 0 or window_size > 50000:


The input of window size should be checked after argparse and if negative rejected (rather than silently changed).

Please don't hard code arbitrary limits like 50000. It should be the users choice to use a larger window if they know what they are doing.

fixed a5aec05

dirk-thomas · 2018-09-10T16:26:02Z

ros2topic/ros2topic/verb/delay.py

                return

-            curr = curr_rostime.to_sec()
+            curr = curr_rostime.nanoseconds * 1e-9


You could avoid the conversion to floating point seconds and keep using the Time class for the operations. That would avoid any rounding errors.

fixed 2c01515
As ROS there is to_sec and to_time method, but I not found similar in ROS2, especially about msg_header.stamp. this is builtin_interface/Time type, does there is function like stamp.to_time()?

Not at the moment: see ros2/rclpy#236

dirk-thomas · 2018-09-10T16:26:05Z

ros2topic/ros2topic/verb/delay.py

-        return t_type, t, msgevalgen(topic[len(t):])
-    else:
-        return None, None, None
+    timer = node.create_timer(1, timer_callback)


You could register rt.print_delay here directly getting rid of the local function.

fixed 9c6ad41

dirk-thomas · 2018-09-10T16:26:13Z

ros2topic/ros2topic/verb/delay.py

                self.msg_tn = curr

-            if len(self.delays) > self.window_size - 1:
+            if len(self.delays) > self.window_size:


Please describe why this was changed.

To fix the issue that no data print when set window as 1,
ros2 topic delay /image -w 1

Don't know why here need to window_size - 1 ?

dirk-thomas · 2018-09-10T16:26:15Z

ros2topic/ros2topic/verb/delay.py


            self.last_msg_tn = self.msg_tn
-        return mean, min_delta, max_delta, std_dev, n + 1
+        return mean, min_delta, max_delta, std_dev, n


Please describe why this was changed.

yechun1 · 2018-09-11T04:15:30Z

thanks for review, I have added changes for fixing.

dirk-thomas · 2018-09-11T16:27:45Z

ros2topic/ros2topic/verb/delay.py

+
+        # can't have infinite window size due to memory restrictions
+        if window_size < 0:
+            window_size = 50000


This "magic" upper bound is nowhere visible to the user. Please use it as a default value of the argument instead (and ensure in code that the user didn't pass a negative value).

this is original design, I have not changed while porting. If user did not set the value. As previous design, the default value of window is -1, and here will re-set to max 50000 as memory restrictions.

Sure, it would still benefit from being improved.

dirk-thomas · 2018-09-11T16:29:23Z

ros2topic/ros2topic/verb/delay.py

+              % (delay, min_delta, max_delta, std_dev, window))
+
+
+def _rostopic_delay(node, topic, window_size=-1, blocking=False):


I would suggest making window_size a required argument - otherwise this function needs to pick an opaque default value.

How does blocking=False work if the msg_class can't be determined yet?

This is original design. I think general user will not concern about window_size and just easy to use the tool. It will give user a little more trouble every time to type --window as required argument. It may also confuse user about the window meaning (first time I use rostopic, I really don't know what this window mean :))

Sorry but not considered when blocking=False, thanks for review, fixed by 0e79d06 (replaced by c3e506f )

set blocking=True by default. commit again: c3e506f

(commit: 2cd09fe): Is this fixing acceptable to set DEFULT_WINDOW_SIZE? (ref to cho.py code). User can input any value (unsigned int) or use default. The DEFULT_WINDOW_SIZE is set as 10000 and could be any other value.

dirk-thomas · 2018-09-11T20:14:33Z

ros2topic/ros2topic/verb/delay.py

+                curr_nanosec = curr_rostime.to_msg().nanosec
+
+                delay = ((curr_sec + curr_nanosec * 1e-9) -
+                         (msg.header.stamp.sec + msg.header.stamp.nanosec * 1e-9))


You should be able to compute the delay (as a Duration) by doing curr_rostime.to_msg() - rclpy.Time.from_msg(msg.header.stamp) (see https://github.com/ros2/rclpy/blob/62012d3650de790d89c725232451c4e998b396fb/rclpy/rclpy/time.py#L131) and store the duration instance in self.delays.

dirk-thomas · 2018-09-12T01:34:51Z

ros2topic/ros2topic/verb/delay.py

+                self.msg_tn = curr
+                self.delays = []
+            else:
+                # store the duration instrance in self.delays


Spelling: "instrance" -> "instance".

dirk-thomas · 2018-09-12T01:35:38Z

ros2topic/ros2topic/verb/delay.py

+            else:
+                # store the duration instrance in self.delays
+                duration = (curr_rostime - Time.from_msg(msg.header.stamp))
+                self.delays.append(duration.nanoseconds * 1e-9)


This isn't doing what the comment two lines above states - it still stores the duration as a floating point number in seconds.

Resubmitted the commit 34e400a

dirk-thomas · 2018-09-12T01:36:22Z

ros2topic/ros2topic/verb/delay.py

+
+        # can't have infinite window size due to memory restrictions
+        if window_size < 0:
+            window_size = 50000


Sure, it would still benefit from being improved.

yechun1 · 2018-09-12T21:47:03Z

@dirk-thomas commits with minor changes are submitted. Do you have any other suggests? so that I could sync with you and fix immediately when you are online:)

dirk-thomas · 2018-09-21T15:34:57Z

Thank you for the updates!

I just ran CI builds for this branch and the results look good:

Linux
Linux-aarch64
macOS
Windows

Can you please add the BSD license to the package manifest (as in https://github.com/ros2/ros2cli/pull/133/files#diff-27285248de861ae535a0217d2fa40c9cR11).

To keep the git history a bit shorter can you then reduce the commits as follows:

keep the first two commits (1-to-1 copy of file, add the link to "upstream")
squash the remaining commits into a third commit

After that this should be ready to be merged.

This file is originally copied from: https://github.com/ros/ros_comm/blob/6e5016f4b2266d8a60c9a1e163c4928b8fc7115e/tools/rostopic/src/rostopic/__init__.py Signed-off-by: Chris Ye <chris.ye@intel.com>

Signed-off-by: Chris Ye <chris.ye@intel.com>

* remove irrelevant code and reserve hz code (ros has only one __init__.py file include all topic commands, ros2 has splitted commands to isolated file) * major functional changes of delay cmd with ROS2 API * update license format to pass test_copyright * use Time duration to compute the delay * check window_size as non-negative integer, fix no print when set window as 1 Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 · 2018-09-29T00:09:30Z

To keep the git history a bit shorter can you then reduce the commits as follows:
keep the first two commits (1-to-1 copy of file, add the link to "upstream")
squash the remaining commits into a third commit

Added BSD license and merged the comments as
b805ac6, 3fd69ec, a317f3c

clalancette · 2018-10-04T00:45:06Z

CI looks good, and the patch set has been redone according to what @dirk-thomas suggested. I also took a brief look at it, and it looks OK to me. I'm going to merge this; thanks for the contribution.

yechun1 · 2018-10-04T11:54:01Z

@dirk-thomas @clalancette thanks for review and merge.

tfoote added the in review Waiting for review (Kanban column) label Aug 24, 2018

yechun1 mentioned this pull request Aug 24, 2018

[ros2topic] enable ros2topic hz delay bw command #132

Closed

3 tasks

dirk-thomas self-requested a review September 6, 2018 17:22

yechun1 force-pushed the port_rostopic_delay branch 2 times, most recently from aa2656c to 623d3ef Compare September 7, 2018 07:03

dirk-thomas added question Further information is requested in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) question Further information is requested labels Sep 7, 2018

dirk-thomas added in progress Actively being worked on (Kanban column) requires-changes and removed in review Waiting for review (Kanban column) labels Sep 7, 2018

yechun1 force-pushed the port_rostopic_delay branch from 623d3ef to 3b02cf8 Compare September 10, 2018 11:20

dirk-thomas reviewed Sep 10, 2018

View reviewed changes

dirk-thomas mentioned this pull request Sep 10, 2018

Enable ros2tpoic hz command #133

Merged

dirk-thomas reviewed Sep 11, 2018

View reviewed changes

dirk-thomas mentioned this pull request Sep 12, 2018

need for a Time data type without a clock ros2/rclpy#236

Closed

dirk-thomas reviewed Sep 12, 2018

View reviewed changes

yechun1 force-pushed the port_rostopic_delay branch from e9d5095 to 34e400a Compare September 12, 2018 01:55

yechun1 force-pushed the port_rostopic_delay branch 2 times, most recently from d057f2c to 2cd09fe Compare September 12, 2018 15:08

dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) requires-changes labels Sep 21, 2018

Copy original file for ros2topic delay porting

b805ac6

This file is originally copied from: https://github.com/ros/ros_comm/blob/6e5016f4b2266d8a60c9a1e163c4928b8fc7115e/tools/rostopic/src/rostopic/__init__.py Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 force-pushed the port_rostopic_delay branch 2 times, most recently from 0e2f7ff to a317f3c Compare September 22, 2018 13:17

yechun1 added 2 commits September 22, 2018 21:21

add rostopic delay original file link

3fd69ec

Signed-off-by: Chris Ye <chris.ye@intel.com>

clalancette merged commit 2049b2f into ros2:master Oct 4, 2018

clalancette removed the in review Waiting for review (Kanban column) label Oct 4, 2018

yechun1 deleted the port_rostopic_delay branch October 4, 2018 11:54

dirk-thomas added the enhancement New feature or request label Oct 6, 2018

	include_hidden_topics=getattr(
	parsed_args, self.include_hidden_topics_key))

	parser.add_argument(
	'--include-hidden-topics', action='store_true',
	help='Consider hidden topics as well')

		% (delay, min_delta, max_delta, std_dev, window))


		def _rostopic_delay(node, topic, window_size=-1, blocking=False):

Conversation

yechun1 commented Aug 24, 2018

Uh oh!

dirk-thomas commented Sep 7, 2018

Uh oh!

yechun1 commented Sep 10, 2018

Uh oh!

yechun1 commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechun1 Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechun1 commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechun1 Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechun1 Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechun1 commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Sep 21, 2018

Uh oh!

yechun1 commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

yechun1 Sep 11, 2018 •

edited

Loading

yechun1 Sep 12, 2018 •

edited

Loading

yechun1 Sep 12, 2018 •

edited

Loading

yechun1 commented Sep 12, 2018 •

edited

Loading

yechun1 commented Sep 29, 2018 •

edited

Loading