[libcamera-devel] [PATCH v2 1/5] libcamera: Create DataValue and DataInfo
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Sep 26 17:17:09 CEST 2019
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 at 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'],
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list