Illuminators and status LED controls

  Hi,

This new proposal cancels the previous 'LED control' patch.

Cheers.
  

Hi,

Looks good to me.

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

On 09/06/2010 08:11 PM, Jean-Francois Moine wrote:
> Hi,
>
> This new proposal cancels the previous 'LED control' patch.
>
> Cheers.
>
> -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
>
>
> led.patch
>
>
> Some media devices (microscopes) may have one or many illuminators,
> and most webcams have a status LED which is normally on when capture is active.
> This patch makes them controlable by the applications.
>
> Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
>
> diff --git a/Documentation/DocBook/v4l/controls.xml b/Documentation/DocBook/v4l/controls.xml
> index 8408caa..77f87ad 100644
> --- a/Documentation/DocBook/v4l/controls.xml
> +++ b/Documentation/DocBook/v4l/controls.xml
> @@ -312,10 +312,27 @@ minimum value disables backlight compensation.</entry>
>   	information and bits 24-31 must be zero.</entry>
>   	</row>
>   	<row>
> +	<entry><constant>V4L2_CID_ILLUMINATORS</constant></entry>
> +	<entry>integer</entry>
> +	<entry>Switch on or off the illuminator(s) of the device
> +		(usually a microscope).
> +	    The control type and values depend on the driver and may be either
> +	    a single boolean (0: off, 1:on) or defined by a menu type.</entry>
> +	</row>
> +	<row id="v4l2_status_led">
> +	<entry><constant>V4L2_CID_STATUS_LED</constant></entry>
> +	<entry>integer</entry>
> +	<entry>Set the status LED behaviour. Possible values for
> +<constant>enum v4l2_status_led</constant>  are:
> +<constant>V4L2_STATUS_LED_AUTO</constant>  (0),
> +<constant>V4L2_STATUS_LED_ON</constant>  (1),
> +<constant>V4L2_STATUS_LED_OFF</constant>  (2).</entry>
> +	</row>
> +	<row>
>   	<entry><constant>V4L2_CID_LASTP1</constant></entry>
>   	<entry></entry>
>   	<entry>End of the predefined control IDs (currently
> -<constant>V4L2_CID_BG_COLOR</constant>  + 1).</entry>
> +<constant>V4L2_CID_STATUS_LED</constant>  + 1).</entry>
>   	</row>
>   	<row>
>   	<entry><constant>V4L2_CID_PRIVATE_BASE</constant></entry>
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 61490c6..75e8869 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1045,8 +1045,16 @@ enum v4l2_colorfx {
>
>   #define V4L2_CID_CHROMA_GAIN                    (V4L2_CID_BASE+36)
>
> +#define V4L2_CID_ILLUMINATORS			(V4L2_CID_BASE+37)
> +#define V4L2_CID_STATUS_LED			(V4L2_CID_BASE+38)
> +enum v4l2_status_led {
> +	V4L2_STATUS_LED_AUTO	= 0,
> +	V4L2_STATUS_LED_ON	= 1,
> +	V4L2_STATUS_LED_OFF	= 2,
> +};
> +
>   /* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+37)
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+39)
>
>   /*  MPEG-class control IDs defined by V4L2 */
>   #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
> Hi,
> 
> This new proposal cancels the previous 'LED control' patch.
> 
> Cheers.
> 
> 

Hi Jean-Francois,

You must also add support for these new controls in v4l2-ctrls.c in
v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().

How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
Wouldn't a bitmask type be more suitable to this than a menu type? There
isn't a bitmask type at the moment, but this seems to be a pretty good
candidate for a type like that.

Actually, for the status led I would also use a bitmask since there may be
multiple leds. I guess you would need two bitmasks: one to select auto vs
manual, and one for the manual settings.

Regards,

	Hans
  

Hi,

On 09/07/2010 09:30 AM, Hans Verkuil wrote:
> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
>> Hi,
>>
>> This new proposal cancels the previous 'LED control' patch.
>>
>> Cheers.
>>
>>
>
> Hi Jean-Francois,
>
> You must also add support for these new controls in v4l2-ctrls.c in
> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
>
> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
> Wouldn't a bitmask type be more suitable to this than a menu type? There
> isn't a bitmask type at the moment, but this seems to be a pretty good
> candidate for a type like that.
>
> Actually, for the status led I would also use a bitmask since there may be
> multiple leds. I guess you would need two bitmasks: one to select auto vs
> manual, and one for the manual settings.
>

So far I've not seen cameras with multiple status leds, I do have seen camera
which have the following settings for their 1 led (logitech uvc cams):
auto
on
off
blinking

So I think a menu type is better suited, and that is what the current (private)
uvc control uses.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Replying to myself.

On 09/07/2010 11:42 AM, Hans de Goede wrote:
> Hi,
>
> On 09/07/2010 09:30 AM, Hans Verkuil wrote:
>> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
>>> Hi,
>>>
>>> This new proposal cancels the previous 'LED control' patch.
>>>
>>> Cheers.
>>>
>>>
>>
>> Hi Jean-Francois,
>>
>> You must also add support for these new controls in v4l2-ctrls.c in
>> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
>>
>> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
>> Wouldn't a bitmask type be more suitable to this than a menu type? There
>> isn't a bitmask type at the moment, but this seems to be a pretty good
>> candidate for a type like that.
>>
>> Actually, for the status led I would also use a bitmask since there may be
>> multiple leds. I guess you would need two bitmasks: one to select auto vs
>> manual, and one for the manual settings.
>>
>
> So far I've not seen cameras with multiple status leds, I do have seen camera
> which have the following settings for their 1 led (logitech uvc cams):
> auto
> on
> off
> blinking
>
> So I think a menu type is better suited, and that is what the current (private)
> uvc control uses.

The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
that we currently don't have a bitmask type I think introducing one without a really
really good reason is a bad idea as any exiting apps won't know how to deal with it.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tuesday, September 07, 2010 11:44:18 Hans de Goede wrote:
> Replying to myself.
> 
> On 09/07/2010 11:42 AM, Hans de Goede wrote:
> > Hi,
> >
> > On 09/07/2010 09:30 AM, Hans Verkuil wrote:
> >> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
> >>> Hi,
> >>>
> >>> This new proposal cancels the previous 'LED control' patch.
> >>>
> >>> Cheers.
> >>>
> >>>
> >>
> >> Hi Jean-Francois,
> >>
> >> You must also add support for these new controls in v4l2-ctrls.c in
> >> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
> >>
> >> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
> >> Wouldn't a bitmask type be more suitable to this than a menu type? There
> >> isn't a bitmask type at the moment, but this seems to be a pretty good
> >> candidate for a type like that.
> >>
> >> Actually, for the status led I would also use a bitmask since there may be
> >> multiple leds. I guess you would need two bitmasks: one to select auto vs
> >> manual, and one for the manual settings.
> >>
> >
> > So far I've not seen cameras with multiple status leds, I do have seen camera
> > which have the following settings for their 1 led (logitech uvc cams):
> > auto
> > on
> > off
> > blinking
> >
> > So I think a menu type is better suited, and that is what the current (private)
> > uvc control uses.
> 
> The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
> that we currently don't have a bitmask type I think introducing one without a really
> really good reason is a bad idea as any exiting apps won't know how to deal with it.

But I can guarantee that we will get video devices with multiple leds in the
future. So we need to think *now* about how to do this. One simple option is of course
to name the controls CID_ILLUMINATOR0 and CID_LED0. That way we can easily add LED1,
LED2, etc. later without running into weird inconsistent control names.

Regards,

	Hans
  

Hi all,

On 09/07/2010 11:47 AM, Hans Verkuil wrote:
> On Tuesday, September 07, 2010 11:44:18 Hans de Goede wrote:
>> Replying to myself.
>>
>> On 09/07/2010 11:42 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 09/07/2010 09:30 AM, Hans Verkuil wrote:
>>>> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
>>>>> Hi,
>>>>>
>>>>> This new proposal cancels the previous 'LED control' patch.
>>>>>
>>>>> Cheers.
>>>>>
>>>>>
>>>>
>>>> Hi Jean-Francois,
>>>>
>>>> You must also add support for these new controls in v4l2-ctrls.c in
>>>> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
>>>>
>>>> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
>>>> Wouldn't a bitmask type be more suitable to this than a menu type? There
>>>> isn't a bitmask type at the moment, but this seems to be a pretty good
>>>> candidate for a type like that.
>>>>
>>>> Actually, for the status led I would also use a bitmask since there may be
>>>> multiple leds. I guess you would need two bitmasks: one to select auto vs
>>>> manual, and one for the manual settings.
>>>>
>>>
>>> So far I've not seen cameras with multiple status leds, I do have seen camera
>>> which have the following settings for their 1 led (logitech uvc cams):
>>> auto
>>> on
>>> off
>>> blinking
>>>
>>> So I think a menu type is better suited, and that is what the current (private)
>>> uvc control uses.
>>
>> The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
>> that we currently don't have a bitmask type I think introducing one without a really
>> really good reason is a bad idea as any exiting apps won't know how to deal with it.
>
> But I can guarantee that we will get video devices with multiple leds in the
> future. So we need to think *now* about how to do this. One simple option is of course
> to name the controls CID_ILLUMINATOR0 and CID_LED0. That way we can easily add LED1,
> LED2, etc. later without running into weird inconsistent control names.
>

Naming them LED0 and ILLUMINATOR0 works for me. Note about the illuminator one,
if you look at the patch it made the illuminator control a menu with the following
options:

Both off
Top on, Bottom off
Top off, Bottom on
Both on

Which raises the question do we leave this as is, or do we make this 2 boolean
controls. I personally would like to vote for keeping it as is, as both lamps
illuminate the same substrate in this case, and esp. switching between
Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
UI wise (iow switch from top to bottom lighting or visa versa.

Regards,

Hans


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi,

On 09/07/2010 04:50 PM, Hans Verkuil wrote:
> On Tuesday, September 07, 2010 13:59:19 Hans de Goede wrote:
>> Hi all,
>>
>> On 09/07/2010 11:47 AM, Hans Verkuil wrote:
>>> On Tuesday, September 07, 2010 11:44:18 Hans de Goede wrote:
>>>> Replying to myself.
>>>>
>>>> On 09/07/2010 11:42 AM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 09/07/2010 09:30 AM, Hans Verkuil wrote:
>>>>>> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This new proposal cancels the previous 'LED control' patch.
>>>>>>>
>>>>>>> Cheers.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Hi Jean-Francois,
>>>>>>
>>>>>> You must also add support for these new controls in v4l2-ctrls.c in
>>>>>> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
>>>>>>
>>>>>> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
>>>>>> Wouldn't a bitmask type be more suitable to this than a menu type? There
>>>>>> isn't a bitmask type at the moment, but this seems to be a pretty good
>>>>>> candidate for a type like that.
>>>>>>
>>>>>> Actually, for the status led I would also use a bitmask since there may be
>>>>>> multiple leds. I guess you would need two bitmasks: one to select auto vs
>>>>>> manual, and one for the manual settings.
>>>>>>
>>>>>
>>>>> So far I've not seen cameras with multiple status leds, I do have seen camera
>>>>> which have the following settings for their 1 led (logitech uvc cams):
>>>>> auto
>>>>> on
>>>>> off
>>>>> blinking
>>>>>
>>>>> So I think a menu type is better suited, and that is what the current (private)
>>>>> uvc control uses.
>>>>
>>>> The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
>>>> that we currently don't have a bitmask type I think introducing one without a really
>>>> really good reason is a bad idea as any exiting apps won't know how to deal with it.
>>>
>>> But I can guarantee that we will get video devices with multiple leds in the
>>> future. So we need to think *now* about how to do this. One simple option is of course
>>> to name the controls CID_ILLUMINATOR0 and CID_LED0. That way we can easily add LED1,
>>> LED2, etc. later without running into weird inconsistent control names.
>>>
>>
>> Naming them LED0 and ILLUMINATOR0 works for me. Note about the illuminator one,
>> if you look at the patch it made the illuminator control a menu with the following
>> options:
>
> Where in the patch? Am I missing something?
>
>>
>> Both off
>> Top on, Bottom off
>> Top off, Bottom on
>> Both on
>>
>> Which raises the question do we leave this as is, or do we make this 2 boolean
>> controls. I personally would like to vote for keeping it as is, as both lamps
>> illuminate the same substrate in this case, and esp. switching between
>> Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
>> UI wise (iow switch from top to bottom lighting or visa versa.
>
> The problem with having one control is that while this makes sense for this
> particular microscope, it doesn't make sense in general.
>

Actual it does atleast for microscopes in general a substrate under a microscope
usually is either illuminated from above or below.

> Standard controls such as proposed by this patch should have a fixed type

Although I agree with that sentiment in general I don't see this as an absolute
need, apps should get the type by doing a query ctrl not by assuming they
know it based on the cid.

And esp. for menu controls this is not true, for example
most devices have a light freq filter menu of:
off
50 hz
60 hz

Which matches what is documented in videodev2.h

Where as some have:
off
50 hz
60 hz
auto hz

Because they can (and default to) detect the light frequency automatically.

> consistent behavior. Note that I am also wondering whether it wouldn't be a
> good idea to use a menu for this, just as for the LEDs.

I do agree that the illuminator ctrls should be a menu, that way the driver
author can also choose wether to group 2 together in a single control or not
we simply should not specify the menu values in the spec (the same can
be said for the led case).

Regards,

Hams
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tuesday, September 07, 2010 13:59:19 Hans de Goede wrote:
> Hi all,
> 
> On 09/07/2010 11:47 AM, Hans Verkuil wrote:
> > On Tuesday, September 07, 2010 11:44:18 Hans de Goede wrote:
> >> Replying to myself.
> >>
> >> On 09/07/2010 11:42 AM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 09/07/2010 09:30 AM, Hans Verkuil wrote:
> >>>> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This new proposal cancels the previous 'LED control' patch.
> >>>>>
> >>>>> Cheers.
> >>>>>
> >>>>>
> >>>>
> >>>> Hi Jean-Francois,
> >>>>
> >>>> You must also add support for these new controls in v4l2-ctrls.c in
> >>>> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
> >>>>
> >>>> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
> >>>> Wouldn't a bitmask type be more suitable to this than a menu type? There
> >>>> isn't a bitmask type at the moment, but this seems to be a pretty good
> >>>> candidate for a type like that.
> >>>>
> >>>> Actually, for the status led I would also use a bitmask since there may be
> >>>> multiple leds. I guess you would need two bitmasks: one to select auto vs
> >>>> manual, and one for the manual settings.
> >>>>
> >>>
> >>> So far I've not seen cameras with multiple status leds, I do have seen camera
> >>> which have the following settings for their 1 led (logitech uvc cams):
> >>> auto
> >>> on
> >>> off
> >>> blinking
> >>>
> >>> So I think a menu type is better suited, and that is what the current (private)
> >>> uvc control uses.
> >>
> >> The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
> >> that we currently don't have a bitmask type I think introducing one without a really
> >> really good reason is a bad idea as any exiting apps won't know how to deal with it.
> >
> > But I can guarantee that we will get video devices with multiple leds in the
> > future. So we need to think *now* about how to do this. One simple option is of course
> > to name the controls CID_ILLUMINATOR0 and CID_LED0. That way we can easily add LED1,
> > LED2, etc. later without running into weird inconsistent control names.
> >
> 
> Naming them LED0 and ILLUMINATOR0 works for me. Note about the illuminator one,
> if you look at the patch it made the illuminator control a menu with the following
> options:

Where in the patch? Am I missing something?

> 
> Both off
> Top on, Bottom off
> Top off, Bottom on
> Both on
> 
> Which raises the question do we leave this as is, or do we make this 2 boolean
> controls. I personally would like to vote for keeping it as is, as both lamps
> illuminate the same substrate in this case, and esp. switching between
> Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
> UI wise (iow switch from top to bottom lighting or visa versa.

The problem with having one control is that while this makes sense for this
particular microscope, it doesn't make sense in general.

Standard controls such as proposed by this patch should have a fixed type and
consistent behavior. Note that I am also wondering whether it wouldn't be a
good idea to use a menu for this, just as for the LEDs. In fact, perhaps they
should use the same menu. While their purpose is different, they are quite similar
in behavior.

BTW, lovely word: 'illuminator'.

Regards,

	Hans

> 
> Regards,
> 
> Hans
> 
> 
>
  

On Tuesday, September 07, 2010 15:04:55 Hans de Goede wrote:
> Hi,
> 
> On 09/07/2010 04:50 PM, Hans Verkuil wrote:
> > On Tuesday, September 07, 2010 13:59:19 Hans de Goede wrote:
> >> Hi all,
> >>
> >> On 09/07/2010 11:47 AM, Hans Verkuil wrote:
> >>> On Tuesday, September 07, 2010 11:44:18 Hans de Goede wrote:
> >>>> Replying to myself.
> >>>>
> >>>> On 09/07/2010 11:42 AM, Hans de Goede wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 09/07/2010 09:30 AM, Hans Verkuil wrote:
> >>>>>> On Monday, September 06, 2010 20:11:05 Jean-Francois Moine wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> This new proposal cancels the previous 'LED control' patch.
> >>>>>>>
> >>>>>>> Cheers.
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> Hi Jean-Francois,
> >>>>>>
> >>>>>> You must also add support for these new controls in v4l2-ctrls.c in
> >>>>>> v4l2_ctrl_get_menu(), v4l2_ctrl_get_name() and v4l2_ctrl_fill().
> >>>>>>
> >>>>>> How is CID_ILLUMINATORS supposed to work in the case of multiple lights?
> >>>>>> Wouldn't a bitmask type be more suitable to this than a menu type? There
> >>>>>> isn't a bitmask type at the moment, but this seems to be a pretty good
> >>>>>> candidate for a type like that.
> >>>>>>
> >>>>>> Actually, for the status led I would also use a bitmask since there may be
> >>>>>> multiple leds. I guess you would need two bitmasks: one to select auto vs
> >>>>>> manual, and one for the manual settings.
> >>>>>>
> >>>>>
> >>>>> So far I've not seen cameras with multiple status leds, I do have seen camera
> >>>>> which have the following settings for their 1 led (logitech uvc cams):
> >>>>> auto
> >>>>> on
> >>>>> off
> >>>>> blinking
> >>>>>
> >>>>> So I think a menu type is better suited, and that is what the current (private)
> >>>>> uvc control uses.
> >>>>
> >>>> The same argument more or less goes for the CID_ILLIMUNATORS controls. Also given
> >>>> that we currently don't have a bitmask type I think introducing one without a really
> >>>> really good reason is a bad idea as any exiting apps won't know how to deal with it.
> >>>
> >>> But I can guarantee that we will get video devices with multiple leds in the
> >>> future. So we need to think *now* about how to do this. One simple option is of course
> >>> to name the controls CID_ILLUMINATOR0 and CID_LED0. That way we can easily add LED1,
> >>> LED2, etc. later without running into weird inconsistent control names.
> >>>
> >>
> >> Naming them LED0 and ILLUMINATOR0 works for me. Note about the illuminator one,
> >> if you look at the patch it made the illuminator control a menu with the following
> >> options:
> >
> > Where in the patch? Am I missing something?
> >
> >>
> >> Both off
> >> Top on, Bottom off
> >> Top off, Bottom on
> >> Both on
> >>
> >> Which raises the question do we leave this as is, or do we make this 2 boolean
> >> controls. I personally would like to vote for keeping it as is, as both lamps
> >> illuminate the same substrate in this case, and esp. switching between
> >> Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
> >> UI wise (iow switch from top to bottom lighting or visa versa.
> >
> > The problem with having one control is that while this makes sense for this
> > particular microscope, it doesn't make sense in general.
> >
> 
> Actual it does atleast for microscopes in general a substrate under a microscope
> usually is either illuminated from above or below.
> 
> > Standard controls such as proposed by this patch should have a fixed type
> 
> Although I agree with that sentiment in general I don't see this as an absolute
> need, apps should get the type by doing a query ctrl not by assuming they
> know it based on the cid.
> 
> And esp. for menu controls this is not true, for example
> most devices have a light freq filter menu of:
> off
> 50 hz
> 60 hz
> 
> Which matches what is documented in videodev2.h
> 
> Where as some have:
> off
> 50 hz
> 60 hz
> auto hz
> 
> Because they can (and default to) detect the light frequency automatically.

The v4l2 api allows drivers to selectively enable items from a menu. So this
control has a fixed menu type and a fixed menu contents. Some of the menu
choices are just not available for some drivers.

There are several advantages of sticking to one standard menu for standard
controls:

1) The contents of the menu can be defined centrally in v4l2-ctrls.c, which
   ensures consistency. Not only of the menu texts, but also of how the
   control behaves in various drivers.
2) It makes it possible to set the control directly from within a program.
   This is currently true for *all* standard controls and breaking this for no
   good reason is something that needs to be avoided. I don't think this
   is a requirement in the spec at the moment, but I think it should be.

Just the fact that you can in theory put anything you want into a control,
doesn't mean that you should. 

This looks pretty decent IMHO:

enum v4l2_illuminator {
        V4L2_ILLUMINATOR_OFF = 0,
        V4L2_ILLUMINATOR_ON = 1,
};
#define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
#define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)

enum v4l2_led {
        V4L2_LED_AUTO = 0,
        V4L2_LED_OFF = 1,
        V4L2_LED_ON = 2,
};
#define V4L2_CID_LED_0              (V4L2_CID_BASE+39)

Simple and straightforward.

> 
> > consistent behavior. Note that I am also wondering whether it wouldn't be a
> > good idea to use a menu for this, just as for the LEDs.
> 
> I do agree that the illuminator ctrls should be a menu, that way the driver
> author can also choose wether to group 2 together in a single control or not
> we simply should not specify the menu values in the spec (the same can
> be said for the led case).

See above. Just because you can do it, doesn't mean you should. In this case
I think it is a bad idea. Standard controls should have standard behavior.

Regards,

	Hans
  

On Tue, 7 Sep 2010 17:30:33 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> enum v4l2_illuminator {
>         V4L2_ILLUMINATOR_OFF = 0,
>         V4L2_ILLUMINATOR_ON = 1,
> };
> #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> 
> enum v4l2_led {
>         V4L2_LED_AUTO = 0,
>         V4L2_LED_OFF = 1,
>         V4L2_LED_ON = 2,
> };
> #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> 
> Simple and straightforward.

Hi,

Hans (de Goede), is this OK for you? I think that if we find more
illuminators or LEDs on some devices, we may add more V4L2_CID_xxx_n
controls.

Hans (Verkuil), may we have the same enum's for both light types?
Something like:

enum v4l2_light {
	V4L2_LIGHT_OFF = 0,
	V4L2_LIGHT_ON = 1,
	V4L2_LIGHT_AUTO = 2,
	V4L2_LIGHT_BLINK = 3,
};

Regards.
  

On Tuesday, September 07, 2010 19:57:18 Jean-Francois Moine wrote:
> On Tue, 7 Sep 2010 17:30:33 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > enum v4l2_illuminator {
> >         V4L2_ILLUMINATOR_OFF = 0,
> >         V4L2_ILLUMINATOR_ON = 1,
> > };
> > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> > 
> > enum v4l2_led {
> >         V4L2_LED_AUTO = 0,
> >         V4L2_LED_OFF = 1,
> >         V4L2_LED_ON = 2,
> > };
> > #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> > 
> > Simple and straightforward.
> 
> Hi,
> 
> Hans (de Goede), is this OK for you? I think that if we find more
> illuminators or LEDs on some devices, we may add more V4L2_CID_xxx_n
> controls.
> 
> Hans (Verkuil), may we have the same enum's for both light types?
> Something like:
> 
> enum v4l2_light {
> 	V4L2_LIGHT_OFF = 0,
> 	V4L2_LIGHT_ON = 1,
> 	V4L2_LIGHT_AUTO = 2,
> 	V4L2_LIGHT_BLINK = 3,
> };

I'm OK with that. Although 'blink' shouldn't be added yet unless we have a
driver that will actually make use of it.

Regards,

	Hans

> 
> Regards.
> 
>
  

Hello,

On Mon, Sep 06, 2010 at 08:11:05PM +0200, ext Jean-Francois Moine wrote:
> Hi,
> 
> This new proposal cancels the previous 'LED control' patch.
> 
> Cheers.
> 
> -- 
> Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
> Jef		|		http://moinejf.free.fr/

Apologies if this has been already discussed but,
doesn't this patch duplicates the same feature present
nowadays under include/linux/leds.h ??

I mean, if you want to control leds, I think we already have that API, no?

BR,

---
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi,

On 09/07/2010 05:30 PM, Hans Verkuil wrote:
> On Tuesday, September 07, 2010 15:04:55 Hans de Goede wrote:
>> Hi,
>>
>> On 09/07/2010 04:50 PM, Hans Verkuil wrote:

<snip>

>>>> Both off
>>>> Top on, Bottom off
>>>> Top off, Bottom on
>>>> Both on
>>>>
>>>> Which raises the question do we leave this as is, or do we make this 2 boolean
>>>> controls. I personally would like to vote for keeping it as is, as both lamps
>>>> illuminate the same substrate in this case, and esp. switching between
>>>> Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
>>>> UI wise (iow switch from top to bottom lighting or visa versa.
>>>
>>> The problem with having one control is that while this makes sense for this
>>> particular microscope, it doesn't make sense in general.
>>>
>>
>> Actual it does atleast for microscopes in general a substrate under a microscope
>> usually is either illuminated from above or below.
>>
>>> Standard controls such as proposed by this patch should have a fixed type
>>
>> Although I agree with that sentiment in general I don't see this as an absolute
>> need, apps should get the type by doing a query ctrl not by assuming they
>> know it based on the cid.
>>
>> And esp. for menu controls this is not true, for example
>> most devices have a light freq filter menu of:
>> off
>> 50 hz
>> 60 hz
>>
>> Which matches what is documented in videodev2.h
>>
>> Where as some have:
>> off
>> 50 hz
>> 60 hz
>> auto hz
>>
>> Because they can (and default to) detect the light frequency automatically.
>
> The v4l2 api allows drivers to selectively enable items from a menu. So this
> control has a fixed menu type and a fixed menu contents. Some of the menu
> choices are just not available for some drivers.

This is not possible:

Quoting from:
http://www.linuxtv.org/downloads/v4l-dvb-apis/re61.html
"Menu items are enumerated by calling VIDIOC_QUERYMENU with successive index values from struct v4l2_queryctrl minimum (0) to maximum, inclusive."

And many apps are coded this way, so we cannot simply skip values in a
menu enum just because a driver does not support them, this would
break apps as they (rightfully) don't expect an error when
calling VIDIOC_QUERYMENU with an index between minimum and
maximum, so given for example:

enum v4l2_led {
          V4L2_LED_AUTO = 0,
	 V4L2_LED_BLINK = 1,
          V4L2_LED_OFF = 2,
          V4L2_LED_ON = 3,
};

Drivers which do not support blink would still need to report a minimum
and maximum of 0 and 3, making any control apps expect 4 menu items not
3 !

And this example is exactly why I'm arguing against defining standard
meanings for standard controls with a menu type.

Also note that at least with the uvc driver that due to how extension
unit controls are working (the uvcvideo driver gets told about these
vendor specific controls from a userspace helper), the menu index is
the value which gets written to the hardware! So one cannot simply
make this match some random enum.

>
> There are several advantages of sticking to one standard menu for standard
> controls:
>
> 1) The contents of the menu can be defined centrally in v4l2-ctrls.c, which
>     ensures consistency. Not only of the menu texts, but also of how the
>     control behaves in various drivers.

No they cannot as v4l2-ctrls.c will not know when to return -EINVAL to
indicate that in the example case the driver does not support blink, and
moreover an app will not expect this and maybe decide to not show the
menu at all, or ...

> 2) It makes it possible to set the control directly from within a program.
>     This is currently true for *all* standard controls

No this is not true, programs still need to know minimum and maximum values
for all integer standard controls, brightness may be 0-15 on one device
and 0-65535 on another, so they cannot simply bang in any value they need to
take into account the query ctrl results.

> This looks pretty decent IMHO:
>
> enum v4l2_illuminator {
>          V4L2_ILLUMINATOR_OFF = 0,
>          V4L2_ILLUMINATOR_ON = 1,
> };
> #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
>

I don't like this, as explained before most microscopes have a top
and a bottom light, and you want to switch between them, or to
all off, or to all on. So having a menu with 4 options for this
makes a lot more sense then having 2 separate controls. Defining
these values as standard values would take away the option for drivers
to do something other then a simple on / off control here. Again
what is wrong with with not defining standard meanings for standard
controls with a menu type. This means less stuff in videodev2.h
and more flexibility wrt using these control ids.


I think we should not even define a type for this one. If we
get microscopes with pwm control for the lights we will want this
to be an integer using one control per light.

We have this excellent infrastructure to automatically discover
control types, ranges and menu item meaning. Why would it be
forbidden to use this for standard controls.

Either we need to drop our aversion for private controls, or
allow somewhat more flexible standard controls!

> enum v4l2_led {
>          V4L2_LED_AUTO = 0,
>          V4L2_LED_OFF = 1,
>          V4L2_LED_ON = 2,
> };
> #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
>
> Simple and straightforward.

Until a cam comes along which only supports auto and on, and
we have a whole in our menu range with the standard does not
allow!

>>> consistent behavior. Note that I am also wondering whether it wouldn't be a
>>> good idea to use a menu for this, just as for the LEDs.
>>
>> I do agree that the illuminator ctrls should be a menu, that way the driver
>> author can also choose wether to group 2 together in a single control or not
>> we simply should not specify the menu values in the spec (the same can
>> be said for the led case).
>
> See above. Just because you can do it, doesn't mean you should. In this case
> I think it is a bad idea. Standard controls should have standard behavior.

How about a compromise, we add a set of standard defines for menu
index meanings, with a note that these are present as a way to standardize
things between drivers, but that some drivers may deviate and that
apps should always use VIDIOC_QUERYMENU ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tuesday, September 07, 2010 20:42:07 Hans Verkuil wrote:
> On Tuesday, September 07, 2010 19:57:18 Jean-Francois Moine wrote:
> > On Tue, 7 Sep 2010 17:30:33 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > 
> > > enum v4l2_illuminator {
> > >         V4L2_ILLUMINATOR_OFF = 0,
> > >         V4L2_ILLUMINATOR_ON = 1,
> > > };
> > > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> > > 
> > > enum v4l2_led {
> > >         V4L2_LED_AUTO = 0,
> > >         V4L2_LED_OFF = 1,
> > >         V4L2_LED_ON = 2,
> > > };
> > > #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> > > 
> > > Simple and straightforward.
> > 
> > Hi,
> > 
> > Hans (de Goede), is this OK for you? I think that if we find more
> > illuminators or LEDs on some devices, we may add more V4L2_CID_xxx_n
> > controls.
> > 
> > Hans (Verkuil), may we have the same enum's for both light types?
> > Something like:
> > 
> > enum v4l2_light {
> > 	V4L2_LIGHT_OFF = 0,
> > 	V4L2_LIGHT_ON = 1,
> > 	V4L2_LIGHT_AUTO = 2,
> > 	V4L2_LIGHT_BLINK = 3,
> > };
> 
> I'm OK with that. Although 'blink' shouldn't be added yet unless we have a
> driver that will actually make use of it.

I realized something else: while for us programmers it is perfectly natural
to start counting at 0, for the rest of the world it is probably more
understandable to start counting at 1. I know it goes against our religion,
but sometimes you just have to conform. :-)

Regards,

	Hans
  

On Tue, 7 Sep 2010, Hans Verkuil wrote:

> On Tuesday, September 07, 2010 20:42:07 Hans Verkuil wrote:
> > On Tuesday, September 07, 2010 19:57:18 Jean-Francois Moine wrote:
> > > On Tue, 7 Sep 2010 17:30:33 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > 
> > > > enum v4l2_illuminator {
> > > >         V4L2_ILLUMINATOR_OFF = 0,
> > > >         V4L2_ILLUMINATOR_ON = 1,
> > > > };
> > > > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > > > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> > > > 
> > > > enum v4l2_led {
> > > >         V4L2_LED_AUTO = 0,
> > > >         V4L2_LED_OFF = 1,
> > > >         V4L2_LED_ON = 2,
> > > > };
> > > > #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> > > > 
> > > > Simple and straightforward.
> > > 
> > > Hi,
> > > 
> > > Hans (de Goede), is this OK for you? I think that if we find more
> > > illuminators or LEDs on some devices, we may add more V4L2_CID_xxx_n
> > > controls.
> > > 
> > > Hans (Verkuil), may we have the same enum's for both light types?
> > > Something like:
> > > 
> > > enum v4l2_light {
> > > 	V4L2_LIGHT_OFF = 0,
> > > 	V4L2_LIGHT_ON = 1,
> > > 	V4L2_LIGHT_AUTO = 2,
> > > 	V4L2_LIGHT_BLINK = 3,
> > > };
> > 
> > I'm OK with that. Although 'blink' shouldn't be added yet unless we have a
> > driver that will actually make use of it.
> 
> I realized something else: while for us programmers it is perfectly natural
> to start counting at 0, for the rest of the world it is probably more
> understandable to start counting at 1. I know it goes against our religion,
> but sometimes you just have to conform. :-)
> 
> Regards,
> 
> 	Hans

