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

added parameter to stop client libraries from generating topics list #1241

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

trobertson-cpr
Copy link
Contributor

The cached parameter /rosout_disable_topics_generation stops rospy and roscpp from generating the list of topics and sending them over the network stack as part of their log messages. It does not stop rosout from printing an empty topics list.

@mikepurvis
Copy link
Member

Adding this Clearpath's testing, appreciating that there will be discussion about naming of the new param.

@dirk-thomas
Copy link
Member

The usual comment about ABI breakage applies. I created #1270 to find a consensus on this in order to avoid this "back-and-forth-negotiation" on each patch.

@mikepurvis mikepurvis changed the base branch from lunar-devel to melodic-devel April 21, 2018 21:04
@mikepurvis
Copy link
Member

We've been running this in production since November, merging.

@mikepurvis mikepurvis merged commit d21a33c into ros:melodic-devel Apr 23, 2018
@mikepurvis mikepurvis deleted the NIM-4214 branch April 23, 2018 20:30
# check parameter server/cache for omit_topics flag
# the same parameter is checked in rosout_appender.cpp for the same purpose
# parameter accesses are cached automatically in python
if rospy.has_param("/rosout_disable_topics_generation"):

Choose a reason for hiding this comment

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

This adds 1 or 2 XMLRPC calls to each log action (?)

Copy link
Member

Choose a reason for hiding this comment

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

The comment indicates that the response is cached, but I agree that this is super bad if it's not the case.

Choose a reason for hiding this comment

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

Looks like it got disabled:

if 1: # disable param cache


// 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_);

Choose a reason for hiding this comment

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

this subscribes almost all roscpp-based nodes to param server updates

Copy link

@mathias-luedtke mathias-luedtke May 23, 2018

Choose a reason for hiding this comment

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

getCached calls ROS_DEBUG_NAMED under the hood. Has this been tested with debug log level?

@mathias-luedtke
Copy link

As a tradeoff this feature could only be activated if the param is present at node start-up.

if not disable_topics_:
topics = get_topic_manager().get_topics()
else:
topics = ""

Choose a reason for hiding this comment

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

Should be []

Copy link
Member

Choose a reason for hiding this comment

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

+1

@k-okada
Copy link
Contributor

k-okada commented Jan 27, 2019

As @ipa-mdl pointed out before, this patch outputs a lot of warning message with a debug log level.

user@ubuntu-18:~/roscpp_ws/src/roscpp$ rosrun roscpp_tutorials talker 
log4cxx: Could not read configuration file [/home/user/roscpp_ws/src/ros_comm/tools/rosconsole/config/rosconsole.config].
[ INFO] [1548596827.350993252]: hello world 0
[ INFO] [1548596827.451063670]: hello world 1
[ INFO] [1548596827.551035597]: hello world 2
[ INFO] [1548596827.651068512]: hello world 3
[ INFO] [1548596827.751063854]: hello world 4
[ INFO] [1548596827.851048033]: hello world 5
[ INFO] [1548596827.951058002]: hello world 6
[ INFO] [1548596828.051039711]: hello world 7
[ INFO] [1548596828.151042505]: hello world 8
[ INFO] [1548596828.251036114]: hello world 9
[ INFO] [1548596828.351057021]: hello world 10 <<<<<< called “rosconsole set talker  ros debug”
[DEBUG] [1548596828.393239467]: Connection::drop(2)
Warning: recursive print statement has occurred.  Throwing out recursive print.
[DEBUG] [1548596828.393360576]: TCP socket [11] closed
Warning: recursive print statement has occurred.  Throwing out recursive print.
[DEBUG] [1548596828.393418968]: Connection::drop(0)
Warning: recursive print statement has occurred.  Throwing out recursive print.
[DEBUG] [1548596828.393475843]: Connection::drop(2)
Warning: recursive print statement has occurred.  Throwing out recursive print.
[DEBUG] [1548596828.393530428]: Connection::drop(2)
Warning: recursive print statement has occurred.  Throwing out recursive print.
[ INFO] [1548596828.451058100]: hello world 11
Warning: recursive print statement has occurred.  Throwing out recursive print.
[ INFO] [1548596828.551057791]: hello world 12
Warning: recursive print statement has occurred.  Throwing out recursive print.
[ INFO] [1548596828.651057262]: hello world 13
Warning: recursive print statement has occurred.  Throwing out recursive print.
[ INFO] [1548596828.751056654]: hello world 14
Warning: recursive print statement has occurred.  Throwing out recursive print.
^C[ INFO] [1548596828.851131428]: hello world 15

And if we activate this feature on the startup, then the warnings disappears

user@ubuntu-18:~/roscpp_ws/src/roscpp$ git diff
diff --git a/clients/roscpp/src/libros/rosout_appender.cpp b/clients/roscpp/src/libros/rosout_appender.cpp
index f22cd9596..3e7597691 100644
--- a/clients/roscpp/src/libros/rosout_appender.cpp
+++ b/clients/roscpp/src/libros/rosout_appender.cpp
@@ -47,6 +47,7 @@ namespace ros
 
 ROSOutAppender::ROSOutAppender()
 : shutting_down_(false)
+, disable_topics_(false)
 , publish_thread_(boost::bind(&ROSOutAppender::logThread, this))
 {
   AdvertiseOptions ops;
@@ -54,6 +55,10 @@ ROSOutAppender::ROSOutAppender()
   ops.latch = true;
   SubscriberCallbacksPtr cbs(boost::make_shared<SubscriberCallbacks>());
   TopicManager::instance()->advertise(ops, cbs);
+
+  // 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_);
 }
 
 ROSOutAppender::~ROSOutAppender()
@@ -104,10 +109,6 @@ void ROSOutAppender::log(::ros::console::Level level, const char* str, const cha
   msg->function = function;
   msg->line = line;
   
-  // 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 (!disable_topics_){
     this_node::getAdvertisedTopics(msg->topics);
   }

Or check hasParam before retrive 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);

How do you think? @dirk-thomas @mikepurvis

@dirk-thomas
Copy link
Member

Please create a PR with the proposed changes. Comments on a closed tickets will likely get lost.

k-okada added a commit to k-okada/ros_comm that referenced this pull request Jan 27, 2019
…nitialization time

ros#1241 outputs a lot of warning message with a debug log level.
c.f. ros#1241 (comment)
flixr pushed a commit to flixr/ros_comm that referenced this pull request Jan 12, 2020
…nitialization time

ros#1241 outputs a lot of warning message with a debug log level.
c.f. ros#1241 (comment)
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.

5 participants