[libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device: Implement get and set controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 22 22:24:20 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Thu, Jun 20, 2019 at 05:31:41PM +0200, Jacopo Mondi wrote:
> Implement getControls() and setControls() operations in V4L2Device class.
>
> Both operations take a V4L2Controls instance and read or write the V4L2
> controls on the V4L2 device.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/include/v4l2_device.h | 3 +
> src/libcamera/v4l2_device.cpp | 193 ++++++++++++++++++++++++++++
> 2 files changed, 196 insertions(+)
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 91a72fcecbcc..3af27c9f7af5 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -15,6 +15,7 @@
> namespace libcamera {
>
> class V4L2ControlInfo;
> +class V4L2Controls;
>
> class V4L2Device : protected Loggable
> {
> @@ -23,6 +24,8 @@ public:
> bool isOpen() const { return fd_ != -1; }
>
> const V4L2ControlInfo *getControlInfo(unsigned int id) const;
> + int getControls(V4L2Controls *ctrls);
> + int setControls(V4L2Controls *ctrls);
>
> const std::string &deviceNode() const { return deviceNode_; }
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index b113ff77e3cf..efe30ef856ae 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -125,6 +125,199 @@ const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const
> return &it->second;
> }
>
> +/**
> + * \brief Read controls from the device
> + * \param[inout] ctrls The list of controls to read
> + *
> + * Read the value of each control contained in \a ctrls, and store the
> + * value in the corresponding \a ctrls entry.
> + *
> + * If an integer number less than the requested number of controls is returned,
> + * only controls up to that index have been successfully read.
> + *
> + * Each V4L2Control instance in \a ctrls should be supported by the device and
> + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise
> + * a negative error code (-EINVAL) is returned
* This method reads the value of all controls contained in \a ctrls, and stores
* their values in the corresponding \a ctrls entry.
*
* If any control in \a ctrls is not supported by the device, is disabled (i.e.
* has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any
* other error occurs during validation of the requested controls, no control is
* read and this method returns -EINVAL.
*
* If an error occurs while reading the controls, the index of the first
* control that couldn't be read is returned. The value of all controls
* below that index are updated in \a ctrls, while the value of all the
* other controls are not changed.
> + *
> + * \return 0 on success or an error code otherwise
> + * \retval -EINVAL One of the control is not supported or not accessible
> + * \retval i The index of the control that failed
> + */
> +int V4L2Device::getControls(V4L2Controls *ctrls)
> +{
> + unsigned int count = ctrls->size();
> + if (count == 0)
> + return 0;
> +
> + const V4L2ControlInfo *controlInfo[count] = {};
> + struct v4l2_ext_control v4l2Ctrls[count] = {};
> +
> + for (unsigned int i = 0; i < count; ++i) {
> + V4L2Control *ctrl = (*ctrls)[i];
> +
> + const V4L2ControlInfo *info = getControlInfo(ctrl->id());
> + if (!info) {
> + LOG(V4L2, Error)
> + << "Control '" << ctrl->id() << "' not found";
> + return -EINVAL;
> + }
> +
> + controlInfo[i] = info;
> + v4l2Ctrls[i].id = info->id();
> + }
> +
> + struct v4l2_ext_controls v4l2ExtCtrls = {};
> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> + v4l2ExtCtrls.controls = v4l2Ctrls;
> + v4l2ExtCtrls.count = count;
> +
> + int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
> + if (ret) {
> + unsigned int errorIdx = v4l2ExtCtrls.error_idx;
> +
> + /* Generic validation error. */
> + if (errorIdx == count) {
> + LOG(V4L2, Error) << "Unable to read controls: "
> + << strerror(ret);
> + return -EINVAL;
> + }
> +
> + /* A specific control failed. */
> + LOG(V4L2, Error) << "Unable to read control " << errorIdx
> + << ": " << strerror(ret);
> + return errorIdx;
This contradicts with the documentation (OK, with my documentation only
:-)), the values of all controls under the error index should be stored
in ctrls. Alternatively we could modify the documentation and not update
any value, but then would returning the error index be useful ? I would
thus write this
int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
if (ret) {
{
unsigned int errorIdx = v4l2ExtCtrls.error_idx;
/* Generic validation error. */
if (errorIdx == 0 || errorIdx >= count) {
LOG(V4L2, Error) << "Unable to read controls: "
<< strerror(ret);
return -EINVAL;
}
/* A specific control failed. */
LOG(V4L2, Error) << "Unable to read control " << errorIdx
<< ": " << strerror(ret);
count = errorIdx - 1;
ret = errorIdx;
}
and return ret at the end of the function.
> + }
> +
> + /*
> + * For each control read from the device, set the value in the
> + * V4L2Control provided by the caller.
> + */
> + for (unsigned int i = 0; i < count; ++i) {
> + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> + const V4L2ControlInfo *info = controlInfo[i];
> + V4L2Control *ctrl = (*ctrls)[i];
> +
> + switch (info->type()) {
> + case V4L2_CTRL_TYPE_INTEGER64:
> + ctrl->setValue(v4l2Ctrl->value64);
> + break;
> + default:
> + /*
> + * \todo To be changed when support for string and
> + * compound controls will be added.
> + */
> + ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
Do you need the cast ? If you do it should be cast to int64_t.
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Write controls to the device
> + * \param[in] ctrls The list of controls to write
> + *
> + * Write the value of each V4L2Control contained in \a ctrls. Each
> + * control should be initialized by the caller with a value, otherwise
> + * the result of the operation is undefined.
> + *
> + * The value of each control is updated to reflect what has been
> + * actually applied on the device.
No need to wrap lines well below the 80 characters limit :-)
> + *
> + * If an integer number less than the requested number of controls is returned,
> + * only controls up to that index have been successfully applied.
> + *
> + * Each V4L2Control instance in \a ctrls should be supported by the device and
> + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise
> + * a negative error code (-EINVAL) is returned
> + *
To mimick getControls(),
* This method writes the value of all controls contained in \a ctrls, and
* stores the values actually applied to the device in the corresponding
* \a ctrls entry.
*
* If any control in \a ctrls is not supported by the device, is disabled (i.e.
* has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, is a
* compound control, or if any other error occurs during validation of
* the requested controls, no control is written and this method returns
* -EINVAL.
*
* If an error occurs while writing the controls, the index of the first
* control that couldn't be written is returned. All controls below that index
* are written and their values are updated in \a ctrls, while all other
* controls are not written and their values are not changed.
> + * \return 0 on success or an error code otherwise
> + * \retval -EINVAL One of the control is not supported or not accessible
> + * \retval i The index of the control that failed
> + */
> +int V4L2Device::setControls(V4L2Controls *ctrls)
> +{
> + unsigned int count = ctrls->size();
> + if (count == 0)
> + return 0;
> +
> + const V4L2ControlInfo *controlInfo[count] = {};
> + struct v4l2_ext_control v4l2Ctrls[count] = {};
> +
> + for (unsigned int i = 0; i < count; ++i) {
> + V4L2Control *ctrl = (*ctrls)[i];
> +
> + const V4L2ControlInfo *info = getControlInfo(ctrl->id());
> + if (!info) {
> + LOG(V4L2, Error)
> + << "Control '" << ctrl->id() << "' not found";
> + return -EINVAL;
> + }
> +
> + controlInfo[i] = info;
> + v4l2Ctrls[i].id = info->id();
> +
> + /* Set the v4l2_ext_control value for the write operation. */
> + switch (info->type()) {
> + case V4L2_CTRL_TYPE_INTEGER64:
> + v4l2Ctrls[i].value64 = ctrl->value();
> + break;
> + default:
> + /*
> + * \todo To be changed when support for string and
> + * compound controls will be added.
> + */
> + v4l2Ctrls[i].value = static_cast<int32_t>(ctrl->value());
Same here, do we need the cast ?
> + break;
> + }
> + }
> +
> + struct v4l2_ext_controls v4l2ExtCtrls = {};
> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> + v4l2ExtCtrls.controls = v4l2Ctrls;
> + v4l2ExtCtrls.count = count;
> +
> + int ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);
> + if (ret) {
> + unsigned int errorIdx = v4l2ExtCtrls.error_idx;
> +
> + /* Generic validation error. */
> + if (errorIdx == count) {
> + LOG(V4L2, Error) << "Unable to read controls: "
> + << strerror(ret);
> + return -EINVAL;
> + }
> +
> + /* A specific control failed. */
> + LOG(V4L2, Error) << "Unable to read control " << errorIdx
> + << ": " << strerror(ret);
> + return errorIdx;
Same here, I think you should update the controls that have been
written.
> + }
> +
> + /* Update the control value to what have actually been applied. */
> + for (unsigned int i = 0; i < count; ++i) {
> + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> + const V4L2ControlInfo *info = controlInfo[i];
> + V4L2Control *ctrl = (*ctrls)[i];
> +
> + switch (info->type()) {
> + case V4L2_CTRL_TYPE_INTEGER64:
> + ctrl->setValue(v4l2Ctrl->value64);
> + break;
> + default:
> + /*
> + * \todo To be changed when support for string and
> + * compound controls will be added.
> + */
> + ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
And here too.
> + break;
> + }
> + }
This loop could possibly be moved to an updateControls() function and
shared between getControls() and setControls().
> +
> + return 0;
> +}
> +
> /**
> * \brief Perform an IOCTL system call on the device node
> * \param[in] request The IOCTL request code
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list