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.

Sign up for GitHub

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

Jump to bottom

Porting to ROS2 #22

Merged
merged 1 commit into from
Jan 31, 2019
Merged

Porting to ROS2 #22

merged 1 commit into from
Jan 31, 2019

Conversation

brawner
Copy link

@brawner brawner commented Jan 7, 2019

This ports rqt_graph to ROS2. Because rosgraph does not exist in ROS2, I grabbed and modified the rosgraph.impl.Graph code. Most new logic for ROS2 was put in the _graph_refresh method.

mlautman
mlautman suggested changes Jan 7, 2019
View reviewed changes
src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Show resolved Hide resolved
@brawner brawner force-pushed the ros2_port branch 4 times, most recently from b9e233c to 3574d8c Compare January 7, 2019 23:28
mlautman
mlautman approved these changes Jan 8, 2019
View reviewed changes
Copy link
Member

@mlautman mlautman left a comment

Choose a reason for hiding this comment

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

+1 after reviewing feedback

src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
test/dotcode_test.py Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
dirk-thomas
dirk-thomas reviewed Jan 8, 2019
View reviewed changes
Copy link
Contributor

@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.

As with previous PRs please separate different unrelated changes into separate commits (or even better PRs).

scripts/rqt_graph Outdated Show resolved Hide resolved
src/rqt_graph/rosgraph2_impl.py Outdated Show resolved Hide resolved
@brawner brawner force-pushed the ros2_port branch 3 times, most recently from 9450e68 to 34a2fdc Compare January 11, 2019 00:05
@brawner
Copy link
Author

brawner commented Jan 11, 2019

@dirk-thomas @mlautman The rosgraph.impl.Graph file has been merged in a separate PR and this PR was rebased.

dirk-thomas
dirk-thomas reviewed Jan 11, 2019
View reviewed changes
Copy link
Contributor

@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.

Again: if the style changes would have been separated from the actual changes it would be easy to apply them on the ROS 1 branch and significantly reduce the divergence between the branches.

package.xml Outdated Show resolved Hide resolved
setup.py Outdated
('share/' + package_name + '/resource', ['resource/RosGraph.ui']),
('share/' + package_name, ['package.xml']),
('share/' + package_name, ['plugin.xml']),
('lib/' + package_name, ['scripts/rqt_graph'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need both - an entry point and the global script?

setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@brawner
Copy link
Author

brawner commented Jan 14, 2019

@dirk-thomas Changes made per your comments

dirk-thomas
dirk-thomas approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@dirk-thomas dirk-thomas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes made per your comments

Since you rewrote history with a force push I can't see the changes you have made since the last review. That means I have to re-review the complete patch again. Please don't do that when incrementally reviewing patches. Either use the "squash and merge" option at the end or rewrite the history before merging but after the review has been finished.

@brawner
Copy link
Author

brawner commented Jan 14, 2019

Oh shoot, sorry Dirk

@mlautman mlautman merged commit 48d14ef into crystal-devel Jan 31, 2019
@mlautman mlautman deleted the ros2_port branch January 31, 2019 17:51
@dirk-thomas
Copy link
Contributor

Does this work with Crystal or is it using any API introduced after the release? In case of the former should this package be released?

@brawner
Copy link
Author

brawner commented Feb 4, 2019

It uses the rclpy functions introduced in this PR (ros2/rclpy#247):

node.get_publisher_names_and_types_by_node(node_name)
node.get_subscriber_names_and_types_by_node(node_name)
node.get_service_names_and_types_by_node(node_name)

@dirk-thomas
Copy link
Contributor

👍 That PR is part of the Crystal release. I will go ahead and run bloom for this repo then.

@dirk-thomas
Copy link
Contributor

ros/rosdistro#20174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlautman mlautman mlautman approved these changes

@dirk-thomas dirk-thomas dirk-thomas approved these changes

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
@brawner @dirk-thomas @mlautman