| Message ID | 20190827095008.11405-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > Add a constructor for the V4L2ControlList class that accepts a list of > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > instance to be used for reading controls only. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_controls.h | 3 +++ > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 10b726504951..86c84e06d741 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -65,6 +65,9 @@ public: > using iterator = std::vector<V4L2Control>::iterator; > using const_iterator = std::vector<V4L2Control>::const_iterator; > > + V4L2ControlList() {} > + V4L2ControlList(std::vector<unsigned int> ids); > + > iterator begin() { return controls_.begin(); } > const_iterator begin() const { return controls_.begin(); } > iterator end() { return controls_.end(); } > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index 84258d9954d0..eeb325cbfff9 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * and prepare to be re-used for a new control write/read sequence. > */ > > +/** > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > + * > + * Construct a V4L2ControlList from a list of control identifiers without any > + * value associated. This constructor is particularly useful to create a > + * V4L2ControlList that is used to read the values of all controls in the > + * \a ids list. The created V4L2ControlList should not be used to write control > + * values unless it is cleared first and then controls with an associated value > + * are manually added to it. > + */ Do you think it would be worth it to add some code to enforce that no controls can be written if it's initialized with this constructor? > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > +{ > + for (auto id : ids) > + add(id); > +} > + > /** > * \typedef V4L2ControlList::iterator > * \brief Iterator on the V4L2 controls contained in the instance > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > > Add a constructor for the V4L2ControlList class that accepts a list of > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > > instance to be used for reading controls only. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_controls.h | 3 +++ > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > index 10b726504951..86c84e06d741 100644 > > --- a/src/libcamera/include/v4l2_controls.h > > +++ b/src/libcamera/include/v4l2_controls.h > > @@ -65,6 +65,9 @@ public: > > using iterator = std::vector<V4L2Control>::iterator; > > using const_iterator = std::vector<V4L2Control>::const_iterator; > > > > + V4L2ControlList() {} > > + V4L2ControlList(std::vector<unsigned int> ids); > > + > > iterator begin() { return controls_.begin(); } > > const_iterator begin() const { return controls_.begin(); } > > iterator end() { return controls_.end(); } > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > index 84258d9954d0..eeb325cbfff9 100644 > > --- a/src/libcamera/v4l2_controls.cpp > > +++ b/src/libcamera/v4l2_controls.cpp > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * and prepare to be re-used for a new control write/read sequence. > > */ > > > > +/** > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > > + * > > + * Construct a V4L2ControlList from a list of control identifiers without any > > + * value associated. This constructor is particularly useful to create a > > + * V4L2ControlList that is used to read the values of all controls in the > > + * \a ids list. The created V4L2ControlList should not be used to write control > > + * values unless it is cleared first and then controls with an associated value > > + * are manually added to it. > > + */ > > Do you think it would be worth it to add some code to enforce that no > controls can be written if it's initialized with this constructor? > How would you do so? The most naive implementation that comes to mind it's to add a 'writable' flag to initialize to true only if the control has a value assigned and check for the flag V4L2Device::setControls() > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > > +{ > > + for (auto id : ids) > > + add(id); > > +} > > + > > /** > > * \typedef V4L2ControlList::iterator > > * \brief Iterator on the V4L2 controls contained in the instance > > -- > > 2.23.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > > > Add a constructor for the V4L2ControlList class that accepts a list of > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > > > instance to be used for reading controls only. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/include/v4l2_controls.h | 3 +++ > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > > > 2 files changed, 20 insertions(+) > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > index 10b726504951..86c84e06d741 100644 > > > --- a/src/libcamera/include/v4l2_controls.h > > > +++ b/src/libcamera/include/v4l2_controls.h > > > @@ -65,6 +65,9 @@ public: > > > using iterator = std::vector<V4L2Control>::iterator; > > > using const_iterator = std::vector<V4L2Control>::const_iterator; > > > > > > + V4L2ControlList() {} > > > + V4L2ControlList(std::vector<unsigned int> ids); > > > + > > > iterator begin() { return controls_.begin(); } > > > const_iterator begin() const { return controls_.begin(); } > > > iterator end() { return controls_.end(); } > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > index 84258d9954d0..eeb325cbfff9 100644 > > > --- a/src/libcamera/v4l2_controls.cpp > > > +++ b/src/libcamera/v4l2_controls.cpp > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > * and prepare to be re-used for a new control write/read sequence. > > > */ > > > > > > +/** > > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > > > + * > > > + * Construct a V4L2ControlList from a list of control identifiers without any > > > + * value associated. This constructor is particularly useful to create a > > > + * V4L2ControlList that is used to read the values of all controls in the > > > + * \a ids list. The created V4L2ControlList should not be used to write control > > > + * values unless it is cleared first and then controls with an associated value > > > + * are manually added to it. > > > + */ > > > > Do you think it would be worth it to add some code to enforce that no > > controls can be written if it's initialized with this constructor? > > > > How would you do so? The most naive implementation that comes to mind > it's to add a 'writable' flag to initialize to true only if the > control has a value assigned and check for the flag > V4L2Device::setControls() I'm not sure how to best implement it, but the constraint is so specific it looked like something that could be described in code. Now that I dig thru the code with fresh eyes I can't find the rational for the V4L2ControlList needs to be cleared before it's attempted to be used for writing. The control values are initialized to 0 using this new constructor and if used for writing V4L2Device::setControls() will validate that the control exists on the device and bail before writing anything if one the controls do not exist. What reason do you think there is for the need to manually add controls with a specific value before writing? This is just a specialisation where all values are initialized to 0, at least in my mind I might be missing something ;-) > > > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > > > +{ > > > + for (auto id : ids) > > > + add(id); > > > +} > > > + > > > /** > > > * \typedef V4L2ControlList::iterator > > > * \brief Iterator on the V4L2 controls contained in the instance > > > -- > > > 2.23.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote: > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote: > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > > > > Add a constructor for the V4L2ControlList class that accepts a list of > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > > > > instance to be used for reading controls only. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/include/v4l2_controls.h | 3 +++ > > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > > > > 2 files changed, 20 insertions(+) > > > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > > index 10b726504951..86c84e06d741 100644 > > > > --- a/src/libcamera/include/v4l2_controls.h > > > > +++ b/src/libcamera/include/v4l2_controls.h > > > > @@ -65,6 +65,9 @@ public: > > > > using iterator = std::vector<V4L2Control>::iterator; > > > > using const_iterator = std::vector<V4L2Control>::const_iterator; > > > > > > > > + V4L2ControlList() {} > > > > + V4L2ControlList(std::vector<unsigned int> ids); > > > > + > > > > iterator begin() { return controls_.begin(); } > > > > const_iterator begin() const { return controls_.begin(); } > > > > iterator end() { return controls_.end(); } > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > > index 84258d9954d0..eeb325cbfff9 100644 > > > > --- a/src/libcamera/v4l2_controls.cpp > > > > +++ b/src/libcamera/v4l2_controls.cpp > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > > * and prepare to be re-used for a new control write/read sequence. > > > > */ > > > > > > > > +/** > > > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > > > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > > > > + * > > > > + * Construct a V4L2ControlList from a list of control identifiers without any > > > > + * value associated. This constructor is particularly useful to create a > > > > + * V4L2ControlList that is used to read the values of all controls in the > > > > + * \a ids list. The created V4L2ControlList should not be used to write control > > > > + * values unless it is cleared first and then controls with an associated value > > > > + * are manually added to it. > > > > + */ > > > > > > Do you think it would be worth it to add some code to enforce that no > > > controls can be written if it's initialized with this constructor? > > > > > > > How would you do so? The most naive implementation that comes to mind > > it's to add a 'writable' flag to initialize to true only if the > > control has a value assigned and check for the flag > > V4L2Device::setControls() > > I'm not sure how to best implement it, but the constraint is so specific > it looked like something that could be described in code. Now that I dig > thru the code with fresh eyes I can't find the rational for the > V4L2ControlList needs to be cleared before it's attempted to be used for > writing. > > The control values are initialized to 0 using this new constructor and > if used for writing V4L2Device::setControls() will validate that the > control exists on the device and bail before writing anything if one the > controls do not exist. Yes, it validates the control exists https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251 but it will silently set the value of the v4l2 control to write to 0 https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265 One could V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... }; device->setControls(&list); and if the controls in the list accept 0 as a value, set all controls to 0 maybe unintentionally. > > What reason do you think there is for the need to manually add controls > with a specific value before writing? This is just a specialisation > where all values are initialized to 0, at least in my mind I might be > missing something ;-) > We have no guarantee the controls in the list accept 0, or that they even accepts integers at all (even if, we only support integers controls at the moment). We cannot prevent anyone from shooting on his own foot, but to me documenting that the list should only be used for reading, or enforcing it in the code as you suggested, is a good thing... > > > > > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > > > > +{ > > > > + for (auto id : ids) > > > > + add(id); > > > > +} > > > > + > > > > /** > > > > * \typedef V4L2ControlList::iterator > > > > * \brief Iterator on the V4L2 controls contained in the instance > > > > -- > > > > 2.23.0 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-08-29 10:56:57 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote: > > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote: > > > Hi Niklas, > > > > > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote: > > > > Hi Jacopo, > > > > > > > > Thanks for your work. > > > > > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > > > > > Add a constructor for the V4L2ControlList class that accepts a list of > > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > > > > > instance to be used for reading controls only. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > src/libcamera/include/v4l2_controls.h | 3 +++ > > > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > > > > > 2 files changed, 20 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > > > index 10b726504951..86c84e06d741 100644 > > > > > --- a/src/libcamera/include/v4l2_controls.h > > > > > +++ b/src/libcamera/include/v4l2_controls.h > > > > > @@ -65,6 +65,9 @@ public: > > > > > using iterator = std::vector<V4L2Control>::iterator; > > > > > using const_iterator = std::vector<V4L2Control>::const_iterator; > > > > > > > > > > + V4L2ControlList() {} > > > > > + V4L2ControlList(std::vector<unsigned int> ids); > > > > > + > > > > > iterator begin() { return controls_.begin(); } > > > > > const_iterator begin() const { return controls_.begin(); } > > > > > iterator end() { return controls_.end(); } > > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > > > index 84258d9954d0..eeb325cbfff9 100644 > > > > > --- a/src/libcamera/v4l2_controls.cpp > > > > > +++ b/src/libcamera/v4l2_controls.cpp > > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > > > * and prepare to be re-used for a new control write/read sequence. > > > > > */ > > > > > > > > > > +/** > > > > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > > > > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > > > > > + * > > > > > + * Construct a V4L2ControlList from a list of control identifiers without any > > > > > + * value associated. This constructor is particularly useful to create a > > > > > + * V4L2ControlList that is used to read the values of all controls in the > > > > > + * \a ids list. The created V4L2ControlList should not be used to write control > > > > > + * values unless it is cleared first and then controls with an associated value > > > > > + * are manually added to it. > > > > > + */ > > > > > > > > Do you think it would be worth it to add some code to enforce that no > > > > controls can be written if it's initialized with this constructor? > > > > > > > > > > How would you do so? The most naive implementation that comes to mind > > > it's to add a 'writable' flag to initialize to true only if the > > > control has a value assigned and check for the flag > > > V4L2Device::setControls() > > > > I'm not sure how to best implement it, but the constraint is so specific > > it looked like something that could be described in code. Now that I dig > > thru the code with fresh eyes I can't find the rational for the > > V4L2ControlList needs to be cleared before it's attempted to be used for > > writing. > > > > The control values are initialized to 0 using this new constructor and > > if used for writing V4L2Device::setControls() will validate that the > > control exists on the device and bail before writing anything if one the > > controls do not exist. > > Yes, it validates the control exists > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251 > > but it will silently set the value of the v4l2 control to write to 0 > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265 > > One could > V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... }; > device->setControls(&list); > > and if the controls in the list accept 0 as a value, set all controls > to 0 maybe unintentionally. Absolutely, that's what I'm saying :-) We already have this feature with the following code: V4L2ControlList list; list.add(V4L2_CID_...); list.add(V4L2_CID_...); device->setControls(&list); So this new constructor is just a more convenient way to do the above and I see no real reason to block this use-case as the behaviour is well defined. If V4L2ControlList::add() value argument did not have an default value set I agree this would be a slightly different change. If we add logic to prevent a V4L2ControlList initialized with this new constructor from being written we should also block V4L2ControlList where controls where added with just an id an no explicit value from being written. > > > > > What reason do you think there is for the need to manually add controls > > with a specific value before writing? This is just a specialisation > > where all values are initialized to 0, at least in my mind I might be > > missing something ;-) > > > We have no guarantee the controls in the list accept 0, or that they > even accepts integers at all (even if, we only support integers > controls at the moment). > > We cannot prevent anyone from shooting on his own foot, but to > me documenting that the list should only be used for reading, or > enforcing it in the code as you suggested, is a good thing... > > > > > > > > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > > > > > +{ > > > > > + for (auto id : ids) > > > > > + add(id); > > > > > +} > > > > > + > > > > > /** > > > > > * \typedef V4L2ControlList::iterator > > > > > * \brief Iterator on the V4L2 controls contained in the instance > > > > > -- > > > > > 2.23.0 > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Thu, Aug 29, 2019 at 11:08:25AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > On 2019-08-29 10:56:57 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote: > > > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote: > > > > Hi Niklas, > > > > > > > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thanks for your work. > > > > > > > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote: > > > > > > Add a constructor for the V4L2ControlList class that accepts a list of > > > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > > > > > > instance to be used for reading controls only. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > --- > > > > > > src/libcamera/include/v4l2_controls.h | 3 +++ > > > > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > > > > > > 2 files changed, 20 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > > > > index 10b726504951..86c84e06d741 100644 > > > > > > --- a/src/libcamera/include/v4l2_controls.h > > > > > > +++ b/src/libcamera/include/v4l2_controls.h > > > > > > @@ -65,6 +65,9 @@ public: > > > > > > using iterator = std::vector<V4L2Control>::iterator; > > > > > > using const_iterator = std::vector<V4L2Control>::const_iterator; > > > > > > > > > > > > + V4L2ControlList() {} > > > > > > + V4L2ControlList(std::vector<unsigned int> ids); > > > > > > + > > > > > > iterator begin() { return controls_.begin(); } > > > > > > const_iterator begin() const { return controls_.begin(); } > > > > > > iterator end() { return controls_.end(); } > > > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > > > > index 84258d9954d0..eeb325cbfff9 100644 > > > > > > --- a/src/libcamera/v4l2_controls.cpp > > > > > > +++ b/src/libcamera/v4l2_controls.cpp > > > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > > > > * and prepare to be re-used for a new control write/read sequence. > > > > > > */ > > > > > > > > > > > > +/** > > > > > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > > > > > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > > > > > > + * > > > > > > + * Construct a V4L2ControlList from a list of control identifiers without any > > > > > > + * value associated. This constructor is particularly useful to create a > > > > > > + * V4L2ControlList that is used to read the values of all controls in the > > > > > > + * \a ids list. The created V4L2ControlList should not be used to write control > > > > > > + * values unless it is cleared first and then controls with an associated value > > > > > > + * are manually added to it. > > > > > > + */ > > > > > > > > > > Do you think it would be worth it to add some code to enforce that no > > > > > controls can be written if it's initialized with this constructor? > > > > > > > > > > > > > How would you do so? The most naive implementation that comes to mind > > > > it's to add a 'writable' flag to initialize to true only if the > > > > control has a value assigned and check for the flag > > > > V4L2Device::setControls() > > > > > > I'm not sure how to best implement it, but the constraint is so specific > > > it looked like something that could be described in code. Now that I dig > > > thru the code with fresh eyes I can't find the rational for the > > > V4L2ControlList needs to be cleared before it's attempted to be used for > > > writing. > > > > > > The control values are initialized to 0 using this new constructor and > > > if used for writing V4L2Device::setControls() will validate that the > > > control exists on the device and bail before writing anything if one the > > > controls do not exist. > > > > Yes, it validates the control exists > > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251 > > > > but it will silently set the value of the v4l2 control to write to 0 > > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265 > > > > One could > > V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... }; > > device->setControls(&list); > > > > and if the controls in the list accept 0 as a value, set all controls > > to 0 maybe unintentionally. > > Absolutely, that's what I'm saying :-) We already have this feature with > the following code: > > V4L2ControlList list; > list.add(V4L2_CID_...); > list.add(V4L2_CID_...); > device->setControls(&list); > > So this new constructor is just a more convenient way to do the above > and I see no real reason to block this use-case as the behaviour is well > defined. If V4L2ControlList::add() value argument did not have an > default value set I agree this would be a slightly different change. > > If we add logic to prevent a V4L2ControlList initialized with this new > constructor from being written we should also block V4L2ControlList > where controls where added with just an id an no explicit value from > being written. > Indeed, this applies to all controls added without a value, not just the lists constructed with the newly introduced constructor, which is just a shortcut to what you have here described... > > > > > > > > What reason do you think there is for the need to manually add controls > > > with a specific value before writing? This is just a specialisation > > > where all values are initialized to 0, at least in my mind I might be > > > missing something ;-) > > > > > We have no guarantee the controls in the list accept 0, or that they > > even accepts integers at all (even if, we only support integers > > controls at the moment). > > > > We cannot prevent anyone from shooting on his own foot, but to > > me documenting that the list should only be used for reading, or > > enforcing it in the code as you suggested, is a good thing... > > > > > > > > > > > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > > > > > > +{ > > > > > > + for (auto id : ids) > > > > > > + add(id); > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * \typedef V4L2ControlList::iterator > > > > > > * \brief Iterator on the V4L2 controls contained in the instance > > > > > > -- > > > > > > 2.23.0 > > > > > > > > > > > > _______________________________________________ > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > > > -- > > > > > Regards, > > > > > Niklas Söderlund > > > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, Thank you for the patch. On Tue, Aug 27, 2019 at 11:50:04AM +0200, Jacopo Mondi wrote: > Add a constructor for the V4L2ControlList class that accepts a list of > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList > instance to be used for reading controls only. I tend to understand "for reading controls only" as meaning for reading controls and nothing else than controls. Is it just me ? Should this be expressed as "to be used only for reading controls" ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_controls.h | 3 +++ > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 10b726504951..86c84e06d741 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -65,6 +65,9 @@ public: > using iterator = std::vector<V4L2Control>::iterator; > using const_iterator = std::vector<V4L2Control>::const_iterator; > > + V4L2ControlList() {} > + V4L2ControlList(std::vector<unsigned int> ids); How about using an initializer_list<unsigned int> instead of a vector ? > + > iterator begin() { return controls_.begin(); } > const_iterator begin() const { return controls_.begin(); } > iterator end() { return controls_.end(); } > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index 84258d9954d0..eeb325cbfff9 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * and prepare to be re-used for a new control write/read sequence. > */ > > +/** > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) > + * > + * Construct a V4L2ControlList from a list of control identifiers without any > + * value associated. This constructor is particularly useful to create a s/value/values/ > + * V4L2ControlList that is used to read the values of all controls in the > + * \a ids list. The created V4L2ControlList should not be used to write control s/should/shall/ > + * values unless it is cleared first and then controls with an associated value > + * are manually added to it. > + */ > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) > +{ > + for (auto id : ids) Let's not abuse auto, this is clearly a case where you can write the full type instead. > + add(id); > +} > + > /** > * \typedef V4L2ControlList::iterator > * \brief Iterator on the V4L2 controls contained in the instance
diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h index 10b726504951..86c84e06d741 100644 --- a/src/libcamera/include/v4l2_controls.h +++ b/src/libcamera/include/v4l2_controls.h @@ -65,6 +65,9 @@ public: using iterator = std::vector<V4L2Control>::iterator; using const_iterator = std::vector<V4L2Control>::const_iterator; + V4L2ControlList() {} + V4L2ControlList(std::vector<unsigned int> ids); + iterator begin() { return controls_.begin(); } const_iterator begin() const { return controls_.begin(); } iterator end() { return controls_.end(); } diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp index 84258d9954d0..eeb325cbfff9 100644 --- a/src/libcamera/v4l2_controls.cpp +++ b/src/libcamera/v4l2_controls.cpp @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * and prepare to be re-used for a new control write/read sequence. */ +/** + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers + * \param ids A list of V4L2 control identifiers (V4L2_CID_*) + * + * Construct a V4L2ControlList from a list of control identifiers without any + * value associated. This constructor is particularly useful to create a + * V4L2ControlList that is used to read the values of all controls in the + * \a ids list. The created V4L2ControlList should not be used to write control + * values unless it is cleared first and then controls with an associated value + * are manually added to it. + */ +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids) +{ + for (auto id : ids) + add(id); +} + /** * \typedef V4L2ControlList::iterator * \brief Iterator on the V4L2 controls contained in the instance
Add a constructor for the V4L2ControlList class that accepts a list of V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList instance to be used for reading controls only. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_controls.h | 3 +++ src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++ 2 files changed, 20 insertions(+)