-
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
Can update options automatically, get notification of value change #12483
Conversation
52396d1
to
2d10190
Compare
954d423
to
6730ac6
Compare
…wake from sleep when stopping
src/core/options-watcher.cpp
Outdated
@@ -16,6 +16,7 @@ options_watcher::options_watcher( std::chrono::milliseconds update_interval ) | |||
|
|||
options_watcher::~options_watcher() | |||
{ | |||
_alive = false; |
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.
Don't need to clear _options
inside a mutex any more; don't need the try-catch, either.
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.
Not locking before clear. try-catch needed because clear and join might throw.
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.
But you don't need the clear()...
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.
And if join() might throw, please catch it inside stop()...
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 prefer to clear explicitly, but locking is not needed.
Also found out clear has a noexcept
overload. Catching join
errors in stop
.
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'm sorry, I don't agree - if you clear, then a lock is necessary.
You don't need to clear.
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.
Removed
f499e19
to
cd0b14e
Compare
src/media/playback/playback_sensor.h
Outdated
@@ -79,7 +79,8 @@ namespace librealsense | |||
|
|||
rsutils::subscription register_options_changed_callback( options_watcher::callback && cb ) override | |||
{ | |||
throw not_implemented_exception( "Registering options value changed callback is not implemented for this sensor" ); | |||
LOG_ERROR( "Registering options value changed callback is not implemented for this sensor" ); |
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.
Why? I don't think this is good...?
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.
If we throw then the viewer cannot play recorded files
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.
Reverted change. Viewer handling the exception
de5c1c8
to
45df20e
Compare
…tions setting callback.
45df20e
to
08f25fb
Compare
8a431c8
to
9bbe102
Compare
Added option_updater class that handles auto query of registered options.
Added C and CPP API to register a value change notification callback .
Added Python API and unit test
Also fixed bug in cpp unit tests - on
restart_first_device_and_return_depth_sensor
old device was used after HW reset instead of new device.