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

Add more logging to publisher update calls. #979

Merged
merged 3 commits into from
Feb 9, 2017
Merged

Add more logging to publisher update calls. #979

merged 3 commits into from
Feb 9, 2017

Conversation

deng02
Copy link
Contributor

@deng02 deng02 commented Feb 8, 2017

Because the publisher update calls are critical for setting
up topic connections between subscribers and publishers,
have added more detailed logging messages. The publishers
are included so you know what the subscriber was told, as
well as when the call succeeded or failed and if so why.
The rosout topic is filtered out to reduce noise.

Because the publisher update calls are critical for setting
up topic connections between subscribers and publishers,
have added more detailed logging messages.  The publishers
are included so you know what the subscriber was told, as
well as when the call succeeded or failed and if so why.
The rosout topic is filtered out to reduce noise.
@mikepurvis
Copy link
Member

Filtering events for the rosout topics seems a bit arbitrary. I can appreciate that this could be noisy with every single node bringing up a rosout publisher, but I'm not sure it's worth it.

@mikepurvis
Copy link
Member

@ros-pull-request-builder retest this please

@deng02
Copy link
Contributor Author

deng02 commented Feb 9, 2017

The rosout topic is "special" because every node automatically publishes to it and it's brought up automatically by the system, hence the special treatment. So I'd argue less arbitrary and more intentional. However I can agree that special treatment in code is not always desirable and after testing, I don't feel this buys us much in terms of data volume savings. I will remove it.

@dirk-thomas
Copy link
Member

I added the commit 34c57ac which moves some code to keep variables and comments closer to where they are used as well as reduce duplication. Since I only wrote the patch in the GitHub web interface it would be great if you could confirm that it still achieves your goal.

@deng02
Copy link
Contributor Author

deng02 commented Feb 9, 2017

@dirk-thomas Confirmed: output with commit 34c57ac achieves my goal.

@mikepurvis
Copy link
Member

LGTM 👍

@dirk-thomas dirk-thomas merged commit 91dbc09 into ros:kinetic-devel Feb 9, 2017
@dirk-thomas
Copy link
Member

Thank you for the contribution.

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

Successfully merging this pull request may close these issues.

None yet

3 participants