[libcamera-devel,03/11] libcamera: controls: Add support for string controls
diff mbox series

Message ID 20200309162414.720306-4-jacopo@jmondi.org
State Accepted
Headers show
Series
  • Adda support for V4L2 array control and strings
Related show

Commit Message

Jacopo Mondi March 9, 2020, 4:24 p.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

String controls are stored internally as an array of char, but the
ControlValue constructor, get() and set() functions operate on an
std::string for convenience. Array of strings are thus not supported.

Unlike for other control types, the ControlInfo range reports the
minimum and maximum allowed lengths of the string (the minimum will
usually be 0), not the minimum and maximum value of each element.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/controls.h         | 30 +++++++++++++++++++-----
 src/libcamera/control_serializer.cpp | 22 +++++++++++++++++
 src/libcamera/controls.cpp           | 35 ++++++++++++++++++++++++----
 src/libcamera/gen-controls.py        | 18 +++++++-------
 4 files changed, 87 insertions(+), 18 deletions(-)

Comments

Jacopo Mondi March 20, 2020, 12:08 p.m. UTC | #1 
Hi Laurent,

On Mon, Mar 09, 2020 at 05:24:06PM +0100, Jacopo Mondi wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> String controls are stored internally as an array of char, but the
> ControlValue constructor, get() and set() functions operate on an
> std::string for convenience. Array of strings are thus not supported.
>
> Unlike for other control types, the ControlInfo range reports the
> minimum and maximum allowed lengths of the string (the minimum will
> usually be 0), not the minimum and maximum value of each element.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/controls.h         | 30 +++++++++++++++++++-----
>  src/libcamera/control_serializer.cpp | 22 +++++++++++++++++
>  src/libcamera/controls.cpp           | 35 ++++++++++++++++++++++++----
>  src/libcamera/gen-controls.py        | 18 +++++++-------
>  4 files changed, 87 insertions(+), 18 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 9c6cbffb88b5..5cf6280e612b 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -26,6 +26,7 @@ enum ControlType {
>  	ControlTypeInteger32,
>  	ControlTypeInteger64,
>  	ControlTypeFloat,
> +	ControlTypeString,
>  };
>
>  namespace details {
> @@ -64,6 +65,11 @@ struct control_type<float> {
>  	static constexpr ControlType value = ControlTypeFloat;
>  };
>
> +template<>
> +struct control_type<std::string> {
> +	static constexpr ControlType value = ControlTypeString;
> +};
> +
>  template<typename T, std::size_t N>
>  struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
>  };
> @@ -76,7 +82,9 @@ public:
>  	ControlValue();
>
>  #ifndef __DOXYGEN__
> -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  	ControlValue(const T &value)
>  		: type_(ControlTypeNone), numElements_(0)
>  	{
> @@ -84,7 +92,9 @@ public:
>  		    &value, 1, sizeof(T));
>  	}
>
> -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  #else
>  	template<typename T>
>  #endif
> @@ -115,7 +125,9 @@ public:
>  	}
>
>  #ifndef __DOXYGEN__
> -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  	T get() const
>  	{
>  		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> @@ -124,7 +136,9 @@ public:
>  		return *reinterpret_cast<const T *>(data().data());
>  	}
>
> -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  #else
>  	template<typename T>
>  #endif
> @@ -139,14 +153,18 @@ public:
>  	}
>
>  #ifndef __DOXYGEN__
> -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  	void set(const T &value)
>  	{
>  		set(details::control_type<std::remove_cv_t<T>>::value, false,
>  		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
>  	}
>
> -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> +						       std::nullptr_t> = nullptr>
>  #else
>  	template<typename T>
>  #endif
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index eef875f4d96c..808419f246c0 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -314,6 +314,22 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
>  	return value;
>  }
>
> +template<>
> +ControlValue ControlSerializer::loadControlValue<std::string>(ByteStreamBuffer &buffer,
> +						 bool isArray,
> +						 unsigned int count)

