[libcamera-devel,6/9] libcamera: v4l2_controls: Add V4L2ControlId
diff mbox series

Message ID 20191007224642.6597-7-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Commit Message

Laurent Pinchart Oct. 7, 2019, 10:46 p.m. UTC 
Add a V4L2 specialisation of the ControlId class, in order to construct
a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use
ControlList for V4L2 controls.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/include/v4l2_controls.h | 12 +++--
 src/libcamera/v4l2_controls.cpp       | 67 +++++++++++++++++++++++----
 src/libcamera/v4l2_device.cpp         |  4 +-
 3 files changed, 68 insertions(+), 15 deletions(-)

Comments

Jacopo Mondi Oct. 9, 2019, 8:35 a.m. UTC | #1 
Hi Laurent,

On Tue, Oct 08, 2019 at 01:46:39AM +0300, Laurent Pinchart wrote:
> Add a V4L2 specialisation of the ControlId class, in order to construct
> a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use
> ControlList for V4L2 controls.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_controls.h | 12 +++--
>  src/libcamera/v4l2_controls.cpp       | 67 +++++++++++++++++++++++----
>  src/libcamera/v4l2_device.cpp         |  4 +-
>  3 files changed, 68 insertions(+), 15 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index b39370b2e90e..71ce377fe4c5 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -20,23 +20,27 @@
>
>  namespace libcamera {
>
> +class V4L2ControlId : public ControlId
> +{
> +public:
> +	V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
> +};
> +

This only add a constructor without any field. Do we need inheritance
or can we just add a constructor to ControlId ?

>  class V4L2ControlInfo
>  {
>  public:
>  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
>
> -	unsigned int id() const { return id_; }
> +	const ControlId &id() const { return id_; }
>  	unsigned int type() const { return type_; }
>  	size_t size() const { return size_; }
> -	const std::string &name() const { return name_; }
>
>  	const ControlRange &range() const { return range_; }
>
>  private:
> -	unsigned int id_;
> +	V4L2ControlId id_;
>  	unsigned int type_;
>  	size_t size_;
> -	std::string name_;
>
>  	ControlRange range_;
>  };
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 6f5f1578b139..a630a2583d33 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -7,6 +7,8 @@
>
>  #include "v4l2_controls.h"
>
> +#include <string.h>
> +
>  /**
>   * \file v4l2_controls.h
>   * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs
> @@ -47,6 +49,60 @@
>
>  namespace libcamera {
>
> +namespace {
> +
> +std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
> +{
> +	size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> +	return std::string(static_cast<const char *>(ctrl.name), len);
> +}
> +
> +ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)
> +{
> +	switch (ctrl.type) {
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		return ControlTypeBool;
> +
> +	case V4L2_CTRL_TYPE_INTEGER:
> +		return ControlTypeInteger32;
> +
> +	case V4L2_CTRL_TYPE_INTEGER64:
> +		return ControlTypeInteger64;
> +
> +	case V4L2_CTRL_TYPE_MENU:
> +	case V4L2_CTRL_TYPE_BUTTON:
> +	case V4L2_CTRL_TYPE_BITMASK:
> +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> +		/*
> +		 * More precise types may be needed, for now use a 32-bit
> +		 * integer type.
> +		 */
> +		return ControlTypeInteger32;
> +
> +	default:
> +		return ControlTypeNone;
> +	}
> +}

Probably it's ok to have inheritance as we need this in
v4l2_controls.cpp

