Re: [PATCH] Illuminators and status LED controls

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

It has already been discussed.  Please check the list archives for the past few days.

Do you know of any V4L2 application developer or development team that prefers to use a separate API just to turn lights on and off, when all other aspects of the incoming video are controlled with the V4L2 control API?

(That question is mostly rhetorical, but I'd still actually be interested from video app developers.)

Regards,
Andy

Eduardo Valentin <eduardo.valentin@nokia.com> wrote:

>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

Re: [PATCH] Illuminators and status LED controls

[Eino-Ville Talvala]

 This is probably a bit OT, but these sorts of indicator LEDs can get quite complicated.

As part of our FCamera sample program on the Nokia N900 (which uses V4L2 way down there), we wanted to reprogram the front indicator LED to flash exactly when a picture is taken.  The N900 front LED is quite a programmable beast [1], with a dedicated microcontroller (the lp5521) that runs little programs that define the blink patterns for the RGB LED.

I'm not really suggesting that the V4L2 control should be able to handle this sort of an LED, but as these sorts of things get cheaper, it may become a case of 'why not?' for manufacturers putting in more complex RGB LEDs.   And if you don't want to encapsulate all that in V4L2, it may be better to leave it to other APIs at some point of complexity (the current lp5521 driver seems to have a sysfs-only interface for now for the programmable patterns, and the standard LED API otherwise)

[1] http://wiki.maemo.org/LED_patterns

Eino-Ville Talvala
Computer Graphics Lab
Stanford University

On 9/7/2010 1:33 PM, Andy Walls wrote:
> It has already been discussed.  Please check the list archives for the past few days.
>
> Do you know of any V4L2 application developer or development team that prefers to use a separate API just to turn lights on and off, when all other aspects of the incoming video are controlled with the V4L2 control API?
>
> (That question is mostly rhetorical, but I'd still actually be interested from video app developers.)
>
> Regards,
> Andy
>
> Eduardo Valentin <eduardo.valentin@nokia.com> wrote:
>
>> 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
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{���bj)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥

Re: [PATCH] Illuminators and status LED controls

[Eduardo Valentin]

Hello,

On Wed, Sep 08, 2010 at 04:16:48AM +0200, ext Eino-Ville Talvala wrote:
> 
>  This is probably a bit OT, but these sorts of indicator LEDs can get quite complicated.
> 
> As part of our FCamera sample program on the Nokia N900 (which uses V4L2 way down there), we wanted to reprogram the front indicator LED to flash exactly when a picture is taken.  The N900 front LED is quite a programmable beast [1], with a dedicated microcontroller (the lp5521) that runs little programs that define the blink patterns for the RGB LED.
> 
> I'm not really suggesting that the V4L2 control should be able to handle this sort of an LED, but as these sorts of things get cheaper, it may become a case of 'why not?' for manufacturers putting in more complex RGB LEDs.   And if you don't want to encapsulate all that in V4L2, it may be better to leave it to other APIs at some point of complexity (the current lp5521 driver seems to have a sysfs-only interface for now for the programmable patterns, and the standard LED API otherwise)
> 
> [1] http://wiki.maemo.org/LED_patterns

Well, that's exactly why duplicating API's is usually a bad idea. If the thing start to get complex, having only one place to hold
the mess is better than keeping it into two (or more) different places. This will become worst and worst. I mean, why do we want two
different APIs to control same stuff?

And yes, application developers must use the correct API to control stuff. Why should kernel duplicate interfaces just because
user land don't want to use two different interfaces? Doesn't this sound a bit ... strange at least?

> 
> Eino-Ville Talvala
> Computer Graphics Lab
> Stanford University
> 
> On 9/7/2010 1:33 PM, Andy Walls wrote:
> > It has already been discussed.  Please check the list archives for the past few days.


OK, will search the logs. But you should probably add some sort of reasoning in your patch
description, explaining why you are duplicating interfaces.

> >
> > Do you know of any V4L2 application developer or development team that prefers to use a separate API just to turn lights on and off, when all other aspects of the incoming video are controlled with the V4L2 control API?
> >
> > (That question is mostly rhetorical, but I'd still actually be interested from video app developers.)
> >
> > Regards,
> > Andy
> >
> > Eduardo Valentin <eduardo.valentin@nokia.com> wrote:
> >
> >> 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
> > N�����r��y���b�X��ǧv�^�)޺{.n�+����{���bj)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
> 

-- 
---
Eduardo Valentin

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Wed, 2010-09-08 at 10:59 +0300, Eduardo Valentin wrote:
> Hello,
> 
> On Wed, Sep 08, 2010 at 04:16:48AM +0200, ext Eino-Ville Talvala wrote:
> > 
> >  This is probably a bit OT, but these sorts of indicator LEDs can get quite complicated.
> > 
> > As part of our FCamera sample program on the Nokia N900 (which uses
> V4L2 way down there), we wanted to reprogram the front indicator LED
> to flash exactly when a picture is taken.
>   The N900 front LED is quite a programmable beast [1], with a
> dedicated microcontroller (the lp5521) that runs little programs that
> define the blink patterns for the RGB LED.
> > 
> > I'm not really suggesting that the V4L2 control should be able to
> handle this sort of an LED, but as these sorts of things get cheaper,
> it may become a case of 'why not?' for manufacturers putting in more
> complex RGB LEDs.   And if you don't want to encapsulate all that in
> V4L2, it may be better to leave it to other APIs at some point of
> complexity (the current lp5521 driver seems to have a sysfs-only
> interface for now for the programmable patterns, and the standard LED
> API otherwise)
> > 
> > [1] http://wiki.maemo.org/LED_patterns
> 
> Well, that's exactly why duplicating API's is usually a bad idea. If
> the thing start to get complex, having only one place to hold
> the mess is better than keeping it into two (or more) different
> places. This will become worst and worst. I mean, why do we want two
> different APIs to control same stuff?

For the case when requiring an application to use two separate APIs adds
more complexity for application developer than it alleviates.

Let's be more specific about "stuff" that may need to be controlled:

1. Illuminators with control lines directly tied to a camera's bridge or
sensor chip

2. Simple status LEDs tied directly to a camera's bridge or sensor chip
(maybe with a simple hardware assisted blink)

3. Generically connected LEDs (some platform GPIO chip or
microcontroller somewhere) with blinking or PWM intensity under CPU or
microcontroller control.

Number 1 & 2 above certainly apply to consumer desktop devices.

Number 3 is likely common on embedded devices.



Incandescent and Halogen lamps that effect an image coming into a camera
are *not* LEDs that blink or flash automatically based on driver or
system trigger events.  They are components of a video capture system
with which a human attempts to adjust the appearance of an image of a
subject by changing the subject's environment.  These illuminators are
not some generically connected device, but controlled by GPIO's on the
camera's bridge or sensor chip itself.  Such an illuminator will
essentially be used only in conjunction with the camera.

Status LEDs integrated into webcam devices that are not generically
connected devices but controlled with GPIOs on the camera's bridge or
sensor chip will also essentially be used only in conjunction with the
camera.

Turning these sorts camera specific illuminators and LEDs on an off
should be as simple to implement for an application developer as it is
to grasp the concept of turning a light bulb on and off.


The LED interface seems more appropriate to use when the LEDs are
connected more generically and will likely be used more generically,
such as in an embedded system.



> And yes, application developers must use the correct API to control
> stuff.

>  Why should kernel duplicate interfaces just because
> user land don't want to use two different interfaces? Doesn't this sound a bit ... strange at least?

Why should the kernel push multiple APIs on application developers to
control a complex federation of small devices all connected behind a
single bridge chip, which the user perceives as a single device?  (BTW a
USB microscope is such a federation which doesn't work at all without
proper subject illumination.)

V4L2 controls are how desktop V4L2 applications currently control
aspects of a incoming image.  Forcing the use of the LED interface in
sysfs to control one aspect of that would be a departure from the norm
for the existing V4L2 desktop applications.

Forcing the use of the LED interface also brings along the complication
of proper association of the illuminator or LED sysfs control node to
the proper video capture/control device node.  I have a laptop with a
built in webcam with a status LED and a USB connected microscope with
two illuminators.  What are the steps for an application to discover the
correct light for the video device and what settings that light is
capable of: using V4L2 controls? using the LED interface?

With the V4L2 controls, association to the correct video devices is no
effort, and current v4l2 control handling code in applications like
v4l2-ctl and qv4l2 can discover the controls and their metadata and
present the control in a UI with no changes to the application.

How does one go about associating LEDs and Illuminators to video device
nodes using the LED sysfs interface?  I'm betting it's not as simple for
applications that use V4L2 controls.

What you suggest is a separate API, and all the burdens it brings, just
to accomplish a simple task of turning a few switches on and off.

That to me is analogous to insisting to use a few bolts made to SI
units, on a vehicle otherwise made with fasteners made to English units.


I do not see how forcing applications to use a second control API, with
no clear video device node<->led sysfs node association semantics,
reduces application complexity, when those applications already support
the V4L2 control API from which application can generically discover
controls and their metadata and automatically know the associated video
device.


Regards,
Andy

> > 
> > Eino-Ville Talvala
> > Computer Graphics Lab
> > Stanford University
> > 
> > On 9/7/2010 1:33 PM, Andy Walls wrote:
> > > It has already been discussed.  Please check the list archives for the past few days.
> 
> 
> OK, will search the logs. But you should probably add some sort of reasoning in your patch
> description, explaining why you are duplicating interfaces.
> 
> > >
> > > Do you know of any V4L2 application developer or development team that prefers to use a separate API just to turn lights on and off, when all other aspects of the incoming video are controlled with the V4L2 control API?
> > >
> > > (That question is mostly rhetorical, but I'd still actually be interested from video app developers.)
> > >
> > > Regards,
> > > Andy
> > >
> > > Eduardo Valentin <eduardo.valentin@nokia.com> wrote:
> > >
> > >> 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
> > > N�����r��y���b�X��ǧv�^�)޺{.n�+����{���bj)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
> > 
> 

Re: [PATCH] Illuminators and status LED controls

[Peter Korsgaard]

>>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:

Hi,

 Andy> Incandescent and Halogen lamps that effect an image coming into a
 Andy> camera are *not* LEDs that blink or flash automatically based on
 Andy> driver or system trigger events.  They are components of a video
 Andy> capture system with which a human attempts to adjust the
 Andy> appearance of an image of a subject by changing the subject's
 Andy> environment.  These illuminators are not some generically
 Andy> connected device, but controlled by GPIO's on the camera's bridge
 Andy> or sensor chip itself.  Such an illuminator will essentially be
 Andy> used only in conjunction with the camera.

Agreed.

 Andy> Status LEDs integrated into webcam devices that are not generically
 Andy> connected devices but controlled with GPIOs on the camera's bridge or
 Andy> sensor chip will also essentially be used only in conjunction with the
 Andy> camera.

Or for any other usage the user envision - E.G. I could imagine using
the status led of the webcam in my macbook for hard disk or wifi
activity. I'm sure other people can come up with more creative use cases
as well.

 Andy> Turning these sorts camera specific illuminators and LEDs on an off
 Andy> should be as simple to implement for an application developer as it is
 Andy> to grasp the concept of turning a light bulb on and off.

The point is that the logic rarely needs to be in the v4l
applications. The status LEDs should by default go on when the v4l
device is active and go off again when not like it used to do. A v4l
application developer would normally not want to worry about such
things, and only care about the video data.

But if a user wants something special / non-standard, then it's just a
matter of changing LED trigger in /sys/class/leds/..

 Andy> The LED interface seems more appropriate to use when the LEDs are
 Andy> connected more generically and will likely be used more generically,
 Andy> such as in an embedded system.

The LED subsystem certainly has it uses in embedded, but it's also used
on PCs - As an example the ath9k wireless driver exports a number of
LEDs. I find the situation with the wlan LEDs pretty comparable to
status LEDs on v4l devices.

 >> And yes, application developers must use the correct API to control
 >> stuff.

 >> Why should kernel duplicate interfaces just because
 >> user land don't want to use two different interfaces? Doesn't this sound a bit ... strange at least?

 Andy> Why should the kernel push multiple APIs on application developers to
 Andy> control a complex federation of small devices all connected behind a
 Andy> single bridge chip, which the user perceives as a single device?  (BTW a
 Andy> USB microscope is such a federation which doesn't work at all without
 Andy> proper subject illumination.)

Because that's the only sensible way to create a bunch of incompatible
custom interfaces  - E.G. a microphone built into a webcam is handled
through also, not v4l, so it works with any sound recording program.

 Andy> V4L2 controls are how desktop V4L2 applications currently control
 Andy> aspects of a incoming image.  Forcing the use of the LED interface in
 Andy> sysfs to control one aspect of that would be a departure from the norm
 Andy> for the existing V4L2 desktop applications.

 Andy> Forcing the use of the LED interface also brings along the complication
 Andy> of proper association of the illuminator or LED sysfs control node to
 Andy> the proper video capture/control device node.  I have a laptop with a
 Andy> built in webcam with a status LED and a USB connected microscope with
 Andy> two illuminators.  What are the steps for an application to discover the
 Andy> correct light for the video device and what settings that light is
 Andy> capable of: using V4L2 controls? using the LED interface?

Again, for status LEDs I don't see any reason why a standard v4l tool
would care. As I mentioned above, illuminators are a different story
(comparable to a gain setting imho).

 Andy> How does one go about associating LEDs and Illuminators to video device
 Andy> nodes using the LED sysfs interface?  I'm betting it's not as simple for
 Andy> applications that use V4L2 controls.

I would imagine each video device would have a (number of) triggers,
similar to how it's done for E.G. the wlan stuff - Something like
"video0-active". The status LED of the video0 device would default to
that trigger.

 Andy> I do not see how forcing applications to use a second control
 Andy> API, with no clear video device node<->led sysfs node association
 Andy> semantics, reduces application complexity, when those
 Andy> applications already support the V4L2 control API from which
 Andy> application can generically discover controls and their metadata
 Andy> and automatically know the associated video device.

Again, I see the sysfs LED interface for status LEDs as more of a
user/administrator interface than a programming API.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Illuminators and status LED controls

[Alex Deucher]

On Wed, Sep 8, 2010 at 2:58 PM, Peter Korsgaard <jacmet@sunsite.dk> wrote:
>>>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
>
> Hi,
>
>  Andy> Incandescent and Halogen lamps that effect an image coming into a
>  Andy> camera are *not* LEDs that blink or flash automatically based on
>  Andy> driver or system trigger events.  They are components of a video
>  Andy> capture system with which a human attempts to adjust the
>  Andy> appearance of an image of a subject by changing the subject's
>  Andy> environment.  These illuminators are not some generically
>  Andy> connected device, but controlled by GPIO's on the camera's bridge
>  Andy> or sensor chip itself.  Such an illuminator will essentially be
>  Andy> used only in conjunction with the camera.
>
> Agreed.
>
>  Andy> Status LEDs integrated into webcam devices that are not generically
>  Andy> connected devices but controlled with GPIOs on the camera's bridge or
>  Andy> sensor chip will also essentially be used only in conjunction with the
>  Andy> camera.
>
> Or for any other usage the user envision - E.G. I could imagine using
> the status led of the webcam in my macbook for hard disk or wifi
> activity. I'm sure other people can come up with more creative use cases
> as well.
>
>  Andy> Turning these sorts camera specific illuminators and LEDs on an off
>  Andy> should be as simple to implement for an application developer as it is
>  Andy> to grasp the concept of turning a light bulb on and off.
>
> The point is that the logic rarely needs to be in the v4l
> applications. The status LEDs should by default go on when the v4l
> device is active and go off again when not like it used to do. A v4l
> application developer would normally not want to worry about such
> things, and only care about the video data.
>
> But if a user wants something special / non-standard, then it's just a
> matter of changing LED trigger in /sys/class/leds/..

I agree with Peter here.  I don't see why a video app would care about
blinking an LED while capturing.  I suspect most apps won't bother to
implement it, or it will be a card specific mess (depending on what
the hw actually provides).  Shouldn't the driver just turn it on or
blink it when capturing is active or whatever.  Why should apps care?
Plus, each app may implement some different behavior or some may not
implement it at all which will further confuse users.

Alex

>
>  Andy> The LED interface seems more appropriate to use when the LEDs are
>  Andy> connected more generically and will likely be used more generically,
>  Andy> such as in an embedded system.
>
> The LED subsystem certainly has it uses in embedded, but it's also used
> on PCs - As an example the ath9k wireless driver exports a number of
> LEDs. I find the situation with the wlan LEDs pretty comparable to
> status LEDs on v4l devices.
>
>  >> And yes, application developers must use the correct API to control
>  >> stuff.
>
>  >> Why should kernel duplicate interfaces just because
>  >> user land don't want to use two different interfaces? Doesn't this sound a bit ... strange at least?
>
>  Andy> Why should the kernel push multiple APIs on application developers to
>  Andy> control a complex federation of small devices all connected behind a
>  Andy> single bridge chip, which the user perceives as a single device?  (BTW a
>  Andy> USB microscope is such a federation which doesn't work at all without
>  Andy> proper subject illumination.)
>
> Because that's the only sensible way to create a bunch of incompatible
> custom interfaces  - E.G. a microphone built into a webcam is handled
> through also, not v4l, so it works with any sound recording program.
>
>  Andy> V4L2 controls are how desktop V4L2 applications currently control
>  Andy> aspects of a incoming image.  Forcing the use of the LED interface in
>  Andy> sysfs to control one aspect of that would be a departure from the norm
>  Andy> for the existing V4L2 desktop applications.
>
>  Andy> Forcing the use of the LED interface also brings along the complication
>  Andy> of proper association of the illuminator or LED sysfs control node to
>  Andy> the proper video capture/control device node.  I have a laptop with a
>  Andy> built in webcam with a status LED and a USB connected microscope with
>  Andy> two illuminators.  What are the steps for an application to discover the
>  Andy> correct light for the video device and what settings that light is
>  Andy> capable of: using V4L2 controls? using the LED interface?
>
> Again, for status LEDs I don't see any reason why a standard v4l tool
> would care. As I mentioned above, illuminators are a different story
> (comparable to a gain setting imho).
>
>  Andy> How does one go about associating LEDs and Illuminators to video device
>  Andy> nodes using the LED sysfs interface?  I'm betting it's not as simple for
>  Andy> applications that use V4L2 controls.
>
> I would imagine each video device would have a (number of) triggers,
> similar to how it's done for E.G. the wlan stuff - Something like
> "video0-active". The status LED of the video0 device would default to
> that trigger.
>
>  Andy> I do not see how forcing applications to use a second control
>  Andy> API, with no clear video device node<->led sysfs node association
>  Andy> semantics, reduces application complexity, when those
>  Andy> applications already support the V4L2 control API from which
>  Andy> application can generically discover controls and their metadata
>  Andy> and automatically know the associated video device.
>
> Again, I see the sysfs LED interface for status LEDs as more of a
> user/administrator interface than a programming API.
>
> --
> Bye, Peter Korsgaard
> --
> 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
>

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Wed, 2010-09-08 at 15:27 -0400, Alex Deucher wrote:
> On Wed, Sep 8, 2010 at 2:58 PM, Peter Korsgaard <jacmet@sunsite.dk> wrote:
> >>>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
> >
> > Hi,
> >
> >  Andy> Incandescent and Halogen lamps that effect an image coming into a
> >  Andy> camera are *not* LEDs that blink or flash automatically based on
> >  Andy> driver or system trigger events.  They are components of a video
> >  Andy> capture system with which a human attempts to adjust the
> >  Andy> appearance of an image of a subject by changing the subject's
> >  Andy> environment.  These illuminators are not some generically
> >  Andy> connected device, but controlled by GPIO's on the camera's bridge
> >  Andy> or sensor chip itself.  Such an illuminator will essentially be
> >  Andy> used only in conjunction with the camera.
> >
> > Agreed.
> >
> >  Andy> Status LEDs integrated into webcam devices that are not generically
> >  Andy> connected devices but controlled with GPIOs on the camera's bridge or
> >  Andy> sensor chip will also essentially be used only in conjunction with the
> >  Andy> camera.
> >
> > Or for any other usage the user envision - E.G. I could imagine using
> > the status led of the webcam in my macbook for hard disk or wifi
> > activity. I'm sure other people can come up with more creative use cases
> > as well.
> >
> >  Andy> Turning these sorts camera specific illuminators and LEDs on an off
> >  Andy> should be as simple to implement for an application developer as it is
> >  Andy> to grasp the concept of turning a light bulb on and off.
> >
> > The point is that the logic rarely needs to be in the v4l
> > applications. The status LEDs should by default go on when the v4l
> > device is active and go off again when not like it used to do. A v4l
> > application developer would normally not want to worry about such
> > things, and only care about the video data.
> >
> > But if a user wants something special / non-standard, then it's just a
> > matter of changing LED trigger in /sys/class/leds/..
> 
> I agree with Peter here.  I don't see why a video app would care about
> blinking an LED while capturing.  I suspect most apps won't bother to
> implement it, or it will be a card specific mess (depending on what
> the hw actually provides).  Shouldn't the driver just turn it on or
> blink it when capturing is active or whatever.  Why should apps care?
> Plus, each app may implement some different behavior or some may not
> implement it at all which will further confuse users.
> 
> Alex

Hi all,

I just got finished a prototype implementation of using the LED API for
the illuminators on the QX3 microscope, so I could learn about the
interface.  (No devices in any of my systems presented an led interface,
so I had.)  I'm too tired right now to discuss what I've found, but I'll
hopefully respond with my observations around noon EDT tomorrow.

Two patches - my previous one using the V4L2 control API, and this
second one using the LED API - can be found as the last two patches
here:

	http://linuxtv.org/hg/~awalls/qx3/

$ diffstat qx3-lamp.diff
 cpia1.c |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 11 deletions(-)

$ diffstat qx3-ledapi.diff
 cpia1.c |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 183 insertions(+), 2 deletions(-)

Regards,
Andy

Re: [PATCH] Illuminators and status LED controls

[Jean-Francois Moine]

On Wed, 08 Sep 2010 20:58:18 +0200
Peter Korsgaard <jacmet@sunsite.dk> wrote:
> >>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
>  Andy> Incandescent and Halogen lamps that effect an image coming
>  Andy> into a camera are *not* LEDs that blink or flash automatically
>  Andy> based on driver or system trigger events.  They are components
>  Andy> of a video capture system with which a human attempts to
>  Andy> adjust the appearance of an image of a subject by changing the
>  Andy> subject's environment.  These illuminators are not some
>  Andy> generically connected device, but controlled by GPIO's on the
>  Andy> camera's bridge or sensor chip itself.  Such an illuminator
>  Andy> will essentially be used only in conjunction with the camera.
> 
> Agreed.
> 
>  Andy> Status LEDs integrated into webcam devices that are not
>  Andy> generically connected devices but controlled with GPIOs on the
>  Andy> camera's bridge or sensor chip will also essentially be used
>  Andy> only in conjunction with the camera.
> 
> Or for any other usage the user envision - E.G. I could imagine using
> the status led of the webcam in my macbook for hard disk or wifi
> activity. I'm sure other people can come up with more creative use
> cases as well.
> 
>  Andy> Turning these sorts camera specific illuminators and LEDs on
>  Andy> an off should be as simple to implement for an application
>  Andy> developer as it is to grasp the concept of turning a light
>  Andy> bulb on and off.
> 
> The point is that the logic rarely needs to be in the v4l
> applications. The status LEDs should by default go on when the v4l
> device is active and go off again when not like it used to do. A v4l
> application developer would normally not want to worry about such
> things, and only care about the video data.
> 
> But if a user wants something special / non-standard, then it's just a
> matter of changing LED trigger in /sys/class/leds/..
	[snip]
> Again, for status LEDs I don't see any reason why a standard v4l tool
> would care. As I mentioned above, illuminators are a different story
> (comparable to a gain setting imho).
	[snip]
> Again, I see the sysfs LED interface for status LEDs as more of a
> user/administrator interface than a programming API.

Hi,

If I may resume this exchange:

- the (microscope or device dependant) illuminators may be controlled
  by v4l2,

- the status LED should be controlled by the LED interface.

Does everybody agree?

Regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

On Thursday, September 09, 2010 08:07:02 Jean-Francois Moine wrote:
> On Wed, 08 Sep 2010 20:58:18 +0200
> Peter Korsgaard <jacmet@sunsite.dk> wrote:
> > >>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
> >  Andy> Incandescent and Halogen lamps that effect an image coming
> >  Andy> into a camera are *not* LEDs that blink or flash automatically
> >  Andy> based on driver or system trigger events.  They are components
> >  Andy> of a video capture system with which a human attempts to
> >  Andy> adjust the appearance of an image of a subject by changing the
> >  Andy> subject's environment.  These illuminators are not some
> >  Andy> generically connected device, but controlled by GPIO's on the
> >  Andy> camera's bridge or sensor chip itself.  Such an illuminator
> >  Andy> will essentially be used only in conjunction with the camera.
> > 
> > Agreed.
> > 
> >  Andy> Status LEDs integrated into webcam devices that are not
> >  Andy> generically connected devices but controlled with GPIOs on the
> >  Andy> camera's bridge or sensor chip will also essentially be used
> >  Andy> only in conjunction with the camera.
> > 
> > Or for any other usage the user envision - E.G. I could imagine using
> > the status led of the webcam in my macbook for hard disk or wifi
> > activity. I'm sure other people can come up with more creative use
> > cases as well.
> > 
> >  Andy> Turning these sorts camera specific illuminators and LEDs on
> >  Andy> an off should be as simple to implement for an application
> >  Andy> developer as it is to grasp the concept of turning a light
> >  Andy> bulb on and off.
> > 
> > The point is that the logic rarely needs to be in the v4l
> > applications. The status LEDs should by default go on when the v4l
> > device is active and go off again when not like it used to do. A v4l
> > application developer would normally not want to worry about such
> > things, and only care about the video data.
> > 
> > But if a user wants something special / non-standard, then it's just a
> > matter of changing LED trigger in /sys/class/leds/..
> 	[snip]
> > Again, for status LEDs I don't see any reason why a standard v4l tool
> > would care. As I mentioned above, illuminators are a different story
> > (comparable to a gain setting imho).
> 	[snip]
> > Again, I see the sysfs LED interface for status LEDs as more of a
> > user/administrator interface than a programming API.
> 
> Hi,
> 
> If I may resume this exchange:
> 
> - the (microscope or device dependant) illuminators may be controlled
>   by v4l2,

Agreed.
 
> - the status LED should be controlled by the LED interface.

I originally was in favor of controlling these through v4l as well, but people
made some good arguments against that. The main one being: why would you want
to show these as a control? What is the end user supposed to do with them? It
makes little sense.

Frankly, why would you want to expose LEDs at all? Shouldn't this be completely
hidden by the driver? No generic application will ever do anything with status
LEDs anyway. So it should be the driver that operates them and in that case the
LEDs do not need to be exposed anywhere.

In some embedded systems there may be a need to actually expose the LEDs, and in
that case the standard LED API can be used. And the upcoming media API can, if
necessary, be used to let the app know where to find the LED device associated
with the video hardware.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Peter Korsgaard]

>>>>> "Hans" == Hans Verkuil <hverkuil@xs4all.nl> writes:

Hi,
 
 >> - the status LED should be controlled by the LED interface.

 Hans> I originally was in favor of controlling these through v4l as
 Hans> well, but people made some good arguments against that. The main
 Hans> one being: why would you want to show these as a control? What is
 Hans> the end user supposed to do with them? It makes little sense.

 Hans> Frankly, why would you want to expose LEDs at all? Shouldn't this
 Hans> be completely hidden by the driver? No generic application will
 Hans> ever do anything with status LEDs anyway. So it should be the
 Hans> driver that operates them and in that case the LEDs do not need
 Hans> to be exposed anywhere.

It's not that it *HAS* to be exposed - But if we can, then it's nice to do
so as it gives flexibility to the user instead of hardcoding policy in
the kernel.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

Hi,

On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>  writes:
>
> Hi,
>
>   >>  - the status LED should be controlled by the LED interface.
>
>   Hans>  I originally was in favor of controlling these through v4l as
>   Hans>  well, but people made some good arguments against that. The main
>   Hans>  one being: why would you want to show these as a control? What is
>   Hans>  the end user supposed to do with them? It makes little sense.
>
>   Hans>  Frankly, why would you want to expose LEDs at all? Shouldn't this
>   Hans>  be completely hidden by the driver? No generic application will
>   Hans>  ever do anything with status LEDs anyway. So it should be the
>   Hans>  driver that operates them and in that case the LEDs do not need
>   Hans>  to be exposed anywhere.
>
> It's not that it *HAS* to be exposed - But if we can, then it's nice to do
> so as it gives flexibility to the user instead of hardcoding policy in
> the kernel.
>

Reading this whole thread I have to agree that if we are going to expose
camera status LEDs it would be done through the sysfs API. I think this
can be done nicely for gspca based drivers (as we can put all the "crud"
in the gspca core having to do it only once), but that is a low priority
nice to have thingy.

This does leave us with the problem of logitech uvc cams where the LED
currently is exposed as a v4l2 control.

Regards,

Hans

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

Hi,

On 09/09/2010 03:29 PM, Hans Verkuil wrote:
>
>> Hi,
>>
>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
>>>
>>> Hi,
>>>
>>>    >>   - the status LED should be controlled by the LED interface.
>>>
>>>    Hans>   I originally was in favor of controlling these through v4l as
>>>    Hans>   well, but people made some good arguments against that. The
>>> main
>>>    Hans>   one being: why would you want to show these as a control? What
>>> is
>>>    Hans>   the end user supposed to do with them? It makes little sense.
>>>
>>>    Hans>   Frankly, why would you want to expose LEDs at all? Shouldn't
>>> this
>>>    Hans>   be completely hidden by the driver? No generic application will
>>>    Hans>   ever do anything with status LEDs anyway. So it should be the
>>>    Hans>   driver that operates them and in that case the LEDs do not need
>>>    Hans>   to be exposed anywhere.
>>>
>>> It's not that it *HAS* to be exposed - But if we can, then it's nice to
>>> do
>>> so as it gives flexibility to the user instead of hardcoding policy in
>>> the kernel.
>>>
>>
>> Reading this whole thread I have to agree that if we are going to expose
>> camera status LEDs it would be done through the sysfs API. I think this
>> can be done nicely for gspca based drivers (as we can put all the "crud"
>> in the gspca core having to do it only once), but that is a low priority
>> nice to have thingy.
>>
>> This does leave us with the problem of logitech uvc cams where the LED
>> currently is exposed as a v4l2 control.
>
> Is it possible for the uvc driver to detect and use a LED control? That's
> how I would expect this to work, but I know that uvc is a bit of a strange
> beast.
>

Unfortunately no, some uvc cameras have "proprietary" controls. The uvc driver
knows nothing about these but offers an API to map these to v4l2 controls
(where userspace tells it the v4l2 cid, type, min, max, etc.).

Currently on logitech cameras the userspace tools if installed will map
the led control to a private v4l2 menu control with the following options:
On
Off
Auto
Blink

The cameras default to auto, where the led is turned on when video
is being streamed and off when there is no streaming going on.

Regards,

Hans

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

> Hi,
>
> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>  writes:
>>
>> Hi,
>>
>>   >>  - the status LED should be controlled by the LED interface.
>>
>>   Hans>  I originally was in favor of controlling these through v4l as
>>   Hans>  well, but people made some good arguments against that. The
>> main
>>   Hans>  one being: why would you want to show these as a control? What
>> is
>>   Hans>  the end user supposed to do with them? It makes little sense.
>>
>>   Hans>  Frankly, why would you want to expose LEDs at all? Shouldn't
>> this
>>   Hans>  be completely hidden by the driver? No generic application will
>>   Hans>  ever do anything with status LEDs anyway. So it should be the
>>   Hans>  driver that operates them and in that case the LEDs do not need
>>   Hans>  to be exposed anywhere.
>>
>> It's not that it *HAS* to be exposed - But if we can, then it's nice to
>> do
>> so as it gives flexibility to the user instead of hardcoding policy in
>> the kernel.
>>
>
> Reading this whole thread I have to agree that if we are going to expose
> camera status LEDs it would be done through the sysfs API. I think this
> can be done nicely for gspca based drivers (as we can put all the "crud"
> in the gspca core having to do it only once), but that is a low priority
> nice to have thingy.
>
> This does leave us with the problem of logitech uvc cams where the LED
> currently is exposed as a v4l2 control.

Is it possible for the uvc driver to detect and use a LED control? That's
how I would expect this to work, but I know that uvc is a bit of a strange
beast.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Thu, 2010-09-09 at 08:07 +0200, Jean-Francois Moine wrote:
> On Wed, 08 Sep 2010 20:58:18 +0200

> Hi,
> 
> If I may resume this exchange:
> 
> - the (microscope or device dependant) illuminators may be controlled
>   by v4l2,

I agree.


> - the status LED should be controlled by the LED interface.

I agree.  However, I think it is overkill based on my perception of
future utilization by end users.

I recommend ultimately implementing something in the v4l2 infrastructure
that helps v4l2 drivers expose LEDs via the LED API easily and
uniformly.  Maybe that can start with a gspca framework implementation,
which then evolves to the v4l2 infrastructure implementation.

Regards,
Andy

Re: [PATCH] Illuminators and status LED controls

[Laurent Pinchart]

Hi Alex,

On Wednesday 08 September 2010 21:27:19 Alex Deucher wrote:
> On Wed, Sep 8, 2010 at 2:58 PM, Peter Korsgaard <jacmet@sunsite.dk> wrote:
> >>>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
> > Hi,
> > 
> >  Andy> Incandescent and Halogen lamps that effect an image coming into a
> >  Andy> camera are *not* LEDs that blink or flash automatically based on
> >  Andy> driver or system trigger events.  They are components of a video
> >  Andy> capture system with which a human attempts to adjust the
> >  Andy> appearance of an image of a subject by changing the subject's
> >  Andy> environment.  These illuminators are not some generically
> >  Andy> connected device, but controlled by GPIO's on the camera's bridge
> >  Andy> or sensor chip itself.  Such an illuminator will essentially be
> >  Andy> used only in conjunction with the camera.
> > 
> > Agreed.
> > 
> >  Andy> Status LEDs integrated into webcam devices that are not
> > generically Andy> connected devices but controlled with GPIOs on the
> > camera's bridge or Andy> sensor chip will also essentially be used only
> > in conjunction with the Andy> camera.
> > 
> > Or for any other usage the user envision - E.G. I could imagine using
> > the status led of the webcam in my macbook for hard disk or wifi
> > activity. I'm sure other people can come up with more creative use cases
> > as well.
> > 
> >  Andy> Turning these sorts camera specific illuminators and LEDs on an
> > off Andy> should be as simple to implement for an application developer
> > as it is Andy> to grasp the concept of turning a light bulb on and off.
> > 
> > The point is that the logic rarely needs to be in the v4l
> > applications. The status LEDs should by default go on when the v4l
> > device is active and go off again when not like it used to do. A v4l
> > application developer would normally not want to worry about such
> > things, and only care about the video data.
> > 
> > But if a user wants something special / non-standard, then it's just a
> > matter of changing LED trigger in /sys/class/leds/..
> 
> I agree with Peter here.  I don't see why a video app would care about
> blinking an LED while capturing.  I suspect most apps won't bother to
> implement it, or it will be a card specific mess (depending on what
> the hw actually provides).  Shouldn't the driver just turn it on or
> blink it when capturing is active or whatever.  Why should apps care?

The main reason I know about to allow applications to turn the status LED off 
is to avoid reflections on glasses.

> Plus, each app may implement some different behavior or some may not
> implement it at all which will further confuse users.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] Illuminators and status LED controls

[Laurent Pinchart]

Hi Hans,

On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
> On 09/09/2010 03:29 PM, Hans Verkuil wrote:
> >> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
> >>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
> >>> 
> >>> I originally was in favor of controlling these through v4l as well, but
> >>> people made some good arguments against that. The main one being: why
> >>> would you want to show these as a control? What is the end user supposed
> >>> to do with them? It makes little sense.

Status LEDs reflect in glasses, making annoying color dots on webcam pictures. 
That's why Logitech allows to turn the status LED off on its webcams.

[snip]

> >> Reading this whole thread I have to agree that if we are going to expose
> >> camera status LEDs it would be done through the sysfs API. I think this
> >> can be done nicely for gspca based drivers (as we can put all the "crud"
> >> in the gspca core having to do it only once), but that is a low priority
> >> nice to have thingy.
> >> 
> >> This does leave us with the problem of logitech uvc cams where the LED
> >> currently is exposed as a v4l2 control.
> > 
> > Is it possible for the uvc driver to detect and use a LED control? That's
> > how I would expect this to work, but I know that uvc is a bit of a
> > strange beast.
> 
> Unfortunately no, some uvc cameras have "proprietary" controls. The uvc
> driver knows nothing about these but offers an API to map these to v4l2
> controls (where userspace tells it the v4l2 cid, type, min, max, etc.).
> 
> Currently on logitech cameras the userspace tools if installed will map
> the led control to a private v4l2 menu control with the following options:
> On
> Off
> Auto
> Blink
> 
> The cameras default to auto, where the led is turned on when video
> is being streamed and off when there is no streaming going on.

I confirm this. If the UVC LED controls were standard the driver could expose 
them through a LED-specific API. As UVC allows devices to implement private 
controls, the driver needs to expose all those private controls (both LED and 
non-LED) through the same API.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

On Monday, September 13, 2010 09:04:18 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
> > On 09/09/2010 03:29 PM, Hans Verkuil wrote:
> > >> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
> > >>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
> > >>> 
> > >>> I originally was in favor of controlling these through v4l as well, but
> > >>> people made some good arguments against that. The main one being: why
> > >>> would you want to show these as a control? What is the end user supposed
> > >>> to do with them? It makes little sense.
> 
> Status LEDs reflect in glasses, making annoying color dots on webcam pictures. 
> That's why Logitech allows to turn the status LED off on its webcams.

That's a really good argument. I didn't think of that one.

I'm happy with a menu control for LEDs, something like:

Auto (default)
Off

and possibly:

On
Blink

Although I'm not so sure we need/want these last two.

It should be a control since otherwise v4l2 apps would need to add support for
the LED interface just for this, whereas if it is a control it will 'just work'.

I think it is up to the driver whether it wants to implement the LED interface
as well.

Regards,

	Hans

> 
> [snip]
> 
> > >> Reading this whole thread I have to agree that if we are going to expose
> > >> camera status LEDs it would be done through the sysfs API. I think this
> > >> can be done nicely for gspca based drivers (as we can put all the "crud"
> > >> in the gspca core having to do it only once), but that is a low priority
> > >> nice to have thingy.
> > >> 
> > >> This does leave us with the problem of logitech uvc cams where the LED
> > >> currently is exposed as a v4l2 control.
> > > 
> > > Is it possible for the uvc driver to detect and use a LED control? That's
> > > how I would expect this to work, but I know that uvc is a bit of a
> > > strange beast.
> > 
> > Unfortunately no, some uvc cameras have "proprietary" controls. The uvc
> > driver knows nothing about these but offers an API to map these to v4l2
> > controls (where userspace tells it the v4l2 cid, type, min, max, etc.).
> > 
> > Currently on logitech cameras the userspace tools if installed will map
> > the led control to a private v4l2 menu control with the following options:
> > On
> > Off
> > Auto
> > Blink
> > 
> > The cameras default to auto, where the led is turned on when video
> > is being streamed and off when there is no streaming going on.
> 
> I confirm this. If the UVC LED controls were standard the driver could expose 
> them through a LED-specific API. As UVC allows devices to implement private 
> controls, the driver needs to expose all those private controls (both LED and 
> non-LED) through the same API.
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Mauro Carvalho Chehab]

Em 13-09-2010 05:06, Hans Verkuil escreveu:
> On Monday, September 13, 2010 09:04:18 Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
>>> On 09/09/2010 03:29 PM, Hans Verkuil wrote:
>>>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
>>>>>>
>>>>>> I originally was in favor of controlling these through v4l as well, but
>>>>>> people made some good arguments against that. The main one being: why
>>>>>> would you want to show these as a control? What is the end user supposed
>>>>>> to do with them? It makes little sense.
>>
>> Status LEDs reflect in glasses, making annoying color dots on webcam pictures. 
>> That's why Logitech allows to turn the status LED off on its webcams.
> 
> That's a really good argument. I didn't think of that one.

There's one difference between illuminators and leds and anything else that we use
currently via CTRL interface: all other controls affects just an internal hardware
capability that are not visible to the user, nor can cause any kind of damage or 
annoyance.

On the other hand, a LED and an illuminator that an application may forget to turn
off could be very annoying, and may eventually reduce the lifecycle or a device (in
the case of non-LED illuminators, for example).

So, a special treatment seems to be required for both cases: if the application that
changed the LED or illuminator to ON dies or closes, the LED/illuminator should be
turned off by the driver.

Maybe we could add an internal flag to be consumed by the controls core, and call it
during release() callback, to be sure that such controls will return to a default state
(0) when users count reaches zero.

> 
> I'm happy with a menu control for LEDs, something like:
> 
> Auto (default)
> Off
> 
> and possibly:
> 
> On
> Blink
> 
> Although I'm not so sure we need/want these last two.

Provided that it will return to Off (or auto) after application shuts down, I don't see
why not offering such control to userspace. While it may not make sense on desktops,
turning LEDs on may be interesting on some cases. For example, there are several Android
applications to turn the webcam "flash" LED on, meant to use the cell phone as an 
emergency light.

> It should be a control since otherwise v4l2 apps would need to add support for
> the LED interface just for this, whereas if it is a control it will 'just work'.
> 
> I think it is up to the driver whether it wants to implement the LED interface
> as well.


> 
> Regards,
> 
> 	Hans
> 
>>
>> [snip]
>>
>>>>> Reading this whole thread I have to agree that if we are going to expose
>>>>> camera status LEDs it would be done through the sysfs API. I think this
>>>>> can be done nicely for gspca based drivers (as we can put all the "crud"
>>>>> in the gspca core having to do it only once), but that is a low priority
>>>>> nice to have thingy.
>>>>>
>>>>> This does leave us with the problem of logitech uvc cams where the LED
>>>>> currently is exposed as a v4l2 control.
>>>>
>>>> Is it possible for the uvc driver to detect and use a LED control? That's
>>>> how I would expect this to work, but I know that uvc is a bit of a
>>>> strange beast.
>>>
>>> Unfortunately no, some uvc cameras have "proprietary" controls. The uvc
>>> driver knows nothing about these but offers an API to map these to v4l2
>>> controls (where userspace tells it the v4l2 cid, type, min, max, etc.).
>>>
>>> Currently on logitech cameras the userspace tools if installed will map
>>> the led control to a private v4l2 menu control with the following options:
>>> On
>>> Off
>>> Auto
>>> Blink
>>>
>>> The cameras default to auto, where the led is turned on when video
>>> is being streamed and off when there is no streaming going on.
>>
>> I confirm this. If the UVC LED controls were standard the driver could expose 
>> them through a LED-specific API. As UVC allows devices to implement private 
>> controls, the driver needs to expose all those private controls (both LED and 
>> non-LED) through the same API.
>>
>>
> 

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Mon, 2010-09-13 at 08:45 -0300, Mauro Carvalho Chehab wrote:
> Em 13-09-2010 05:06, Hans Verkuil escreveu:
> > On Monday, September 13, 2010 09:04:18 Laurent Pinchart wrote:
> >> Hi Hans,
> >>
> >> On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
> >>> On 09/09/2010 03:29 PM, Hans Verkuil wrote:
> >>>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
> >>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
> >>>>>>
> >>>>>> I originally was in favor of controlling these through v4l as well, but
> >>>>>> people made some good arguments against that. The main one being: why
> >>>>>> would you want to show these as a control? What is the end user supposed
> >>>>>> to do with them? It makes little sense.
> >>
> >> Status LEDs reflect in glasses, making annoying color dots on webcam pictures. 
> >> That's why Logitech allows to turn the status LED off on its webcams.
> > 
> > That's a really good argument. I didn't think of that one.
> 
> There's one difference between illuminators and leds and anything else that we use
> currently via CTRL interface: all other controls affects just an internal hardware
> capability that are not visible to the user, nor can cause any kind of damage or 
> annoyance.
> 
> On the other hand, a LED and an illuminator that an application may forget to turn
> off could be very annoying, and may eventually reduce the lifecycle or a device (in
> the case of non-LED illuminators, for example).

Yes, I can appreciate that.  On driver unload and suspend that should
certainly be the case for illuminators.

However, I don't think that's a good idea for final close on a file
descriptor though.  That's a departure from normal V4L2 behavior.

For a USB connected device, turning off the illuminator after the fact
is simple, if the user has no other recourse: unplug the device. :)


