Skip to content

Various fixes#142

Merged
mikaelarguedas merged 14 commits intomasterfrom
various_fixes
Jun 28, 2017
Merged

Various fixes#142
mikaelarguedas merged 14 commits intomasterfrom
various_fixes

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas commented Jun 27, 2017 •
edited
Loading

Not in this PR:

  • move the pendulum demo executables (because they need to be on the path given the current launch logic)

Depends on ros2/robot_state_publisher#8
Depends on ros2/ros2cli#23

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Jun 27, 2017
@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 27, 2017

if we dont want to address "make topic_monitor launchfiles runnable with launch" this is ready for review

wjwwood
wjwwood approved these changes Jun 27, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, but I don't know how to answer the question about topic_monitor needing to be launch-able by launch.

@dhood
Copy link
Copy Markdown
Member

dhood commented Jun 27, 2017 •
edited
Loading

sorry, didn't see the 'question' because updating the description doesn't send notifications (nor does editing in an @-mention). This looks good as is and I'll have a look at the topic monitor launching

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 27, 2017

sounds good, thank you both for the the input

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 27, 2017

waiting for ros2/ros2cli#23 to be resolved to update this before merging

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 28, 2017

ok I think this is ready for another review (sorry I rebased so it's not easy to see identify new commits since the last review). I'll wait for ros2/ros2cli#23 to be merged and then merge this along updating all the tutorials accordingly

@dhood I updated the topic_monitor README to match the current state, I'll leave it to you to update anything necessary if you decide to iterate on the launchfiles 😉

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jun 28, 2017

This might need to be updated when ament/ament_python#2 is merged to use the same approach as in the referenced PRs.

@mikaelarguedas mikaelarguedas merged commit a4d48aa into master Jun 28, 2017
@mikaelarguedas mikaelarguedas deleted the various_fixes branch June 28, 2017 03:04
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Jun 28, 2017
@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 28, 2017

thanks @dhood for the review and fixes!

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jun 28, 2017

This previous comment still needs to be addressed. Otherwise the moved executables aren't runnable with ros2 run.

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Jun 28, 2017

Yeah I fell asleep before opening a PR. I'll do that this morning

@mikaelarguedas mikaelarguedas mentioned this pull request Jun 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

+1 more reviewer

@dhood dhood dhood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@mikaelarguedas @dhood @dirk-thomas @wjwwood