This considered
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +
> +} /* namespace */
> +
> +/**
> + * \class V4L2ControlId
> + * \brief V4L2 control static metadata
> + *
> + * The V4L2ControlId class is a specialisation of the ControlId for V4L2
> + * controls.
> + */
> +
> +/**
> + * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl
> + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> + */
> +V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> +	: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))
> +{
> +}
> +
>  /**
>   * \class V4L2ControlInfo
>   * \brief Information on a V4L2 control
> @@ -66,13 +122,12 @@ namespace libcamera {
>
>  /**
>   * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> - * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
>   */
>  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> +	: id_(ctrl)
>  {
> -	id_ = ctrl.id;
>  	type_ = ctrl.type;
> -	name_ = static_cast<const char *>(ctrl.name);
>  	size_ = ctrl.elem_size * ctrl.elems;
>
>  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> @@ -101,12 +156,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>   * \return The V4L2 control value data size
>   */
>
> -/**
> - * \fn V4L2ControlInfo::name()
> - * \brief Retrieve the control user readable name
> - * \return The V4L2 control user readable name
> - */
> -
>  /**
>   * \fn V4L2ControlInfo::range()
>   * \brief Retrieve the control value range
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 3bd82ff23212..466c3d41f6e3 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -184,7 +184,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
>
>  		const V4L2ControlInfo *info = &iter->second;
>  		controlInfo[i] = info;
> -		v4l2Ctrls[i].id = info->id();
> +		v4l2Ctrls[i].id = ctrl->id();
>  	}
>
>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> @@ -259,7 +259,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
>
>  		const V4L2ControlInfo *info = &iter->second;
>  		controlInfo[i] = info;
> -		v4l2Ctrls[i].id = info->id();
> +		v4l2Ctrls[i].id = ctrl->id();
>
>  		/* Set the v4l2_ext_control value for the write operation. */
>  		switch (info->type()) {
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 9, 2019, 9:05 a.m. UTC | #2 
Hi Jacopo,

On Wed, Oct 09, 2019 at 10:35:20AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 01:46:39AM +0300, Laurent Pinchart wrote:
> > Add a V4L2 specialisation of the ControlId class, in order to construct
> > a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use
> > ControlList for V4L2 controls.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_controls.h | 12 +++--
> >  src/libcamera/v4l2_controls.cpp       | 67 +++++++++++++++++++++++----
> >  src/libcamera/v4l2_device.cpp         |  4 +-
> >  3 files changed, 68 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index b39370b2e90e..71ce377fe4c5 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -20,23 +20,27 @@
> >
> >  namespace libcamera {
> >
> > +class V4L2ControlId : public ControlId
> > +{
> > +public:
> > +	V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
> > +};
> > +
> 
> This only add a constructor without any field. Do we need inheritance
> or can we just add a constructor to ControlId ?

We could, but I think it's best to keep the ControlId constructor
protected, otherwise applications could create new ControlId instances.

> >  class V4L2ControlInfo
> >  {
> >  public:
> >  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >
> > -	unsigned int id() const { return id_; }
> > +	const ControlId &id() const { return id_; }
> >  	unsigned int type() const { return type_; }
> >  	size_t size() const { return size_; }
> > -	const std::string &name() const { return name_; }
> >
> >  	const ControlRange &range() const { return range_; }
> >
> >  private:
> > -	unsigned int id_;
> > +	V4L2ControlId id_;
> >  	unsigned int type_;
> >  	size_t size_;
> > -	std::string name_;
> >
> >  	ControlRange range_;
> >  };
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 6f5f1578b139..a630a2583d33 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -7,6 +7,8 @@
> >
> >  #include "v4l2_controls.h"
> >
> > +#include <string.h>
> > +
> >  /**
> >   * \file v4l2_controls.h
> >   * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs
> > @@ -47,6 +49,60 @@
> >
> >  namespace libcamera {
> >
> > +namespace {
> > +
> > +std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > +	return std::string(static_cast<const char *>(ctrl.name), len);
> > +}
> > +
> > +ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	switch (ctrl.type) {
> > +	case V4L2_CTRL_TYPE_BOOLEAN:
> > +		return ControlTypeBool;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER:
> > +		return ControlTypeInteger32;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER64:
> > +		return ControlTypeInteger64;
> > +
> > +	case V4L2_CTRL_TYPE_MENU:
> > +	case V4L2_CTRL_TYPE_BUTTON:
> > +	case V4L2_CTRL_TYPE_BITMASK:
> > +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +		/*
> > +		 * More precise types may be needed, for now use a 32-bit
> > +		 * integer type.
> > +		 */
> > +		return ControlTypeInteger32;
> > +
> > +	default:
> > +		return ControlTypeNone;
> > +	}
> > +}
> 
> Probably it's ok to have inheritance as we need this in
> v4l2_controls.cpp
> 
> This considered
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class V4L2ControlId
> > + * \brief V4L2 control static metadata
> > + *
> > + * The V4L2ControlId class is a specialisation of the ControlId for V4L2
> > + * controls.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl
> > + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + */
> > +V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> > +	: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))
> > +{
> > +}
> > +
> >  /**
> >   * \class V4L2ControlInfo
> >   * \brief Information on a V4L2 control
> > @@ -66,13 +122,12 @@ namespace libcamera {
> >
> >  /**
> >   * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > - * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >   */
> >  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +	: id_(ctrl)
> >  {
> > -	id_ = ctrl.id;
> >  	type_ = ctrl.type;
> > -	name_ = static_cast<const char *>(ctrl.name);
> >  	size_ = ctrl.elem_size * ctrl.elems;
> >
> >  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > @@ -101,12 +156,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * \return The V4L2 control value data size
> >   */
> >
> > -/**
> > - * \fn V4L2ControlInfo::name()
> > - * \brief Retrieve the control user readable name
> > - * \return The V4L2 control user readable name
> > - */
> > -
> >  /**
> >   * \fn V4L2ControlInfo::range()
> >   * \brief Retrieve the control value range
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 3bd82ff23212..466c3d41f6e3 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -184,7 +184,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = info->id();
> > +		v4l2Ctrls[i].id = ctrl->id();
> >  	}
> >
> >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > @@ -259,7 +259,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = info->id();
> > +		v4l2Ctrls[i].id = ctrl->id();
> >
> >  		/* Set the v4l2_ext_control value for the write operation. */
> >  		switch (info->type()) {

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
index b39370b2e90e..71ce377fe4c5 100644
--- a/src/libcamera/include/v4l2_controls.h
+++ b/src/libcamera/include/v4l2_controls.h
@@ -20,23 +20,27 @@ 
 
 namespace libcamera {
 
+class V4L2ControlId : public ControlId
+{
+public:
+	V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
+};
+
 class V4L2ControlInfo
 {
 public:
 	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
 
-	unsigned int id() const { return id_; }
+	const ControlId &id() const { return id_; }
 	unsigned int type() const { return type_; }
 	size_t size() const { return size_; }
-	const std::string &name() const { return name_; }
 
 	const ControlRange &range() const { return range_; }
 
 private:
-	unsigned int id_;
+	V4L2ControlId id_;
 	unsigned int type_;
 	size_t size_;
-	std::string name_;
 
 	ControlRange range_;
 };
diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
index 6f5f1578b139..a630a2583d33 100644
--- a/src/libcamera/v4l2_controls.cpp
+++ b/src/libcamera/v4l2_controls.cpp
@@ -7,6 +7,8 @@ 
 
 #include "v4l2_controls.h"
 
+#include <string.h>
+
 /**
  * \file v4l2_controls.h
  * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs
@@ -47,6 +49,60 @@ 
 
 namespace libcamera {
 
+namespace {
+
+std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
+{
+	size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
+	return std::string(static_cast<const char *>(ctrl.name), len);
+}
+
+ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)
+{
+	switch (ctrl.type) {
+	case V4L2_CTRL_TYPE_BOOLEAN:
+		return ControlTypeBool;
+
+	case V4L2_CTRL_TYPE_INTEGER:
+		return ControlTypeInteger32;
+
+	case V4L2_CTRL_TYPE_INTEGER64:
+		return ControlTypeInteger64;
+
+	case V4L2_CTRL_TYPE_MENU:
+	case V4L2_CTRL_TYPE_BUTTON:
+	case V4L2_CTRL_TYPE_BITMASK:
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+		/*
+		 * More precise types may be needed, for now use a 32-bit
+		 * integer type.
+		 */
+		return ControlTypeInteger32;
+
+	default:
+		return ControlTypeNone;
+	}
+}
+
+} /* namespace */
+
+/**
+ * \class V4L2ControlId
+ * \brief V4L2 control static metadata
+ *
+ * The V4L2ControlId class is a specialisation of the ControlId for V4L2
+ * controls.
+ */
+
+/**
+ * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl
+ * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
+ */
+V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
+	: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))
+{
+}
+
 /**
  * \class V4L2ControlInfo
  * \brief Information on a V4L2 control
@@ -66,13 +122,12 @@  namespace libcamera {
 
 /**
  * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
- * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
+ * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
  */
 V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
