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

1985 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 |  84 +++++++++
 include/libcamera/meson.build  |   1 +
 src/libcamera/data_value.cpp   | 317 +++++++++++++++++++++++++++++++++
 src/libcamera/meson.build      |   1 +
 test/data_value/data_value.cpp |  59 ++++++
 test/data_value/meson.build    |  12 ++
 test/meson.build               |   1 +
 7 files changed, 475 insertions(+)
 create mode 100644 include/libcamera/data_value.h
 create mode 100644 src/libcamera/data_value.cpp
 create mode 100644 test/data_value/data_value.cpp
 create mode 100644 test/data_value/meson.build

--
2.23.0

Hi Jacopo,

On 18/09/2019 11:34, 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.

So my
"[RFC PATCH v2 1/9] libcamera: Value: Provide abstract value class"

concept comes back to life at last :-D

> 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 |  84 +++++++++
>  include/libcamera/meson.build  |   1 +
>  src/libcamera/data_value.cpp   | 317 +++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build      |   1 +
>  test/data_value/data_value.cpp |  59 ++++++
>  test/data_value/meson.build    |  12 ++
>  test/meson.build               |   1 +
>  7 files changed, 475 insertions(+)
>  create mode 100644 include/libcamera/data_value.h
>  create mode 100644 src/libcamera/data_value.cpp
>  create mode 100644 test/data_value/data_value.cpp
>  create mode 100644 test/data_value/meson.build
> 
> diff --git a/include/libcamera/data_value.h b/include/libcamera/data_value.h
> new file mode 100644
> index 000000000000..0fcd9bd04a65
> --- /dev/null
> +++ b/include/libcamera/data_value.h
> @@ -0,0 +1,84 @@
> +/* 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,
> +	DataTypeNumber,
> +};
> +
> +static constexpr size_t DataSize[DataTypeNumber] = {
> +	[DataTypeNone]		= 1,

Shouldn't this be 0?

> +	[DataTypeBool]		= 4,

sizeof(bool) == 1 ...



> +	[DataTypeInteger]	= 4,
> +	[DataTypeInteger64]	= 8,

You could instead do:

static constexpr size_t DataSize[DataTypeNumber] = {
	[DataTypeNone]		= 0,
	[DataTypeBool]		= sizeof(bool),
	[DataTypeInteger]	= sizeof(int32_t),
	[DataTypeInteger64]	= sizeof(int64_t),
};

> +};
> +
> +class DataValue
> +{
> +public:
> +	DataValue();
> +	DataValue(const DataValue &other);
> +	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, const DataValue &max)
> +	{
> +		min_ = min;
> +		max_ = max;
> +	}
> +
> +	const DataValue &min() const { return min_; }
> +	const DataValue &max() const { return max_; }
> +
> +private:
> +	DataValue max_;
> +	DataValue min_;
> +};
> +
> +} /* 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..f07b5fb75a8a
> --- /dev/null
> +++ b/src/libcamera/data_value.cpp
> @@ -0,0 +1,317 @@
> +/* 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
> + */
> +


You have a rogue if 0 below

> +#if 0
> +/**
> + * \class Data
> + * \brief Base class for data value and data info instances
> + *
> + * DataValue and DataInfo share basic information like size and type.
> + * This class provides common fields and operations for them.
> + */
> +
> +/**
> + * \brief Construct a Data of \a type
> + * \param[in] type The Data type defined by DataType
> + */
> +Data::Data(DataType type)
> +	: type_(type)
> +{
> +	/*
> +	 * \todo The size of compound data types depend on the number of
> +	 * elements too.
> +	 */
> +	size_ = DataSize[type_];
> +}
> +
> +/**
> + * \var Data::type_
> + * \brief The data type
> + */
> +
> +/**
> + * \var Data::size_
> + * \brief The data size
> + */
> +
> +#endif


To here ...


> +
> +/**
> + * \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_])
> +{
> +}
> +
> +/**
> + * \brief Construct a DataValue copying data from \a other
> + * \param[in] other The DataValue to copy data from
> + *
> + * DataValue copy constructor.
> + */
> +DataValue::DataValue(const DataValue &other)
> +	: type_(other.type()), size_(DataSize[type_])

