[libcamera-devel] [PATCH 01/12] libcamera: controls: Rename ControlValueType to ControlType

Laurent Pinchart laurent.pinchart at ideasonboard.com
Sun Sep 29 13:24:48 CEST 2019


Hi Jacopo,

On Sun, Sep 29, 2019 at 10:52:26AM +0200, Jacopo Mondi wrote:
> On Sat, Sep 28, 2019 at 06:22:27PM +0300, Laurent Pinchart wrote:
> > The type of a control value is also the type of the control. Shorten the
> > ControlValueType enumeration to ControlType, and rename ControlValue* to
> > ControlType* for better clarity.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/controls.h   | 20 +++++++-------
> >  src/libcamera/controls.cpp     | 50 +++++++++++++++++-----------------
> >  src/libcamera/gen-controls.awk |  2 +-
> >  test/controls/control_info.cpp |  4 +--
> >  4 files changed, 38 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index fbb3a62274c6..ffba880a66ff 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -18,11 +18,11 @@ namespace libcamera {
> >
> >  class Camera;
> >
> > -enum ControlValueType {
> > -	ControlValueNone,
> > -	ControlValueBool,
> > -	ControlValueInteger,
> > -	ControlValueInteger64,
> > +enum ControlType {
> > +	ControlTypeNone,
> > +	ControlTypeBool,
> > +	ControlTypeInteger,
> > +	ControlTypeInteger64,
> 
> I wonder if these would not be worth being ControlBool,
> ControlInteger etc...

It's a very good question, and something we may want to think of more
globally. Should enumerated values contain the enum name ? We have
precedents for both in libcamera, and unifying the code with a single
policy would be more consistent. The only issue I have at the moment is
that I'm not sure what the rule should be :-)

> In any case
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  };
> >
> >  class ControlValue
> > @@ -33,8 +33,8 @@ public:
> >  	ControlValue(int value);
> >  	ControlValue(int64_t value);
> >
> > -	ControlValueType type() const { return type_; };
> > -	bool isNone() const { return type_ == ControlValueNone; };
> > +	ControlType type() const { return type_; };
> > +	bool isNone() const { return type_ == ControlTypeNone; };
> >
> >  	void set(bool value);
> >  	void set(int value);
> > @@ -47,7 +47,7 @@ public:
> >  	std::string toString() const;
> >
> >  private:
> > -	ControlValueType type_;
> > +	ControlType type_;
> >
> >  	union {
> >  		bool bool_;
> > @@ -59,7 +59,7 @@ private:
> >  struct ControlIdentifier {
> >  	ControlId id;
> >  	const char *name;
> > -	ControlValueType type;
> > +	ControlType type;
> >  };
> >
> >  class ControlInfo
> > @@ -70,7 +70,7 @@ public:
> >
> >  	ControlId id() const { return ident_->id; }
> >  	const char *name() const { return ident_->name; }
> > -	ControlValueType type() const { return ident_->type; }
> > +	ControlType type() const { return ident_->type; }
> >
> >  	const ControlValue &min() const { return min_; }
> >  	const ControlValue &max() const { return max_; }
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 727fdbd9450d..9960a30dfa03 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -25,16 +25,16 @@ namespace libcamera {
> >  LOG_DEFINE_CATEGORY(Controls)
> >
> >  /**
> > - * \enum ControlValueType
> > - * \brief Define the data type of value represented by a ControlValue
> > - * \var ControlValueNone
> > - * Identifies an unset control value
> > - * \var ControlValueBool
> > - * Identifies controls storing a boolean value
> > - * \var ControlValueInteger
> > - * Identifies controls storing an integer value
> > - * \var ControlValueInteger64
> > - * Identifies controls storing a 64-bit integer value
> > + * \enum ControlType
> > + * \brief Define the data type of a Control
> > + * \var ControlTypeNone
> > + * Invalid type, for empty values
> > + * \var ControlTypeBool
> > + * The control stores a boolean value
> > + * \var ControlTypeInteger
> > + * The control stores an integer value
> > + * \var ControlTypeInteger64
> > + * The control stores a 64-bit integer value
> >   */
> >
> >  /**
> > @@ -46,7 +46,7 @@ LOG_DEFINE_CATEGORY(Controls)
> >   * \brief Construct an empty ControlValue.
> >   */
> >  ControlValue::ControlValue()
> > -	: type_(ControlValueNone)
> > +	: type_(ControlTypeNone)
> >  {
> >  }
> >
> > @@ -55,7 +55,7 @@ ControlValue::ControlValue()
> >   * \param[in] value Boolean value to store
> >   */
> >  ControlValue::ControlValue(bool value)
> > -	: type_(ControlValueBool), bool_(value)
> > +	: type_(ControlTypeBool), bool_(value)
> >  {
> >  }
> >
> > @@ -64,7 +64,7 @@ ControlValue::ControlValue(bool value)
> >   * \param[in] value Integer value to store
> >   */
> >  ControlValue::ControlValue(int value)
> > -	: type_(ControlValueInteger), integer_(value)
> > +	: type_(ControlTypeInteger), integer_(value)
> >  {
> >  }
> >
> > @@ -73,7 +73,7 @@ ControlValue::ControlValue(int value)
> >   * \param[in] value Integer value to store
> >   */
> >  ControlValue::ControlValue(int64_t value)
> > -	: type_(ControlValueInteger64), integer64_(value)
> > +	: type_(ControlTypeInteger64), integer64_(value)
> >  {
> >  }
> >
> > @@ -86,7 +86,7 @@ ControlValue::ControlValue(int64_t value)
> >  /**
> >   * \fn ControlValue::isNone()
> >   * \brief Determine if the value is not initialised
> > - * \return True if the value type is ControlValueNone, false otherwise
> > + * \return True if the value type is ControlTypeNone, false otherwise
> >   */
> >
> >  /**
> > @@ -95,7 +95,7 @@ ControlValue::ControlValue(int64_t value)
> >   */
> >  void ControlValue::set(bool value)
> >  {
> > -	type_ = ControlValueBool;
> > +	type_ = ControlTypeBool;
> >  	bool_ = value;
> >  }
> >
> > @@ -105,7 +105,7 @@ void ControlValue::set(bool value)
> >   */
> >  void ControlValue::set(int value)
> >  {
> > -	type_ = ControlValueInteger;
> > +	type_ = ControlTypeInteger;
> >  	integer_ = value;
> >  }
> >
> > @@ -115,7 +115,7 @@ void ControlValue::set(int value)
> >   */
> >  void ControlValue::set(int64_t value)
> >  {
> > -	type_ = ControlValueInteger64;
> > +	type_ = ControlTypeInteger64;
> >  	integer64_ = value;
> >  }
> >
> > @@ -128,7 +128,7 @@ void ControlValue::set(int64_t value)
> >   */
> >  bool ControlValue::getBool() const
> >  {
> > -	ASSERT(type_ == ControlValueBool);
> > +	ASSERT(type_ == ControlTypeBool);
> >
> >  	return bool_;
> >  }
> > @@ -142,7 +142,7 @@ bool ControlValue::getBool() const
> >   */
> >  int ControlValue::getInt() const
> >  {
> > -	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> > +	ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> >
> >  	return integer_;
> >  }
> > @@ -156,7 +156,7 @@ int ControlValue::getInt() const
> >   */
> >  int64_t ControlValue::getInt64() const
> >  {
> > -	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> > +	ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> >
> >  	return integer64_;
> >  }
> > @@ -168,13 +168,13 @@ int64_t ControlValue::getInt64() const
> >  std::string ControlValue::toString() const
> >  {
> >  	switch (type_) {
> > -	case ControlValueNone:
> > +	case ControlTypeNone:
> >  		return "<None>";
> > -	case ControlValueBool:
> > +	case ControlTypeBool:
> >  		return bool_ ? "True" : "False";
> > -	case ControlValueInteger:
> > +	case ControlTypeInteger:
> >  		return std::to_string(integer_);
> > -	case ControlValueInteger64:
> > +	case ControlTypeInteger64:
> >  		return std::to_string(integer64_);
> >  	}
> >
> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > index f3d068123012..a3f291e7071c 100755
> > --- a/src/libcamera/gen-controls.awk
> > +++ b/src/libcamera/gen-controls.awk
> > @@ -92,7 +92,7 @@ function GenerateTable(file) {
> >  	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> >  	print "controlTypes {" > file
> >  	for (i=1; i <= id; ++i) {
> > -		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> > +		printf "\t{ %s, { %s, \"%s\", ControlType%s } },\n", names[i], names[i], names[i], types[i] > file
> >  	}
> >  	print "};" > file
> >  	ExitNameSpace(file)
> > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> > index aa3a65b1e5ef..8cda860b9fe9 100644
> > --- a/test/controls/control_info.cpp
> > +++ b/test/controls/control_info.cpp
> > @@ -26,7 +26,7 @@ protected:
> >  		ControlInfo info(Brightness);
> >
> >  		if (info.id() != Brightness ||
> > -		    info.type() != ControlValueInteger ||
> > +		    info.type() != ControlTypeInteger ||
> >  		    info.name() != std::string("Brightness")) {
> >  			cout << "Invalid control identification for Brightness" << endl;
> >  			return TestFail;
> > @@ -44,7 +44,7 @@ protected:
> >  		info = ControlInfo(Contrast, 10, 200);
> >
> >  		if (info.id() != Contrast ||
> > -		    info.type() != ControlValueInteger ||
> > +		    info.type() != ControlTypeInteger ||
> >  		    info.name() != std::string("Contrast")) {
> >  			cout << "Invalid control identification for Contrast" << endl;
> >  			return TestFail;
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list