> So, a special treatment seems to be required for both cases: if the application that
> changed the LED or illuminator to ON dies or closes, the LED/illuminator should be
> turned off by the driver.

That will break cases like these:

$ v4l2-ctl -d /dev/video0 -c illuminator_2=1
$ (command to run app that doesn't present all controls, e.g. cheese)

Regards,
Andy

> Maybe we could add an internal flag to be consumed by the controls core, and call it
> during release() callback, to be sure that such controls will return to a default state
> (0) when users count reaches zero.
> 
> > 
> > I'm happy with a menu control for LEDs, something like:
> > 
> > Auto (default)
> > Off
> > 
> > and possibly:
> > 
> > On
> > Blink
> > 
> > Although I'm not so sure we need/want these last two.
> 
> Provided that it will return to Off (or auto) after application shuts down, I don't see
> why not offering such control to userspace. While it may not make sense on desktops,
> turning LEDs on may be interesting on some cases. For example, there are several Android
> applications to turn the webcam "flash" LED on, meant to use the cell phone as an 
> emergency light.

Re: [PATCH] Illuminators and status LED controls

[Mauro Carvalho Chehab]

Em 13-09-2010 10:49, Andy Walls escreveu:
> On Mon, 2010-09-13 at 08:45 -0300, Mauro Carvalho Chehab wrote:
>> Em 13-09-2010 05:06, Hans Verkuil escreveu:
>>> On Monday, September 13, 2010 09:04:18 Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
>>>>> On 09/09/2010 03:29 PM, Hans Verkuil wrote:
>>>>>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
>>>>>>>>
>>>>>>>> I originally was in favor of controlling these through v4l as well, but
>>>>>>>> people made some good arguments against that. The main one being: why
>>>>>>>> would you want to show these as a control? What is the end user supposed
>>>>>>>> to do with them? It makes little sense.
>>>>
>>>> Status LEDs reflect in glasses, making annoying color dots on webcam pictures. 
>>>> That's why Logitech allows to turn the status LED off on its webcams.
>>>
>>> That's a really good argument. I didn't think of that one.
>>
>> There's one difference between illuminators and leds and anything else that we use
>> currently via CTRL interface: all other controls affects just an internal hardware
>> capability that are not visible to the user, nor can cause any kind of damage or 
>> annoyance.
>>
>> On the other hand, a LED and an illuminator that an application may forget to turn
>> off could be very annoying, and may eventually reduce the lifecycle or a device (in
>> the case of non-LED illuminators, for example).
> 
> Yes, I can appreciate that.  On driver unload and suspend that should
> certainly be the case for illuminators.
> 
> However, I don't think that's a good idea for final close on a file
> descriptor though.  That's a departure from normal V4L2 behavior.

This doesn't seem to be a good reason. Keeping a LED after its usage is annoying to 
the user, can cause damage on devices (reduce lifetime) and can draw lots of power 
from the batteries (on battery-powered devices).

> For a USB connected device, turning off the illuminator after the fact
> is simple, if the user has no other recourse: unplug the device. :)

