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 population of recursive message fields #24

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Jun 27, 2017

Fixes #22.

Ready for review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Jun 27, 2017
@dirk-thomas dirk-thomas self-assigned this Jun 27, 2017
Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

confirm this fixes #22


def set_msg_fields(msg, values):
for field_name, field_value in values.items():
print(field_name, field_value)
Copy link
Member

Choose a reason for hiding this comment

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

remove debug print?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Gone.

except TypeError:
value = field_type()
set_msg_fields(value, field_value)
setattr(msg, field_name, value)
Copy link
Member

Choose a reason for hiding this comment

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

I recommend catching exceptions here and saying which field failed, otherwise here's an example of giving 35 elements to Covariance instead of 36:

$ ros2 topic pub asdf geometry_msgs/PoseWithCovariance "pose:
  position: {x: 0.0, y: 0.0, z: 0.0}
  orientation: {x: 0.0, y: 0.0, z: 0.0, w: 0.0}
covariance: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0,
  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0,
  0.0, 0.0, 0.0, 0.0, 0.0]" 
covariance [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]
Traceback (most recent call last):
  File "/home/dhood/ros2_ws/install_isolated/ros2cli/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.0.0', 'console_scripts', 'ros2')()
  File "/home/dhood/ros2_ws/install_isolated/ros2cli/lib/python3.5/site-packages/ros2cli/cli.py", line 64, in main
    rc = extension.main(parser=parser, args=args)
  File "/home/dhood/ros2_ws/install_isolated/ros2topic/lib/python3.5/site-packages/ros2topic/command/topic.py", line 38, in main
    return extension.main(args=args)
  File "/home/dhood/ros2_ws/install_isolated/ros2topic/lib/python3.5/site-packages/ros2topic/verb/pub.py", line 43, in main
    return main(args)
  File "/home/dhood/ros2_ws/install_isolated/ros2topic/lib/python3.5/site-packages/ros2topic/verb/pub.py", line 47, in main
    publisher(args.message_type, args.topic_name, args.values)
  File "/home/dhood/ros2_ws/install_isolated/ros2topic/lib/python3.5/site-packages/ros2topic/verb/pub.py", line 67, in publisher
    set_msg_fields(msg, values_dictionary)
  File "/home/dhood/ros2_ws/install_isolated/ros2topic/lib/python3.5/site-packages/ros2topic/verb/pub.py", line 86, in set_msg_fields
    setattr(msg, field_name, value)
  File "/home/dhood/ros2_ws/install_isolated/geometry_msgs/lib/python3.5/site-packages/geometry_msgs/msg/_pose_with_covariance.py", line 102, in covariance
    True)
AssertionError

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see the updated patch. Can you try it with an even more nested message?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the assert would contain a reasonable message (which I added for this manually) I get the following now:

Failed to populate field 'covariance': The 'covariance' field must be a seq or list with length 36 and float values

Copy link
Member Author

Choose a reason for hiding this comment

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

See ros2/rosidl#224 for adding assert messages to all cases in the Python generator.

Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

Tested with PoseWithCovarianceStamped which has 3 or 4 levels of nesting and this works well, thanks:

ros2 topic pub initialpose geometry_msgs/PoseWithCovarianceStamped "header:
  stamp:
    sec: 0
    nanosec: 0
  frame_id: 'map'   
pose:
  pose:
    position: {x: 5.5, y: 4.5, z: 0.0}
    orientation: {x: 0.0, y: 0.0, z: 0.0, w: 1.0}
  covariance: [0.5, 0.5, 0.0, 0.0, 0.0, 0.0, 0.0, 0.5, 0.5, 0.0, 0.0, 0.0, 0.0, 0.0,    0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0,
    0.0, 0.0, 0.0, 0.0, 0.0, 0.0]"
publisher: beginning loop
publishing geometry_msgs.msg.PoseWithCovarianceStamped(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id='map'), pose=geometry_msgs.msg.PoseWithCovariance(pose=geometry_msgs.msg.Pose(position=geometry_msgs.msg.Point(x=5.5, y=4.5, z=0.0), orientation=geometry_msgs.msg.Quaternion(x=0.0, y=0.0, z=0.0, w=1.0)), covariance=[0.5, 0.5, 0.0, 0.0, 0.0, 0.0, 0.0, 0.5, 0.5, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]))

@dirk-thomas dirk-thomas merged commit 578369e into master Jun 27, 2017
@dirk-thomas dirk-thomas deleted the recursive_msg_population branch June 27, 2017 22:41
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 27, 2017
@dirk-thomas
Copy link
Member Author

See #27 for another improvement when passing wrong types.

esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
Signed-off-by: Matthew Hansen <matthew.k.hansen@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants