DDS device signals, bulk query-option & misc #12517
Conversation
maloel
force-pushed
the
device-signals
branch
from
0c5e21d
to
171f6b0
Compare
maloel
force-pushed
the
device-signals
branch
from
171f6b0
to
d47b6ae
Compare
| @@ -442,6 +431,23 @@ void dds_device::impl::write_control_message( topics::flexible_msg && msg, nlohm | |||
| //LOG_DEBUG( "got reply: " << actual_reply ); | |||
| *reply = std::move( actual_reply ); | |||
| _replies.erase( this_sequence_number ); | |||
|
|
|||
| if( rsutils::json::get( *reply, status_key, status_ok ) != status_ok ) | |||
There was a problem hiding this comment.
Can we brake this to functions
if( reply )
wait_for_reply( reply );
check_status( reply );
There was a problem hiding this comment.
It looks different in the final code:
*reply = std::move( actual_reply );
_replies.erase( this_sequence_number );
// Throw if there's an error
dds_device::check_reply( *reply );
| on_frame_release_callback _on_frame_release = nullptr; | ||
| on_frame_ready_callback _on_frame_ready = nullptr; | ||
| on_metadata_dropped_callback _on_metadata_dropped = nullptr; | ||
| on_frame_release_callback _on_frame_release; |
There was a problem hiding this comment.
Why did you remove the initialization?
Even if it is the same, explicit is better then implicit :-)
There was a problem hiding this comment.
I agree unless it's obvious. Anyway if you decide to do something different in the ctor then this will be in conflict. I'd rather people went to the ctor to check (unless there's none).
| options = &stream_it->second->options(); | ||
| } | ||
|
|
||
| auto update_option = [&]( std::string const & option_name, float const new_value ) |
There was a problem hiding this comment.
I don't like naming a lambda and calling it later by name. Why not make it a regular function?
There was a problem hiding this comment.
Because making it a regular function means a whole lot more code that sits separate (and probably would need to be declared in the class header) from the function where it's being used.
If we need to refactor or reuse this code later on, I agree. But for now this is self-contained.
| LOG_DEBUG( "[" << debug_name() << "] option '" << option_name << "': not found" ); | ||
| }; | ||
|
|
||
| rsutils::json::nested value_j( j, value_key ); |
There was a problem hiding this comment.
Can we break this long (and rather complicated) function?
Make it call - update_option, update_options_array etc...
There was a problem hiding this comment.
The whole function is 80 lines and all three use-cases reuse the lambdas that are common. It's really not that complicated.
I can put comments that explain which use-case we're dealing with but I don't want to complicate it even more with different functions, arguments, etc.:
// Without a 'value', the only use-case we have is for a query-option with "option-name":[] (empty array)
// that returns option-value pairs in 'option-values':
// The 'option-name' should be an array of options for which values are returned in a similarly-sized array
// A single value for a single option: option-name should be a string
There was a problem hiding this comment.
The comments will help, please add them
| { | ||
| auto status_j = rsutils::json::nested( reply, status_key ); | ||
| if( ! status_j ) | ||
| return true; |
There was a problem hiding this comment.
Yeah, default status is OK
| device = dds.device( participant, device_info ) | ||
| device.wait_until_ready() | ||
|
|
||
| with test.closure( 'Query single option, option-name: "Option 3"' ): |
There was a problem hiding this comment.
We should also test device level options
There was a problem hiding this comment.
They're currently covered in another test. I'll see if it can easily be moved.
As part of the work on DFU, I realized that we have a proxy-to-device relationship of many-to-one. That means that, if we want notifications from the device, for example, the current implementation is not sufficient!
Also, this has a bulk
query-optionimplementation with documentation - basically allowing a bulk query of option values, with a test.Commits are self-contained. Go one at a time.
Tracked on [LRS-988]