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

Simplify time-service usage #12219

Merged
merged 8 commits into from
Oct 1, 2023
Merged

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Sep 23, 2023

The class time_service was polymorphic before, and "created" by the backend.
The idea was nice, but in reality usage was mixed between the backend and the environment. I tried centralizing it in context but ran into difficulties requiring refactoring in other places, and in the end decided that it just wasn't worth the effort and code complexity:

  • The backends (all of them) ended up using the default one in the environment (which is why mixing usage worked), so the polymorphism wasn't in use
  • Its place in environment is really not good, as that's really used for the extrinsics information and we were abusing it

The code is now much simpler and easier to read/write. No weird backend usage in weird places, etc.

Tracked on [LRS-889]

@maloel maloel requested a review from ev-mp September 23, 2023 08:51
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.

Is it possible that the polymorphism was used in Software Device?

@@ -305,8 +305,7 @@ namespace librealsense

rs2_time_t ds_timestamp_reader::get_frame_timestamp(const std::shared_ptr<frame_interface>& frame)
{
std::lock_guard<std::recursive_mutex> lock(_mtx);
return _ts->get_time();
return time_service::get_time();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the reason for the mutex originally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I don't know... but now that you mention it, I'll check if the mutex can be removed, too...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked: the mutex is now between the frame counter changes (frame_timestamp_reader::get_frame_counter() when a frame is received and ::reset() when a sensor is stopped). Before it included getting the current time (also on frame received). Maybe because frames can be received from parallel threads? Otherwise there'd be no reason for the mutex for counters, either...

@maloel
Copy link
Collaborator Author

maloel commented Sep 28, 2023

Is it possible that the polymorphism was used in Software Device?

I don't see how... there's no way to set the time-service per device. And I had to touch no code in the software device itself. I had 0 problems removing the polymorphism, and it was only exposed internally, inside librealsense.

@maloel maloel requested a review from ev-mp September 28, 2023 17:22
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.

A welcomed simplification.
LGTM

@maloel maloel merged commit eee0cf0 into IntelRealSense:development Oct 1, 2023
15 checks passed
@maloel maloel deleted the no-time-service branch October 1, 2023 05:31
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