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

Support field selection #174

Merged
merged 7 commits into from
May 16, 2019
Merged

Conversation

juanrh
Copy link
Contributor

@juanrh juanrh commented Mar 20, 2019

Signed-off-by: Juan Rodriguez Hortala hortala@amazon.com

This change extends the mapping file format to supported selecting sub-fields for the ROS 1 part of the entries of the fields_1_to_2 section. Before the change only field names can be used, after the change the user can specify a . separated list of fields, corresponding to a sequence of field selection.

For example that allows using header.stamp: 'stamp' in the mapping from rosgraph_msgs.Log to rcl_interfaces.Log, which would help fixing #159 by adding a new mapping file for rcl_interfaces

This is a work in progress pull request, this change hasn't been fully tested. Also, this PR should be extended to support field selection for ROS 2 fields too.

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Mar 20, 2019
@gonzodepedro
Copy link
Contributor

gonzodepedro commented Mar 22, 2019

I reviewed and tested this PR together with PR #178 and it seems to work.

@juanrh are you going to continue working on this? or may I take it from here?

@juanrh
Copy link
Contributor Author

juanrh commented Mar 22, 2019

@gonzodepedro Thanks for taking a look. I'd like to finish this PR, I expect to have that complete by early next week

@juanrh juanrh changed the title [WIP] Support field selection Support field selection Mar 26, 2019
@juanrh
Copy link
Contributor Author

juanrh commented Mar 26, 2019

I have added support for field selection on ROS 2 messages, and a description of field selections in the documentation. I have also tested this on my laptop with success, with a modification of the demo publisher that publishes messages of type rosgraph_msgs/Log.

Please let me know if anything is missing

@gonzodepedro
Copy link
Contributor

LGTM

@dirk-thomas
Copy link
Member

If the PR is ready to be reviewed please replace the "in progress" label with the "in review" label (which will move the ticket into the review column on waffle.io).

@juanrh
Copy link
Contributor Author

juanrh commented Mar 26, 2019

@thomas-moulard
Copy link

thomas-moulard commented Mar 26, 2019

Build Status

@thomas-moulard
Copy link

thomas-moulard commented Mar 28, 2019

Build Status

@thomas-moulard
Copy link

@juanrh the nightly is fine but not this build so there is something we need to fix there - https://ci.ros2.org/view/packaging/job/packaging_linux/1414/ Can you investigate?

@ros2 ros2 deleted a comment from dirk-thomas Apr 4, 2019
@juanrh
Copy link
Contributor Author

juanrh commented Apr 5, 2019

@thomas-moulard - run the following CI job:

@prajakta-gokhale
Copy link

@juanrh when you get the chance, could you please rebase this change and request a CI re-run? Thanks!

@dirk-thomas dirk-thomas added the enhancement New feature or request label May 9, 2019
Copy link
Contributor

@gonzodepedro gonzodepedro left a comment

Choose a reason for hiding this comment

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

Currently this conflicts when rebasing to master.

is a sequence of field names separated by `.`, that specifies the path to a field starting from a message.
For example starting from the message `rosgraph_msgs/Log` the field selection `header.stamp` specifies a
path that goes through the field `header` of `rosgraph_msgs/Log`, that has a message of type `std_msgs/Header`,
and ending up in the field `stamp` of `std_msgs/Header`, that has type `time`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be good to state that the mapping works for ROS1 and ROS2 messages.

@gonzodepedro
Copy link
Contributor

@juanrh The commit conflicting with the rebase may be this

@juanrh
Copy link
Contributor Author

juanrh commented May 14, 2019

Thanks @gonzodepedro. I'm coming out of vacation but I should be able to rebase by the end of this week

@dirk-thomas
Copy link
Member

dirk-thomas commented May 14, 2019

I should be able to rebase by the end of this week

Feature freeze for Dashing is scheduled for this Thursday (see https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#timeline-before-the-release).

@gonzodepedro
Copy link
Contributor

@juanrh I think I'll take care of the rebase, so we can get this into Dashing feature freeze.

@juanrh
Copy link
Contributor Author

juanrh commented May 15, 2019

ok @gonzodepedro thanks for taking care of that. Please let me know if I can help with anything

Juan Rodriguez Hortala and others added 5 commits May 15, 2019 17:45
Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>
Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>
Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>
Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@gonzodepedro gonzodepedro force-pushed the ros1_field_selection branch from 5efb222 to 32911db Compare May 15, 2019 21:30
resource/interface_factories.cpp.em Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
ros1_bridge/__init__.py Outdated Show resolved Hide resolved
ros1_bridge/__init__.py Outdated Show resolved Hide resolved
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@dirk-thomas
Copy link
Member

Please run a CI build for this change and post the badge here.

Have you tried this patch together with ros2/rcl_interfaces#67 and can confirm that bridging the log message works with the not-FastRTPS RMW impl.?

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@gonzodepedro
Copy link
Contributor

gonzodepedro commented May 15, 2019

Have you tried this patch together with ros2/rcl_interfaces#67 and can confirm that bridging the log message works with the not-FastRTPS RMW impl.?

I've tried. I confirm it works with the not-FastRTPS RMW imp. And with FastRTPS RMW imp using the workaround for /ros2/rcl_interfaces#61 _log_disable_rosout:=true

@gonzodepedro
Copy link
Contributor

Build Status

@dirk-thomas dirk-thomas merged commit 43e2fca into ros2:master May 16, 2019
@dirk-thomas dirk-thomas removed the in progress Actively being worked on (Kanban column) label May 16, 2019
dhananjaysathe pushed a commit to rapyuta-robotics/ros1_bridge that referenced this pull request Aug 22, 2019
* Add field selection for ROS 1 fields to mappings

Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>

* Add field selection for ROS 2 fields to mappings

Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>

* Add documentation for field selection

Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>

* Fix linting issues with flake8 and pep257

Signed-off-by: Juan Rodriguez Hortala <hortala@amazon.com>

* rebase consolidation

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

* Linter and review comments

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

* Use isinstance instead of type()

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Signed-off-by: Dhananjay Sathe <dhananjay.sathe@rapyuta-robotics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants