Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

[rostopic] Fix rostopic echo for non rosmsg field#909

Merged
dirk-thomas merged 4 commits intoros:kinetic-develfrom
wkentaro:echo-not-rosmsg
Sep 30, 2016
Merged

[rostopic] Fix rostopic echo for non rosmsg field#909
dirk-thomas merged 4 commits intoros:kinetic-develfrom
wkentaro:echo-not-rosmsg

Conversation

@wkentaro
Copy link
Copy Markdown
Contributor

@wkentaro wkentaro commented Sep 29, 2016

Close #908

@wkentaro wkentaro mentioned this pull request Sep 29, 2016
Closed
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Sep 29, 2016

If you run roslaunch rospy_tutorials talker_listener.launch and rostopic echo /chatter --nostr it prints:

data: <string length: 25>

Therefore I would expect rostopic echo /chatter --nostr -n 1 to print:

<string length: 25>

but it actually prints:

None

dirk-thomas
dirk-thomas reviewed Sep 29, 2016
View reviewed changes
tools/rostopic/src/rostopic/__init__.py Outdated
def value_transform(val):
if not isinstance(val, genpy.Message):
if echo_nostr and isinstance(val, str):
return None
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this instead:

return '<string length: %s>' % len(val)

Copy link
Copy Markdown
Contributor Author

@wkentaro wkentaro Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

dirk-thomas
dirk-thomas reviewed Sep 29, 2016
View reviewed changes
tools/rostopic/src/rostopic/__init__.py Outdated
if echo_nostr and isinstance(val, str):
return None
elif echo_noarr and isinstance(val, list):
return None
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I don't see how we could print the type information (<array type: ??? ...>):

return '<array length: %s>' % len(val)

@wkentaro
Copy link
Copy Markdown
Contributor Author

wkentaro commented Sep 29, 2016

With 47adb6d,

% rostopic echo /rosout/msg --nostr
<string length: 24>
---

% rostopic echo /rosout/topics --noarr
<array type: string, length: 2>
---

@wkentaro
Copy link
Copy Markdown
Contributor Author

wkentaro commented Sep 29, 2016 •
edited
Loading

With 9dd94cf,

Before

% rostopic echo /rosout --nostr -n1
header:
  seq: 13891
  stamp:
    secs: 1475187530
    nsecs: 501231908
  frame_id: <string length: 0>
level: 2
name: <string length: 9>
msg: <string length: 41>
file: <string length: 11>
function: <string length: 8>
line: 43
topics: <string length: 2>

After

% rostopic echo /rosout --nostr -n1
header:
  seq: 14060
  stamp:
    secs: 1475187547
    nsecs: 301131010
  frame_id: <string length: 0>
level: 2
name: <string length: 9>
msg: <string length: 41>
file: <string length: 11>
function: <string length: 8>
line: 43
topics: <array type: string, length: 2>
---

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Sep 29, 2016

The current patch duplicates a lot of code / logic. I would recommend to create a function like this:

def _value_transform(type_information, val, echo_nostr, echo_noarr):
    if echo_noarr and '[' in type_information:
        return '<array type: %s, length: %s>' % \
            (type_information.rstrip('[]'), len(val))
    elif echo_nostr and type_information == 'string':
        return '<string length: %s>' % len(val)
    elif echo_nostr and type_information == 'string[]':
        return '<array type: string, length: %s>' % len(val)
    return None

and call it from the two locations accordingly.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Sep 30, 2016

This looks good to me. Thank you for providing this patch so fast.

@dirk-thomas dirk-thomas merged commit 8babe82 into ros:kinetic-devel Sep 30, 2016
@wkentaro wkentaro deleted the echo-not-rosmsg branch September 30, 2016 19:03
@mikepurvis
Copy link
Copy Markdown
Member

mikepurvis commented Sep 30, 2016

Thanks for the quick turnaround, all.

@wjwwood wjwwood mentioned this pull request Oct 21, 2016
Closed
@dirk-thomas dirk-thomas mentioned this pull request Mar 2, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wkentaro @dirk-thomas @mikepurvis