Sorry about the long silence from here, but there has been illness in the 
family. I do keep trying to watch whatever is going on.

Hans, I agree with your general characterization of the public's 
perception of 0 versus 1. But on this particular occasion I suspect that 
the general public would see that 0 corresponds more naturally to "off"
than 1 does.

Hoping that all is well with you and others. 

Cheers, and this is just my two cents on a trivial issue.

Theodore Kilgore
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Wednesday, September 08, 2010 00:29:51 Theodore Kilgore wrote:
> 
> On Tue, 7 Sep 2010, Hans Verkuil wrote:
> 
> > On Tuesday, September 07, 2010 20:42:07 Hans Verkuil wrote:
> > > On Tuesday, September 07, 2010 19:57:18 Jean-Francois Moine wrote:
> > > > On Tue, 7 Sep 2010 17:30:33 +0200
> > > > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > 
> > > > > enum v4l2_illuminator {
> > > > >         V4L2_ILLUMINATOR_OFF = 0,
> > > > >         V4L2_ILLUMINATOR_ON = 1,
> > > > > };
> > > > > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > > > > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> > > > > 
> > > > > enum v4l2_led {
> > > > >         V4L2_LED_AUTO = 0,
> > > > >         V4L2_LED_OFF = 1,
> > > > >         V4L2_LED_ON = 2,
> > > > > };
> > > > > #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> > > > > 
> > > > > Simple and straightforward.
> > > > 
> > > > Hi,
> > > > 
> > > > Hans (de Goede), is this OK for you? I think that if we find more
> > > > illuminators or LEDs on some devices, we may add more V4L2_CID_xxx_n
> > > > controls.
> > > > 
> > > > Hans (Verkuil), may we have the same enum's for both light types?
> > > > Something like:
> > > > 
> > > > enum v4l2_light {
> > > > 	V4L2_LIGHT_OFF = 0,
> > > > 	V4L2_LIGHT_ON = 1,
> > > > 	V4L2_LIGHT_AUTO = 2,
> > > > 	V4L2_LIGHT_BLINK = 3,
> > > > };
> > > 
> > > I'm OK with that. Although 'blink' shouldn't be added yet unless we have a
> > > driver that will actually make use of it.
> > 
> > I realized something else: while for us programmers it is perfectly natural
> > to start counting at 0, for the rest of the world it is probably more
> > understandable to start counting at 1. I know it goes against our religion,
> > but sometimes you just have to conform. :-)
> > 
> > Regards,
> > 
> > 	Hans
> 
> Sorry about the long silence from here, but there has been illness in the 
> family. I do keep trying to watch whatever is going on.
> 
> Hans, I agree with your general characterization of the public's 
> perception of 0 versus 1. But on this particular occasion I suspect that 
> the general public would see that 0 corresponds more naturally to "off"
> than 1 does.

