Hi
On Mon, Nov 18, 2024 at 4:59 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> On 14-Nov-24 9:03 PM, Ricardo Ribalda wrote:
> > Hi Gergo
> >
> > Sorry, I forgot to reply to your comment in v14.
> >
> > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@xxxxxx> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> >>>
> >>> + },
> >>> + {
> >>> + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> >>> + .entity = UVC_GUID_UVC_CAMERA,
> >>> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
> >>> + .size = 16,
> >>> + .offset = 64,
> >>> + .v4l2_type = V4L2_CTRL_TYPE_BITMASK,
> >>> + .data_type = UVC_CTRL_DATA_TYPE_BITMASK,
> >>> + .name = "Region Of Interest Auto Controls",
> >>> + },
> >>> };
> >>>
> >>
> >> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?
> >
> > If I create 8 Booleans, they will always be shown in the device. And
> > the user will not have a way to know which values are available and
> > which are not.
> >
> > We will also fail the v4l2-compliance test, because there will be up
> > to 7 boolean controls that will not be able to be set to 1, eventhough
> > they are writable.
>
> So why can't these other controls be set to 1? Because only one
> of the options in the bitmask can be selected at a time ?
>
> If only 1 bit in the UVC_CTRL_DATA_TYPE_BITMASK for this can be one
> at the time, then this should be mapped to a V4L2_CTRL_TYPE_MENU
> just like how that is done for V4L2_CID_EXPOSURE_AUTO already.
>
> Actually looking at existing comments about UVC_CTRL_DATA_TYPE_BITMASK
> in the driver there is this comment on top of uvc_mapping_get_menu_value()
>
> * For controls of type UVC_CTRL_DATA_TYPE_BITMASK, the UVC control value is
> * expressed as a bitmask and is thus guaranteed to have a single bit set.
>
> Assuming this "guaranteed to have a single bit set" comment is valid for
> the V4L2_CID_UVC_REGION_OF_INTEREST_AUTO part of UVC_CT_REGION_OF_INTEREST_CONTROL
> too then I think we should simply map this to a menu similar to how
> this is done for V4L2_CID_EXPOSURE_AUTO.
>
> Note V4L2_CID_EXPOSURE_AUTO is the only existing user of UVC_CTRL_DATA_TYPE_BITMASK
> at the moment.
>
> Mapping this to a menu should nicely address Gergo's concerns here.
The UVC standard is not very clear re bmAutoControls. It says:
"""
The bmAutoControls bitmask determines which, if any, on board features
should track to the region of interest. To detect if a device supports
a particular Auto Control, use GET_MAX which returns a mask indicating
all supported Auto Controls.
GET_CUR returns the current Region of Interest (RoI) being employed by
the device. This RoI should be the same as specified in most recent
SET_CUR except in the case where the ‘Auto Detect and Track’ and/or
‘Image Stabilization’ bit have been set.
"""
Which makes me believe that you can set another Auto value + one of
these ones. So I do not think that we can assume "guaranteed to have a
single bit set".
The behaviour will vary module to module. So I'd rather take a
conservative approach here and let the hardware clamp the value and
not us.
>
> Regards,
>
> Hans
>
