Race condition in time reset for sensors

[osrf-migration]

Original report (archived issue) by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

Sensors may not be successfully reset by a world_control message. There is a race condition between the Sensor::OnControl and World::OnControl functions since they are in different threads. If Sensor::OnControl executes first, then each sensor's lastUpdateTime will be reset to 0. However, a Sensor::Update may run before World::OnControl executes, which may increase the value of lastUpdateTime to the current sim time.

Basically, the sensors shouldn't be reset until the World has been reset, and these OnControl callbacks don't guarantee that.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

I've found a workaround by editing Sensor::Update to not update if the world is paused (see commit to my dev branch) (a9f8370).

With this change, you can get consistent behavior by resetting the world while paused, though resetting while unpaused is still unpredictable.

I haven't done a pull request because I'm not sure how this change would impact other sensor behavior and possibly rendering while paused.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

I've added a failing test to branch issue_236 to illustrate the problem. On my desktop in an xterm, it fails every time that I run it. If I ssh to my own machine, so that I don't have X windows, it fails between 10% and 50% of the time.

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• changed version from "None" to "1.5"

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

I merged branch issue_236 with default to resolve a conflict.

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

see pull request #332

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• changed state from "new" to "resolved"

Fixed in pull request #332

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• changed state from "resolved" to "open"

I just noticed a regression and modified Sensor_TEST.cc in 15c9e12 (branch issue_236_regression) to illustrate the failure. The test on that branches gives the following console output:

Dbg ServerFixture load in 0.1 seconds, timeout after 60 seconds
Dbg default::hokuyo::link::laser loaded with update rate of 30 Hz
Dbg counted 20 messages in 0.681 seconds
Dbg counted 64 messages in 2.178 seconds
Dbg sent reset world message
Dbg world time is now 0.098
Dbg counted 0 messages in 1.995 seconds. Expected[29.925]
/home/scpeters/osrf/gazebo2/gazebo/sensors/Sensor_TEST.cc:134: Failure
Expected: (static_cast<double>(hokuyoMsgCount)) > (updateRate*now * 0.5), actual: 0 vs 29.925
Dbg counted 56 messages in 2.094 seconds
Dbg ServerFixture load in 0.1 seconds, timeout after 60 seconds
Dbg default::hokuyo::link::laser loaded with update rate of 30 Hz
Dbg counted 20 messages in 0.681 seconds
Dbg counted 64 messages in 2.178 seconds
Dbg sent reset world message
Dbg world time is now 0.098
Dbg counted 0 messages in 1.995 seconds. Expected[29.925]
/home/scpeters/osrf/gazebo2/gazebo/sensors/Sensor_TEST.cc:134: Failure
Expected: (static_cast<double>(hokuyoMsgCount)) > (updateRate*now * 0.5), actual: 0 vs 29.925
Dbg counted 56 messages in 2.094 seconds

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• changed state from "open" to "on hold"

On hold until after June 2013.

[osrf-migration]

Original comment by Ian Chen (Bitbucket: Ian Chen, GitHub: iche033).

---

• changed state from "on hold" to "open"

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

See pull request #685

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• changed state from "open" to "resolved"

pull request #685

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• changed state from "resolved" to "open"

this is broken for ImuSensor since lastMeasurementTime is not reset after world resets

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• set assignee_account_id to "557058:5de38267-b118-494c-aa76-4fab35448816"
• set assignee to "scpeters (Bitbucket: scpeters, GitHub: scpeters)"

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

Added failing test in branch issue_236_imu_4.1 (d8317a1).

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

The following kinda works:

diff -r d8317a11c87a09a2091d5b142e5c4e284cedfc90 gazebo/sensors/Sensor.cc
--- a/gazebo/sensors/Sensor.cc	Tue Feb 17 16:27:44 2015 -0800
+++ b/gazebo/sensors/Sensor.cc	Tue Feb 17 16:34:59 2015 -0800
@@ -392,4 +392,5 @@
 {
   boost::mutex::scoped_lock lock(this->mutexLastUpdateTime);
   this->lastUpdateTime = 0.0;
+  this->lastMeasurementTime = 0.0;
 }

but is not a true fix because mutexLastUpdateTime is a private member of the Sensor class and does not protect against the modification of lastMeasurementTime in ImuSensor::UpdateImpl.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

see pull request #1446

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

Even better fix in pull request #1448

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• changed state from "open" to "resolved"

pull request #1448

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• changed state from "resolved" to "closed"