[libcamera-devel] [PATCH 1/5] libcamera: Create DataValue and DataInfo
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 20 12:03:55 CEST 2019
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list