[libcamera-devel] [PATCH 1/5] libcamera: Create DataValue and DataInfo
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 23 13:02:57 CEST 2019
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 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?
>
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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190923/8a1ae297/attachment.sig>
More information about the libcamera-devel
mailing list