[libcamera-devel,v2,11/24] libcamera: v4l2_controls: Fix control range construction for bool
diff mbox series

Message ID 20191108205409.18845-12-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Control serialization and IPA C API
Related show

Commit Message

Laurent Pinchart Nov. 8, 2019, 8:53 p.m. UTC 
V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
with a ControlRange of int32_t values. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Nov. 15, 2019, 4:22 p.m. UTC | #1 
Hi Laurent,

On Fri, Nov 08, 2019 at 10:53:56PM +0200, Laurent Pinchart wrote:
> V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
> with a ControlRange of int32_t values. Fix it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

indeed!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j
> ---
>  src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 2462c3e2c264..b6547a7c627c 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -118,12 +118,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
>   */
>  V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
>  {
> -	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> +	switch (ctrl.type) {
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
> +						     static_cast<bool>(ctrl.maximum)));
> +		break;
> +
> +	case V4L2_CTRL_TYPE_INTEGER64:
>  		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
>  						     static_cast<int64_t>(ctrl.maximum)));
> -	else
> +		break;
> +
> +	default:
>  		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
>  						     static_cast<int32_t>(ctrl.maximum)));
> +		break;
> +	}
>  }
>
>  } /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Nov. 18, 2019, 5:19 p.m. UTC | #2 
Hi Laurent,

Thanks for your patch.

On 2019-11-08 22:53:56 +0200, Laurent Pinchart wrote:
> V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
> with a ControlRange of int32_t values. Fix it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 2462c3e2c264..b6547a7c627c 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -118,12 +118,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
>   */
>  V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
>  {
> -	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> +	switch (ctrl.type) {
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
> +						     static_cast<bool>(ctrl.maximum)));
> +		break;
> +
> +	case V4L2_CTRL_TYPE_INTEGER64:
>  		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
>  						     static_cast<int64_t>(ctrl.maximum)));
> -	else
> +		break;
> +
> +	default:
>  		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
>  						     static_cast<int32_t>(ctrl.maximum)));
> +		break;
> +	}
>  }
>  
>  } /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Nov. 19, 2019, 4:26 p.m. UTC | #3 
Hi Laurent,

On 08/11/2019 20:53, Laurent Pinchart wrote:
> V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
> with a ControlRange of int32_t values. Fix it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 2462c3e2c264..b6547a7c627c 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -118,12 +118,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
>   */
>  V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
>  {
> -	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> +	switch (ctrl.type) {
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
> +						     static_cast<bool>(ctrl.maximum)));

Hrm shouldn't the minimum and maximum of a bool control be fairly const?

If so would this be better written as:

 ControlRange::operator=(ControlRange(false, true));

Otherwise,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>




> +		break;
> +
> +	case V4L2_CTRL_TYPE_INTEGER64:
>  		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
>  						     static_cast<int64_t>(ctrl.maximum)));
> -	else
> +		break;
> +
> +	default:
>  		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
>  						     static_cast<int32_t>(ctrl.maximum)));
> +		break;
> +	}
>  }
>  
>  } /* namespace libcamera */
>
Laurent Pinchart Nov. 19, 2019, 10:38 p.m. UTC | #4 
Hi Kieran,

On Tue, Nov 19, 2019 at 04:26:04PM +0000, Kieran Bingham wrote:
> On 08/11/2019 20:53, Laurent Pinchart wrote:
> > V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
> > with a ControlRange of int32_t values. Fix it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 2462c3e2c264..b6547a7c627c 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -118,12 +118,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> >   */
> >  V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
> >  {
> > -	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > +	switch (ctrl.type) {
> > +	case V4L2_CTRL_TYPE_BOOLEAN:
> > +		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
> > +						     static_cast<bool>(ctrl.maximum)));
> 
> Hrm shouldn't the minimum and maximum of a bool control be fairly const?
> 
> If so would this be better written as:
> 
>  ControlRange::operator=(ControlRange(false, true));

A V4L2 device could decide to expose a control but only support a single
value. The main use case would be read-only controls that report a
device configuration, but it could also be useful to show that a feature
is only partly implemented/supported.

> Otherwise,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +		break;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER64:
> >  		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
> >  						     static_cast<int64_t>(ctrl.maximum)));
> > -	else
> > +		break;
> > +
> > +	default:
> >  		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
> >  						     static_cast<int32_t>(ctrl.maximum)));
> > +		break;
> > +	}
> >  }
> >  
> >  } /* namespace libcamera */
> >

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
index 2462c3e2c264..b6547a7c627c 100644
--- a/src/libcamera/v4l2_controls.cpp
+++ b/src/libcamera/v4l2_controls.cpp
@@ -118,12 +118,22 @@  V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
  */
 V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
 {
-	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
+	switch (ctrl.type) {
+	case V4L2_CTRL_TYPE_BOOLEAN:
+		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
+						     static_cast<bool>(ctrl.maximum)));
+		break;
+
+	case V4L2_CTRL_TYPE_INTEGER64:
 		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
 						     static_cast<int64_t>(ctrl.maximum)));
-	else
+		break;
+
+	default:
 		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
 						     static_cast<int32_t>(ctrl.maximum)));
+		break;
+	}
 }
 
 } /* namespace libcamera */