[libcamera-devel,v2,1/5] libcamera: Create DataValue and DataInfo

1994 diff mbox series

Add the data_value.[h|c] files that provide an abstracted polymorphic
data value type and a generic data info type that represent static
information associated with a data value.

DataValue is a slightly re-worked copy of the ControlValue type defined
in controls.h, provided in a generic header to be used a foundation for
both Controls and V4L2Controls classes that will be re-worked in the
next patches in the series.

Add a test for data value which is an adapted copy of the control value
test, that will be removed in the next patch.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/data_value.h |  82 +++++++++++
 include/libcamera/meson.build  |   1 +
 src/libcamera/data_value.cpp   | 259 +++++++++++++++++++++++++++++++++
 src/libcamera/meson.build      |   1 +
 test/data_value.cpp            |  59 ++++++++
 test/meson.build               |   1 +
 6 files changed, 403 insertions(+)
 create mode 100644 include/libcamera/data_value.h
 create mode 100644 src/libcamera/data_value.cpp
 create mode 100644 test/data_value.cpp

Hello,

On Tue, Sep 24, 2019 at 07:14:36PM +0200, Jacopo Mondi wrote:
> +/**
> + * \brief Assemble and return a string describing the value
> + * \return A string describing the DataValue
> + */
> +std::string DataValue::toString() const
> +{
> +	switch (type_) {
> +	case DataTypeBool:
> +		return bool_ ? "True" : "False";
> +	case DataTypeInteger:
> +		return std::to_string(integer_);
> +	case DataTypeInteger64:
> +		return std::to_string(integer64_);
> +	case DataTypeNone:
> +		return "<None>";
> +	}
> +}

Gcc complains there, clang does not. I just noticed after sending this
out... Will fix

Hi Jacopo,