Try to unplug the flash led on your cell phone ;)

>> So, a special treatment seems to be required for both cases: if the application that
>> changed the LED or illuminator to ON dies or closes, the LED/illuminator should be
>> turned off by the driver.
> 
> That will break cases like these:
> 
> $ v4l2-ctl -d /dev/video0 -c illuminator_2=1
> $ (command to run app that doesn't present all controls, e.g. cheese)

True, but it may have an alternative syntax for it, like:

v4l2-ctl -d /dev/video0 -c illuminator_2=1 --run cheese [<args>]

This way, if cheese or v4l2-ctl abends, the illuminator will be turned off.

Of course, we'll likely need to have a flag visible on userspace, to say that such
control resets to an "off" state when the application dies, to avoid someone to use it
like:
	v4l2-ctl -d /dev/video0 -c illuminator_2=1

Cheers,
Mauro

Re: [PATCH] Illuminators and status LED controls

[Laurent Pinchart]

Hi Mauro,

On Monday 13 September 2010 16:38:03 Mauro Carvalho Chehab wrote:
> Em 13-09-2010 10:49, Andy Walls escreveu:
> > On Mon, 2010-09-13 at 08:45 -0300, Mauro Carvalho Chehab wrote:
> >> Em 13-09-2010 05:06, Hans Verkuil escreveu:
> >>> On Monday, September 13, 2010 09:04:18 Laurent Pinchart wrote:
> >>>> On Thursday 09 September 2010 13:48:58 Hans de Goede wrote:
> >>>>> On 09/09/2010 03:29 PM, Hans Verkuil wrote:
> >>>>>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
> >>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
> >>>>>>>> 
> >>>>>>>> I originally was in favor of controlling these through v4l as
> >>>>>>>> well, but people made some good arguments against that. The main
> >>>>>>>> one being: why would you want to show these as a control? What is
> >>>>>>>> the end user supposed to do with them? It makes little sense.
> >>>> 
> >>>> Status LEDs reflect in glasses, making annoying color dots on webcam
> >>>> pictures. That's why Logitech allows to turn the status LED off on
> >>>> its webcams.
> >>> 
> >>> That's a really good argument. I didn't think of that one.
> >> 
> >> There's one difference between illuminators and leds and anything else
> >> that we use currently via CTRL interface: all other controls affects
> >> just an internal hardware capability that are not visible to the user,
> >> nor can cause any kind of damage or annoyance.
> >> 
> >> On the other hand, a LED and an illuminator that an application may
> >> forget to turn off could be very annoying, and may eventually reduce
> >> the lifecycle or a device (in the case of non-LED illuminators, for
> >> example).
> > 
> > Yes, I can appreciate that.  On driver unload and suspend that should
> > certainly be the case for illuminators.
> > 
> > However, I don't think that's a good idea for final close on a file
> > descriptor though.  That's a departure from normal V4L2 behavior.
> 
> This doesn't seem to be a good reason. Keeping a LED after its usage is
> annoying to the user, can cause damage on devices (reduce lifetime) and
> can draw lots of power from the batteries (on battery-powered devices).
> 
> > For a USB connected device, turning off the illuminator after the fact
> > is simple, if the user has no other recourse: unplug the device. :)
> 
> Try to unplug the flash led on your cell phone ;)

