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

move check parameter for /rosout_disable_topics_generation only for initialization time #1597

Closed
wants to merge 1 commit into from

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Jan 27, 2019

PR #1241 outputs a lot of warning message with a debug log level.
c.f. #1241 (comment)

other workaround is to check hasParam before retriving a cached parameter.

diff --git a/clients/roscpp/src/libros/rosout_appender.cpp b/clients/roscpp/src/libros/rosout_appender.cpp
index f22cd9596..c7adaca73 100644
--- a/clients/roscpp/src/libros/rosout_appender.cpp
+++ b/clients/roscpp/src/libros/rosout_appender.cpp
@@ -106,7 +106,8 @@ void ROSOutAppender::log(::ros::console::Level level, const char* str, const cha
   
   // check parameter server/cache for omit_topics flag
   // the same parameter is checked in rosout.py for the same purpose
-  ros::param::getCached("/rosout_disable_topics_generation", disable_topics_);
+  if (ros::param::has("/rosout_disable_topics_generation"))
+    ros::param::getCached("/rosout_disable_topics_generation", disable_topics_);
 
   if (!disable_topics_){
     this_node::getAdvertisedTopics(msg->topics);

…nitialization time

ros#1241 outputs a lot of warning message with a debug log level.
c.f. ros#1241 (comment)
Copy link
Member

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

Moving the logic from the log function to the constructor changes behavior. Without the patch it is possible to set the parameter afterwards to influence the behavior at runtime. The patch removes that possibility. Therefore I think this shouldn't be applied as is.

@paulbovbel

This comment has been minimized.

@flixr
Copy link
Contributor

flixr commented Jan 12, 2020

While this patch changes the behavior, I would still be in favor or merging this.
It is the lesser of two evils...

@dirk-thomas
Copy link
Member

Resolved via #1823 instead.

@dirk-thomas dirk-thomas closed this Feb 3, 2020
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.

None yet

4 participants