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

global_timestamp_reader: no blocking. #6150

Merged
merged 11 commits into from
Aug 4, 2020

Conversation

doronhi
Copy link
Contributor

@doronhi doronhi commented Mar 29, 2020

prevent calls for get_device_time_ms from frame thread.
continue system_time-hw_time equation through time loop.

base_x = -max_device_time;
else
return false;
LOG_DEBUG("CLinearCoefficients:update_samples_base(" << base_x << ")");
Copy link
Collaborator

@ev-mp ev-mp Mar 29, 2020

Choose a reason for hiding this comment

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

Use _FUNCTION_ macro instead of CLinearCoefficients:update_samples_base


double CLinearCoefficients::get_last_hw_time() const
{
return (_last_values.front()._x);
Copy link
Collaborator

Choose a reason for hiding this comment

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

_last_values can be empty

@@ -161,23 +193,21 @@ namespace librealsense
{
if (!_users_count)
throw wrong_api_call_sequence_exception("time_diff_keeper::update_diff_time called before object started.");
std::lock_guard<std::recursive_mutex> lock(_mtx);
double system_time_start = duration<double, std::milli>(system_clock::now().time_since_epoch()).count();

double sample_hw_time = _device->get_device_time_ms();
double system_time_finish = duration<double, std::milli>(system_clock::now().time_since_epoch()).count();
if (system_time_finish - system_time_start > 2.0)
throw io_exception("get_device_time_ms() took too long (more then 2 mSecs)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be added to log as error/warning. No need for exception here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seems cleaner to use exception then LOG +"return false" since already inside a try-catch section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a recoverable case which does not necessary requires user interaction, as the system can recover from it automatically.
Adding it to log is needed in any case.
Also - I noticed in the latest v2.34.0 under linux the message appears much often than 2 sec in Viewer.

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

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

Also need to startt TmeKeeper earlier, preferable before probe/commit and launching the query asynchronously

{
update_diff_time();
// A time loop happend:
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

…he first 3 seconds.

Delay is currently caused by global_time_stamp_reader blocking the first frame while it is still in an unready condition.
@doronhi doronhi force-pushed the handle_device_ts_delay branch from 3ca4cfa to bc72a88 Compare August 3, 2020 06:55
@doronhi doronhi force-pushed the handle_device_ts_delay branch from e098c89 to 2b409fb Compare August 3, 2020 11:54
@@ -183,15 +213,15 @@ namespace librealsense
_min_command_delay = command_delay;
}
double system_time(system_time_finish - _min_command_delay);
if (sample_hw_time < _last_sample_hw_time)
std::lock_guard<std::recursive_mutex> lock(_read_mtx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that mutex shall be applied in all places the internal buffer is being evaluated. For review

//LOG_DEBUG("time_diff_keeper::call reset()");
_coefs.reset();
double un_used;
_coefs.update_samples_base(sample_hw_time, un_used);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second parameter is not used, can it be dropped?

@@ -216,30 +246,28 @@ namespace librealsense

void time_diff_keeper::polling(dispatcher::cancellable_timer cancellable_timer)
{
update_diff_time();
unsigned int time_to_sleep = _poll_intervals_ms + _coefs.is_full() * (9 * _poll_intervals_ms);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls add explanation comment here

REQUIRE_NOTHROW(subdevice.stop());
REQUIRE_NOTHROW(subdevice.close());

lock.unlock();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do it after cv is unblocked.

CAPTURE(median_diff_gts_on);
CAPTURE(median_diff_gts_off);

REQUIRE(median_diff_gts_on > 0.5*median_diff_gts_off);
Copy link
Collaborator

Choose a reason for hiding this comment

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

During 10 second Gyto will generate 4000 frames. Imo the median will not suffice.

@doronhi doronhi force-pushed the handle_device_ts_delay branch from 2b409fb to 6e06078 Compare August 4, 2020 12:23
@doronhi doronhi force-pushed the handle_device_ts_delay branch from 6e06078 to d29667f Compare August 4, 2020 12:30
@ev-mp ev-mp merged commit d863745 into IntelRealSense:development Aug 4, 2020
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.

2 participants