Ah, I see I was ambiguous in what I wrote. I referred to the '0' in
V4L2_CID_LED_0/V4L2_CID_ILLUMINATOR_0 (and their corresponding names as
reported to the user as "LED 0"/"Illuminator 0"), not to the 0 in the enum.

Regards,

	Hans

> 
> Hoping that all is well with you and others. 
> 
> Cheers, and this is just my two cents on a trivial issue.
> 
> Theodore Kilgore
>
  

On Tuesday, September 07, 2010 23:14:10 Hans de Goede wrote:
> Hi,
> 
> On 09/07/2010 05:30 PM, Hans Verkuil wrote:
> > On Tuesday, September 07, 2010 15:04:55 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 09/07/2010 04:50 PM, Hans Verkuil wrote:
> 
> <snip>
> 
> >>>> Both off
> >>>> Top on, Bottom off
> >>>> Top off, Bottom on
> >>>> Both on
> >>>>
> >>>> Which raises the question do we leave this as is, or do we make this 2 boolean
> >>>> controls. I personally would like to vote for keeping it as is, as both lamps
> >>>> illuminate the same substrate in this case, and esp. switching between
> >>>> Top on, Bottom off to Top off, Bottom on in one go is a good feature to have
> >>>> UI wise (iow switch from top to bottom lighting or visa versa.
> >>>
> >>> The problem with having one control is that while this makes sense for this
> >>> particular microscope, it doesn't make sense in general.
> >>>
> >>
> >> Actual it does atleast for microscopes in general a substrate under a microscope
> >> usually is either illuminated from above or below.
> >>
> >>> Standard controls such as proposed by this patch should have a fixed type
> >>
> >> Although I agree with that sentiment in general I don't see this as an absolute
> >> need, apps should get the type by doing a query ctrl not by assuming they
> >> know it based on the cid.
> >>
> >> And esp. for menu controls this is not true, for example
> >> most devices have a light freq filter menu of:
> >> off
> >> 50 hz
> >> 60 hz
> >>
> >> Which matches what is documented in videodev2.h
> >>
> >> Where as some have:
> >> off
> >> 50 hz
> >> 60 hz
> >> auto hz
> >>
> >> Because they can (and default to) detect the light frequency automatically.
> >
> > The v4l2 api allows drivers to selectively enable items from a menu. So this
> > control has a fixed menu type and a fixed menu contents. Some of the menu
> > choices are just not available for some drivers.
> 
> This is not possible:
> 
> Quoting from:
> http://www.linuxtv.org/downloads/v4l-dvb-apis/re61.html
> "Menu items are enumerated by calling VIDIOC_QUERYMENU with successive index values from struct v4l2_queryctrl minimum (0) to maximum, inclusive."

Actually it is possible, although not well described in the spec (I thought
it was much better described, but looking at it it clearly needs improvement)
and in fact it is in use for years: 

You enumerate menu items from 'minimum' to 'maximum', but if a particular index
is not supported the driver can return -EINVAL.

It's used heavily for the MPEG controls where menus are often a list of options
as defined by the MPEG standard and drivers support only a subset of those.

The control framework also has support for this: drivers just provide a mask to
the framework of the menu items that have to be skipped.

> And many apps are coded this way, so we cannot simply skip values in a
> menu enum just because a driver does not support them, this would
> break apps as they (rightfully) don't expect an error when
> calling VIDIOC_QUERYMENU with an index between minimum and
> maximum, so given for example:

It is true that many apps are coded that way (unless they support the MPEG
controls). With the control framework in place it would be quite easy to map
the menu indices into a contiguous range. But then you loose the ability to
hardcode specific menu items.

> 
> enum v4l2_led {
>           V4L2_LED_AUTO = 0,
> 	 V4L2_LED_BLINK = 1,
>           V4L2_LED_OFF = 2,
>           V4L2_LED_ON = 3,
> };
> 
> Drivers which do not support blink would still need to report a minimum
> and maximum of 0 and 3, making any control apps expect 4 menu items not
> 3 !
> 
> And this example is exactly why I'm arguing against defining standard
> meanings for standard controls with a menu type.
> 
> Also note that at least with the uvc driver that due to how extension
> unit controls are working (the uvcvideo driver gets told about these
> vendor specific controls from a userspace helper), the menu index is
> the value which gets written to the hardware! So one cannot simply
> make this match some random enum.

I actually consider that a bug. The userspace helper *does* know about the
menu mapping and should do the right thing here.

It's not a big deal in practice, but it would actually be nice if this could
be changed at some time in the future.
 
> >
> > There are several advantages of sticking to one standard menu for standard
> > controls:
> >
> > 1) The contents of the menu can be defined centrally in v4l2-ctrls.c, which
> >     ensures consistency. Not only of the menu texts, but also of how the
> >     control behaves in various drivers.
> 
> No they cannot as v4l2-ctrls.c will not know when to return -EINVAL to
> indicate that in the example case the driver does not support blink, and
> moreover an app will not expect this and maybe decide to not show the
> menu at all, or ...
> 
> > 2) It makes it possible to set the control directly from within a program.
> >     This is currently true for *all* standard controls
> 
> No this is not true, programs still need to know minimum and maximum values
> for all integer standard controls, brightness may be 0-15 on one device
> and 0-65535 on another, so they cannot simply bang in any value they need to
> take into account the query ctrl results.