Flash are much more complex than "simple" illuminators. They require pre-flash 
timing information and flash current for instance. The flash API will need a 
control to set the flash duration, and the driver (or hardware) should turn 
the flash off automatically after a security timeout (depending on the 
current, the hardware, ...).

> >> So, a special treatment seems to be required for both cases: if the
> >> application that changed the LED or illuminator to ON dies or closes,
> >> the LED/illuminator should be turned off by the driver.
> > 
> > That will break cases like these:
> > 
> > $ v4l2-ctl -d /dev/video0 -c illuminator_2=1
> > $ (command to run app that doesn't present all controls, e.g. cheese)
> 
> True, but it may have an alternative syntax for it, like:
> 
> v4l2-ctl -d /dev/video0 -c illuminator_2=1 --run cheese [<args>]
> 
> This way, if cheese or v4l2-ctl abends, the illuminator will be turned off.
> 
> Of course, we'll likely need to have a flag visible on userspace, to say
> that such control resets to an "off" state when the application dies, to
> avoid someone to use it like:
> 	v4l2-ctl -d /dev/video0 -c illuminator_2=1

-- 
Regards,

Laurent Pinchart

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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.
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Laurent Pinchart]

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.


-- 
Regards,

Laurent Pinchart

Re: [PATCH] Illuminators and status LED controls

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Fri, 2010-09-10 at 09:19 +0200, Peter Korsgaard wrote:
> >>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:
> 
> Hi,
> 
>  Andy> Given choices I made when I patched up gspca/cpia1.c for my
>  Andy> prototype LED API usage, I got these associations
> 
>  Andy> By exposed LED name:
> 
>  Andy> 	/sys/class/leds/video0:white:illuminator0
> 
> Indeed. But didn't we just decide that illuminators were an integral
> part of the video handling (similar to gain control), and only use the
> LED API for status LEDs that don't directly interfere with the video
> data?

