-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
globalTimeStamp - going to idle #4073
Conversation
src/device.h
Outdated
@@ -78,6 +78,8 @@ namespace librealsense | |||
|
|||
virtual bool contradicts(const stream_profile_interface* a, const std::vector<stream_profile>& others) const override; | |||
virtual double get_device_time(); // Returns time in miliseconds. | |||
virtual void start_time_keeper() {}; |
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 would suggest to refactor start/stop_time_keeper
into enable_time_keeper(bool)
.
The start/stop_time_keeper
operations are internal to the device's flow (i.e. bound to the start/stop device operations implicitly) and shall be not exposed in the class public API.
On the other hand - enable_time_keeper
will be connected to the user's dev.set_option(...) operation
@@ -111,9 +112,26 @@ namespace librealsense | |||
|
|||
void time_diff_keeper::start() | |||
{ | |||
_users_count++; |
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.
Start/Stop shall be muted-protected as the invocation can be done from multiple threads.
src/media/record/record_device.h
Outdated
@@ -47,6 +47,9 @@ namespace librealsense | |||
void tag_profiles(stream_profiles profiles) const override { m_device->tag_profiles(profiles); } | |||
bool compress_while_record() const override { return true; } | |||
bool contradicts(const stream_profile_interface* a, const std::vector<stream_profile>& others) const override { return m_device->contradicts(a, others); } | |||
double get_device_time() { return 0; }; // Returns time in miliseconds. |
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.
It makes sense to incorporate units into the signature get_device_time_ms()
to avoid ambiguity
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.
In general, I don't like how time_keeper APIs are starting to pollute device interface. Perhaps it can be better to extract it to an independent interface (and possibly base implementation) that relevant devices can inherit from?
_active_object.start(); | ||
} | ||
|
||
void time_diff_keeper::stop() | ||
{ | ||
if (_users_count <= 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.
Couple of options -
a. Use regular uint instead of an atomic and wrap read/writes with a mutex
b. Use atomic compare-exchange pattern
disable global_timestamp_reader calls to device if it is not started.
In order to allow device to fall back to idle mode this PR only enables periodic calls to device if there is an open sensor.
Tracked on: DSO-12794