I strongly disagree. If an app wants to program the brightness to 50%, then that
is easy to do: get the min and max values and set the brightness to (max - min) / 2.

Now try the same with a menu. Without known menu indices there is no way you can
ever do this.

The extended controls in particular work like this and with the increasing
importance of embedded v4l drivers it is really important we can support this
use case.

After all, the application running on an embedded system will be responsible
for controlling many of the controls that in e.g. timetv are set by the user.
Other than simple controls like brightness, contrast, etc. Those will often
still be exposed to the end-user.
 
> > This looks pretty decent IMHO:
> >
> > enum v4l2_illuminator {
> >          V4L2_ILLUMINATOR_OFF = 0,
> >          V4L2_ILLUMINATOR_ON = 1,
> > };
> > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> >
> 
> I don't like this, as explained before most microscopes have a top
> and a bottom light, and you want to switch between them, or to
> all off, or to all on. So having a menu with 4 options for this
> makes a lot more sense then having 2 separate controls. Defining
> these values as standard values would take away the option for drivers
> to do something other then a simple on / off control here. Again
> what is wrong with with not defining standard meanings for standard
> controls with a menu type. This means less stuff in videodev2.h
> and more flexibility wrt using these control ids.

I've reconsidered as well. These controls should become booleans (as per the
original proposal). If we need more complex behavior, then we should add
new controls for that. E.g., suppose the brightness can be controlled: then
we should add an ILLUMINATOR_BRIGHTNESS_0 control. Or if we use this to control
the flash of a camera: in that case we need probably a ILLUMINATOR_FLASH_DURATION
control and a ILLUMINATOR_FLASH button control.

