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

globalTimeStamp - going to idle #4073

Merged
merged 13 commits into from
Jun 5, 2019
Merged

Conversation

doronhi
Copy link
Contributor

@doronhi doronhi commented May 26, 2019

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

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() {};
Copy link
Collaborator

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++;
Copy link
Collaborator

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.

@@ -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.
Copy link
Collaborator

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

Copy link
Contributor

@dorodnic dorodnic left a 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)
Copy link
Contributor

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

@ev-mp ev-mp changed the base branch from master to development May 28, 2019 14:35
@ev-mp ev-mp merged commit 50269a3 into IntelRealSense:development Jun 5, 2019
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.

3 participants