Hi Peter,

Correct, I still want illuminators controlled by the V4L2 control API.

The QX3 is the only unit I had to test with.    I needed to prototype
something so I could understand how the LED API worked.  The
Documentation/led-class.txt file does not explain things enough for an
end user nor a developer to use the interface.  Nothing in my desktop
system currently exported LEDs.

(git grep revealed that drivers dedicated to LED or GPIO chips, laptop
led drivers,  a few NIC drivers, and a some graphics devices were the
major exporters of LEDs via the LED API.  Nothing in my desktop system
exported LEDs.)


>  Andy> The LED API has some shortcomings/annoyances:
> 
>  Andy> - The method a driver provides to set the LED brightness cannot sleep,
>  Andy> so a workqueue is needed to simply turn a hardware light on and off for
>  Andy> USB devices.
> 
>  Andy> - The Documentation is not very good for end-users or kernel developers
>  Andy> on using the LED API. 
> 
> No? I agree that the documentation is pretty minimalistic, but ok - It's
> not that complicated.

It is opaque to the unfamiliar.  In a few Google searches, I could not
find userspace application code or scripts that demonstrated use of the
API.   An average user will give up on using it, before rummaging
through kernel code.

Even after reading the minimal documentation and looking at the code, I
improperly thought that more than one trigger could be set on an LED.

