Skip to content
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

updated correct timestamp GazeboYarpLaserSensorDriver #604

Merged
merged 4 commits into from
Jan 25, 2022

Conversation

ste93
Copy link
Contributor

@ste93 ste93 commented Jan 13, 2022

since Lidar2DDeviceBase has a timestamp inside and lasersensor inherits from it, I would suggest use the timestamp from taht class. It has also an api getLastInputStamp () to get that stamp.

@ste93
Copy link
Contributor Author

ste93 commented Jan 13, 2022

@randaz81

@randaz81 randaz81 self-assigned this Jan 13, 2022
@randaz81 randaz81 self-requested a review January 13, 2022 16:33
Copy link
Member

@randaz81 randaz81 left a comment

Choose a reason for hiding this comment

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

thank you, this was an important bugfix

@randaz81
Copy link
Member

randaz81 commented Jan 13, 2022

Additionally, since yarp 3.6 has been released, I think that we can safely remove the dependency from IPreceiselyTimed @traversaro https://github.com/robotology/gazebo-yarp-plugins/pull/604/files#diff-a33e9fff4431518b3add5ded498eae3af09ef01c83956d6fc6abd0a70de405b8R67-R70

REASON: in general IPreceiselyTimed interface should be avoided as much as possible, and possibly deprecated. Timestamps must be obtained together with the data using a single call to a get method(). Otherwise, it cannot be guaranteed that the timestamp is really associated with the retrieved data. (Threads can be interrupted etc.)

@traversaro
Copy link
Member

Yes, feel free to remove the inheritance from IPreceiselyTimed and require YARP 3.6 as suggested by @randaz81 , thanks!

@ste93 ste93 force-pushed the fix_timestamp_lasersensor_master branch from 5339133 to 3630a4c Compare January 25, 2022 14:26
@ste93
Copy link
Contributor Author

ste93 commented Jan 25, 2022

@randaz81 updated, for me it can be merged. FYI @traversaro

@traversaro
Copy link
Member

@randaz81 updated, for me it can be merged. FYI @traversaro

To require YARP 3.6 you need to modify the CMake in https://github.com/robotology/gazebo-yarp-plugins/blob/master/CMakeLists.txt#L49 . Could you also update the CHANGELOG?

@ste93
Copy link
Contributor Author

ste93 commented Jan 25, 2022

related to #598

@traversaro
Copy link
Member

Thanks @ste93 .

@traversaro traversaro merged commit bb5afed into robotology:master Jan 25, 2022
@ste93
Copy link
Contributor Author

ste93 commented Jan 26, 2022

thanks @traversaro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants