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.

Sign up for GitHub

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

Jump to bottom

updated correct timestamp GazeboYarpLaserSensorDriver #604

Merged
merged 4 commits into from
Jan 25, 2022
Merged

updated correct timestamp GazeboYarpLaserSensorDriver #604

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
randaz81
randaz81 approved these changes Jan 13, 2022
View reviewed changes
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 •
edited
Loading

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
Reviewers

@randaz81 randaz81 randaz81 approved these changes

@traversaro traversaro traversaro approved these changes

Assignees

@randaz81 randaz81

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ste93 @randaz81 @traversaro