[rostopic] Show stat when rostopic echo --noarr/nostr by wkentaro · Pull Request #724 · ros/ros_comm

wkentaro · 2015-12-30T18:19:14Z

Should this be another option like --statarr?
Because this kind of function is not needed for string for me but for only array field.

wjwwood · 2015-12-30T20:28:36Z

tools/rostopic/src/rostopic/__init__.py

This might be easier to read like this:

value_untransform_fn = None if value_transform: val, value_untransform_fn = value_transform(val) text = genpy.message.strify_message(val, indent=indent, time_offset=time_offset, current_time=current_time, field_filter=field_filter, fixed_numeric_width=fixed_numeric_width) if value_untransform_fn is not None: # this operation is necessary because value_transform does change the type of message # and that causes not strified values next time val = value_untransform_fn(val)

Thanks, I updated the commit.

wjwwood · 2015-12-30T20:32:48Z

Looks reasonable to me.

wkentaro · 2016-01-03T01:50:52Z

Thanks for the feedback, I updated the commit.

dirk-thomas · 2016-03-02T00:03:31Z

tools/rostopic/src/rostopic/__init__.py

                 offset_time=False, count=None,
-                 field_filter_fn=None, fixed_numeric_width=None):
+                 field_filter_fn=None, value_transform_fn=None,
+                 fixed_numeric_width=None):


Please do not change the order of existing arguments but add new arguments at the end.

Same below.

wkentaro · 2016-03-02T08:27:24Z

I have updated strified -> stringified.

dirk-thomas · 2016-03-02T19:15:43Z

Some of the unit tests are currently failing. Please see the referenced Jenkins builds.

dirk-thomas · 2016-03-06T07:06:14Z

tools/rostopic/src/rostopic/__init__.py

        :param count: number of messages to echo, ``None`` for infinite, ``int``
-        :param field_filter_fn: filter the fields that are strified for Messages, ``fn(Message)->iter(str)``
+        :param field_filter_fn: filter the fields that are stringified for Messages, ``fn(Message)->iter(str)``
+        :param value_transform_fn: transform the values of Messages, ``fn(Message)->Message``


Duplicate of line 622.

thanks, I have fixed.

dirk-thomas · 2016-03-08T01:37:18Z

tools/rostopic/src/rostopic/__init__.py

+        return val, untransform_fn
+    return value_transform
+
+def create_field_filter(echo_nostr, echo_noarr):


I am not sure if this function is used anywhere outside by now it doesn't filter anything anymore.

wkentaro · 2016-05-07T00:13:56Z

Removed the field_filter function.

dirk-thomas · 2016-05-10T18:37:22Z

Since you have removed all unit tests for create_field_filter are you planning to readd them for the new function?

wkentaro · 2016-05-11T03:44:41Z

I'm not planning to readd them, because filtering field it not supported after this PR.
Aside from actually removing(filtering) the fields (string/array) this PR shows the stats for them.

wkentaro · 2016-05-18T08:40:33Z

Is this solution OK? I removed the create_field_filter function which will be no longer used.

dirk-thomas · 2016-05-18T23:15:36Z

create_field_filter is a public function of this Python package. While it might not be used within this file anymore other packages might still be using it and would break if it gets removed. So I don't think it should be removed since such a breakage is not necessary.

wkentaro · 2016-05-19T06:03:30Z

@dirk-thomas
OK, I restored the create_field_filter function.

And also, I found a bug of this code with nested messages as below.
It is fixed now with the last commit.
BEFORE

% rostopic echo /raw_image_bgr/image_color --nostr --noarr
header: 
  seq: 48
  stamp: 
    secs: 1463637710
    nsecs: 575849056
  frame_id: camera
height: 1080
width: 1920
encoding: <string length: 4>
is_bigendian: 0
step: 5760
data: <array type: uint8, length: 6220800>

AFTER

% rostopic echo /raw_image_bgr/image_color --nostr --noarr
header: 
  seq: 46
  stamp: 
    secs: 1463637665
    nsecs: 575885057
  frame_id: <string length: 6>
height: 1080
width: 1920
encoding: <string length: 4>
is_bigendian: 0
step: 5760
data: <array type: uint8, length: 6220800>

wkentaro · 2016-05-19T18:23:57Z

@ros-pull-request-builder rostest this please

wkentaro · 2016-05-22T08:45:35Z

Test passed. Please review again when you have time.

dirk-thomas · 2016-06-03T16:18:03Z

The patch looks good. But I was thinking if the effort for transforming and then untransforming the message is necessary. Wouldn't it be easier to just copy the message and update the copied message? For efficiency you could also implement a custom copy operation which already avoids to copy the string / array fields in the first place?

wkentaro · 2016-06-03T18:11:46Z

The patch looks good. But I was thinking if the effort for transforming and then untransforming the message is necessary. Wouldn't it be easier to just copy the message and update the copied message? For efficiency you could also implement a custom copy operation which already avoids to copy the string / array fields in the first place?

Thanks for the suggestion. I worked around for a while, and the problem when the transformed message was untransformed will be fixed by correct _field_type of the message class. So I created TransformedMessage as the dummy class for printing.

wkentaro · 2016-07-15T18:03:57Z

ping

wkentaro · 2016-08-05T12:08:49Z

ping

dirk-thomas · 2016-08-08T20:34:35Z

I am sorry for the late response but I currently have to focus on a different project and don't have much time to review ROS tickets.

The patch looks good now. Thank you for iterating on it. I cherry-picked the changed to the kinetic-devel branch with two small cosmetic changes: e0f6397

--- a/tools/rostopic/src/rostopic/__init__.py
+++ b/tools/rostopic/src/rostopic/__init__.py
@@ -831,8 +831,7 @@ class CallbackEcho(object):
             val = [ord(x) for x in val]
         if value_transform is not None:
             val = value_transform(val)
