Enable service unavailable log in rospy wait_for_service #1026
Conversation
| @@ -136,7 +136,7 @@ def contact_service(resolved_name, timeout=10.0): | |||
| except: # service not actually up | |||
| if first: | |||
| first = False | |||
| rospy.core.logerr("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | |||
| rospy.core.loginfo("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | |||
There was a problem hiding this comment.
This message would be printed every 0.3 s. I think that is too verbose. Please consider using a throttled log call (http://wiki.ros.org/rospy/Overview/Logging#Logging_Periodically). Maybe once every 10s is ok?
| @@ -154,7 +154,7 @@ def contact_service(resolved_name, timeout=10.0): | |||
| except: # service not actually up | |||
| if first: | |||
| first = False | |||
| rospy.core.logerr("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | |||
| rospy.core.logwarn("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | |||
There was a problem hiding this comment.
When no timeout is specified the default value is 10s. So the message would get printed repeatedly - not only once. Therefore I would argue both log levels should be the same. Not sure if info or warn make more sense though.
There was a problem hiding this comment.
It gets printed only once because of the first flag.
There was a problem hiding this comment.
Does the user get any feedback once the service has become available?
There was a problem hiding this comment.
No, can add that.
There was a problem hiding this comment.
Anyways, I two ways for this (and also the other comment above):
- Publish a warning once when we failed to contact service and then publish a info when it becomes available
- Publish a throttled info continuously (and maybe publish a info when it becomes available?)
1. is easy to miss, but 2. might be a bit spamy. I personally prefer spamy over missing stuff, so I would go for two (with the "now available" info)
Conflicts: clients/rospy/src/rospy/impl/tcpros_service.py
| first = False | ||
| rospy.core.logerr("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | ||
| contact_failed = True | ||
| rospy.core.loginfo("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) |
There was a problem hiding this comment.
Should this call logwarn instead?
| first = False | ||
| rospy.core.logerr("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) | ||
| contact_failed = True | ||
| rospy.core.logwarn("wait_for_service(%s): failed to contact [%s], will keep trying"%(resolved_name, uri)) |
There was a problem hiding this comment.
With #1091 being merged this could be updated to call rospy.core.logwarn_throttle(10, "...") to reduce the verbosity of this line.
Same for the other warning above.
|
Thank you for the patch and iterating on it. |
|
This patch introduced a bug: (https://travis-ci.org/ros-controls/ros_control/jobs/295104386#L4754) Is |
|
@ipa-mdl Thank you for reporting this. Can you please confirm that #1213 fixes the problem. |
There is no log published if the service is down and we are waiting for it.
Enables (the code was already there) logging a message.
I changed the error message to a info in case we have a timeout and a warning in case we have no timeout.
In case of no timeout this will be the only message telling you that you are stuck in an endless loop in case the service does not show up. That's why I thought a warning would be better than just a log.