I wasted time on figuring out these things, where I think the
documentation could have easily helped:

- names of kernel provided LED trigger modules
- function/operation of each kernel provided LED trigger module
- of the LED triggers that took parameters, what were the units
- implementation difference between a simple and a complex LED trigger,
not just the LED class design objective differences
- location of a top level sysfs node that provides a list of all the LED
triggers available - there is none.  You need at least one LED exposed
to see the global list.
- that the LED core provides a simple suspend/resume, if drivers don't
want to provide their own.


And these, for which documentation will probably always lag:

- What drivers expose LEDs
- What drivers possibly provide LED trigger events, and what triggers
pick them up.



>  Andy> - For an LED trigger not to override a user's desire to inhibit an LED,
>  Andy> the user needs to know to cancel all the triggers on an LED before
>  Andy> setting the LEDs brightness to 0.
> 
> Only a single trigger can be active at a time for a given LED.

Hmmm.  I had supposed if I had a USB camera that has an LED and a button
then I could provide triggers:

	video0_stream (for video stream stop/start events)
	video0_button0 (for button press events from the first button)

then I could use these together to automatically turn the LED on and off
when a stream started or stopped and allow the user to extinguish the
LED by pressing the button.  (As a prototype test case for the LED API,
I was thinking of walking the two QX3 microscope illuminators through
different brightness sequences when pressing the button, and having them
both turn off when the stream stopped.)

So instead I guess I would have write a single LED trigger that responds
to all the event types I care to handle at once.  Again, it would have
been nice, if the documentation provided that as guidance.  Explaining
binding a trigger to an LED by referring to how a user changes I/O
schedulers, doesn't help most users.  Most end users have never needed
to change the I/O scheduler.



>  Andy> Again, that happens to be the only real compelling use case I see for
>  Andy> using the LED API.  However, I doubt many users will try to take
>  Andy> advantage of it, and I suspect even less will succeed in getting it
>  Andy> configured right.  Good documentation could go a long way in correcting
>  Andy> that.
> 
> That and using the LED for something else (perhaps with another trigger
> like I eplained earlier with wlan/hdd activity).

I personally don't think all the extra code in the V4L2 drivers is worth
it for sharing a camera LED for other purposes.  I don't see the average
user really using it for Network or HDD activity, when userspace tools
like GNOME system monitor provides a convenient display right on the
desktop bar.

Regards,
Andy

Re: [PATCH] Illuminators and status LED controls

[Peter Korsgaard]

>>>>> "Andy" == Andy Walls <awalls@md.metrocast.net> writes:

Hi,

 Andy> Given choices I made when I patched up gspca/cpia1.c for my
 Andy> prototype LED API usage, I got these associations

 Andy> By exposed LED name:

 Andy> 	/sys/class/leds/video0:white:illuminator0

Indeed. But didn't we just decide that illuminators were an integral
part of the video handling (similar to gain control), and only use the
LED API for status LEDs that don't directly interfere with the video
data?

 Andy> The LED API has some shortcomings/annoyances:

 Andy> - The method a driver provides to set the LED brightness cannot sleep,
 Andy> so a workqueue is needed to simply turn a hardware light on and off for
 Andy> USB devices.

 Andy> - The Documentation is not very good for end-users or kernel developers
 Andy> on using the LED API. 

No? I agree that the documentation is pretty minimalistic, but ok - It's
not that complicated.

 Andy> - For an LED trigger not to override a user's desire to inhibit an LED,
 Andy> the user needs to know to cancel all the triggers on an LED before
 Andy> setting the LEDs brightness to 0.

Only a single trigger can be active at a time for a given LED.

 Andy> Again, that happens to be the only real compelling use case I see for
 Andy> using the LED API.  However, I doubt many users will try to take
 Andy> advantage of it, and I suspect even less will succeed in getting it
 Andy> configured right.  Good documentation could go a long way in correcting
 Andy> that.

That and using the LED for something else (perhaps with another trigger
like I eplained earlier with wlan/hdd activity).

 Andy> If a user configures multiple LED triggers on an LED, those triggers
 Andy> will compete with each other.  The net result is the most recent event
 Andy> from the driver, any LED triggers wins, or user manipulation of sysfs
 Andy> brightness.

Only a single trigger can be active at a time for a given LED.

 Andy> With indicators that's annoying, but not a failure.  With illuminators,
 Andy> that is a failure.

Again, I don't think we should use the LED API for something as integral
to the video signal as illuminators.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Thu, 2010-09-09 at 16:17 +0200, Hans Verkuil wrote:
> > Hans,
> > I'll have more later, but I can say, if LED API is what we agree to, we
> > should have infrastructure in v4l2 at a level higher than gspca for
> > helping drivers use the LED interface and triggers.
> >
> >
> > Specifically this is needed to make discovery and association of v4l2
> > devices, exposed v4l2 LEDs, and v4l2 LED triggers easier for userspace,
> > and to provide a logical, consistent naming scheme.  It may also help with
> > logical association to a v4l2 media controller later on.
> 
> The association between a v4l device and sysfs LEDs is something that
> should be exposed in the upcoming media API (although I guess we should
> take a look first as to how that should be done exactly).

