Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Return result of client execute#938

Merged
dirk-thomas merged 1 commit intoros:kinetic-develfrom
efernandez:fix_execute
Jan 13, 2017
Merged

Return result of client execute#938
dirk-thomas merged 1 commit intoros:kinetic-develfrom
efernandez:fix_execute

Conversation

@efernandez
Copy link
Copy Markdown
Contributor

@efernandez efernandez commented Nov 23, 2016

This replaces #928

All test passing. Note that with return ok; some rostests fail.

@efernandez
Copy link
Copy Markdown
Contributor Author

efernandez commented Nov 23, 2016

FYI @afakihcpr @mikepurvis @jasonimercer @dirk-thomas

@mikepurvis
Copy link
Copy Markdown
Member

mikepurvis commented Nov 23, 2016

LGTM. +1

@jasonimercer
Copy link
Copy Markdown
Contributor

jasonimercer commented Nov 23, 2016

+1

tspicer01 pushed a commit to clearpathrobotics/ros_comm that referenced this pull request Nov 23, 2016
Merge PR ros#938
ec13b70
@efernandez
Copy link
Copy Markdown
Contributor Author

efernandez commented Nov 29, 2016

ping @tfoote @dirk-thomas

@mikepurvis mikepurvis mentioned this pull request Jan 10, 2017
Closed
@mikepurvis
Copy link
Copy Markdown
Member

mikepurvis commented Jan 10, 2017 •
edited
Loading

@jspricke @dirk-thomas We've been running this at Clearpath for several months; hasn't caused any regressions in our use-case.

@jspricke
Copy link
Copy Markdown
Member

jspricke commented Jan 13, 2017

Woah, that function looks quite entangled already. I would prefer to simplify it. But otherwise +1 from me for the fix.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Jan 13, 2017

@efernandez Sorry for the long delay and thank you for the patch.

@mikepurvis Thank you for testing this change.

@jspricke Thanks for the review.

@dirk-thomas dirk-thomas merged commit 1c13690 into ros:kinetic-devel Jan 13, 2017
@efernandez efernandez deleted the fix_execute branch January 14, 2017 16:41
@efernandez
Copy link
Copy Markdown
Contributor Author

efernandez commented Jan 14, 2017

Thanks @dirk-thomas

BTW, @afakihcpr has an additional fix for the case when the ROS initialization is done from a separate thread and it gets shutdown while still initializing. I'm not sure if there's a PR upstream for that one or not.
It solves crashes on nodes like move_base, that does the ROS initialization from a thread other than the main one.

@mikepurvis mikepurvis mentioned this pull request Feb 14, 2017
Closed
@dirk-thomas dirk-thomas mentioned this pull request Mar 2, 2017
Merged
rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017
@dirk-thomas
Merge pull request ros#938 from efernandez/fix_execute
b594e96 
Return result of client execute
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@efernandez @mikepurvis @jasonimercer @jspricke @dirk-thomas