+	: id_(ctrl)
 {
-	id_ = ctrl.id;
 	type_ = ctrl.type;
-	name_ = static_cast<const char *>(ctrl.name);
 	size_ = ctrl.elem_size * ctrl.elems;
 
 	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
@@ -101,12 +156,6 @@  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
  * \return The V4L2 control value data size
  */
 
-/**
- * \fn V4L2ControlInfo::name()
- * \brief Retrieve the control user readable name
- * \return The V4L2 control user readable name
- */
-
 /**
  * \fn V4L2ControlInfo::range()
  * \brief Retrieve the control value range
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 3bd82ff23212..466c3d41f6e3 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -184,7 +184,7 @@  int V4L2Device::getControls(V4L2ControlList *ctrls)
 
 		const V4L2ControlInfo *info = &iter->second;
 		controlInfo[i] = info;
-		v4l2Ctrls[i].id = info->id();
+		v4l2Ctrls[i].id = ctrl->id();
 	}
 
 	struct v4l2_ext_controls v4l2ExtCtrls = {};
@@ -259,7 +259,7 @@  int V4L2Device::setControls(V4L2ControlList *ctrls)
 
 		const V4L2ControlInfo *info = &iter->second;
 		controlInfo[i] = info;
-		v4l2Ctrls[i].id = info->id();
+		v4l2Ctrls[i].id = ctrl->id();
 
 		/* Set the v4l2_ext_control value for the write operation. */
 		switch (info->type()) {