Does this type need a copy constructor currently?
Won't C++ just copy the whole object, which would be equivalent?

That said, if/when this class becomes more complex then it would likely
need some sort of specialisation - so it doesn't hurt to have it already.

> +{
> +	switch (type_) {
> +	case DataTypeBool:
> +		bool_ = other.getBool();
> +		break;
> +	case DataTypeInteger:
> +		integer_ = other.getInt();
> +		break;
> +	case DataTypeInteger64:
> +		integer64_ = other.getInt64();
> +		break;
> +	default:
> +		bool_ = integer_ = integer64_ = 0;
> +		break;
> +	}
> +}
> +
> +/**
> + * \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 assignement operator.

s/assignement/assignment/

> + *
> + * \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;
> +	default:
> +		bool_ = integer_ = integer64_ = 0;
> +		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;
> +}
> +
> +/**
> + * \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_);
> +	default:
> +		return "<None>";


I haven't tried to compile this yet, but doesn't this generate a
compiler warning?

I thought C++ was very picky about switch statements not explicitly
stating all enum types... Perhaps not when a default statement is provided.

changing this to:

	...

	case DataTypeNone:
		return "<None>";
	}

	return "<INVALID TYPE>";
}

would in that case ensure that this function is always updated (forced
by the compiler) whenever a new type is added?

> +	}
> +}
> +
> +/**
> + * \class DataInfo
> + * \brief Validation informations associated with a data value
> + *
> + * The DataInfo class represents static information associated with data
> + * types that represent plymorhpic values abstracted by the DataValue class.

s/plymorhpic/polymorphic/


> + *
> + * DataInfo stores static informations such as the value minimum and maximum
> + * 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/data_value.cpp b/test/data_value/data_value.cpp
> new file mode 100644
> index 000000000000..64061000c1e4
> --- /dev/null
> +++ b/test/data_value/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;
> +
> +		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/data_value/meson.build b/test/data_value/meson.build
> new file mode 100644
> index 000000000000..3858e6085a1f
> --- /dev/null
> +++ b/test/data_value/meson.build
> @@ -0,0 +1,12 @@
> +data_value_tests = [
> +    [ 'data_value',   'data_value.cpp' ],
> +]
> +
> +foreach t : data_value_tests
> +    exe = executable(t[0], t[1],
> +                     dependencies : libcamera_dep,
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +    test(t[0], exe, suite : 'data_value', is_parallel : false)
> +endforeach


Does data_value need a whole subdirectory for a single test? Will it
have multiple tests in the (near) future?

I really dislike adding a subdirectory/test suite just for a single test
unless we really know it's going to be extended.

> +
> diff --git a/test/meson.build b/test/meson.build
> index 84722cceb35d..5d414b22dc0c 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -2,6 +2,7 @@ subdir('libtest')
> 
>  subdir('camera')
>  subdir('controls')
> +subdir('data_value')
>  subdir('ipa')
>  subdir('ipc')
>  subdir('log')
> --
> 2.23.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Kieran,

On Fri, Sep 20, 2019 at 11:03:55AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 18/09/2019 11:34, 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.
>
> So my
> "[RFC PATCH v2 1/9] libcamera: Value: Provide abstract value class"
>
> concept comes back to life at last :-D
>

Possibly, sorry for having ignored the patch at the time

> > 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 |  84 +++++++++
> >  include/libcamera/meson.build  |   1 +
> >  src/libcamera/data_value.cpp   | 317 +++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build      |   1 +
> >  test/data_value/data_value.cpp |  59 ++++++
> >  test/data_value/meson.build    |  12 ++
> >  test/meson.build               |   1 +
> >  7 files changed, 475 insertions(+)
> >  create mode 100644 include/libcamera/data_value.h
> >  create mode 100644 src/libcamera/data_value.cpp
> >  create mode 100644 test/data_value/data_value.cpp
> >  create mode 100644 test/data_value/meson.build
> >
> > diff --git a/include/libcamera/data_value.h b/include/libcamera/data_value.h
> > new file mode 100644
> > index 000000000000..0fcd9bd04a65
> > --- /dev/null
> > +++ b/include/libcamera/data_value.h
> > @@ -0,0 +1,84 @@
> > +/* 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,
> > +	DataTypeNumber,
> > +};
> > +
> > +static constexpr size_t DataSize[DataTypeNumber] = {
> > +	[DataTypeNone]		= 1,
>
> Shouldn't this be 0?
>

I would prefer 1, to avoid division by 0

> > +	[DataTypeBool]		= 4,
>
> sizeof(bool) == 1 ...
>

TIL
>
>
> > +	[DataTypeInteger]	= 4,
> > +	[DataTypeInteger64]	= 8,
>
> You could instead do:
>
> static constexpr size_t DataSize[DataTypeNumber] = {
> 	[DataTypeNone]		= 0,
> 	[DataTypeBool]		= sizeof(bool),
> 	[DataTypeInteger]	= sizeof(int32_t),
> 	[DataTypeInteger64]	= sizeof(int64_t),

better, I'll take this is with TypeNone = 1

> };
>
> > +};
> > +
> > +class DataValue
> > +{
> > +public:
> > +	DataValue();
> > +	DataValue(const DataValue &other);
> > +	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, const DataValue &max)
> > +	{
> > +		min_ = min;
> > +		max_ = max;
> > +	}
> > +
> > +	const DataValue &min() const { return min_; }
> > +	const DataValue &max() const { return max_; }
> > +
> > +private:
> > +	DataValue max_;
> > +	DataValue min_;
> > +};
> > +
> > +} /* 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..f07b5fb75a8a
> > --- /dev/null
> > +++ b/src/libcamera/data_value.cpp
> > @@ -0,0 +1,317 @@
> > +/* 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
> > + */
> > +
>
>
> You have a rogue if 0 below
>