Given choices I made when I patched up gspca/cpia1.c for my prototype
LED API usage, I got these associations

By exposed LED name:

	/sys/class/leds/video0:white:illuminator0


By the exposed LED having the same parent struct device:

	/sys/class/leds/video0:white:illuminator0/device/video4linux/video0

	/sys/class/video4linux/video0/device/leds/video0:white:illuminator0

	/sys/bus/pci/devices/0000:00:12.0/usb3/3-2/3-2:1.0/leds/video0:white:illuminator0

	/sys/bus/pci/devices/0000:00:12.0/usb3/3-2/3-2:1.0/video4linux/video0

and similar results for video0:white:illuminator1.

These have all the usual sysfs gore, but video0's "led" illuminators are
clearly associated with video0 no matter how one runs through sysfs to
discover leds provided by a v4l2 device driver.

The Documentation/leds-class.txt file recommends, rather loosely, that
led device names formed as "devicename:color:function".    Across the
existing drives that expose LEDs, devicename is somewhat inconsistently
chosen, and I don't recall seeing one that chose to expose that it may
be one of multiple instances of a device.  For v4l2 devices, I would
recommend devicename be "videoN" or "mcN", so that the user can shortcut
some sysfs traversal to discover the association.


> But I feel I am missing something: who is supposed to use these LEDs?

The most compelling use-cases I heard so far were::

1. A user who wants to force the LED on the camera to be:

	always off (get rid of annoyance or surreptitious recording?)
	steady on (illusion of continuous recording?)
	auto (driver controlled)
	blinking (obvious warning that recording is happening?)

I do not think this use-case forces the use of either API, but V4L2
controls take less lines of code and have working user permissions out
of the box.

Any driver can be written to support both APIs (my prototype patches to
gspca/cpia1.c do that).  I'm debating if that would be good to do for
illuminators.  I'm sure it's not a good idea for LED indicators.



2. A kernel video device driver controlling a camera that doesn't have
it's own LED, but provides an LED trigger and LED event notifications.
That way a user can hook the video device driver up to a "spare" LED in
his system to get actual LED indications for the camera's operation
without a separate userspace utility.

(This could also apply to some sort of photography setup, where a kernel
driver needs to control external illuminators or flashbulbs without any
help from userspace utilities.)

This is the compelling case for camera device drivers to at least
provide LED API triggers and events, as V4L2 cannot make this happen
without the v4l2_event API and a userspace helper program.  Then at the
point a driver provides LED triggers and events, it makes a lot of sense
to use those same triggers and events to drive any LEDs it might have on
any supported hardware.

The LED API has some shortcomings/annoyances:

- The method a driver provides to set the LED brightness cannot sleep,
so a workqueue is needed to simply turn a hardware light on and off for
USB devices.

- The Documentation is not very good for end-users or kernel developers
on using the LED API. 

- Triggers are available globally, so trigger name clashes are possible,
especially with drivers that support multiple instances of hardware.  I
can't name an LED trigger provided by the cpia1 driver as "cpia_button",
for if I have 2 QX3 microscopes hooked to my system, only the first
QX3's button events are going to effect the LED.  I would suggest LED
trigger names of the form "videoN_eventclass" such as "video0_dock" (the
QX3 microscope body docks in a cradle which has the bottom lamp),
"video0_button1", "video0_streaming", etc.

- For an LED trigger not to override a user's desire to inhibit an LED,
the user needs to know to cancel all the triggers on an LED before
setting the LEDs brightness to 0.

- By default only root has permissions on the sysfs nodes, even though
the user may have permissions for the video nodes.

- Stock triggers are loadable modules (e.g. ledtrig-timer.ko) which only
root can load, so they are unavailable, if root hasn't preloaded them.

- LED triggers can be compiled out of the kernel.  video drivers that
want to use LED triggers for their own LED handling, will need alternate
code paths (#ifdef CONFIG_LED_TRIGGER, IIRC) or the Kconfig for the
driver will have to select/depend on LED_TRIGGER or loose it's lights.


> Turning LEDs in e.g. webcams on or off is a job for the driver, never for
> a userspace application. For that matter, if the driver handles the LEDs,
> can we still expose the API to userspace?

Yes.  You can use the LED triggers and LED events for the driver to
handle it's own LEDs for it's own purposes.  in that case, if the driver
handles a piece of hardware without and LED, the user can hook up the
trigger to a "spare" LED somewhere else in the system, so the driver's
LED events are given to that user specified LED.

Again, that happens to be the only real compelling use case I see for
using the LED API.  However, I doubt many users will try to take
advantage of it, and I suspect even less will succeed in getting it
configured right.  Good documentation could go a long way in correcting
that.

Of note, is that userspace, and some random LED trigger the user has
hooked to your driver's LED, has no sensitivity to commands that may
glitch or corrupt the capture stream.  Video drivers have to have
brightness_set() functions that are smart enough to make sure that
doesn't happen.  It is not sufficient to have the driver's own LED
triggers and timing of LED events handle not glitching the video stream.


>  Wouldn't those two interfere
> with one another? I know nothing about the LED interface in sysfs, but I
> can imagine that will be a problem.

If a user configures multiple LED triggers on an LED, those triggers
will compete with each other.  The net result is the most recent event
from the driver, any LED triggers wins, or user manipulation of sysfs
brightness.

With indicators that's annoying, but not a failure.  With illuminators,
that is a failure.



> > Sysfs entry ownership, unix permissions, and ACL permissions consistency
> > with /dev/videoN will be the immediate usability problem for end users in
> > any case.
> 
> Again, why would end users or application need or want to manipulate such
> LEDs in any case?

To quash the LED shining in one's eye, as one tries to look at the
webcam lens (situated right next to said annoying LED) in the lid of
one's laptop.



> > BTW, what did you mean by uvc discovering an LED control?
> 
> In order for the uvc driver to be able to turn LEDs on or off it needs to
> detect that the uvc camera actually has a LED. It is my impression that
> that is something that the driver has to discover from the uvc camera
> itself. Either that, or it is passed in from the userspace utility that
> uses the usb ID to determine the uvc extension controls and tells the
> driver about it.
> 
> I'm probably not using the right terminology, but I hope I didn't make too
> much of a mess of it :-)
> 
> In any case, the uvc driver has to discover somehow if there are leds and
> how to turn them on/off.


Ah, I understand.  Thanks.

Regards,
Andy

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

On Thu, 2010-09-09 at 15:17 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/09/2010 04:41 PM, Andy Walls wrote:
> > Hans de Goede,
> >
> > The uvc API that creates v4l2 ctrls on behalf of userspace could intercept those calls and create an LED interface instead of, or in addition to, the v4l2 ctrl.
> 
> That would mean special casing certain extension controls which I
> don't think is something which we want to do.

I concur, it's a kludge.

I was careful to word my original statement with "could".

Regards,
Andy


> Regards,
> 
> Hans

Re: [PATCH] Illuminators and status LED controls

[Peter Korsgaard]

>>>>> "Hans" == Hans Verkuil <hverkuil@xs4all.nl> writes:

Hi,

 Hans> But I feel I am missing something: who is supposed to use these LEDs?
 Hans> Turning LEDs in e.g. webcams on or off is a job for the driver, never for
 Hans> a userspace application.

Agreed - By default, the driver should just turn on the LED when the
device is active and off again when it is not.

 Hans> For that matter, if the driver handles the LEDs,
 Hans> can we still expose the API to userspace? Wouldn't those two interfere
 Hans> with one another? I know nothing about the LED interface in sysfs, but I
 Hans> can imagine that will be a problem.

Yes, you expose the LED using the LED class, and add a LED trigger per
video device (named something like "videoX-active"). Furthermore you set
the default trigger for that LED to be videoX-active.

So the logic of how to turn on/off the LED is seperated from the policy
about WHEN it should be turned on/off.

 >> Sysfs entry ownership, unix permissions, and ACL permissions consistency
 >> with /dev/videoN will be the immediate usability problem for end users in
 >> any case.

 Hans> Again, why would end users or application need or want to manipulate such
 Hans> LEDs in any case?

In most cases they don't - Not using the LED sysfs or v4l. But if they
do, then they CAN.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

Hans de Goede,

The uvc API that creates v4l2 ctrls on behalf of userspace could intercept those calls and create an LED interface instead of, or in addition to, the v4l2 ctrl.

Until udev, hal, pam, and polkit userspace configurations catch up, one still has the problem of the sysfs LED files not being usable by the user due to permissions.

Regards,
Andy

Hans de Goede <hdegoede@redhat.com> wrote:

>Hi,
>
>On 09/09/2010 03:29 PM, Hans Verkuil wrote:
>>
>>> Hi,
>>>
>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>   writes:
>>>>
>>>> Hi,
>>>>
>>>>    >>   - the status LED should be controlled by the LED interface.
>>>>
>>>>    Hans>   I originally was in favor of controlling these through v4l as
>>>>    Hans>   well, but people made some good arguments against that. The
>>>> main
>>>>    Hans>   one being: why would you want to show these as a control? What
>>>> is
>>>>    Hans>   the end user supposed to do with them? It makes little sense.
>>>>
>>>>    Hans>   Frankly, why would you want to expose LEDs at all? Shouldn't
>>>> this
>>>>    Hans>   be completely hidden by the driver? No generic application will
>>>>    Hans>   ever do anything with status LEDs anyway. So it should be the
>>>>    Hans>   driver that operates them and in that case the LEDs do not need
>>>>    Hans>   to be exposed anywhere.
>>>>
>>>> It's not that it *HAS* to be exposed - But if we can, then it's nice to
>>>> do
>>>> so as it gives flexibility to the user instead of hardcoding policy in
>>>> the kernel.
>>>>
>>>
>>> Reading this whole thread I have to agree that if we are going to expose
>>> camera status LEDs it would be done through the sysfs API. I think this
>>> can be done nicely for gspca based drivers (as we can put all the "crud"
>>> in the gspca core having to do it only once), but that is a low priority
>>> nice to have thingy.
>>>
>>> This does leave us with the problem of logitech uvc cams where the LED
>>> currently is exposed as a v4l2 control.
>>
>> Is it possible for the uvc driver to detect and use a LED control? That's
>> how I would expect this to work, but I know that uvc is a bit of a strange
>> beast.
>>
>
>Unfortunately no, some uvc cameras have "proprietary" controls. The uvc driver
>knows nothing about these but offers an API to map these to v4l2 controls
>(where userspace tells it the v4l2 cid, type, min, max, etc.).
>
>Currently on logitech cameras the userspace tools if installed will map
>the led control to a private v4l2 menu control with the following options:
>On
>Off
>Auto
>Blink
>
>The cameras default to auto, where the led is turned on when video
>is being streamed and off when there is no streaming going on.
>
>Regards,
>
>Hans

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