On 24/09/2019 18:14, Jacopo Mondi wrote:
> Add the data_value.[h|c] files that provide an abstracted polymorphic
> data value type and a generic data info type that represent static
> information associated with a data value.
> 
> DataValue is a slightly re-worked copy of the ControlValue type defined
> in controls.h, provided in a generic header to be used a foundation for
> both Controls and V4L2Controls classes that will be re-worked in the
> next patches in the series.
> 
> Add a test for data value which is an adapted copy of the control value
> test, that will be removed in the next patch.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/data_value.h |  82 +++++++++++
>  include/libcamera/meson.build  |   1 +
>  src/libcamera/data_value.cpp   | 259 +++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build      |   1 +
>  test/data_value.cpp            |  59 ++++++++
>  test/meson.build               |   1 +
>  6 files changed, 403 insertions(+)
>  create mode 100644 include/libcamera/data_value.h
>  create mode 100644 src/libcamera/data_value.cpp
>  create mode 100644 test/data_value.cpp
> 
> diff --git a/include/libcamera/data_value.h b/include/libcamera/data_value.h
> new file mode 100644
> index 000000000000..c2c95d28b0f2
> --- /dev/null
> +++ b/include/libcamera/data_value.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * data_value.h - Polymorphic data value type
> + */
> +#ifndef __LIBCAMERA_DATA_VALUE_H__
> +#define __LIBCAMERA_DATA_VALUE_H__
> +
> +#include <cstdint>
> +#include <string>
> +
> +namespace libcamera {
> +
> +enum DataType {
> +	DataTypeNone,
> +	DataTypeBool,
> +	DataTypeInteger,
> +	DataTypeInteger64,
> +};
> +
> +static constexpr size_t DataSize[] = {
> +	[DataTypeNone]		= 1,
> +	[DataTypeBool]		= sizeof(bool),
> +	[DataTypeInteger]	= sizeof(int32_t),
> +	[DataTypeInteger64]	= sizeof(int64_t),
> +};
> +
> +class DataValue
> +{
> +public:
> +	DataValue();
> +	DataValue(bool value);
> +	DataValue(int value);
> +	DataValue(int64_t value);
> +
> +	DataValue &operator=(const DataValue &other);
> +
> +	enum DataType type() const { return type_; }
> +	size_t size() const { return size_; }
> +
> +	void set(bool value);
> +	void set(int value);
> +	void set(int64_t value);
> +
> +	bool getBool() const;
> +	int getInt() const;
> +	int64_t getInt64() const;
> +
> +	std::string toString() const;
> +
> +private:
> +	DataType type_;
> +	size_t size_;
> +
> +	union {
> +		bool bool_;
> +		int integer_;
> +		int64_t integer64_;
> +	};
> +};
> +
> +class DataInfo
> +{
> +public:
> +	DataInfo(const DataValue &min = 0, const DataValue &max = 0)
> +	{
> +		min_ = min;
> +		max_ = max;
> +	}
> +
> +	const DataValue &min() const { return min_; }
> +	const DataValue &max() const { return max_; }
> +
> +private:
> +	DataValue min_;
> +	DataValue max_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_DATA_VALUE_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 868f1a6bf1ab..e3f3ad504446 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_api = files([
>      'camera_manager.h',
>      'control_ids.h',
>      'controls.h',
> +    'data_value.h',
>      'event_dispatcher.h',
>      'event_notifier.h',
>      'geometry.h',
> diff --git a/src/libcamera/data_value.cpp b/src/libcamera/data_value.cpp
> new file mode 100644
> index 000000000000..2d90100116ab
> --- /dev/null
> +++ b/src/libcamera/data_value.cpp
> @@ -0,0 +1,259 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * data_value.cpp - Polymorphic data value type
> + */
> +
> +#include <libcamera/data_value.h>
> +
> +#include <cstdint>
> +#include <sstream>
> +#include <string>
> +
> +#include "utils.h"
> +#include "log.h"
> +
> +/**
> + * \file data_value.h
> + * \brief Polymorphic data type
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \enum DataType
> + * \brief Define the supported data types
> + * \var DataTypeNone
> + * Identifies an unset value
> + * \var DataTypeBool
> + * Identifies DataType storing a boolean value
> + * \var DataTypeInteger
> + * Identifies DataType storing an integer value
> + * \var DataTypeInteger64
> + * Identifies DataType storing a 64-bit integer value
> + */
> +
> +/**
> + * \var DataSize
> + * \brief Associate to each data type the memory size in bytes
> + */
> +
> +/**
> + * \class DataValue
> + * \brief Polymorphic data value
> + *
> + * DataValue holds values of different types, defined by DataType providing
> + * a unified interface to access and modify the data content.
> + *
> + * DataValue instances are used by classes that need to represent and store
> + * numerical values of different types.
> + *
> + * \todo Add support for compound data types, such as arrays.
> + */
> +
> +/**
> + * \brief Construct an empty DataValue
> + */
> +DataValue::DataValue()
> +	: type_(DataTypeNone), size_(DataSize[type_])

Do we need to initialise the value to zero or anything here?
If so - I'd just set the integer64_(0)


> +{
> +}
> +
> +/**
> + * \brief Construct a boolean DataValue
> + * \param[in] value Boolean value to store
> + */
> +DataValue::DataValue(bool value)
> +	: type_(DataTypeBool), size_(DataSize[type_]), bool_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct an integer DataValue
> + * \param[in] value Integer value to store
> + */
> +DataValue::DataValue(int value)
> +	: type_(DataTypeInteger), size_(DataSize[type_]), integer_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct a 64 bit integer DataValue
> + * \param[in] value 64-bits integer value to store
> + */
> +DataValue::DataValue(int64_t value)
> +	: type_(DataTypeInteger64), size_(DataSize[type_]), integer64_(value)
> +{
> +}
> +
> +/**
> + * \brief Assign the DataValue type and value using the ones from \a other
> + * \param[in] other The DataValue to copy type and value from
> + *
> + * DataValue assignment operator.
> + *
> + * \return The DataValue with fields updated using the ones from \a other
> + */
> +DataValue &DataValue::operator=(const DataValue &other)
> +{
> +	DataType newType = other.type();
> +	type_ = newType;
> +	size_ = DataSize[type_];
> +
> +	switch (newType) {
> +	case DataTypeBool:
> +		bool_ = other.getBool();
> +		break;
> +	case DataTypeInteger:
> +		integer_ = other.getInt();
> +		break;
> +	case DataTypeInteger64:
> +		integer64_ = other.getInt64();
> +		break;
> +	case DataTypeNone:
> +		bool_ = 0;
> +		integer_ = 0;
> +		integer64_ = 0;

As these are all members of a union, I think we only need to set the
biggest.

But as it's DataTypeNone, the actual value is undefined, and you are
simply initialising (so perhaps the DataTypeNone constructor should also
initialise a value?, see above)

Technically, I think setting
		integer64_ = other.getInt64();
for the TypeNone case would be more accurate - but I don't expect this
to be used really.

Perhaps could just do a fallthrough:

 +      case DataTypeNone:
 +	case DataTypeInteger64:
 +		integer64_ = other.getInt64();
 +		break;

or just not set it at all.


> +		break;
> +	}
> +
> +	return *this;
> +}
> +
> +/**
> + * \fn DataValue::type()
> + * \brief Retrieve the data type of the data
> + * \return The type of the data
> + */
> +
> +/**
> + * \fn DataValue::size()
> + * \brief Retrieve the size in bytes of the data
> + * \return The size in bytes of the data
> + */
> +
> +/**
> + * \brief Set the value with a boolean
> + * \param[in] value Boolean value to store
> + */
> +void DataValue::set(bool value)
> +{
> +	type_ = DataTypeBool;
> +	size_ = DataSize[type_];
> +	bool_ = value;
> +}
> +
> +/**
> + * \brief Set the value with an integer
> + * \param[in] value Integer value to store
> + */
> +void DataValue::set(int value)
> +{
> +	type_ = DataTypeInteger;
> +	size_ = DataSize[type_];
> +	integer_ = value;
> +}
> +
> +/**
> + * \brief Set the value with a 64 bit integer
> + * \param[in] value 64 bit integer value to store
> + */
> +void DataValue::set(int64_t value)
> +{
> +	type_ = DataTypeInteger64;
> +	size_ = DataSize[type_];
> +	integer64_ = value;
> +}

Should we support setting from a DataValue too?

Essentially with just a call to the copy constructor...?

> +
> +/**
> + * \brief Get the boolean value
> + *
> + * The value type must be Boolean.
> + *
> + * \return The boolean value
> + */
> +bool DataValue::getBool() const
> +{
> +	ASSERT(type_ == DataTypeBool);
> +
> +	return bool_;
> +}
> +
> +/**
> + * \brief Get the integer value
> + *
> + * The value type must be Integer or Integer64.
> + *
> + * \return The integer value
> + */
> +int DataValue::getInt() const
> +{
> +	ASSERT(type_ == DataTypeInteger || type_ == DataTypeInteger64);
> +
> +	return integer_;
> +}
> +
> +/**
> + * \brief Get the 64-bit integer value
> + *
> + * The value type must be Integer or Integer64.
> + *
> + * \return The 64-bit integer value
> + */
> +int64_t DataValue::getInt64() const
> +{
> +	ASSERT(type_ == DataTypeInteger || type_ == DataTypeInteger64);
> +
> +	return integer64_;
> +}
> +
> +/**
> + * \brief Assemble and return a string describing the value
> + * \return A string describing the DataValue
> + */
> +std::string DataValue::toString() const
> +{
> +	switch (type_) {
> +	case DataTypeBool:
> +		return bool_ ? "True" : "False";
> +	case DataTypeInteger:
> +		return std::to_string(integer_);
> +	case DataTypeInteger64:
> +		return std::to_string(integer64_);
> +	case DataTypeNone:
> +		return "<None>";
> +	}


Is the compiler happy with no return statement outside of this switch,
even with no default: ? I guess it knows that all the DataType enums are
used (which is good).


> +}
> +
> +/**
> + * \class DataInfo
> + * \brief Validation informations associated with a data value


s/informations/information/

> + *
> + * The DataInfo class represents static information associated with data
> + * types that represent polymorphic values abstracted by the DataValue class.
> + *
> + * DataInfo stores static informations such as the value minimum and maximum

s/informations/information/

Information is an 'uncountable noun'
	https://blog.harwardcommunications.com/2010/11/09/information/


> + * values and for compound values the maximum and minimum number of elements.
> + */
> +
> +/**
> + * \fn DataInfo::DataInfo
> + * \brief Construct a data info with \a min and \a max values
> + * \param[in] min The minimum allowed value
> + * \param[in] max The maximum allowed value
> + */
> +
> +/**
> + * \fn DataInfo::min()
> + * \brief Retrieve the DataInfo minimum allowed value
> + * \return A DataValue representing the minimum allowed value
> + */
> +
> +/**
> + * \fn DataInfo::max()
> + * \brief Retrieve the DataInfo maximum allowed value
> + * \return A DataValue representing the maximum allowed value
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 755149c55c7b..c3100a1709e0 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'controls.cpp',
> +    'data_value.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'event_dispatcher.cpp',
> diff --git a/test/data_value.cpp b/test/data_value.cpp
> new file mode 100644
> index 000000000000..64061000c1e4
> --- /dev/null
> +++ b/test/data_value.cpp
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * data_value.cpp - DataValue tests
> + */
> +
> +#include <iostream>
> +
> +#include <libcamera/data_value.h>
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class DataValueTest : public Test
> +{
> +protected:
> +	int run()
> +	{
> +		DataValue integer(1234);
> +		DataValue boolean(true);
> +
> +		/* Just a string conversion output test... no validation */
> +		cout << "Int: " << integer.toString()
> +		     << " Bool: " << boolean.toString()
> +		     << endl;

I think Laurent suggested changing this in the original test.

Something like :

   	stringstream ss;

	ss.str(integer.toString());
	if (ss.str() != "1234")
		return TestFail;

	ss.str(boolean.toString());
	if (ss.str() != "True")
		return TestFail;

or such.

Seems like a good time to fix it.


> +
> +		if (integer.getInt() != 1234) {
> +			cerr << "Failed to get Integer" << endl;
> +			return TestFail;
> +		}
> +
> +		if (boolean.getBool() != true) {
> +			cerr << "Failed to get Boolean" << endl;
> +			return TestFail;
> +		}
> +
> +		/* Test an uninitialised value, and updating it. */
> +
> +		DataValue value;
> +		value.set(true);
> +		if (value.getBool() != true) {
> +			cerr << "Failed to get Booleans" << endl;
> +			return TestFail;
> +		}
> +
> +		value.set(10);
> +		if (value.getInt() != 10) {
> +			cerr << "Failed to get Integer" << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(DataValueTest)
> diff --git a/test/meson.build b/test/meson.build
> index 84722cceb35d..19e3031244a3 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -13,6 +13,7 @@ subdir('v4l2_subdevice')
>  subdir('v4l2_videodevice')
>  
>  public_tests = [
> +    ['data_value',                      'data_value.cpp'],
>      ['geometry',                        'geometry.cpp'],
>      ['list-cameras',                    'list-cameras.cpp'],
>      ['signal',                          'signal.cpp'],
>