-        text = genpy.message.strify_message(val, indent=indent, time_offset=time_offset, current_time=current_time, field_filter=field_filter, fixed_numeric_width=fixed_numeric_width)
-        return text
+        return genpy.message.strify_message(val, indent=indent, time_offset=time_offset, current_time=current_time, field_filter=field_filter, fixed_numeric_width=fixed_numeric_width)

     def callback(self, data, callback_args, current_time=None):
         """
@@ -1355,7 +1354,7 @@ def create_value_transform(echo_nostr, echo_noarr):
             f_val = getattr(val, f)
             if echo_noarr and '[' in t:
                 setattr(val_trans, f, '<array type: %s, length: %s>' %
-                                (t.rstrip('[]'), len(f_val)))
+                                      (t.rstrip('[]'), len(f_val)))
                 val_trans._slot_types[index] = 'string'
             elif echo_nostr and 'string' in t:
                 setattr(val_trans, f, '<string length: %s>' % len(f_val))

wkentaro · 2016-08-09T01:07:50Z

Thanks!

wkentaro force-pushed the rostopic-echo-show-stat branch from b30b6e0 to 5037d5b Compare December 30, 2015 18:22

wjwwood reviewed Dec 30, 2015
View reviewed changes

wkentaro force-pushed the rostopic-echo-show-stat branch from 5037d5b to a59ac5a Compare December 31, 2015 04:17

dirk-thomas reviewed Mar 2, 2016
View reviewed changes

wkentaro force-pushed the rostopic-echo-show-stat branch from dc650c3 to 9b7b996 Compare March 2, 2016 23:33

dirk-thomas added the requires-changes label Mar 3, 2016

dirk-thomas reviewed Mar 6, 2016
View reviewed changes

wkentaro force-pushed the rostopic-echo-show-stat branch from da1963f to afbb4b1 Compare March 6, 2016 08:37

dirk-thomas removed the requires-changes label Mar 6, 2016

dirk-thomas reviewed Mar 8, 2016
View reviewed changes

dirk-thomas added the requires-changes label Mar 8, 2016

wkentaro force-pushed the rostopic-echo-show-stat branch from afbb4b1 to b57a5bb Compare May 7, 2016 00:12

dirk-thomas added more-information-needed and removed requires-changes labels May 10, 2016

wkentaro changed the title ~~rostopic echo: Show stat when --noarr/nostr~~ [rostopic] Show stat when rostopic echo --noarr/nostr May 11, 2016

dirk-thomas added requires-changes and removed more-information-needed labels May 18, 2016

rostopic echo: Show stat when --noarr/nostr

84211ed

wkentaro added 3 commits May 19, 2016 14:31

feedback: Fix style of code

96558e9

feedback: strified -> stringified

4b4d505

Correctly untransform message value and type

1da29f8

wkentaro force-pushed the rostopic-echo-show-stat branch from b57a5bb to 1da29f8 Compare May 19, 2016 05:32

Fix transformation for nested str/arr

9d363df

wkentaro closed this May 19, 2016

wkentaro reopened this May 19, 2016

dirk-thomas added more-information-needed and removed requires-changes labels Jun 3, 2016

Make rostopic echo --noarr --nostr work with --plot option

239bc04

Copy before transforming message and remove untransforming

19fb710

wkentaro force-pushed the rostopic-echo-show-stat branch from 1a62372 to 19fb710 Compare June 3, 2016 18:13

dirk-thomas added enhancement and removed more-information-needed labels Aug 8, 2016

dirk-thomas closed this Aug 8, 2016

wkentaro deleted the rostopic-echo-show-stat branch August 9, 2016 01:07

dirk-thomas mentioned this pull request Aug 16, 2016

'rostopic echo' returning incomplete message data for some topics #868

Closed

wkentaro restored the rostopic-echo-show-stat branch August 23, 2016 04:46

wkentaro deleted the rostopic-echo-show-stat branch August 23, 2016 04:57

mikepurvis mentioned this pull request Dec 15, 2016

rostopic: pluggable message printers #950

Closed

dirk-thomas mentioned this pull request Mar 1, 2017

changes between 1.11.20 and 1.12.7 for backporting #1008

Merged

Conversation

wkentaro commented Dec 30, 2015

Uh oh!

wjwwood Dec 30, 2015

Choose a reason for hiding this comment

Uh oh!

wkentaro Dec 31, 2015

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Dec 30, 2015

Uh oh!

wkentaro commented Jan 3, 2016

Uh oh!

dirk-thomas Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

dirk-thomas Mar 6, 2016

Choose a reason for hiding this comment

Uh oh!

wkentaro Mar 6, 2016

Choose a reason for hiding this comment

Uh oh!

dirk-thomas Mar 8, 2016

Choose a reason for hiding this comment

Uh oh!

wkentaro commented May 7, 2016

Uh oh!

dirk-thomas commented May 10, 2016

Uh oh!

wkentaro commented May 11, 2016

Uh oh!

wkentaro commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented May 18, 2016

Uh oh!

wkentaro commented May 19, 2016

Uh oh!

wkentaro commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wkentaro commented May 22, 2016

Uh oh!

dirk-thomas commented Jun 3, 2016

Uh oh!

wkentaro commented Jun 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wkentaro commented Jul 15, 2016

Uh oh!

wkentaro commented Aug 5, 2016

Uh oh!

dirk-thomas commented Aug 8, 2016

Uh oh!

wkentaro commented Aug 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wkentaro commented May 18, 2016 •

edited

Loading

wkentaro commented May 19, 2016 •

edited

Loading

wkentaro commented Jun 3, 2016 •

edited

Loading