From 66f5aca6e3415e425a06593a6d5bf8706c42e8da Mon Sep 17 00:00:00 2001 From: Eran Date: Sun, 11 Feb 2024 09:57:08 +0200 Subject: [PATCH] final touches --- .../realdds/include/realdds/dds-option.h | 33 ++++++++++--------- third-party/realdds/py/pyrealdds.cpp | 25 -------------- third-party/realdds/src/dds-option.cpp | 23 +++++++------ unit-tests/dds/test-option-value.py | 16 ++++++++- 4 files changed, 45 insertions(+), 52 deletions(-) diff --git a/third-party/realdds/include/realdds/dds-option.h b/third-party/realdds/include/realdds/dds-option.h index 81375214fd6..3eef1c48103 100644 --- a/third-party/realdds/include/realdds/dds-option.h +++ b/third-party/realdds/include/realdds/dds-option.h @@ -17,18 +17,19 @@ namespace realdds { class dds_stream_base; -// Abstract base class for all option types +// Abstract base class for all options. +// Derivations specialize the TYPE of the option value: float, int, string, etc... // // Options values can be: -// - Read-only: No default value or range +// - Read-only: No default value or range (implicit if no default is present) // - Optional: A 'null' value is accepted, meaning no value is set // // An option is meant to be expressed in JSON, and can be created from it: -// - R/O options [name, value, description, [properties]] -// - Simple options [name, value, default, description, [properties]] -// - Ranged options [name, value, range..., default, description, [properties]] +// - R/O options [name, value, description, [properties...]] +// - Simple options [name, value, default, description, [properties...]] +// - Ranged options [name, value, range..., default, description, [properties...]] // Where range... is one or more parameters that describe the range. -// The value (and default) must adhere to the range (if not null). +// The value and default value must adhere to the range (if not null). A range requires a default value. // class dds_option { @@ -43,8 +44,8 @@ class dds_option std::string _description; unsigned _flags; // from the properties - rsutils::json _j; - rsutils::json _default_j; + rsutils::json _value; + rsutils::json _default_value; rsutils::json _minimum_value; rsutils::json _maximum_value; @@ -60,12 +61,12 @@ class dds_option dds_option(); - // These init functions must happen before the first set_value() - void init( std::string name, std::string description ); - void init_default_value( rsutils::json ); - void init_range( rsutils::json const & min, rsutils::json const & max, rsutils::json const & step ); - void init_properties( option_properties && ); - void init_value( rsutils::json ); + // These init functions must happen before the first set_value(), and should be called in order: + virtual void init( std::string name, std::string description ); + virtual void init_properties( option_properties && ); + virtual void init_default_value( rsutils::json ); + virtual void init_range( rsutils::json const & min, rsutils::json const & max, rsutils::json const & step ); + virtual void init_value( rsutils::json ); bool is_read_only() const { return _flags & flag::read_only; } bool is_optional() const { return _flags & flag::optional; } @@ -77,7 +78,7 @@ class dds_option std::shared_ptr< dds_stream_base > stream() const { return _stream.lock(); } - rsutils::json const & get_value() const { return _j; } + rsutils::json const & get_value() const { return _value; } bool is_valid() const { return ! get_value().is_null(); } rsutils::json const & get_minimum_value() const { return _minimum_value; } @@ -88,7 +89,7 @@ class dds_option virtual void check_value( rsutils::json & ) const; virtual void check_type( rsutils::json const & ) const = 0; - rsutils::json const & get_default_value() const { return _default_j; } + rsutils::json const & get_default_value() const { return _default_value; } bool is_default_valid() const { return ! get_default_value().is_null(); } virtual rsutils::json to_json() const; diff --git a/third-party/realdds/py/pyrealdds.cpp b/third-party/realdds/py/pyrealdds.cpp index b564cdd7894..961c98722b1 100644 --- a/third-party/realdds/py/pyrealdds.cpp +++ b/third-party/realdds/py/pyrealdds.cpp @@ -659,31 +659,6 @@ PYBIND11_MODULE(NAME, m) { return os.str(); } ); - // TODO remove - struct json_w - { - json j; - }; - py::class_< json_w >( m, "json" ) - .def( py::init<>() ) - .def( py::init< json const & >() ) - .def_static( "parse", - []( std::string const & s ) { // - return json_w{ json::parse( s ) }; - } ) - .def_readwrite( "j", &json_w::j ) - .def( "type", []( json_w const & self ) { return (int) self.j.type(); } ) - .def( "type_name", []( json_w const & self ) { return self.j.type_name(); } ) - .def( "size", []( json_w const & self ) { return self.j.size(); } ) - .def( "empty", []( json_w const & self ) { return self.j.empty(); } ) - .def( "__repr__", - []( json_w const & self ) - { - std::ostringstream os; - os << self.j; - return os.str(); - } ); - using realdds::dds_video_encoding; py::class_< dds_video_encoding > video_encoding( m, "video_encoding" ); video_encoding // diff --git a/third-party/realdds/src/dds-option.cpp b/third-party/realdds/src/dds-option.cpp index e1f37a5cd1d..c0ca5cb0883 100644 --- a/third-party/realdds/src/dds-option.cpp +++ b/third-party/realdds/src/dds-option.cpp @@ -13,8 +13,8 @@ namespace realdds { dds_option::dds_option() : _flags( 0 ) - , _j( rsutils::missing_json ) - , _default_j( rsutils::missing_json ) + , _value( rsutils::missing_json ) + , _default_value( rsutils::missing_json ) , _minimum_value( rsutils::null_json ) , _maximum_value( rsutils::null_json ) , _stepping( rsutils::null_json ) @@ -24,7 +24,7 @@ dds_option::dds_option() void dds_option::verify_uninitialized() const { - if( _j.exists() ) + if( _value.exists() ) DDS_THROW( runtime_error, "option is already initialized" ); } @@ -52,7 +52,7 @@ void dds_option::init_properties( option_properties && props ) if( props.erase( "optional" ) ) _flags |= flag::optional; - if( !props.empty() ) + if( ! props.empty() ) DDS_THROW( runtime_error, "invalid option properties" ); } @@ -62,7 +62,7 @@ void dds_option::init_default_value( json value ) verify_uninitialized(); check_value( value ); - _default_j = std::move( value ); + _default_value = std::move( value ); } @@ -81,7 +81,7 @@ void dds_option::init_range( rsutils::json const & minimum_value, { // This is read-only...! _flags |= flag::read_only; - _default_j = rsutils::missing_json; + _default_value = rsutils::missing_json; return; } @@ -130,7 +130,7 @@ void dds_option::init_value( json value ) if( ! get_default_value().exists() ) _flags |= (unsigned) flag::read_only; - _j = std::move( value ); + _value = std::move( value ); } @@ -148,7 +148,7 @@ void dds_option::init_stream( std::shared_ptr< dds_stream_base > const & stream void dds_option::set_value( rsutils::json value ) { check_value( value ); - _j = std::move( value ); + _value = std::move( value ); } @@ -410,12 +410,15 @@ json dds_option::props_to_json() const { try { + // 1.1 is expressed as 1.10000002 as float but 1.1000000000000001 in double... + // Really hard to check that we don't lose precision unless we convert back to string which will be rounded + // anyway. return value.get< type >(); } catch( ... ) { - DDS_THROW( runtime_error, "not convertible to a float: " << value ); } + DDS_THROW( runtime_error, "not convertible to a float: " << value ); } @@ -433,7 +436,7 @@ void dds_float_option::check_type( json const & value ) const return value.get< type >(); case json::value_t::number_unsigned: - if( value.get< type >() != value.get< json::number_unsigned_t >() ) + if( value.get< json::number_unsigned_t >() > (uint64_t)std::numeric_limits< type >::max() ) break; return value.get< type >(); diff --git a/unit-tests/dds/test-option-value.py b/unit-tests/dds/test-option-value.py index fb5ec665fca..35a87c29095 100644 --- a/unit-tests/dds/test-option-value.py +++ b/unit-tests/dds/test-option-value.py @@ -16,6 +16,9 @@ test.check_equal_lists( dds.option.from_json( # default==min==max, step==0 -> read-only! ['Stereo Baseline',50.20994567871094,50.20994567871094,50.20994567871094,0.0,50.20994567871094,'...',['read-only']] ).to_json(), ['Stereo Baseline',50.20994567871094,'...'] ) + test.check_equal( # 1.1 => 1.10000002 as float, => 1.1000000000000001 as double + dds.option.from_json( ['a', 1.1, 'desc'] ).get_value(), + 1.1 ) with test.closure( 'r/o options are still settable' ): # NOTE: the DDS options do not enforce logic post initialization; they serve only to COMMUNICATE any state and limits @@ -29,7 +32,11 @@ test.check_false( dds.option.from_json( ['1', 0, 'desc'] ).is_optional() ) test.check( dds.option.from_json( ['Asic Temperature',None,-40.0,125.0,0.0,0.0,'Current Asic Temperature (degree celsius)',['optional','read-only']] ).is_optional() ) dds.option.from_json( ['4', 'string-value', None, 'desc', ['optional']] ) - dds.option.from_json( ['5', None, 'default-string-value', 'desc', ['optional']] ) + dds.option.from_json( ['5', None, 'default-string-value', 'desc', ['optional']] ) # string type is deduced + test.check_throws( lambda: + dds.option.from_json( ['a', None, 'desc', ['optional']] ), + RuntimeError, 'cannot deduce value type: ["a",null,"desc",["optional"]]' ) + with test.closure( 'mixed types' ): test.check_equal( dds.option.from_json( ['i', 0, 'desc'] ).value_type(), 'int' ) @@ -46,6 +53,13 @@ dds.option.from_json( ['3f', 3, 1, 5, 1, 2.2, '', ['int']] ), RuntimeError, 'not convertible to a signed integer: 2.2' ) + dds.option.from_json( ['a', 9223372036854775807, 'desc'] ) # max int64 + test.check_throws( lambda: + dds.option.from_json( ['a', 9223372036854775808, 'desc'] ), # uint64 + RuntimeError, 'not convertible to a signed integer: 9223372036854775808' ) + dds.option.from_json( ['a', -9223372036854775808, 'desc'] ) # min int64 + # -9223372036854775809 cannot be converted to json + test.check_equal( dds.option.from_json( ['Brightness',0,-64,64,1,0,'UVC image brightness'] ).value_type(), 'int' ) with test.closure( 'range' ):