[libcamera-devel] [PATCH 7/9] libcamera: v4l2_device: Replace V4L2ControlList with ControlList

[Jacopo Mondi]

Hi Laurent

On Wed, Oct 09, 2019 at 02:54:31PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 09, 2019 at 11:56:55AM +0200, Jacopo Mondi wrote:
> > On Wed, Oct 09, 2019 at 12:38:54PM +0300, Laurent Pinchart wrote:
> > > On Wed, Oct 09, 2019 at 11:13:53AM +0200, Jacopo Mondi wrote:
> > >> Hi Laurent,
> > >>     just a note that this break compilation, but you know that already
> > >
> > > Isn't it patch 9/9 that breaks compilation ?
> >

[snip]

> > >>> - * In order to set and get controls, user of the libcamera V4L2 control
> > >>> - * framework should operate on instances of the V4L2ControlList class, and use
> > >>> - * them as argument for the V4L2Device::setControls() and
> > >>> - * V4L2Device::getControls() operations, which write and read a list of
> > >>> - * controls to or from a V4L2 device (a video device or a subdevice).
> > >>
> > >> I would not remove this part, as it explains how to use the
> > >> V4L2ControlList, and it still applies today even if those methods
> > >> accepts a ControlList *
> > >
> > > Doesn't it belong to V4L2Device though ?
> >
> > Possibly, but it also seems to me that it explains how to use a
> > V4L2ControlList. Up to you..
>
> I thought about adding a reference to setControls() and getControls(),
> but then realised that those two methods take a ControlList, not a
> V4L2ControlList. How about the following ?
>
>  * V4L2ControlList allows easy construction of a ControlList containing V4L2
>  * controls for a device. It can be used to construct the list of controls
>  * passed to the V4L2Device::getControls() and V4L2Device::setControls()
>  * methods. The class should however not be used in place of ControlList in
>  * APIs unless strictly required.

I like it!

>
[snip]

> > >>> -	return &controls_[index];
> > >>> +	return ControlList::contains(ctrl->second.id());
> > >>
> > >> Ah wait, we have
> > >> ControlList::controls_ and
> > >> V4L2ControlList::controls_
> > >>
> > >> is this intended ?
> > >
> > > Yes. I could rename the latter if you think it would be better. What
> > > name would you prefer ?
> >
> > I'm a bit bothered by having two different things with the same name,
> > even if their scope is different... I would name the latter
> > controlInfoMap_ even if it's a bit longer
>
> How about infoMap_ or even just info_ ?

You know, I would have kept 'control' somewhere in the name, but I'm
fine dropping it for something shorter like 'info_'.

Minor stuff though, do whatever you like the most!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191009/c6760ced/attachment.sig>