But for basic on/off functionality it is much better to have a standard boolean.
And as an added bonus that also makes sense from a GUI perspective.

> 
> 
> I think we should not even define a type for this one. If we
> get microscopes with pwm control for the lights we will want this
> to be an integer using one control per light.
> 
> We have this excellent infrastructure to automatically discover
> control types, ranges and menu item meaning. Why would it be
> forbidden to use this for standard controls.

Well, the problem is that we actually do not have infrastructure to discover
menu item meaning. We can show the user the menu items, but we (i.e. the app)
has no idea about the menu item meaning, unless we have enums as well that
correspond 1-to-1 to the menu item.

And that's the way it currently works. So breaking the 1-to-1 mapping (let alone
allowing any type) will also break any possibility of setting it from an
application. This wasn't much of a consideration several years ago (and definitely
not when the API was designed), but it is now with all the embedded systems.

Having to care for embedded systems makes life more interesting :-), but I
think it is a good thing. Because anything added to the API now has to make
sense for both SoCs and 'regular' webcams/capture boards. That forces us to
think about it just that extra bit and in the end makes the API better than
it would otherwise be.

> Either we need to drop our aversion for private controls, or
> allow somewhat more flexible standard controls!

I don't have an aversion against private controls. In fact, they will become
much more prominent in embedded systems and sub-devices. But too often private
controls are added for no good reason. Either because they shouldn't be exposed
in the first place, or because they should be a standard control, or because
it is the wrong tool for the job. Past experience made it a bit of a red flag
for me, requiring more scrutiny than usual.

> 
> > enum v4l2_led {
> >          V4L2_LED_AUTO = 0,
> >          V4L2_LED_OFF = 1,
> >          V4L2_LED_ON = 2,
> > };
> > #define V4L2_CID_LED_0              (V4L2_CID_BASE+39)
> >
> > Simple and straightforward.
> 
> Until a cam comes along which only supports auto and on, and
> we have a whole in our menu range with the standard does not
> allow!
> 
> >>> consistent behavior. Note that I am also wondering whether it wouldn't be a
> >>> good idea to use a menu for this, just as for the LEDs.
> >>
> >> I do agree that the illuminator ctrls should be a menu, that way the driver
> >> author can also choose wether to group 2 together in a single control or not
> >> we simply should not specify the menu values in the spec (the same can
> >> be said for the led case).
> >
> > See above. Just because you can do it, doesn't mean you should. In this case
> > I think it is a bad idea. Standard controls should have standard behavior.
> 
> How about a compromise, we add a set of standard defines for menu
> index meanings, with a note that these are present as a way to standardize
> things between drivers, but that some drivers may deviate and that
> apps should always use VIDIOC_QUERYMENU ?

Let's use boolean for these illuminator controls instead. Problem solved :-)

LED controls are a separate issue, see the discussion elsewhere.