> Hans,
> I'll have more later, but I can say, if LED API is what we agree to, we
> should have infrastructure in v4l2 at a level higher than gspca for
> helping drivers use the LED interface and triggers.
>
>
> Specifically this is needed to make discovery and association of v4l2
> devices, exposed v4l2 LEDs, and v4l2 LED triggers easier for userspace,
> and to provide a logical, consistent naming scheme.  It may also help with
> logical association to a v4l2 media controller later on.

The association between a v4l device and sysfs LEDs is something that
should be exposed in the upcoming media API (although I guess we should
take a look first as to how that should be done exactly).

But I feel I am missing something: who is supposed to use these LEDs?
Turning LEDs in e.g. webcams on or off is a job for the driver, never for
a userspace application. For that matter, if the driver handles the LEDs,
can we still expose the API to userspace? Wouldn't those two interfere
with one another? I know nothing about the LED interface in sysfs, but I
can imagine that will be a problem.

> Sysfs entry ownership, unix permissions, and ACL permissions consistency
> with /dev/videoN will be the immediate usability problem for end users in
> any case.

Again, why would end users or application need or want to manipulate such
LEDs in any case?

> Sucessfully setting up or disabling LED triggers without much
> documentation will likely be the other largest hurdle for the average
> user.  (To disable an indicator LED that is manipulated automatically by a
> driver using its own Default LED trigger, the end user must disable the
> trigger in addition to setting the brightness to 0.)
>
> I still want to add trigger use to my prototype to discover the nuances.
>
>
> BTW, what did you mean by uvc discovering an LED control?

In order for the uvc driver to be able to turn LEDs on or off it needs to
detect that the uvc camera actually has a LED. It is my impression that
that is something that the driver has to discover from the uvc camera
itself. Either that, or it is passed in from the userspace utility that
uses the usb ID to determine the uvc extension controls and tells the
driver about it.

I'm probably not using the right terminology, but I hope I didn't make too
much of a mess of it :-)

In any case, the uvc driver has to discover somehow if there are leds and
how to turn them on/off.

Regards,

        Hans

>
> Regards,
> Andy
>
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>>
>>> Hi,
>>>
>>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>  writes:
>>>>
>>>> Hi,
>>>>
>>>>   >>  - the status LED should be controlled by the LED interface.
>>>>
>>>>   Hans>  I originally was in favor of controlling these through v4l as
>>>>   Hans>  well, but people made some good arguments against that. The
>>>> main
>>>>   Hans>  one being: why would you want to show these as a control?
>>>> What
>>>> is
>>>>   Hans>  the end user supposed to do with them? It makes little sense.
>>>>
>>>>   Hans>  Frankly, why would you want to expose LEDs at all? Shouldn't
>>>> this
>>>>   Hans>  be completely hidden by the driver? No generic application
>>>> will
>>>>   Hans>  ever do anything with status LEDs anyway. So it should be the
>>>>   Hans>  driver that operates them and in that case the LEDs do not
>>>> need
>>>>   Hans>  to be exposed anywhere.
>>>>
>>>> It's not that it *HAS* to be exposed - But if we can, then it's nice
>>>> to
>>>> do
>>>> so as it gives flexibility to the user instead of hardcoding policy in
>>>> the kernel.
>>>>
>>>
>>> Reading this whole thread I have to agree that if we are going to
>>> expose
>>> camera status LEDs it would be done through the sysfs API. I think this
>>> can be done nicely for gspca based drivers (as we can put all the
>>> "crud"
>>> in the gspca core having to do it only once), but that is a low
>>> priority
>>> nice to have thingy.
>>>
>>> This does leave us with the problem of logitech uvc cams where the LED
>>> currently is exposed as a v4l2 control.
>>
>>Is it possible for the uvc driver to detect and use a LED control? That's
>>how I would expect this to work, but I know that uvc is a bit of a
>> strange
>>beast.
>>
>>Regards,
>>
>>         Hans
>>
>>--
>>Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of
>> Cisco
>>
>
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

I'm of the mind that independent boolean illuminator controls are Ok.  I think that scales better.  Not that I could imagine many in use for 1 camera anyway, but some may be colors other than white.

Illuminator0 should always correspond to the most common default application of the device.

I really just want them to show up in an app.  (Maybe I'll write a MythMicroscope plugin so I can preview the subject illumination settings and then use MythTV scheduling to turn on the lights every few hours and record a few frames of bread mold growing...)

Regards,
Andy

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>
>> 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
>
>-- 
>Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
>
>--
>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

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

Hans,
I'll have more later, but I can say, if LED API is what we agree to, we should have infrastructure in v4l2 at a level higher than gspca for helping drivers use the LED interface and triggers.


Specifically this is needed to make discovery and association of v4l2 devices, exposed v4l2 LEDs, and v4l2 LED triggers easier for userspace, and to provide a logical, consistent naming scheme.  It may also help with logical association to a v4l2 media controller later on.

Sysfs entry ownership, unix permissions, and ACL permissions consistency with /dev/videoN will be the immediate usability problem for end users in any case.  

Sucessfully setting up or disabling LED triggers without much documentation will likely be the other largest hurdle for the average user.  (To disable an indicator LED that is manipulated automatically by a driver using its own Default LED trigger, the end user must disable the trigger in addition to setting the brightness to 0.)

I still want to add trigger use to my prototype to discover the nuances.


BTW, what did you mean by uvc discovering an LED control?

Regards,
Andy

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>
>> Hi,
>>
>> On 09/09/2010 08:55 AM, Peter Korsgaard wrote:
>>>>>>>> "Hans" == Hans Verkuil<hverkuil@xs4all.nl>  writes:
>>>
>>> Hi,
>>>
>>>   >>  - the status LED should be controlled by the LED interface.
>>>
>>>   Hans>  I originally was in favor of controlling these through v4l as
>>>   Hans>  well, but people made some good arguments against that. The
>>> main
>>>   Hans>  one being: why would you want to show these as a control? What
>>> is
>>>   Hans>  the end user supposed to do with them? It makes little sense.
>>>
>>>   Hans>  Frankly, why would you want to expose LEDs at all? Shouldn't
>>> this
>>>   Hans>  be completely hidden by the driver? No generic application will
>>>   Hans>  ever do anything with status LEDs anyway. So it should be the
>>>   Hans>  driver that operates them and in that case the LEDs do not need
>>>   Hans>  to be exposed anywhere.
>>>
>>> It's not that it *HAS* to be exposed - But if we can, then it's nice to
>>> do
>>> so as it gives flexibility to the user instead of hardcoding policy in
>>> the kernel.
>>>
>>
>> Reading this whole thread I have to agree that if we are going to expose
>> camera status LEDs it would be done through the sysfs API. I think this
>> can be done nicely for gspca based drivers (as we can put all the "crud"
>> in the gspca core having to do it only once), but that is a low priority
>> nice to have thingy.
>>
>> This does leave us with the problem of logitech uvc cams where the LED
>> currently is exposed as a v4l2 control.
>
>Is it possible for the uvc driver to detect and use a LED control? That's
>how I would expect this to work, but I know that uvc is a bit of a strange
>beast.
>
>Regards,
>
>         Hans
>
>-- 
>Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
>

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

> 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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

Hi,

On 09/09/2010 04:41 PM, Andy Walls wrote:
> Hans de Goede,
>
> The uvc API that creates v4l2 ctrls on behalf of userspace could intercept those calls and create an LED interface instead of, or in addition to, the v4l2 ctrl.

That would mean special casing certain extension controls which I don't think is something which we want to do.

Regards,

Hans

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

Hi,

On 09/09/2010 04:14 PM, Andy Walls wrote:
> I'm of the mind that independent boolean illuminator controls are Ok.  I think that scales better.  Not that I could imagine many in use for 1 camera anyway, but some may be colors other than white.
>
> Illuminator0 should always correspond to the most common default application of the device.

Ok, booleans it is then. JF can you do a new rfc / documentation + videodev2.h patch and
then lets get the qx3 light control patch Andy did modified to match and merge it.

Regards,

Hans

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Theodore Kilgore]

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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Eduardo Valentin]

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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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.
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Jean-Francois Moine]

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Illuminators and status LED controls

[Andy Walls]

Look for a recent patch I sent to the list for gspca_cpia for the Intel Play QX3 microscope. (The cpia2 driver handles the QX5)

Illuminator seems to be the standard term in both microscopy and IR photgraphy.  I also saw it in plain photography contexts.  Just ask the Google...

Regards,
Andy

Hans Verkuil <hverkuil@xs4all.nl> 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.
>
>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
>> 
>> 
>> 
>
>-- 
>Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
>--
>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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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
> 
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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

Re: [PATCH] Illuminators and status LED controls

[Hans Verkuil]

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [PATCH] Illuminators and status LED controls

[Hans de Goede]

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)

[PATCH] Illuminators and status LED controls

[Jean-Francois Moine]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

Hi,

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

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[-- Attachment #2: led.patch --]
[-- Type: text/x-patch, Size: 2456 bytes --]

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)