Send supported options in discovery stream-header #11164
Conversation
|
In general, I don't like |
|
Parameters can exist on multiple levels, and I'd say the first level is that of the device, not stream. In fact, I'd even say stream parameters are the last we want to worry about and that most can be handled at the level of the device. Why not start by adding a parameter-initialization message that we can use for any entity - device/stream/etc.? |
I don't agree with this. If we have trouble, it's very indicative of improper destruction. You can switch domains, reset the directory, etc. -- hiding the problem is not good and we need FastDDS to address this. |
| dds_option( nlohmann::json const & j ); | ||
|
|
||
| public: | ||
| dds_option() = default; |
There was a problem hiding this comment.
Why do we need this? I imagine the minimum ever would get an option name, but even then it would be protected and overwritten?
Also, _value, _range are uninitialized. Aren't you getting a warning?
There was a problem hiding this comment.
I did not want to create a constructor with many parameters, especially when you can set the values later
(rs-dds-server uses this to create an option and then set values it gets from LibRS interface)
When I started I didn't use option name, only added later and I agree it can be a minimum.
(I still need to consider your comment about device options for now I also think we will need stream name if using stream level options)
I don't see why we need shared memory to be used. Note that on our new use cases we will probably have 100% "improper destructions". |
|
I separated the supported options message from the device/stream-header messages. This PR handles the discovery and listing the supported messages. Setting/querying and maybe changing LibRS style options (float, with range) to type hierarchy in next PRs |
unit-tests/dds/test-options.py
Outdated
| # | ||
| ############################################################################################# | ||
| # | ||
| test.start( "Test no options..." ) |
There was a problem hiding this comment.
No need to add ... on every test postfix - IMO
unit-tests/dds/test-options.py
Outdated
| test.check( device.is_running() ) | ||
|
|
||
| options = device.options(); | ||
| test.check_equal( len( options ), 3 ) |
There was a problem hiding this comment.
I think that it will be clearer if options-server will be more generic and will get the values here, is this possible?
Here we compare the values to values that were introduces somewhere else
@Nir-Az I don't see any use-case here. This is realdds, and it shouldn't enforce anything. The proper way to do this is with a parameter.
What're these new use-cases? I don't see why we should have improper destructions... |
src/context.cpp
Outdated
| { | ||
| } | ||
|
|
||
| void set( float value ) override {}; //TODO - implement |
There was a problem hiding this comment.
extra semicolons in this and next lines
You are right, about RealDDS and about the improper destructions. |
| dds_options options; | ||
| for( auto & option : j["options"] ) | ||
| { | ||
| options.push_back( dds_option::from_json( option, stream_it->second->name() ) ); |
There was a problem hiding this comment.
I think that init_options should set the owner name, not here...
There was a problem hiding this comment.
And you will allow passing the dds_option object around, setting new owner?
There was a problem hiding this comment.
No, owner should only be set once
| @@ -136,12 +135,12 @@ void dds_participant::init( dds_domain_id domain_id, std::string const & partici | |||
| pqos.wire_protocol().builtin.discovery_config.leaseDuration = { 10, 0 }; // [sec,nsec] | |||
|
|
|||
| //Disable shared memory, use only UDP | |||
| //Disabling because sometimes, after improper destruction (e.g. stopping debug) the shared memory is not opened | |||
There was a problem hiding this comment.
Next time, please please please add a space between the // and the text and keep everything looking similar.
Added dds_option class to handle option objects
rs-dds-server tool gets all supported options and sends them in discovery, currently in stream header message. If we will see that the messages are to big we will split later.
The options are received by dds-device-impl and registered to the software device.
Also disabled shared memory transport as
dds_participant.init()stuck many times because of it