[libcamera-devel] [PATCH 1/5] libcamera: Create DataValue and DataInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 27 00:12:40 CEST 2019
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 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
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')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list