Skip to content

Import message and support setting values of complex arrays/sequence#52

Closed
mikaelarguedas wants to merge 3 commits intoros2:masterfrom
mikaelarguedas:import_message
Closed

Import message and support setting values of complex arrays/sequence#52
mikaelarguedas wants to merge 3 commits intoros2:masterfrom
mikaelarguedas:import_message

Conversation

@mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented May 2, 2019

Version of #33 not changing the API exposed by message packages.

This is a redo of ros2/ros2cli#197 targetting this repository and updated to match what is returned by get_fields_and_field_types.

I left the tests commented out for now, they were passing about 2 days ago but since the message fixtures have been changed and the rosidl_runtime_py tests were not passing.
Submitting for review without updated tests. Hopefully we can update the tests shortly once the test_msgs situation stabilizes

Tests have been updated, ready for review

tested using command from ros2/ros2cli#191 (comment):

ros2 topic pub some_pose_array geometry_msgs/PoseArray "{header: {stamp: {sec: 1, nanosec: 2}, frame_id: some_frame}, poses: [{position: {x: 0, y: 1, z: 2}, orientation: {x: 3, y: 4, z: 5, w: 6}}]}"

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 2, 2019
@mikaelarguedas mikaelarguedas requested a review from dirk-thomas May 2, 2019 20:54
# TODO(sloretz) node API to get topic types should indicate if action or msg
middle_module = 'msg'
if topic_name.endswith('/_action/feedback'):
middle_module = 'action'
Copy link
Member

Choose a reason for hiding this comment

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

Once this problem has been addressed the API would not need to take topic_name anymore which would break users. Therefore I don't think this API should added as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect in the future the import function to be something like https://github.com/ros2/rosidl_python/pull/33/files#diff-2283820db2117771f9254b0ff5ac87f0 using the abstract types.

What would be an acceptable API in the meantime? a keyword argument for the topic_name that would default to an empty string? (note that it would still break API the day that keyword argument is removed).

Or would you rather keep the original function in ros2topic and make a duplicated function here without this argument at all ?

Copy link
Member

Choose a reason for hiding this comment

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

I would rather not add API here which is known that it needs to change as soon as something else gets fixed. So for now the API can stay were it is and once a stable API can be created it can land in the rosidl_runtime_py package.

Also I just created #54 which adds the SLOT_TYPES to each message. Maybe that is sufficient for what you want to achieve?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, just to clarify my understanding:
The suggestion is to use the SLOT_TYPES from #54
introduce here the new import_message_from_namespaced_type from #33
and modify set_message to use these 2 new bits in order to fill Nested ROS messages

Is that right ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright I integrated the changes from #54 and updated #33 accordingly.

@mikaelarguedas
Copy link
Member Author

Superseeded by #33

@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label May 3, 2019
@mikaelarguedas mikaelarguedas deleted the import_message branch May 3, 2019 15:16
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