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

add -d option for 'rosmaster' to output LOG_API#1173

Closed
k-okada wants to merge 1 commit intoros:lunar-develfrom
k-okada:add_debug_log_api
Closed

add -d option for 'rosmaster' to output LOG_API#1173
k-okada wants to merge 1 commit intoros:lunar-develfrom
k-okada:add_debug_log_api

Conversation

@k-okada
Copy link
Copy Markdown
Contributor

@k-okada k-okada commented Sep 30, 2017

Sometimes, we had seen the CPU Usage of rosmaster is quit high (more than 50%). Mostly, this because of calling service call multiple times, without using persistent or put ServiceProxy within the loop.
To find the node calling these services, enabling LOG_API is very helpful, but currently we do not have to modify source code at this moment ( https://github.com/ros/ros_comm/blob/lunar-devel/tools/rosmaster/src/rosmaster/master_api.py#L137 )

rosmaster --core -p 11311 -d __log:=rosmaster.log
tail -f rosmaster.log

I'm not sure if we should pass this option from roscore -> roslaunch -> rosmaster like 08f6bf0#diff-8517d9bf19c03b642c67e5d937dae851

@clalancette
Copy link
Copy Markdown
Contributor

The code looks OK to me. Since this is a deep debugging option, it seems to me like it is fine to just have it on rosmaster for now, and then add it to roscore later. @dirk-thomas does that seem reasonable to you?

@dirk-thomas
Copy link
Copy Markdown
Member

I'm not sure if we should pass this option from roscore -> roslaunch -> rosmaster

How are you currently using the new option? Is that sufficient for you? If not the argument good be passed along the same way.

help="override the socket connection timeout (in seconds).", metavar="TIMEOUT")
parser.add_option("-d", "--debug",
dest="debug", action="store_true", default=False,
help="print debug information")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the help text (and eventually even the option name) should be more specific to what exactly it is doing, namely enabling logging API calls on the master. Maybe the logging level shouldn't be controlled with the same option. That should be configurable already with the rospy logging config file, no?

@k-okada
Copy link
Copy Markdown
Contributor Author

k-okada commented Oct 5, 2017

@dirk-thomas > How are you currently using the new option? Is that sufficient for you? If not the argument good be passed along the same way.

I'm currently using this option by rosmaster --core -p 11311 -d __log:=rosmaster.log. It's ok for me for now, because this is a deep debugging case as @clalancette noted. But, of course, if many people willing to check this deep debugging message, we should expose this option to roslaunch level.

namely enabling logging API calls on the master. Maybe the logging level shouldn't be controlled with the same option. That should be configurable already with the rospy logging config file, no?

I tried to enable this message by rospy logging features, but as far as I understand, it turns out that rosmaster is not 'rosnode', it did not run rospy.init_node() so we can not configure through rospy tools. Am I correct?

@dirk-thomas
Copy link
Copy Markdown
Member

if many people willing to check this deep debugging message, we should expose this option to roslaunch level.

If you think it would be useful please add this to the current patch. If not I am fine to merge this as-is (after you addressed the pending comment about the option name / help text).

I tried to enable this message by rospy logging features, but as far as I understand, it turns out that rosmaster is not 'rosnode', it did not run rospy.init_node() so we can not configure through rospy tools. Am I correct?

I haven't tried it but that could indeed be the case.

@k-okada
Copy link
Copy Markdown
Contributor Author

k-okada commented Oct 6, 2017

thank you for comments, I have create new PR #1180 , which passes --set-master-logger-level from roscore to rosmaster. Please review when you have time @wjwwood

@dirk-thomas
Copy link
Copy Markdown
Member

Closing in favor of #1180.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants