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

roscpp: Adding AsyncSpinner::canStart to check if a spinner can be started #377

Closed

Conversation

jbohren
Copy link
Member

@jbohren jbohren commented Mar 11, 2014

This adds the introspection described in #277

Should a PR also be made against hydro?

return true;
}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

Please simplify the code:

return spinlock.owns_lock();

@jbohren
Copy link
Member Author

jbohren commented Mar 11, 2014

Corrections made.

@tfoote
Copy link
Member

tfoote commented Mar 11, 2014

+1

" a second AsyncSpinner with a custom callback"
" queue, but another AsyncSpinner already exists,"
" so your custom callback queue will not be serviced"
" by this AsyncSpinner.");
Copy link
Member Author

Choose a reason for hiding this comment

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

This error message actually can't be this precise. It's possible that the other AsyncSpinner was created with a non-default callback queue, so not even the default queue will be serviced.

@jbohren
Copy link
Member Author

jbohren commented Mar 11, 2014

I've updated the warning message to better characterize the issue with starting two AsyncSpinners.

@dirk-thomas
Copy link
Member

+1 Manually merged in e6f48a8 with a minor whitespace change.

@dirk-thomas
Copy link
Member

I will cherry-pick the change to the hydro-devel branch once it passed CI - you don't have to create a PR for that.

@jbohren
Copy link
Member Author

jbohren commented Mar 11, 2014

I will cherry-pick the change to the hydro-devel branch once it passed CI - you don't have to create a PR for that.

Cool. Thanks, Dirk.

@dirk-thomas
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants