Add volatile flag to options #12431
Add volatile flag to options #12431
Conversation
| @@ -38,6 +38,7 @@ namespace rs2 | |||
| { | |||
| option.range = options->get_option_range(opt); | |||
| option.read_only = options->is_option_read_only(opt); | |||
| option._volatile = options->is_option_volatile( opt ); | |||
There was a problem hiding this comment.
Why the _ when all other fields doen't have it? looks weird
There was a problem hiding this comment.
volatile is a saved keyword. I thought about adding _ prefix to all members, as is the current convention, but did not want to make too big a change in this PR. I preferred this over is_volatile because verb is a function notation.
There was a problem hiding this comment.
Got it :)
If we make additional changes please consider adding a comment adding _ prefix due to volatile is a saved keyword
at the parameter definition?
There was a problem hiding this comment.
Sure.
If you prefer I can make a commit that adds the prefix to all members :-)
| * \param[out] error if non-null, receives any error that occurs during this call, otherwise, errors are ignored | ||
| * \return true if option is volatile | ||
| */ | ||
| int rs2_is_option_volatile( const rs2_options * options, rs2_option option, rs2_error ** error ); |
There was a problem hiding this comment.
The major concern is if we add this API and we have many options that are volatile and can be changed by other, it will return false on them.
Even when they can be changed..
Thoughts?
There was a problem hiding this comment.
This is an addition for future product, currently we didn't have the need so all options are "not volatile".
I looked at the options, didn't see many that might be changed to "volatile". Maybe "inter cam sync mode" or "thermal compensation" that are more HW/FW related. If there will be a need we can always mark some current options as "volatile".
We can present this to the SW architect and ask his opinion if other options can benefit from this addition.
| @@ -9,6 +9,7 @@ void init_options(py::module &m) { | |||
| py::class_<rs2::options> options(m, "options", "Base class for options interface. Should be used via sensor or processing_block."); // No docstring in C++ | |||
| options.def("is_option_read_only", &rs2::options::is_option_read_only, "Check if particular option " | |||
| "is read only.", "option"_a) | |||
| .def("is_option_volatile", &rs2::options::is_option_volatile, "Check if particular option is volatile.", "option"_a) | |||
There was a problem hiding this comment.
Lets add a python test that verify this API works.
We can think of something
|
As discussed - volatile property should be internal and not in the API. LibRS will periodically query volatile options. The API should offer a notification mechanism on value change. |
Some options that are not read only might change without us setting them.
We want the viewer to query them periodically to reflect their current value, like it does with read only options.