Ups! thanks!

> > +#if 0
> > +/**
> > + * \class Data
> > + * \brief Base class for data value and data info instances
> > + *
> > + * DataValue and DataInfo share basic information like size and type.
> > + * This class provides common fields and operations for them.
> > + */
> > +
> > +/**
> > + * \brief Construct a Data of \a type
> > + * \param[in] type The Data type defined by DataType
> > + */
> > +Data::Data(DataType type)
> > +	: type_(type)
> > +{
> > +	/*
> > +	 * \todo The size of compound data types depend on the number of
> > +	 * elements too.
> > +	 */
> > +	size_ = DataSize[type_];
> > +}
> > +
> > +/**
> > + * \var Data::type_
> > + * \brief The data type
> > + */
> > +
> > +/**
> > + * \var Data::size_
> > + * \brief The data size
> > + */
> > +
> > +#endif
>
>
> To here ...
>
>
> > +
> > +/**
> > + * \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_])
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct a DataValue copying data from \a other
> > + * \param[in] other The DataValue to copy data from
> > + *
> > + * DataValue copy constructor.
> > + */
> > +DataValue::DataValue(const DataValue &other)
> > +	: type_(other.type()), size_(DataSize[type_])
>
> Does this type need a copy constructor currently?

I think it will be used during serialization implementation

> Won't C++ just copy the whole object, which would be equivalent?
>

Possibly, I wanted this explicit because of the above assignements to
the correct field. I'll check if I need this.

> That said, if/when this class becomes more complex then it would likely
> need some sort of specialisation - so it doesn't hurt to have it already.
>
> > +{
> > +	switch (type_) {
> > +	case DataTypeBool:
> > +		bool_ = other.getBool();
> > +		break;
> > +	case DataTypeInteger:
> > +		integer_ = other.getInt();
> > +		break;
> > +	case DataTypeInteger64:
> > +		integer64_ = other.getInt64();
> > +		break;
> > +	default:
> > +		bool_ = integer_ = integer64_ = 0;
> > +		break;
> > +	}
> > +}
> > +
> > +/**
> > + * \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 assignement operator.
>
> s/assignement/assignment/
>
> > + *
> > + * \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;
> > +	default:
> > +		bool_ = integer_ = integer64_ = 0;
> > +		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;
> > +}
> > +
> > +/**
> > + * \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_);
> > +	default:
> > +		return "<None>";
>
>
> I haven't tried to compile this yet, but doesn't this generate a
> compiler warning?
>
> I thought C++ was very picky about switch statements not explicitly
> stating all enum types... Perhaps not when a default statement is provided.
>
> changing this to:
>
> 	...
>
> 	case DataTypeNone:
> 		return "<None>";
> 	}
>
> 	return "<INVALID TYPE>";
> }
>
> would in that case ensure that this function is always updated (forced
> by the compiler) whenever a new type is added?

Yeah, default catches all the remaning cases but hides issues when we
miss adding a value. I'll change.

>
> > +	}
> > +}
> > +
> > +/**
> > + * \class DataInfo
> > + * \brief Validation informations associated with a data value
> > + *
> > + * The DataInfo class represents static information associated with data
> > + * types that represent plymorhpic values abstracted by the DataValue class.
>
> s/plymorhpic/polymorphic/
>
>
> > + *
> > + * DataInfo stores static informations such as the value minimum and maximum
> > + * 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/data_value.cpp b/test/data_value/data_value.cpp
> > new file mode 100644
> > index 000000000000..64061000c1e4
> > --- /dev/null
> > +++ b/test/data_value/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;
> > +
> > +		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/data_value/meson.build b/test/data_value/meson.build
> > new file mode 100644
> > index 000000000000..3858e6085a1f
> > --- /dev/null
> > +++ b/test/data_value/meson.build
> > @@ -0,0 +1,12 @@
> > +data_value_tests = [
> > +    [ 'data_value',   'data_value.cpp' ],
> > +]
> > +
> > +foreach t : data_value_tests
> > +    exe = executable(t[0], t[1],
> > +                     dependencies : libcamera_dep,
> > +                     link_with : test_libraries,
> > +                     include_directories : test_includes_internal)
> > +    test(t[0], exe, suite : 'data_value', is_parallel : false)
> > +endforeach
>
>
> Does data_value need a whole subdirectory for a single test? Will it
> have multiple tests in the (near) future?

I thought so, but in the end I only added one

>
> I really dislike adding a subdirectory/test suite just for a single test
> unless we really know it's going to be extended.
>

Where would you put this?

> > +
> > diff --git a/test/meson.build b/test/meson.build
> > index 84722cceb35d..5d414b22dc0c 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -2,6 +2,7 @@ subdir('libtest')
> >
> >  subdir('camera')
> >  subdir('controls')
> > +subdir('data_value')
> >  subdir('ipa')
> >  subdir('ipc')
> >  subdir('log')
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On 23/09/2019 12:02, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Sep 20, 2019 at 11:03:55AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>

<snip>

>>> +TEST_REGISTER(DataValueTest)
>>> diff --git a/test/data_value/meson.build b/test/data_value/meson.build
>>> new file mode 100644
>>> index 000000000000..3858e6085a1f
>>> --- /dev/null
>>> +++ b/test/data_value/meson.build
>>> @@ -0,0 +1,12 @@
>>> +data_value_tests = [
>>> +    [ 'data_value',   'data_value.cpp' ],
>>> +]
>>> +
>>> +foreach t : data_value_tests
>>> +    exe = executable(t[0], t[1],
>>> +                     dependencies : libcamera_dep,
>>> +                     link_with : test_libraries,
>>> +                     include_directories : test_includes_internal)
>>> +    test(t[0], exe, suite : 'data_value', is_parallel : false)
>>> +endforeach
>>
>>
>> Does data_value need a whole subdirectory for a single test? Will it
>> have multiple tests in the (near) future?
> 
> I thought so, but in the end I only added one
> 
>>
>> I really dislike adding a subdirectory/test suite just for a single test
>> unless we really know it's going to be extended.
>>
> 
> Where would you put this?

just test/data_value.cpp for the moment!

>> Regards
>> --
>> Kieran

Hello,

On Mon, Sep 23, 2019 at 01:02:57PM +0200, Jacopo Mondi wrote:
> On Fri, Sep 20, 2019 at 11:03:55AM +0100, Kieran Bingham wrote:
> > On 18/09/2019 11:34, 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.
> >
> > So my
> > "[RFC PATCH v2 1/9] libcamera: Value: Provide abstract value class"
> >
> > concept comes back to life at last :-D
> 
> Possibly, sorry for having ignored the patch at the time
> 
> > > 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 |  84 +++++++++
> > >  include/libcamera/meson.build  |   1 +
> > >  src/libcamera/data_value.cpp   | 317 +++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build      |   1 +
> > >  test/data_value/data_value.cpp |  59 ++++++
> > >  test/data_value/meson.build    |  12 ++
> > >  test/meson.build               |   1 +
> > >  7 files changed, 475 insertions(+)
> > >  create mode 100644 include/libcamera/data_value.h
> > >  create mode 100644 src/libcamera/data_value.cpp
> > >  create mode 100644 test/data_value/data_value.cpp
> > >  create mode 100644 test/data_value/meson.build
> > >
> > > diff --git a/include/libcamera/data_value.h b/include/libcamera/data_value.h
> > > new file mode 100644
> > > index 000000000000..0fcd9bd04a65
> > > --- /dev/null
> > > +++ b/include/libcamera/data_value.h
> > > @@ -0,0 +1,84 @@
> > > +/* 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,
> > > +	DataTypeNumber,
> > > +};
> > > +
> > > +static constexpr size_t DataSize[DataTypeNumber] = {
> > > +	[DataTypeNone]		= 1,
> >
> > Shouldn't this be 0?
> 
> I would prefer 1, to avoid division by 0
> 
> > > +	[DataTypeBool]		= 4,
> >
> > sizeof(bool) == 1 ...
> 
> TIL

https://en.cppreference.com/w/cpp/language/types

Boolean type

    bool - type, capable of holding one of the two values: true or
    false. The value of sizeof(bool) is implementation defined and might
    differ from 1.

If you need to ensure inter-operability between compilers (for instance
if the sizes are used to serialise/deserialise data) then you probably
want to specify the size of a bool explicitly. I think I would do the
same for the other types then.

> > > +	[DataTypeInteger]	= 4,
> > > +	[DataTypeInteger64]	= 8,
> >
> > You could instead do:
> >
> > static constexpr size_t DataSize[DataTypeNumber] = {
> > 	[DataTypeNone]		= 0,
> > 	[DataTypeBool]		= sizeof(bool),
> > 	[DataTypeInteger]	= sizeof(int32_t),
> > 	[DataTypeInteger64]	= sizeof(int64_t),
> 
> better, I'll take this is with TypeNone = 1
> 
> > };
> >
> > > +};
> > > +
> > > +class DataValue
> > > +{
> > > +public:
> > > +	DataValue();
> > > +	DataValue(const DataValue &other);
> > > +	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, const DataValue &max)
> > > +	{
> > > +		min_ = min;
> > > +		max_ = max;
> > > +	}
> > > +
> > > +	const DataValue &min() const { return min_; }
> > > +	const DataValue &max() const { return max_; }
> > > +
> > > +private:
> > > +	DataValue max_;
> > > +	DataValue min_;
> > > +};
> > > +
> > > +} /* 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..f07b5fb75a8a
> > > --- /dev/null
> > > +++ b/src/libcamera/data_value.cpp
> > > @@ -0,0 +1,317 @@
> > > +/* 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
> > > + */
> > > +
> >
> > You have a rogue if 0 below
> 
> Ups! thanks!
> 
> > > +#if 0
> > > +/**
> > > + * \class Data
> > > + * \brief Base class for data value and data info instances
> > > + *
> > > + * DataValue and DataInfo share basic information like size and type.
> > > + * This class provides common fields and operations for them.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a Data of \a type
> > > + * \param[in] type The Data type defined by DataType
> > > + */
> > > +Data::Data(DataType type)
> > > +	: type_(type)
> > > +{
> > > +	/*
> > > +	 * \todo The size of compound data types depend on the number of
> > > +	 * elements too.
> > > +	 */
> > > +	size_ = DataSize[type_];
> > > +}
> > > +
> > > +/**
> > > + * \var Data::type_
> > > + * \brief The data type
> > > + */
> > > +
> > > +/**
> > > + * \var Data::size_
> > > + * \brief The data size
> > > + */
> > > +
> > > +#endif
> >
> > To here ...
> >
> > > +
> > > +/**
> > > + * \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_])
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a DataValue copying data from \a other
> > > + * \param[in] other The DataValue to copy data from
> > > + *
> > > + * DataValue copy constructor.
> > > + */
> > > +DataValue::DataValue(const DataValue &other)
> > > +	: type_(other.type()), size_(DataSize[type_])
> >
> > Does this type need a copy constructor currently?
> 
> I think it will be used during serialization implementation
> 
> > Won't C++ just copy the whole object, which would be equivalent?
> 
> Possibly, I wanted this explicit because of the above assignements to
> the correct field. I'll check if I need this.
> 
> > That said, if/when this class becomes more complex then it would likely
> > need some sort of specialisation - so it doesn't hurt to have it already.
> >
> > > +{
> > > +	switch (type_) {
> > > +	case DataTypeBool:
> > > +		bool_ = other.getBool();
> > > +		break;
> > > +	case DataTypeInteger:
> > > +		integer_ = other.getInt();
> > > +		break;
> > > +	case DataTypeInteger64:
> > > +		integer64_ = other.getInt64();
> > > +		break;
> > > +	default:
> > > +		bool_ = integer_ = integer64_ = 0;
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * \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 assignement operator.
> >
> > s/assignement/assignment/
> >
> > > + *
> > > + * \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;
> > > +	default:
> > > +		bool_ = integer_ = integer64_ = 0;
> > > +		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;
> > > +}
> > > +
> > > +/**
> > > + * \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_);
> > > +	default:
> > > +		return "<None>";
> >
> > I haven't tried to compile this yet, but doesn't this generate a
> > compiler warning?
> >
> > I thought C++ was very picky about switch statements not explicitly
> > stating all enum types... Perhaps not when a default statement is provided.
> >
> > changing this to:
> >
> > 	...
> >
> > 	case DataTypeNone:
> > 		return "<None>";
> > 	}
> >
> > 	return "<INVALID TYPE>";
> > }
> >
> > would in that case ensure that this function is always updated (forced
> > by the compiler) whenever a new type is added?
> 
> Yeah, default catches all the remaning cases but hides issues when we
> miss adding a value. I'll change.
> 
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * \class DataInfo
> > > + * \brief Validation informations associated with a data value
> > > + *
> > > + * The DataInfo class represents static information associated with data
> > > + * types that represent plymorhpic values abstracted by the DataValue class.
> >
> > s/plymorhpic/polymorphic/
> >
> > > + *
> > > + * DataInfo stores static informations such as the value minimum and maximum
> > > + * 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/data_value.cpp b/test/data_value/data_value.cpp
> > > new file mode 100644
> > > index 000000000000..64061000c1e4
> > > --- /dev/null
> > > +++ b/test/data_value/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;
> > > +
> > > +		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/data_value/meson.build b/test/data_value/meson.build
> > > new file mode 100644
> > > index 000000000000..3858e6085a1f
> > > --- /dev/null
> > > +++ b/test/data_value/meson.build
> > > @@ -0,0 +1,12 @@
> > > +data_value_tests = [
> > > +    [ 'data_value',   'data_value.cpp' ],
> > > +]
> > > +
> > > +foreach t : data_value_tests
> > > +    exe = executable(t[0], t[1],
> > > +                     dependencies : libcamera_dep,
> > > +                     link_with : test_libraries,
> > > +                     include_directories : test_includes_internal)
> > > +    test(t[0], exe, suite : 'data_value', is_parallel : false)
> > > +endforeach
> >
> >
> > Does data_value need a whole subdirectory for a single test? Will it
> > have multiple tests in the (near) future?
> 
> I thought so, but in the end I only added one
> 
> > I really dislike adding a subdirectory/test suite just for a single test
> > unless we really know it's going to be extended.
> 
> Where would you put this?
> 
> > > +
> > > diff --git a/test/meson.build b/test/meson.build
> > > index 84722cceb35d..5d414b22dc0c 100644
> > > --- a/test/meson.build
> > > +++ b/test/meson.build
> > > @@ -2,6 +2,7 @@ subdir('libtest')
> > >
> > >  subdir('camera')
> > >  subdir('controls')
> > > +subdir('data_value')
> > >  subdir('ipa')
> > >  subdir('ipc')
> > >  subdir('log')