Regards,

	Hans
  

Hi,

On 09/09/2010 08:55 AM, Hans Verkuil wrote:
> On Tuesday, September 07, 2010 23:14:10 Hans de Goede wrote:

<snip>

>> How about a compromise, we add a set of standard defines for menu
>> index meanings, with a note that these are present as a way to standardize
>> things between drivers, but that some drivers may deviate and that
>> apps should always use VIDIOC_QUERYMENU ?
>
> Let's use boolean for these illuminator controls instead. Problem solved :-)

Erm, no. If you take a look at the current qx5 microscope support code in the
cpia2 driver it currently is a menu with the following possible values:
Off
Top
Bottom
Both

So now lets say we create standard controls for illuminators and make them
booleans and use 2 booleans. And then modify the cpia2 driver to follow the
new standard.

The user behavior then goes from:
- user things lets switch from top to bottom lighting
- go to control
- click menu drops down select top / bottom
-> easy

To:
- user things lets switch from top to bottom lighting
- go to control
- heuh 2 checkboxes ?
- click one check box off
- clock other check box on
-> not easy

If I were a user I would call this change a regression, and as such I find
the boolean proposal unacceptable. Maybe we should call the control
V4L2_CID_MICROSCOPE_ILLUMINATOR

To make it more clear that the menu variant of this is meant for
microscopes (which typically have either only a bottom illuminator
or both a bottom and a top one). And if we then ever need to support
some other kind of illuminator we can add a separate cid for that/

Otherwise I think it might be best to just keep this as a private control.

Regards,

Hans



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

> Hi,
>
> On 09/09/2010 08:55 AM, Hans Verkuil wrote:
>> On Tuesday, September 07, 2010 23:14:10 Hans de Goede wrote:
>
> <snip>
>
>>> How about a compromise, we add a set of standard defines for menu
>>> index meanings, with a note that these are present as a way to
>>> standardize
>>> things between drivers, but that some drivers may deviate and that
>>> apps should always use VIDIOC_QUERYMENU ?
>>
>> Let's use boolean for these illuminator controls instead. Problem solved
>> :-)
>
> Erm, no. If you take a look at the current qx5 microscope support code in
> the
> cpia2 driver it currently is a menu with the following possible values:
> Off
> Top
> Bottom
> Both
>
> So now lets say we create standard controls for illuminators and make them
> booleans and use 2 booleans. And then modify the cpia2 driver to follow
> the
> new standard.
>
> The user behavior then goes from:
> - user things lets switch from top to bottom lighting
> - go to control
> - click menu drops down select top / bottom
> -> easy
>
> To:
> - user things lets switch from top to bottom lighting
> - go to control
> - heuh 2 checkboxes ?
> - click one check box off
> - clock other check box on
> -> not easy

So two clicks in the case of a menu and two in the case of a checkbox.
Personally I don't see this as a big deal. But it will be good to get
other people's opinion on this.

>
> If I were a user I would call this change a regression, and as such I find
> the boolean proposal unacceptable. Maybe we should call the control
> V4L2_CID_MICROSCOPE_ILLUMINATOR
>
> To make it more clear that the menu variant of this is meant for
> microscopes (which typically have either only a bottom illuminator
> or both a bottom and a top one). And if we then ever need to support
> some other kind of illuminator we can add a separate cid for that/
>
> Otherwise I think it might be best to just keep this as a private control.

V4L2_CID_MICROSCOPE_ILLUMINATOR might be an option, but then the question
is whether the top/bottom illuminator combination is standard for all (or
at least the majority) of microscopes. If that is indeed the case, then we
can consider this. Although I still think that checkboxes work just as
well.

But if this arrangement and number of illuminators is specific to this
range of microscopes, then a private control is an option.

An other option is to have ILLUMINATOR_TOP and ..._BOTTOM boolean
controls. That way at least the name presented to the user makes sense (if
the user can read english of course, but that's a discussion for another -
very rainy - day).

Regards,

        Hans
  

Hi Hans,

On Tuesday 07 September 2010 11:47:22 Hans Verkuil wrote:

[snip]

> But I can guarantee that we will get video devices with multiple leds in
> the future.

What about devices with illumination LEDs that can be dimmed ?

> So we need to think *now* about how to do this. One simple
> option is of course to name the controls CID_ILLUMINATOR0 and CID_LED0.
> That way we can easily add LED1, LED2, etc. later without running into
> weird inconsistent control names.
  

Hi Hans,

On Tuesday 07 September 2010 23:14:10 Hans de Goede wrote:
> On 09/07/2010 05:30 PM, Hans Verkuil wrote:

[snip]

> Also note that at least with the uvc driver that due to how extension
> unit controls are working (the uvcvideo driver gets told about these
> vendor specific controls from a userspace helper), the menu index is
> the value which gets written to the hardware! So one cannot simply
> make this match some random enum.

That's not correct. The UVC driver maps the menu index to the hardware value, 
so there shouldn't be any problem here.

[snip]

> > This looks pretty decent IMHO:
> > 
> > enum v4l2_illuminator {
> > 
> >          V4L2_ILLUMINATOR_OFF = 0,
> >          V4L2_ILLUMINATOR_ON = 1,
> > 
> > };
> > #define V4L2_CID_ILLUMINATOR_0              (V4L2_CID_BASE+37)
> > #define V4L2_CID_ILLUMINATOR_1              (V4L2_CID_BASE+38)
> 
> I don't like this, as explained before most microscopes have a top
> and a bottom light, and you want to switch between them, or to
> all off, or to all on. So having a menu with 4 options for this
> makes a lot more sense then having 2 separate controls. Defining
> these values as standard values would take away the option for drivers
> to do something other then a simple on / off control here. Again
> what is wrong with with not defining standard meanings for standard
> controls with a menu type. This means less stuff in videodev2.h
> and more flexibility wrt using these control ids.
> 
> I think we should not even define a type for this one. If we
> get microscopes with pwm control for the lights we will want this
> to be an integer using one control per light.

LED dimming using PWM is something we can surely expect in the future, if not 
already available in existing devices.
  

On Monday, September 13, 2010 08:47:24 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday 07 September 2010 11:47:22 Hans Verkuil wrote:
> 
> [snip]
> 
> > But I can guarantee that we will get video devices with multiple leds in
> > the future.
> 
> What about devices with illumination LEDs that can be dimmed ?

That will be a separate control (e.g. ILLUMINATOR_BRIGHTNESS or something
like that). This is just basic on/off.

Regards,

	Hans

> 
> > So we need to think *now* about how to do this. One simple
> > option is of course to name the controls CID_ILLUMINATOR0 and CID_LED0.
> > That way we can easily add LED1, LED2, etc. later without running into
> > weird inconsistent control names.
> 
>