add specific exception for time jumping backwards #492

jowave · 2014-08-19T15:09:19Z

reason: ROS shutdown and time jumps should be distinguishable.
ROSTimeMovedBackwardsException inherits from ROSInterruptException
to preserve the API.
fixes #485

jowave · 2014-08-19T15:10:51Z

Recreated the pull request against indigo-devel.
Sorry for the inconvenience.

ros-pull-request-builder · 2014-08-19T15:38:09Z

Can one of the admins verify this patch?

dirk-thomas · 2014-08-19T15:44:53Z

@ros-pull-request-builder

dirk-thomas · 2014-08-19T16:06:31Z

Can you please move the exception class to clients/rospy/src/rospy/exceptions.py where most other exceptions are defined.

Beside this minor detail the patch lgtm.

jowave · 2014-09-15T08:47:42Z

I moved the exception and updated the pull request.

dirk-thomas · 2014-09-17T12:13:28Z

@ros-pull-request-builder

ros-pull-request-builder · 2014-09-17T12:27:08Z

Test failed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/121/

dirk-thomas · 2014-09-18T21:15:43Z

Please update your patch to pass all unit tests. It currently breaks 145 tests.

dirk-thomas · 2014-10-13T19:14:37Z

Please update your patch to pass all unit tests. It currently breaks 145 tests.

ros-pull-request-builder · 2014-10-16T12:33:07Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/131/

jowave · 2014-10-16T13:13:13Z

I have updated the patch. Apparently it passed the tests now.

wjwwood · 2014-10-16T17:23:44Z

+1

dirk-thomas · 2014-10-16T17:29:47Z

clients/rospy/src/rospy/timer.py

@@ -128,7 +131,8 @@ def sleep(duration):
                rostime_cond.wait(0.5)

        if rospy.rostime.get_rostime() < initial_rostime:
-            raise rospy.exceptions.ROSInterruptException("ROS time moved backwards")
+            rospy.core.logerr("ROS time moved backwards")


Can you please include the time offset in the error message so that the user has a better idea how much the time jumped.

jowave · 2014-10-17T08:14:11Z

I added the time offset in the error message and a small test for the exception.

ros-pull-request-builder · 2014-10-18T00:50:29Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/134/

tlind · 2014-10-18T15:04:15Z

I am worried that commit d934e53 might break existing code which is trying to catch the exception. Since there was previously no specific exception for this case, the only way of specifically catching the time-moved-backwards issue was to check the exception message, e.g.:

try:
    r.sleep()
except rospy.exceptions.ROSInterruptException as e:
    if e.message == "ROS time moved backwards":
        rospy.logwarn("Time has moved backwards (probably ROS clock has been reset)!")

However, it seems that supplying the time offset as an additional argument to the exception constructor will break that message:

>>> print rospy.exceptions.ROSInterruptException("ROS time moved backwards").message
ROS time moved backwards
>>> print rospy.exceptions.ROSInterruptException("ROS time moved backwards", -10.0).message
[empty string]

Might it be a better idea to only rospy.logerr() the time offset, and leave the exception message as-is? Or add an additional field to the exception class which contains the time offset?

EDIT: I just read that Exception.message seems to be deprecated since 2.6, though I've been using it a few times and never saw a warning anywhere. Not sure how much other code depends on it. The correct way seems to be to use e.args[0], which returns the same result in both cases.

dirk-thomas · 2014-10-18T17:27:01Z

@tlind That is a good point.

@jowave Can you please update the new exception class to store the time offset in a member variable and keep the message of the exception as-is.

ros-pull-request-builder · 2014-10-20T10:53:46Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/135/

jowave · 2014-10-20T14:58:30Z

I modified the code to work with Exception.message and added a test to check for the right message.

dirk-thomas · 2014-10-22T01:11:08Z

test/test_rospy/test/unit/test_rospy_exceptions.py

+        except ROSInterruptException as e:
+            # ensure the message is not changed, because old code may check it
+            self.assertEqual("ROS time moved backwards", e.message)
+            pass


This line should be obsolete.

dirk-thomas · 2014-10-22T01:11:33Z

Please squash your commits after removing the unnecessary pass line.

reason: ROS shutdown and time jumps should be distinguishable. ROSTimeMovedBackwardsException inherits from ROSInterruptException to preserve the API. fixes #485

ros-pull-request-builder · 2014-10-23T12:03:41Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/138/

dirk-thomas · 2014-10-23T20:34:35Z

Thank you for you efforts on this. Merging...

add specific exception for time jumping backwards

jowave · 2014-10-24T07:09:39Z

You're welcome. Thank you too.

Edit: Hmm, the ros-pull-request-builder is answering. Should i not have replied to a closed pull request?

dirk-thomas · 2014-10-24T07:16:12Z

No worries. The Jenkins check is still experimental. We hopefully can improve that in the future.

peci1 · 2017-04-05T11:51:04Z

clients/rospy/src/rospy/timer.py

@@ -128,7 +131,9 @@ def sleep(duration):
                rostime_cond.wait(0.5)

        if rospy.rostime.get_rostime() < initial_rostime:
-            raise rospy.exceptions.ROSInterruptException("ROS time moved backwards")
+            time_jump = (initial_rostime - rospy.rostime.get_rostime()).to_sec()
+            rospy.core.logerr("ROS time moved backwards: %ss", time_jump)


Is it really needed to log the exception? I use ROS nodes together with Gazebo, where restarting the time is quite normal, and these messages start being annoying. The previous behavior also did not log the exception.

I agree with you that the log message is not strictly necessary. On the other hand this code has been integrated over two years ago and since then been released in three ROS distributions. Since no concerns were raised in the meantime I don't see the log message as a problem either.

Please feel free to create a pull request against the latest development branch (currently lunar-devel).

The log message doesn't make sense IMO, because there is still the exception being thrown. So the user can either catch it (and then he knows best whether it is an error or not), or he'll not catch it, and then he'll see the message printed in the stack trace. This is a followup of comment ros#492 (comment) . My use-case: I have a ROS node running alongside Gazebo, and resetting Gazebo always prints this error to the log, even if it is no error for me, since I expect the time to be restarted.

add specific exception for time jumping backwards

The log message doesn't make sense IMO, because there is still the exception being thrown. So the user can either catch it (and then he knows best whether it is an error or not), or he'll not catch it, and then he'll see the message printed in the stack trace. This is a followup of comment ros#492 (comment) . My use-case: I have a ROS node running alongside Gazebo, and resetting Gazebo always prints this error to the log, even if it is no error for me, since I expect the time to be restarted.

dirk-thomas force-pushed the indigo-devel branch from 4533777 to ebd9e49 Compare October 2, 2014 17:44

dirk-thomas reviewed Oct 16, 2014
View reviewed changes

dirk-thomas reviewed Oct 22, 2014
View reviewed changes

add specific exception for time jumping backwards

ff96b0f

reason: ROS shutdown and time jumps should be distinguishable. ROSTimeMovedBackwardsException inherits from ROSInterruptException to preserve the API. fixes #485

dirk-thomas added a commit that referenced this pull request Oct 23, 2014

Merge pull request #492 from jowave/rospy_sleep_time_backwards

4d2b423

add specific exception for time jumping backwards

dirk-thomas merged commit 4d2b423 into ros:indigo-devel Oct 23, 2014

jowave deleted the rospy_sleep_time_backwards branch November 26, 2014 07:44

peci1 reviewed Apr 5, 2017

View reviewed changes

peci1 mentioned this pull request Apr 6, 2017

Removed "ROS time moved backwards" log message #1027

Merged

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017

Merge pull request ros#492 from jowave/rospy_sleep_time_backwards

afde5f5

add specific exception for time jumping backwards

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add specific exception for time jumping backwards #492

add specific exception for time jumping backwards #492

jowave commented Aug 19, 2014

jowave commented Aug 19, 2014

ros-pull-request-builder commented Aug 19, 2014

dirk-thomas commented Aug 19, 2014

dirk-thomas commented Aug 19, 2014

jowave commented Sep 15, 2014

dirk-thomas commented Sep 17, 2014

ros-pull-request-builder commented Sep 17, 2014

dirk-thomas commented Sep 18, 2014

dirk-thomas commented Oct 13, 2014

ros-pull-request-builder commented Oct 16, 2014

jowave commented Oct 16, 2014

wjwwood commented Oct 16, 2014

dirk-thomas Oct 16, 2014

jowave commented Oct 17, 2014

ros-pull-request-builder commented Oct 18, 2014

tlind commented Oct 18, 2014

dirk-thomas commented Oct 18, 2014

ros-pull-request-builder commented Oct 20, 2014

jowave commented Oct 20, 2014

dirk-thomas Oct 22, 2014

dirk-thomas commented Oct 22, 2014

ros-pull-request-builder commented Oct 23, 2014

dirk-thomas commented Oct 23, 2014

jowave commented Oct 24, 2014

dirk-thomas commented Oct 24, 2014

peci1 Apr 5, 2017

dirk-thomas Apr 5, 2017

add specific exception for time jumping backwards #492

add specific exception for time jumping backwards #492

Conversation

jowave commented Aug 19, 2014

jowave commented Aug 19, 2014

ros-pull-request-builder commented Aug 19, 2014

dirk-thomas commented Aug 19, 2014

dirk-thomas commented Aug 19, 2014

jowave commented Sep 15, 2014

dirk-thomas commented Sep 17, 2014

ros-pull-request-builder commented Sep 17, 2014

dirk-thomas commented Sep 18, 2014

dirk-thomas commented Oct 13, 2014

ros-pull-request-builder commented Oct 16, 2014

jowave commented Oct 16, 2014

wjwwood commented Oct 16, 2014

dirk-thomas Oct 16, 2014

Choose a reason for hiding this comment

jowave commented Oct 17, 2014

ros-pull-request-builder commented Oct 18, 2014

tlind commented Oct 18, 2014

dirk-thomas commented Oct 18, 2014

ros-pull-request-builder commented Oct 20, 2014

jowave commented Oct 20, 2014

dirk-thomas Oct 22, 2014

Choose a reason for hiding this comment

dirk-thomas commented Oct 22, 2014

ros-pull-request-builder commented Oct 23, 2014

dirk-thomas commented Oct 23, 2014

jowave commented Oct 24, 2014

dirk-thomas commented Oct 24, 2014

peci1 Apr 5, 2017

Choose a reason for hiding this comment

dirk-thomas Apr 5, 2017

Choose a reason for hiding this comment