Not may way around this specialization I gueess..
I see std::string being defined as std::basic_string<char> and I was
wondering if we could reuse the <char>  template argument, but I don't
see a clear way to do so

Anyway, ControlTypeFloat documentation removal apart
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +{
> +	ControlValue value;
> +
> +	const char *data = buffer.read<char>(count);
> +	if (!data)
> +		return value;
> +
> +	value.set(std::string{ data, count });
> +
> +	return value;
> +}
> +
>  ControlValue ControlSerializer::loadControlValue(ControlType type,
>  						 ByteStreamBuffer &buffer,
>  						 bool isArray,
> @@ -335,6 +351,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
>  	case ControlTypeFloat:
>  		return loadControlValue<float>(buffer, isArray, count);
>
> +	case ControlTypeString:
> +		return loadControlValue<std::string>(buffer, isArray, count);
> +
>  	case ControlTypeNone:
>  		return ControlValue();
>  	}
> @@ -345,6 +364,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
>  ControlInfo ControlSerializer::loadControlInfo(ControlType type,
>  					       ByteStreamBuffer &b)
>  {
> +	if (type == ControlTypeString)
> +		type = ControlTypeInteger32;
> +
>  	ControlValue min = loadControlValue(type, b);
>  	ControlValue max = loadControlValue(type, b);
>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 53649fe897db..68d040db4daa 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -57,6 +57,7 @@ static constexpr size_t ControlValueSize[] = {
>  	[ControlTypeInteger32]		= sizeof(int32_t),
>  	[ControlTypeInteger64]		= sizeof(int64_t),
>  	[ControlTypeFloat]		= sizeof(float),
> +	[ControlTypeString]		= sizeof(char),
>  };
>
>  } /* namespace */
> @@ -74,8 +75,8 @@ static constexpr size_t ControlValueSize[] = {
>   * The control stores a 32-bit integer value
>   * \var ControlTypeInteger64
>   * The control stores a 64-bit integer value
> - * \var ControlTypeFloat
> - * The control stores a 32-bit floating point value

What did the poor Float type do to you ? :(

> + * \var ControlTypeString
> + * The control stores a string value as an array of char
>   */
>
>  /**
> @@ -164,7 +165,8 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
>   * \brief Retrieve the number of elements stored in the ControlValue
>   *
>   * For instances storing an array, this function returns the number of elements
> - * in the array. Otherwise, it returns 1.
> + * in the array. For instances storing a string, it returns the length of the
> + * string, not counting the terminating '\0'. Otherwise, it returns 1.
>   *
>   * \return The number of elements stored in the ControlValue
>   */
> @@ -192,6 +194,11 @@ std::string ControlValue::toString() const
>  		return "<ValueType Error>";
>
>  	const uint8_t *data = ControlValue::data().data();
> +
> +	if (type_ == ControlTypeString)
> +		return std::string(reinterpret_cast<const char *>(data),
> +				   numElements_);
> +
>  	std::string str(isArray_ ? "[ " : "");
>
>  	for (unsigned int i = 0; i < numElements_; ++i) {
> @@ -222,6 +229,7 @@ std::string ControlValue::toString() const
>  			break;
>  		}
>  		case ControlTypeNone:
> +		case ControlTypeString:
>  			break;
>  		}
>
> @@ -439,12 +447,22 @@ ControlInfo::ControlInfo(const ControlValue &min,
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> + *
> + * For string controls, this is the minimum length of the string, not counting
> + * the terminating '\0'. For all other control types, this is the minimum value
> + * of each element.
> + *
>   * \return A ControlValue with the minimum value for the control
>   */
>
>  /**
>   * \fn ControlInfo::max()
>   * \brief Retrieve the maximum value of the control
> + *
> + * For string controls, this is the maximum length of the string, not counting
> + * the terminating '\0'. For all other control types, this is the maximum value
> + * of each element.
> + *
>   * \return A ControlValue with the maximum value for the control
>   */
>
> @@ -653,7 +671,16 @@ void ControlInfoMap::generateIdmap()
>  	idmap_.clear();
>
>  	for (const auto &ctrl : *this) {
> -		if (ctrl.first->type() != ctrl.second.min().type()) {
> +		/*
> +		 * For string controls, min and max define the valid
> +		 * range for the string size, not for the individual
> +		 * values.
> +		 */
> +		ControlType rangeType = ctrl.first->type() == ControlTypeString
> +				      ? ControlTypeInteger32 : ctrl.first->type();
> +		const ControlInfo &info = ctrl.second;
> +
> +		if (info.min().type() != rangeType) {
>  			LOG(Controls, Error)
>  				<< "Control " << utils::hex(ctrl.first->id())
>  				<< " type and info type mismatch";
> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
> index ff8bda6b16c1..87c3d52ada6d 100755
> --- a/src/libcamera/gen-controls.py
> +++ b/src/libcamera/gen-controls.py
> @@ -42,10 +42,11 @@ ${description}
>          name, ctrl = ctrl.popitem()
>          id_name = snake_case(name).upper()
>
> -        if ctrl.get('size'):
> -            ctrl_type = 'Span<const %s>' % ctrl['type']
> -        else:
> -            ctrl_type = ctrl['type']
> +        ctrl_type = ctrl['type']
> +        if ctrl_type == 'string':
> +            ctrl_type = 'std::string'
> +        elif ctrl.get('size'):
> +            ctrl_type = 'Span<const %s>' % ctrl_type
>
>          info = {
>              'name': name,
> @@ -97,10 +98,11 @@ def generate_h(controls):
>
>          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
>
> -        if ctrl.get('size'):
> -            ctrl_type = 'Span<const %s>' % ctrl['type']
> -        else:
> -            ctrl_type = ctrl['type']
> +        ctrl_type = ctrl['type']
> +        if ctrl_type == 'string':
> +            ctrl_type = 'std::string'
> +        elif ctrl.get('size'):
> +            ctrl_type = 'Span<const %s>' % ctrl_type
>
>          info = {
>              'name': name,
> --
> 2.25.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 20, 2020, 12:12 p.m. UTC | #2 
Hi Jacopo,

On Fri, Mar 20, 2020 at 01:08:44PM +0100, Jacopo Mondi wrote:
> On Mon, Mar 09, 2020 at 05:24:06PM +0100, Jacopo Mondi wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > String controls are stored internally as an array of char, but the
> > ControlValue constructor, get() and set() functions operate on an
> > std::string for convenience. Array of strings are thus not supported.
> >
> > Unlike for other control types, the ControlInfo range reports the
> > minimum and maximum allowed lengths of the string (the minimum will
> > usually be 0), not the minimum and maximum value of each element.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/controls.h         | 30 +++++++++++++++++++-----
> >  src/libcamera/control_serializer.cpp | 22 +++++++++++++++++
> >  src/libcamera/controls.cpp           | 35 ++++++++++++++++++++++++----
> >  src/libcamera/gen-controls.py        | 18 +++++++-------
> >  4 files changed, 87 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 9c6cbffb88b5..5cf6280e612b 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -26,6 +26,7 @@ enum ControlType {
> >  	ControlTypeInteger32,
> >  	ControlTypeInteger64,
> >  	ControlTypeFloat,
> > +	ControlTypeString,
> >  };
> >
> >  namespace details {
> > @@ -64,6 +65,11 @@ struct control_type<float> {
> >  	static constexpr ControlType value = ControlTypeFloat;
> >  };
> >
> > +template<>
> > +struct control_type<std::string> {
> > +	static constexpr ControlType value = ControlTypeString;
> > +};
> > +
> >  template<typename T, std::size_t N>
> >  struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> >  };
> > @@ -76,7 +82,9 @@ public:
> >  	ControlValue();
> >
> >  #ifndef __DOXYGEN__
> > -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  	ControlValue(const T &value)
> >  		: type_(ControlTypeNone), numElements_(0)
> >  	{
> > @@ -84,7 +92,9 @@ public:
> >  		    &value, 1, sizeof(T));
> >  	}
> >
> > -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  #else
> >  	template<typename T>
> >  #endif
> > @@ -115,7 +125,9 @@ public:
> >  	}
> >
> >  #ifndef __DOXYGEN__
> > -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  	T get() const
> >  	{
> >  		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> > @@ -124,7 +136,9 @@ public:
> >  		return *reinterpret_cast<const T *>(data().data());
> >  	}
> >
> > -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  #else
> >  	template<typename T>
> >  #endif
> > @@ -139,14 +153,18 @@ public:
> >  	}
> >
> >  #ifndef __DOXYGEN__
> > -	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > +						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  	void set(const T &value)
> >  	{
> >  		set(details::control_type<std::remove_cv_t<T>>::value, false,
> >  		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
> >  	}
> >
> > -	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > +	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > +						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> > +						       std::nullptr_t> = nullptr>
> >  #else
> >  	template<typename T>
> >  #endif
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index eef875f4d96c..808419f246c0 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -314,6 +314,22 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
> >  	return value;
> >  }
> >
> > +template<>
> > +ControlValue ControlSerializer::loadControlValue<std::string>(ByteStreamBuffer &buffer,
> > +						 bool isArray,
> > +						 unsigned int count)
> 
> Not may way around this specialization I gueess..
> I see std::string being defined as std::basic_string<char> and I was
> wondering if we could reuse the <char>  template argument, but I don't
> see a clear way to do so

This is also the part of this patch that I dislike, but for now I think
it's the best option.

> Anyway, ControlTypeFloat documentation removal apart
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > +{
> > +	ControlValue value;
> > +
> > +	const char *data = buffer.read<char>(count);
> > +	if (!data)
> > +		return value;
> > +
> > +	value.set(std::string{ data, count });
> > +
> > +	return value;
> > +}
> > +
> >  ControlValue ControlSerializer::loadControlValue(ControlType type,
> >  						 ByteStreamBuffer &buffer,
> >  						 bool isArray,
> > @@ -335,6 +351,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
> >  	case ControlTypeFloat:
> >  		return loadControlValue<float>(buffer, isArray, count);
> >
> > +	case ControlTypeString:
> > +		return loadControlValue<std::string>(buffer, isArray, count);
> > +
> >  	case ControlTypeNone:
> >  		return ControlValue();
> >  	}
> > @@ -345,6 +364,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
> >  ControlInfo ControlSerializer::loadControlInfo(ControlType type,
> >  					       ByteStreamBuffer &b)
> >  {
> > +	if (type == ControlTypeString)
> > +		type = ControlTypeInteger32;
> > +
> >  	ControlValue min = loadControlValue(type, b);
> >  	ControlValue max = loadControlValue(type, b);
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 53649fe897db..68d040db4daa 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -57,6 +57,7 @@ static constexpr size_t ControlValueSize[] = {
> >  	[ControlTypeInteger32]		= sizeof(int32_t),
> >  	[ControlTypeInteger64]		= sizeof(int64_t),
> >  	[ControlTypeFloat]		= sizeof(float),
> > +	[ControlTypeString]		= sizeof(char),
> >  };
> >
> >  } /* namespace */
> > @@ -74,8 +75,8 @@ static constexpr size_t ControlValueSize[] = {
> >   * The control stores a 32-bit integer value
> >   * \var ControlTypeInteger64
> >   * The control stores a 64-bit integer value
> > - * \var ControlTypeFloat
> > - * The control stores a 32-bit floating point value
> 
> What did the poor Float type do to you ? :(

Oops. My sincere apologies to all the Floats in this world, I never
meant to wish for your disappearance, even if you can't express the
speed of light with enough accuracy
(https://git.linuxtv.org/libcamera.git/tree/test/controls/control_value.cpp#n228).

> > + * \var ControlTypeString
> > + * The control stores a string value as an array of char
> >   */
> >
> >  /**
> > @@ -164,7 +165,8 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
> >   * \brief Retrieve the number of elements stored in the ControlValue
> >   *
> >   * For instances storing an array, this function returns the number of elements
> > - * in the array. Otherwise, it returns 1.
> > + * in the array. For instances storing a string, it returns the length of the
> > + * string, not counting the terminating '\0'. Otherwise, it returns 1.
> >   *
> >   * \return The number of elements stored in the ControlValue
> >   */
> > @@ -192,6 +194,11 @@ std::string ControlValue::toString() const
> >  		return "<ValueType Error>";
> >
> >  	const uint8_t *data = ControlValue::data().data();
> > +
> > +	if (type_ == ControlTypeString)
> > +		return std::string(reinterpret_cast<const char *>(data),
> > +				   numElements_);
> > +
> >  	std::string str(isArray_ ? "[ " : "");
> >
> >  	for (unsigned int i = 0; i < numElements_; ++i) {
> > @@ -222,6 +229,7 @@ std::string ControlValue::toString() const
> >  			break;
> >  		}
> >  		case ControlTypeNone:
> > +		case ControlTypeString:
> >  			break;
> >  		}
> >
> > @@ -439,12 +447,22 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >  /**
> >   * \fn ControlInfo::min()
> >   * \brief Retrieve the minimum value of the control
> > + *
> > + * For string controls, this is the minimum length of the string, not counting
> > + * the terminating '\0'. For all other control types, this is the minimum value
> > + * of each element.
> > + *
> >   * \return A ControlValue with the minimum value for the control
> >   */
> >
> >  /**
> >   * \fn ControlInfo::max()
> >   * \brief Retrieve the maximum value of the control
> > + *
> > + * For string controls, this is the maximum length of the string, not counting
> > + * the terminating '\0'. For all other control types, this is the maximum value
> > + * of each element.
> > + *
> >   * \return A ControlValue with the maximum value for the control
> >   */
> >
> > @@ -653,7 +671,16 @@ void ControlInfoMap::generateIdmap()
> >  	idmap_.clear();
> >
> >  	for (const auto &ctrl : *this) {
> > -		if (ctrl.first->type() != ctrl.second.min().type()) {
> > +		/*
> > +		 * For string controls, min and max define the valid
> > +		 * range for the string size, not for the individual
> > +		 * values.
> > +		 */
> > +		ControlType rangeType = ctrl.first->type() == ControlTypeString
> > +				      ? ControlTypeInteger32 : ctrl.first->type();
> > +		const ControlInfo &info = ctrl.second;
> > +
> > +		if (info.min().type() != rangeType) {
> >  			LOG(Controls, Error)
> >  				<< "Control " << utils::hex(ctrl.first->id())
> >  				<< " type and info type mismatch";
> > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
> > index ff8bda6b16c1..87c3d52ada6d 100755
> > --- a/src/libcamera/gen-controls.py
> > +++ b/src/libcamera/gen-controls.py
> > @@ -42,10 +42,11 @@ ${description}
> >          name, ctrl = ctrl.popitem()
> >          id_name = snake_case(name).upper()
> >
> > -        if ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl['type']
> > -        else:
> > -            ctrl_type = ctrl['type']
> > +        ctrl_type = ctrl['type']
> > +        if ctrl_type == 'string':
> > +            ctrl_type = 'std::string'
> > +        elif ctrl.get('size'):
> > +            ctrl_type = 'Span<const %s>' % ctrl_type
> >
> >          info = {
> >              'name': name,
> > @@ -97,10 +98,11 @@ def generate_h(controls):
> >
> >          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> >
> > -        if ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl['type']
> > -        else:
> > -            ctrl_type = ctrl['type']
> > +        ctrl_type = ctrl['type']
> > +        if ctrl_type == 'string':
> > +            ctrl_type = 'std::string'
> > +        elif ctrl.get('size'):
> > +            ctrl_type = 'Span<const %s>' % ctrl_type
> >
> >          info = {
> >              'name': name,

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 9c6cbffb88b5..5cf6280e612b 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -26,6 +26,7 @@  enum ControlType {
 	ControlTypeInteger32,
 	ControlTypeInteger64,
 	ControlTypeFloat,
+	ControlTypeString,
 };
 
 namespace details {
@@ -64,6 +65,11 @@  struct control_type<float> {
 	static constexpr ControlType value = ControlTypeFloat;
 };
 
+template<>
+struct control_type<std::string> {
+	static constexpr ControlType value = ControlTypeString;
+};
+
 template<typename T, std::size_t N>
 struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
 };
@@ -76,7 +82,9 @@  public:
 	ControlValue();
 
 #ifndef __DOXYGEN__
-	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
+						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 	ControlValue(const T &value)
 		: type_(ControlTypeNone), numElements_(0)
 	{
@@ -84,7 +92,9 @@  public:
 		    &value, 1, sizeof(T));
 	}
 
-	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
+						       std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 #else
 	template<typename T>
 #endif
@@ -115,7 +125,9 @@  public:
 	}
 
 #ifndef __DOXYGEN__
-	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
+						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 	T get() const
 	{
 		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
@@ -124,7 +136,9 @@  public:
 		return *reinterpret_cast<const T *>(data().data());
 	}
 
-	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
+						       std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 #else
 	template<typename T>
 #endif
@@ -139,14 +153,18 @@  public:
 	}
 
 #ifndef __DOXYGEN__
-	template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
+						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 	void set(const T &value)
 	{
 		set(details::control_type<std::remove_cv_t<T>>::value, false,
 		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
 	}
 
-	template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
+	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
+						       std::is_same<std::string, std::remove_cv_t<T>>::value,
+						       std::nullptr_t> = nullptr>
 #else
 	template<typename T>
 #endif
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index eef875f4d96c..808419f246c0 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -314,6 +314,22 @@  ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
 	return value;
 }
 
+template<>
+ControlValue ControlSerializer::loadControlValue<std::string>(ByteStreamBuffer &buffer,
+						 bool isArray,
+						 unsigned int count)
+{
+	ControlValue value;
+
+	const char *data = buffer.read<char>(count);
+	if (!data)
+		return value;
+
+	value.set(std::string{ data, count });
+
+	return value;
+}
+
 ControlValue ControlSerializer::loadControlValue(ControlType type,
 						 ByteStreamBuffer &buffer,
 						 bool isArray,
@@ -335,6 +351,9 @@  ControlValue ControlSerializer::loadControlValue(ControlType type,
 	case ControlTypeFloat:
 		return loadControlValue<float>(buffer, isArray, count);
 
+	case ControlTypeString:
+		return loadControlValue<std::string>(buffer, isArray, count);
+
 	case ControlTypeNone:
 		return ControlValue();
 	}
@@ -345,6 +364,9 @@  ControlValue ControlSerializer::loadControlValue(ControlType type,
 ControlInfo ControlSerializer::loadControlInfo(ControlType type,
 					       ByteStreamBuffer &b)
 {
+	if (type == ControlTypeString)
+		type = ControlTypeInteger32;
+
 	ControlValue min = loadControlValue(type, b);
 	ControlValue max = loadControlValue(type, b);
 
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 53649fe897db..68d040db4daa 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -57,6 +57,7 @@  static constexpr size_t ControlValueSize[] = {
 	[ControlTypeInteger32]		= sizeof(int32_t),
 	[ControlTypeInteger64]		= sizeof(int64_t),
 	[ControlTypeFloat]		= sizeof(float),
+	[ControlTypeString]		= sizeof(char),
 };
 
 } /* namespace */
@@ -74,8 +75,8 @@  static constexpr size_t ControlValueSize[] = {
  * The control stores a 32-bit integer value
  * \var ControlTypeInteger64
  * The control stores a 64-bit integer value
- * \var ControlTypeFloat
- * The control stores a 32-bit floating point value
+ * \var ControlTypeString
+ * The control stores a string value as an array of char
  */
 
 /**
@@ -164,7 +165,8 @@  ControlValue &ControlValue::operator=(const ControlValue &other)
  * \brief Retrieve the number of elements stored in the ControlValue
  *
  * For instances storing an array, this function returns the number of elements
- * in the array. Otherwise, it returns 1.
+ * in the array. For instances storing a string, it returns the length of the
+ * string, not counting the terminating '\0'. Otherwise, it returns 1.
  *
  * \return The number of elements stored in the ControlValue
  */
@@ -192,6 +194,11 @@  std::string ControlValue::toString() const
 		return "<ValueType Error>";
 
 	const uint8_t *data = ControlValue::data().data();
+
+	if (type_ == ControlTypeString)
+		return std::string(reinterpret_cast<const char *>(data),
+				   numElements_);
+
 	std::string str(isArray_ ? "[ " : "");
 
 	for (unsigned int i = 0; i < numElements_; ++i) {
@@ -222,6 +229,7 @@  std::string ControlValue::toString() const
 			break;
 		}
 		case ControlTypeNone:
+		case ControlTypeString:
 			break;
 		}
 
@@ -439,12 +447,22 @@  ControlInfo::ControlInfo(const ControlValue &min,
 /**
  * \fn ControlInfo::min()
  * \brief Retrieve the minimum value of the control
+ *
+ * For string controls, this is the minimum length of the string, not counting
+ * the terminating '\0'. For all other control types, this is the minimum value
+ * of each element.
+ *
  * \return A ControlValue with the minimum value for the control
  */
 
 /**
  * \fn ControlInfo::max()
  * \brief Retrieve the maximum value of the control
+ *
+ * For string controls, this is the maximum length of the string, not counting
+ * the terminating '\0'. For all other control types, this is the maximum value
+ * of each element.
+ *
  * \return A ControlValue with the maximum value for the control
  */
 
@@ -653,7 +671,16 @@  void ControlInfoMap::generateIdmap()
 	idmap_.clear();
 
 	for (const auto &ctrl : *this) {
-		if (ctrl.first->type() != ctrl.second.min().type()) {
+		/*
+		 * For string controls, min and max define the valid
+		 * range for the string size, not for the individual
+		 * values.
+		 */
+		ControlType rangeType = ctrl.first->type() == ControlTypeString
+				      ? ControlTypeInteger32 : ctrl.first->type();
+		const ControlInfo &info = ctrl.second;
+
+		if (info.min().type() != rangeType) {
 			LOG(Controls, Error)
 				<< "Control " << utils::hex(ctrl.first->id())
 				<< " type and info type mismatch";
diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
index ff8bda6b16c1..87c3d52ada6d 100755
--- a/src/libcamera/gen-controls.py
+++ b/src/libcamera/gen-controls.py
@@ -42,10 +42,11 @@  ${description}
         name, ctrl = ctrl.popitem()
         id_name = snake_case(name).upper()
 
-        if ctrl.get('size'):
-            ctrl_type = 'Span<const %s>' % ctrl['type']
-        else:
-            ctrl_type = ctrl['type']
+        ctrl_type = ctrl['type']
+        if ctrl_type == 'string':
+            ctrl_type = 'std::string'
+        elif ctrl.get('size'):
+            ctrl_type = 'Span<const %s>' % ctrl_type
 
         info = {
             'name': name,
@@ -97,10 +98,11 @@  def generate_h(controls):
 
         ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
 
-        if ctrl.get('size'):
-            ctrl_type = 'Span<const %s>' % ctrl['type']
-        else:
-            ctrl_type = ctrl['type']
+        ctrl_type = ctrl['type']
+        if ctrl_type == 'string':
+            ctrl_type = 'std::string'
+        elif ctrl.get('size'):
+            ctrl_type = 'Span<const %s>' % ctrl_type
 
         info = {
             'name': name,