Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the setting of array variables. #59

Merged
merged 2 commits into from
May 30, 2019
Merged

Conversation

clalancette
Copy link
Contributor

If the type of the field is array.array, then a simple

x = array.array(mylist)

doesn't work; the typecode must be specified. Special-case
arrays here so that we do:

x = array.array(typecode, list)

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix ros2/ros2cli#270

If the type of the field is array.array, then a simple

  x = array.array(mylist)

doesn't work; the typecode *must* be specified.  Special-case
arrays here so that we do:

  x = array.array(typecode, list)

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
field_typecode = getattr(msg, field_name).typecode
value = field_type(field_typecode, field_value)
else:
value = field_type(field_value)
except TypeError:
value = field_type()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we ever hit a TypeError when the field_type is array.array, then we'll throw a different exception here since you can't instantiate an array.array without a typecode. However, I couldn't see how that would happen, so I didn't make any changes here.

Copy link
Member

Choose a reason for hiding this comment

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

Please apply the same conditional logic in the exception case to construct and empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a29f80f

@clalancette clalancette added the in review Waiting for review (Kanban column) label May 30, 2019
@dirk-thomas
Copy link
Member

Can you please check if sequences (with a dynamic size) are also affect and need a similar fix.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Works locally, LGTM!

@@ -34,7 +36,11 @@ def set_message_fields(msg: Any, values: Dict[str, str]) -> None:
for field_name, field_value in values.items():
field_type = type(getattr(msg, field_name))
try:
value = field_type(field_value)
if field_type == array.array:
Copy link
Contributor

Choose a reason for hiding this comment

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

@clalancette nit:

Suggested change
if field_type == array.array:
if field_type is array.array:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a29f80f

@clalancette
Copy link
Contributor Author

Can you please check if sequences (with a dynamic size) are also affect and need a similar fix.

Good call, will check on that next.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

Good call, will check on that next.

As far as I can tell, the sequence stuff works (at least, basically). I ran:

ros2 topic pub -r 5 /foo std_msgs/Int32MultiArray '{layout: {dim: [{label: 'foo', size: 5, stride: 4}], data_offset: 12}, data: [1, 5]}'

Which seemed to work fine. In retrospect, that's because Sequences (lists and tuples, at least) can be instantiated with just val = list(list) or whatever. I'll point out that ranges (which are technically a Sequence) don't have this property and need to be instantiated with at least one value. Is that something we worry about?

There is also a different bug in there, in that sending the wrong type for the data gives an opaque error message, but I'll ticket that separately.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

LGTM (haven't tried it though have tried it and it resolves the referenced ticket)

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Haven't tried it, but looks good.

@clalancette
Copy link
Contributor Author

#61 is the follow-up. It's in the same area, but I'm running out of time today. Since it's a cosmetic issue, I think we can punt to after Dashing. CI up to rosidl_runtime_py:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette clalancette merged commit a00a2db into master May 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-array-publish branch May 30, 2019 21:13
@dirk-thomas
Copy link
Member

See #62 for a revised patch which doesn't result in #61.

jacobperron pushed a commit to ros2/rosidl_runtime_py that referenced this pull request Sep 19, 2019
* Fix the setting of array variables.

If the type of the field is array.array, then a simple

  x = array.array(mylist)

doesn't work; the typecode *must* be specified.  Special-case
arrays here so that we do:

  x = array.array(typecode, list)

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publishing an array from the command-line doesn't work in Dashing
4 participants