| Message ID | 20221202124005.3643-2-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via libcamera-devel wrote: > Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 control > is read-only or not. This flag only makes sense for V2L2 controls at preset, so > only update the ControlInfo constructor signature used by the V4L2Device class > to set the value of the flag. > It feels a bit the three constructors are mis-aligned now :/ I would have gone for a default parameter for all three of them to be honest... What do others think ? > Add a ControlInfo::readOnly() member function to retrive this flag. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/controls.h | 5 ++++- > src/libcamera/controls.cpp | 17 +++++++++++++---- > src/libcamera/v4l2_device.cpp | 12 ++++++++---- > 3 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index cf94205577a5..488663a7ba04 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -270,7 +270,8 @@ class ControlInfo > public: > explicit ControlInfo(const ControlValue &min = {}, > const ControlValue &max = {}, > - const ControlValue &def = {}); > + const ControlValue &def = {}, > + bool readOnly = false); > explicit ControlInfo(Span<const ControlValue> values, > const ControlValue &def = {}); > explicit ControlInfo(std::set<bool> values, bool def); > @@ -279,6 +280,7 @@ public: > const ControlValue &min() const { return min_; } > const ControlValue &max() const { return max_; } > const ControlValue &def() const { return def_; } > + bool readOnly() const { return readOnly_; } > const std::vector<ControlValue> &values() const { return values_; } > > std::string toString() const; > @@ -297,6 +299,7 @@ private: > ControlValue min_; > ControlValue max_; > ControlValue def_; > + bool readOnly_; > std::vector<ControlValue> values_; > }; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 6dbf9b348709..fc66abad600d 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > * \param[in] min The control minimum value > * \param[in] max The control maximum value > * \param[in] def The control default value > + * \param[in] readOnly Read-only status of a V4L2 control > */ > ControlInfo::ControlInfo(const ControlValue &min, > const ControlValue &max, > - const ControlValue &def) > - : min_(min), max_(max), def_(def) > + const ControlValue &def, > + bool readOnly) > + : min_(min), max_(max), def_(def), readOnly_(readOnly) > { > } > > @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > * default value is \a def. > */ > ControlInfo::ControlInfo(std::set<bool> values, bool def) > - : min_(false), max_(true), def_(def), values_({ false, true }) > + : min_(false), max_(true), def_(def), readOnly_(false), > + values_({ false, true }) > { > ASSERT(values.count(def) && values.size() == 2); > } > @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool def) > * value. The minimum, maximum, and default values will all be \a value. > */ > ControlInfo::ControlInfo(bool value) > - : min_(value), max_(value), def_(value) > + : min_(value), max_(value), def_(value), readOnly_(false) > { > values_ = { value }; > } > @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value) > * \return A ControlValue with the default value for the control > */ > > +/** > + * \fn ControlInfo::readOnly() > + * \brief Identifies if a V4L2 control is flagged as read-only > + * \return True if the V4L2 control is read-only, false otherwise I would not mention V4L2 here and keep the documentation generic > + */ > + > /** > * \fn ControlInfo::values() > * \brief Retrieve the list of valid values > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 57a88d96b12c..9018f1b0b9e1 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -535,17 +535,20 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > case V4L2_CTRL_TYPE_U8: > return ControlInfo(static_cast<uint8_t>(ctrl.minimum), > static_cast<uint8_t>(ctrl.maximum), > - static_cast<uint8_t>(ctrl.default_value)); > + static_cast<uint8_t>(ctrl.default_value), > + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); > > case V4L2_CTRL_TYPE_BOOLEAN: > return ControlInfo(static_cast<bool>(ctrl.minimum), > static_cast<bool>(ctrl.maximum), > - static_cast<bool>(ctrl.default_value)); > + static_cast<bool>(ctrl.default_value), > + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); > > case V4L2_CTRL_TYPE_INTEGER64: > return ControlInfo(static_cast<int64_t>(ctrl.minimum), > static_cast<int64_t>(ctrl.maximum), > - static_cast<int64_t>(ctrl.default_value)); > + static_cast<int64_t>(ctrl.default_value), > + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); > > case V4L2_CTRL_TYPE_INTEGER_MENU: > case V4L2_CTRL_TYPE_MENU: > @@ -554,7 +557,8 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > default: > return ControlInfo(static_cast<int32_t>(ctrl.minimum), > static_cast<int32_t>(ctrl.maximum), > - static_cast<int32_t>(ctrl.default_value)); > + static_cast<int32_t>(ctrl.default_value), > + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); > } > } > > -- > 2.25.1 >
Hi Jacopo, Thank you for your feedback! On Mon, 12 Dec 2022 at 09:50, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush > > On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via > libcamera-devel wrote: > > Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 > control > > is read-only or not. This flag only makes sense for V2L2 controls at > preset, so > > only update the ControlInfo constructor signature used by the V4L2Device > class > > to set the value of the flag. > > > > It feels a bit the three constructors are mis-aligned now :/ > > I would have gone for a default parameter for all three of them to be > honest... What do others think ? > I do kind of agree with this, the constructor signatures were the one bit where I was unsure what to do. Happy to change all the constructors to have a readOnly param if others agree! Regards, Naush > > > Add a ControlInfo::readOnly() member function to retrive this flag. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/controls.h | 5 ++++- > > src/libcamera/controls.cpp | 17 +++++++++++++---- > > src/libcamera/v4l2_device.cpp | 12 ++++++++---- > > 3 files changed, 25 insertions(+), 9 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index cf94205577a5..488663a7ba04 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -270,7 +270,8 @@ class ControlInfo > > public: > > explicit ControlInfo(const ControlValue &min = {}, > > const ControlValue &max = {}, > > - const ControlValue &def = {}); > > + const ControlValue &def = {}, > > + bool readOnly = false); > > explicit ControlInfo(Span<const ControlValue> values, > > const ControlValue &def = {}); > > explicit ControlInfo(std::set<bool> values, bool def); > > @@ -279,6 +280,7 @@ public: > > const ControlValue &min() const { return min_; } > > const ControlValue &max() const { return max_; } > > const ControlValue &def() const { return def_; } > > + bool readOnly() const { return readOnly_; } > > const std::vector<ControlValue> &values() const { return values_; } > > > > std::string toString() const; > > @@ -297,6 +299,7 @@ private: > > ControlValue min_; > > ControlValue max_; > > ControlValue def_; > > + bool readOnly_; > > std::vector<ControlValue> values_; > > }; > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 6dbf9b348709..fc66abad600d 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool > isArray, std::size_t numElemen > > * \param[in] min The control minimum value > > * \param[in] max The control maximum value > > * \param[in] def The control default value > > + * \param[in] readOnly Read-only status of a V4L2 control > > */ > > ControlInfo::ControlInfo(const ControlValue &min, > > const ControlValue &max, > > - const ControlValue &def) > > - : min_(min), max_(max), def_(def) > > + const ControlValue &def, > > + bool readOnly) > > + : min_(min), max_(max), def_(def), readOnly_(readOnly) > > { > > } > > > > @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> > values, > > * default value is \a def. > > */ > > ControlInfo::ControlInfo(std::set<bool> values, bool def) > > - : min_(false), max_(true), def_(def), values_({ false, true }) > > + : min_(false), max_(true), def_(def), readOnly_(false), > > + values_({ false, true }) > > { > > ASSERT(values.count(def) && values.size() == 2); > > } > > @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool > def) > > * value. The minimum, maximum, and default values will all be \a value. > > */ > > ControlInfo::ControlInfo(bool value) > > - : min_(value), max_(value), def_(value) > > + : min_(value), max_(value), def_(value), readOnly_(false) > > { > > values_ = { value }; > > } > > @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value) > > * \return A ControlValue with the default value for the control > > */ > > > > +/** > > + * \fn ControlInfo::readOnly() > > + * \brief Identifies if a V4L2 control is flagged as read-only > > + * \return True if the V4L2 control is read-only, false otherwise > > I would not mention V4L2 here and keep the documentation generic > > > + */ > > + > > /** > > * \fn ControlInfo::values() > > * \brief Retrieve the list of valid values > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > index 57a88d96b12c..9018f1b0b9e1 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -535,17 +535,20 @@ std::optional<ControlInfo> > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > case V4L2_CTRL_TYPE_U8: > > return ControlInfo(static_cast<uint8_t>(ctrl.minimum), > > static_cast<uint8_t>(ctrl.maximum), > > - > static_cast<uint8_t>(ctrl.default_value)); > > + > static_cast<uint8_t>(ctrl.default_value), > > + !!(ctrl.flags & > V4L2_CTRL_FLAG_READ_ONLY)); > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > static_cast<bool>(ctrl.maximum), > > - static_cast<bool>(ctrl.default_value)); > > + static_cast<bool>(ctrl.default_value), > > + !!(ctrl.flags & > V4L2_CTRL_FLAG_READ_ONLY)); > > > > case V4L2_CTRL_TYPE_INTEGER64: > > return ControlInfo(static_cast<int64_t>(ctrl.minimum), > > static_cast<int64_t>(ctrl.maximum), > > - > static_cast<int64_t>(ctrl.default_value)); > > + > static_cast<int64_t>(ctrl.default_value), > > + !!(ctrl.flags & > V4L2_CTRL_FLAG_READ_ONLY)); > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > case V4L2_CTRL_TYPE_MENU: > > @@ -554,7 +557,8 @@ std::optional<ControlInfo> > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > default: > > return ControlInfo(static_cast<int32_t>(ctrl.minimum), > > static_cast<int32_t>(ctrl.maximum), > > - > static_cast<int32_t>(ctrl.default_value)); > > + > static_cast<int32_t>(ctrl.default_value), > > + !!(ctrl.flags & > V4L2_CTRL_FLAG_READ_ONLY)); > > } > > } > > > > -- > > 2.25.1 > > >
Quoting Naushir Patuck via libcamera-devel (2022-12-12 10:16:35) > Hi Jacopo, > > Thank you for your feedback! > > On Mon, 12 Dec 2022 at 09:50, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Naush > > > > On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via > > libcamera-devel wrote: > > > Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 > > control > > > is read-only or not. This flag only makes sense for V2L2 controls at > > preset, so > > > only update the ControlInfo constructor signature used by the V4L2Device > > class > > > to set the value of the flag. > > > > > > > It feels a bit the three constructors are mis-aligned now :/ > > > > > I would have gone for a default parameter for all three of them to be > > honest... What do others think ? > > > > I do kind of agree with this, the constructor signatures were the one bit > where I was unsure what to do. > > Happy to change all the constructors to have a readOnly param if others > agree! It's tough to know without knowing future use-cases, but if we have one type of read-only control, it's probably fair to assume or expect that it's a 'feature' of controls, and should be possible to set on all types. So I think this is worth having on all of them yes. > > Regards, > Naush > > > > > > > > Add a ControlInfo::readOnly() member function to retrive this flag. s/retrive/retrieve/ > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/controls.h | 5 ++++- > > > src/libcamera/controls.cpp | 17 +++++++++++++---- > > > src/libcamera/v4l2_device.cpp | 12 ++++++++---- > > > 3 files changed, 25 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index cf94205577a5..488663a7ba04 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -270,7 +270,8 @@ class ControlInfo > > > public: > > > explicit ControlInfo(const ControlValue &min = {}, > > > const ControlValue &max = {}, > > > - const ControlValue &def = {}); > > > + const ControlValue &def = {}, > > > + bool readOnly = false); > > > explicit ControlInfo(Span<const ControlValue> values, > > > const ControlValue &def = {}); > > > explicit ControlInfo(std::set<bool> values, bool def); > > > @@ -279,6 +280,7 @@ public: > > > const ControlValue &min() const { return min_; } > > > const ControlValue &max() const { return max_; } > > > const ControlValue &def() const { return def_; } > > > + bool readOnly() const { return readOnly_; } > > > const std::vector<ControlValue> &values() const { return values_; } > > > > > > std::string toString() const; > > > @@ -297,6 +299,7 @@ private: > > > ControlValue min_; > > > ControlValue max_; > > > ControlValue def_; > > > + bool readOnly_; > > > std::vector<ControlValue> values_; > > > }; > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 6dbf9b348709..fc66abad600d 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool > > isArray, std::size_t numElemen > > > * \param[in] min The control minimum value > > > * \param[in] max The control maximum value > > > * \param[in] def The control default value > > > + * \param[in] readOnly Read-only status of a V4L2 control > > > */ > > > ControlInfo::ControlInfo(const ControlValue &min, > > > const ControlValue &max, > > > - const ControlValue &def) > > > - : min_(min), max_(max), def_(def) > > > + const ControlValue &def, > > > + bool readOnly) > > > + : min_(min), max_(max), def_(def), readOnly_(readOnly) > > > { > > > } > > > > > > @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> > > values, > > > * default value is \a def. > > > */ > > > ControlInfo::ControlInfo(std::set<bool> values, bool def) > > > - : min_(false), max_(true), def_(def), values_({ false, true }) > > > + : min_(false), max_(true), def_(def), readOnly_(false), > > > + values_({ false, true }) > > > { > > > ASSERT(values.count(def) && values.size() == 2); > > > } > > > @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool > > def) > > > * value. The minimum, maximum, and default values will all be \a value. > > > */ > > > ControlInfo::ControlInfo(bool value) > > > - : min_(value), max_(value), def_(value) > > > + : min_(value), max_(value), def_(value), readOnly_(false) > > > { > > > values_ = { value }; > > > } > > > @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value) > > > * \return A ControlValue with the default value for the control > > > */ > > > > > > +/** > > > + * \fn ControlInfo::readOnly() > > > + * \brief Identifies if a V4L2 control is flagged as read-only > > > + * \return True if the V4L2 control is read-only, false otherwise > > > > I would not mention V4L2 here and keep the documentation generic > > > > > + */ > > > + > > > /** > > > * \fn ControlInfo::values() > > > * \brief Retrieve the list of valid values > > > diff --git a/src/libcamera/v4l2_device.cpp > > b/src/libcamera/v4l2_device.cpp > > > index 57a88d96b12c..9018f1b0b9e1 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -535,17 +535,20 @@ std::optional<ControlInfo> > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > case V4L2_CTRL_TYPE_U8: > > > return ControlInfo(static_cast<uint8_t>(ctrl.minimum), > > > static_cast<uint8_t>(ctrl.maximum), > > > - > > static_cast<uint8_t>(ctrl.default_value)); > > > + > > static_cast<uint8_t>(ctrl.default_value), > > > + !!(ctrl.flags & > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > static_cast<bool>(ctrl.maximum), > > > - static_cast<bool>(ctrl.default_value)); > > > + static_cast<bool>(ctrl.default_value), > > > + !!(ctrl.flags & > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > case V4L2_CTRL_TYPE_INTEGER64: > > > return ControlInfo(static_cast<int64_t>(ctrl.minimum), > > > static_cast<int64_t>(ctrl.maximum), > > > - > > static_cast<int64_t>(ctrl.default_value)); > > > + > > static_cast<int64_t>(ctrl.default_value), > > > + !!(ctrl.flags & > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > case V4L2_CTRL_TYPE_MENU: > > > @@ -554,7 +557,8 @@ std::optional<ControlInfo> > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > default: > > > return ControlInfo(static_cast<int32_t>(ctrl.minimum), > > > static_cast<int32_t>(ctrl.maximum), > > > - > > static_cast<int32_t>(ctrl.default_value)); > > > + > > static_cast<int32_t>(ctrl.default_value), > > > + !!(ctrl.flags & > > V4L2_CTRL_FLAG_READ_ONLY)); > > > } > > > } > > > > > > -- > > > 2.25.1 > > > > >
Quoting Kieran Bingham (2023-01-30 13:39:07) > Quoting Naushir Patuck via libcamera-devel (2022-12-12 10:16:35) > > Hi Jacopo, > > > > Thank you for your feedback! > > > > On Mon, 12 Dec 2022 at 09:50, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Hi Naush > > > > > > On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via > > > libcamera-devel wrote: > > > > Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 > > > control > > > > is read-only or not. This flag only makes sense for V2L2 controls at > > > preset, so > > > > only update the ControlInfo constructor signature used by the V4L2Device > > > class > > > > to set the value of the flag. > > > > > > > > > > It feels a bit the three constructors are mis-aligned now :/ > > > > > > > > I would have gone for a default parameter for all three of them to be > > > honest... What do others think ? > > > > > > > I do kind of agree with this, the constructor signatures were the one bit > > where I was unsure what to do. > > > > Happy to change all the constructors to have a readOnly param if others > > agree! > > It's tough to know without knowing future use-cases, but if we have one > type of read-only control, it's probably fair to assume or expect that > it's a 'feature' of controls, and should be possible to set on all > types. > > So I think this is worth having on all of them yes. So - I've been hitting the issue resolved by this series with a driver that has a read-only HBLANK control but the min/max are not == value. I don't know if that makes sense yet in the driver, and I 'fixed' this locally by setting min==max==value in the driver - but it feels somewhat like changing the kernel to fix a libcamera limitation... So - resuming this series. It seems quite difficult to introduce the extra argument to all the constructors without facing issues though, where this affects which constructor gets chosen ... ../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp: In member function ‘void libcamera::UVCCameraData::addControl(uint32_t, const libcamera::ControlInfo&, libcamera::ControlInfoMap::Map*)’: ../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:629:57: error: narrowing conversion of ‘((float)(min - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing] 629 | { static_cast<float>(min - def) / scale }, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ ../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:629:57: error: narrowing conversion of ‘((float)(min - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing] ../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:630:57: error: narrowing conversion of ‘((float)(max - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing] 630 | { static_cast<float>(max - def) / scale }, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ ../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:632:17: error: narrowing conversion of ‘0.0f’ from ‘float’ to ‘bool’ [-Wnarrowing] 632 | }; | ^ I presume this is because the compiler could implicitly map those types to bool when trying to construct the usual 3 parameter ControlInfo which could match the now 3 parameter bool constructor. I wonder if maybe it's easier to just leave it as supported on the single min/max/def constructor indeed. It could be extended if it became required later... Here's my current diff/fixup on this patch: diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 802dd906937b..8af304bdeac9 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -484,7 +484,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen * \param[in] min The control minimum value * \param[in] max The control maximum value * \param[in] def The control default value - * \param[in] readOnly Read-only status of a V4L2 control + * \param[in] readOnly True if the control reports a dynamic property */ ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max, @@ -510,6 +510,7 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, min_ = values.front(); max_ = values.back(); def_ = !def.isNone() ? def : values.front(); + readOnly_ = false; values_.reserve(values.size()); for (const ControlValue &value : values) @@ -541,7 +542,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool def) * value. The minimum, maximum, and default values will all be \a value. */ ControlInfo::ControlInfo(bool value) - : min_(value), max_(value), def_(value), readOnly_(false) + : min_(value), max_(value), def_(value), readOnly_(true) { values_ = { value }; } @@ -576,8 +577,12 @@ ControlInfo::ControlInfo(bool value) /** * \fn ControlInfo::readOnly() - * \brief Identifies if a V4L2 control is flagged as read-only - * \return True if the V4L2 control is read-only, false otherwise + * \brief Identifies if a control is flagged as read-only + * \return True if the control is read-only, false otherwise + * + * Read-only controls may signify that the control is a volatile property and + * can not be set. Adding a read-only control to a control list may cause the + * list to fail when the list is submitted. */ /** -- Kieran > > > > > > > Regards, > > Naush > > > > > > > > > > > > > Add a ControlInfo::readOnly() member function to retrive this flag. > > s/retrive/retrieve/ > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/controls.h | 5 ++++- > > > > src/libcamera/controls.cpp | 17 +++++++++++++---- > > > > src/libcamera/v4l2_device.cpp | 12 ++++++++---- > > > > 3 files changed, 25 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > index cf94205577a5..488663a7ba04 100644 > > > > --- a/include/libcamera/controls.h > > > > +++ b/include/libcamera/controls.h > > > > @@ -270,7 +270,8 @@ class ControlInfo > > > > public: > > > > explicit ControlInfo(const ControlValue &min = {}, > > > > const ControlValue &max = {}, > > > > - const ControlValue &def = {}); > > > > + const ControlValue &def = {}, > > > > + bool readOnly = false); > > > > explicit ControlInfo(Span<const ControlValue> values, > > > > const ControlValue &def = {}); > > > > explicit ControlInfo(std::set<bool> values, bool def); > > > > @@ -279,6 +280,7 @@ public: > > > > const ControlValue &min() const { return min_; } > > > > const ControlValue &max() const { return max_; } > > > > const ControlValue &def() const { return def_; } > > > > + bool readOnly() const { return readOnly_; } > > > > const std::vector<ControlValue> &values() const { return values_; } > > > > > > > > std::string toString() const; > > > > @@ -297,6 +299,7 @@ private: > > > > ControlValue min_; > > > > ControlValue max_; > > > > ControlValue def_; > > > > + bool readOnly_; > > > > std::vector<ControlValue> values_; > > > > }; > > > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > index 6dbf9b348709..fc66abad600d 100644 > > > > --- a/src/libcamera/controls.cpp > > > > +++ b/src/libcamera/controls.cpp > > > > @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool > > > isArray, std::size_t numElemen > > > > * \param[in] min The control minimum value > > > > * \param[in] max The control maximum value > > > > * \param[in] def The control default value > > > > + * \param[in] readOnly Read-only status of a V4L2 control > > > > */ > > > > ControlInfo::ControlInfo(const ControlValue &min, > > > > const ControlValue &max, > > > > - const ControlValue &def) > > > > - : min_(min), max_(max), def_(def) > > > > + const ControlValue &def, > > > > + bool readOnly) > > > > + : min_(min), max_(max), def_(def), readOnly_(readOnly) > > > > { > > > > } > > > > > > > > @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> > > > values, > > > > * default value is \a def. > > > > */ > > > > ControlInfo::ControlInfo(std::set<bool> values, bool def) > > > > - : min_(false), max_(true), def_(def), values_({ false, true }) > > > > + : min_(false), max_(true), def_(def), readOnly_(false), > > > > + values_({ false, true }) > > > > { > > > > ASSERT(values.count(def) && values.size() == 2); > > > > } > > > > @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool > > > def) > > > > * value. The minimum, maximum, and default values will all be \a value. > > > > */ > > > > ControlInfo::ControlInfo(bool value) > > > > - : min_(value), max_(value), def_(value) > > > > + : min_(value), max_(value), def_(value), readOnly_(false) > > > > { > > > > values_ = { value }; > > > > } > > > > @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value) > > > > * \return A ControlValue with the default value for the control > > > > */ > > > > > > > > +/** > > > > + * \fn ControlInfo::readOnly() > > > > + * \brief Identifies if a V4L2 control is flagged as read-only > > > > + * \return True if the V4L2 control is read-only, false otherwise > > > > > > I would not mention V4L2 here and keep the documentation generic > > > > > > > + */ > > > > + > > > > /** > > > > * \fn ControlInfo::values() > > > > * \brief Retrieve the list of valid values > > > > diff --git a/src/libcamera/v4l2_device.cpp > > > b/src/libcamera/v4l2_device.cpp > > > > index 57a88d96b12c..9018f1b0b9e1 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -535,17 +535,20 @@ std::optional<ControlInfo> > > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > > case V4L2_CTRL_TYPE_U8: > > > > return ControlInfo(static_cast<uint8_t>(ctrl.minimum), > > > > static_cast<uint8_t>(ctrl.maximum), > > > > - > > > static_cast<uint8_t>(ctrl.default_value)); > > > > + > > > static_cast<uint8_t>(ctrl.default_value), > > > > + !!(ctrl.flags & > > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > > static_cast<bool>(ctrl.maximum), > > > > - static_cast<bool>(ctrl.default_value)); > > > > + static_cast<bool>(ctrl.default_value), > > > > + !!(ctrl.flags & > > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > > > case V4L2_CTRL_TYPE_INTEGER64: > > > > return ControlInfo(static_cast<int64_t>(ctrl.minimum), > > > > static_cast<int64_t>(ctrl.maximum), > > > > - > > > static_cast<int64_t>(ctrl.default_value)); > > > > + > > > static_cast<int64_t>(ctrl.default_value), > > > > + !!(ctrl.flags & > > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > > case V4L2_CTRL_TYPE_MENU: > > > > @@ -554,7 +557,8 @@ std::optional<ControlInfo> > > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > > default: > > > > return ControlInfo(static_cast<int32_t>(ctrl.minimum), > > > > static_cast<int32_t>(ctrl.maximum), > > > > - > > > static_cast<int32_t>(ctrl.default_value)); > > > > + > > > static_cast<int32_t>(ctrl.default_value), > > > > + !!(ctrl.flags & > > > V4L2_CTRL_FLAG_READ_ONLY)); > > > > } > > > > } > > > > > > > > -- > > > > 2.25.1 > > > > > > >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index cf94205577a5..488663a7ba04 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -270,7 +270,8 @@ class ControlInfo public: explicit ControlInfo(const ControlValue &min = {}, const ControlValue &max = {}, - const ControlValue &def = {}); + const ControlValue &def = {}, + bool readOnly = false); explicit ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}); explicit ControlInfo(std::set<bool> values, bool def); @@ -279,6 +280,7 @@ public: const ControlValue &min() const { return min_; } const ControlValue &max() const { return max_; } const ControlValue &def() const { return def_; } + bool readOnly() const { return readOnly_; } const std::vector<ControlValue> &values() const { return values_; } std::string toString() const; @@ -297,6 +299,7 @@ private: ControlValue min_; ControlValue max_; ControlValue def_; + bool readOnly_; std::vector<ControlValue> values_; }; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 6dbf9b348709..fc66abad600d 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen * \param[in] min The control minimum value * \param[in] max The control maximum value * \param[in] def The control default value + * \param[in] readOnly Read-only status of a V4L2 control */ ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max, - const ControlValue &def) - : min_(min), max_(max), def_(def) + const ControlValue &def, + bool readOnly) + : min_(min), max_(max), def_(def), readOnly_(readOnly) { } @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, * default value is \a def. */ ControlInfo::ControlInfo(std::set<bool> values, bool def) - : min_(false), max_(true), def_(def), values_({ false, true }) + : min_(false), max_(true), def_(def), readOnly_(false), + values_({ false, true }) { ASSERT(values.count(def) && values.size() == 2); } @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool def) * value. The minimum, maximum, and default values will all be \a value. */ ControlInfo::ControlInfo(bool value) - : min_(value), max_(value), def_(value) + : min_(value), max_(value), def_(value), readOnly_(false) { values_ = { value }; } @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value) * \return A ControlValue with the default value for the control */ +/** + * \fn ControlInfo::readOnly() + * \brief Identifies if a V4L2 control is flagged as read-only + * \return True if the V4L2 control is read-only, false otherwise + */ + /** * \fn ControlInfo::values() * \brief Retrieve the list of valid values diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 57a88d96b12c..9018f1b0b9e1 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -535,17 +535,20 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl case V4L2_CTRL_TYPE_U8: return ControlInfo(static_cast<uint8_t>(ctrl.minimum), static_cast<uint8_t>(ctrl.maximum), - static_cast<uint8_t>(ctrl.default_value)); + static_cast<uint8_t>(ctrl.default_value), + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); case V4L2_CTRL_TYPE_BOOLEAN: return ControlInfo(static_cast<bool>(ctrl.minimum), static_cast<bool>(ctrl.maximum), - static_cast<bool>(ctrl.default_value)); + static_cast<bool>(ctrl.default_value), + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); case V4L2_CTRL_TYPE_INTEGER64: return ControlInfo(static_cast<int64_t>(ctrl.minimum), static_cast<int64_t>(ctrl.maximum), - static_cast<int64_t>(ctrl.default_value)); + static_cast<int64_t>(ctrl.default_value), + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); case V4L2_CTRL_TYPE_INTEGER_MENU: case V4L2_CTRL_TYPE_MENU: @@ -554,7 +557,8 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl default: return ControlInfo(static_cast<int32_t>(ctrl.minimum), static_cast<int32_t>(ctrl.maximum), - static_cast<int32_t>(ctrl.default_value)); + static_cast<int32_t>(ctrl.default_value), + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)); } }
Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 control is read-only or not. This flag only makes sense for V2L2 controls at preset, so only update the ControlInfo constructor signature used by the V4L2Device class to set the value of the flag. Add a ControlInfo::readOnly() member function to retrive this flag. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/controls.h | 5 ++++- src/libcamera/controls.cpp | 17 +++++++++++++---- src/libcamera/v4l2_device.cpp | 12 ++++++++---- 3 files changed, 25 insertions(+), 9 deletions(-)