-
Notifications
You must be signed in to change notification settings - Fork 774
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
Correct the timestamp used by the camera (kinetic-devel) #540
Conversation
… generated the sensor data, so ought to be used. updateRate doesn't seem that useful. The other cameras need similar fixes to have the proper timestamps.
…ntly outdated, will fix them similar to how the regular camera was fixed.
…lso reuse the same update code
…ght timestamp #408- appears to be working (though only looking at horizon) but getting these sdf errors: Error [SDF.cc:789] Missing element description for [pointCloudTopicName] Error [SDF.cc:789] Missing element description for [depthImageCameraInfoTopicName] Error [SDF.cc:789] Missing element description for [pointCloudCutoff]
…to receive the messages (still well less than 2.0 seconds). Also all the tests can be run with run_tests_gazebo_plugins_rostest but only with the -j1 flag #409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See minor comments
# if GAZEBO_MAJOR_VERSION >= 7 | ||
common::Time sensor_update_time = this->parentSensor_->LastMeasurementTime(); | ||
# else | ||
common::Time sensor_update_time = this->parentSensor_->GetLastMeasurementTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a use case when the kinetic-devel branch will be used with a version of Gazebo less than 7? Seems this compiler directive can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with removing support for gazebo < 7 in the kinetic branch but let's do it a single PR and do not spread removals all over the place to keep PRs clean.
&CameraTest::imageCallback, | ||
dynamic_cast<CameraTest*>(this)); | ||
|
||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explain when this test should be re-enabled (link to issue) or remove this commented out code
test/camera/camera.test | ||
test/camera/camera.cpp) | ||
target_link_libraries(camera-test ${catkin_LIBRARIES}) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome tests!
The build is failing because of a CMake warning from gazebo_ros_control:
That I believe I fixed in my PR :) |
{ port of pull request #410 }
Fix for #408
The last measurement time is the time that gazebo generated the sensor data, so ought to be used. The other cameras need similar fixes to have the proper timestamps. There is a test to show that the timestamp is good, but issue #409 has lead me to comment it out for now.
There probably ought to be a separate issue about what updateRate is supposed to be for, and if it isn't useful remove it (from all the sensors that have it) in another PR.