linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] Illuminators and status LED controls
@ 2010-09-07 20:33 Andy Walls
  2010-09-08  2:16 ` Eino-Ville Talvala
  0 siblings, 1 reply; 56+ messages in thread
From: Andy Walls @ 2010-09-07 20:33 UTC (permalink / raw)
  To: eduardo.valentin, ext Jean-Francois Moine; +Cc: linux-media@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 56+ messages in thread
* Re: [PATCH] Illuminators and status LED controls
@ 2010-09-09 14:41 Andy Walls
  2010-09-09 13:17 ` Hans de Goede
  0 siblings, 1 reply; 56+ messages in thread
From: Andy Walls @ 2010-09-09 14:41 UTC (permalink / raw)
  To: Hans de Goede, Hans Verkuil
  Cc: Peter Korsgaard, Jean-Francois Moine, linux-media@vger.kernel.org,
	eduardo.valentin, ext Eino-Ville Talvala

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

^ permalink raw reply	[flat|nested] 56+ messages in thread
* Re: [PATCH] Illuminators and status LED controls
@ 2010-09-09 14:14 Andy Walls
  2010-09-09 13:16 ` Hans de Goede
  0 siblings, 1 reply; 56+ messages in thread
From: Andy Walls @ 2010-09-09 14:14 UTC (permalink / raw)
  To: Hans Verkuil, Hans de Goede; +Cc: Jean-Francois Moine, linux-media

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

^ permalink raw reply	[flat|nested] 56+ messages in thread
* Re: [PATCH] Illuminators and status LED controls
@ 2010-09-09 14:01 Andy Walls
  2010-09-09 14:17 ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Andy Walls @ 2010-09-09 14:01 UTC (permalink / raw)
  To: Hans Verkuil, Hans de Goede
  Cc: Peter Korsgaard, Jean-Francois Moine, linux-media@vger.kernel.org,
	eduardo.valentin, ext Eino-Ville Talvala

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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread
* Re: [PATCH] Illuminators and status LED controls
@ 2010-09-07 16:35 Andy Walls
  0 siblings, 0 replies; 56+ messages in thread
From: Andy Walls @ 2010-09-07 16:35 UTC (permalink / raw)
  To: Hans Verkuil, Hans de Goede; +Cc: Jean-Francois Moine, linux-media

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

^ permalink raw reply	[flat|nested] 56+ messages in thread
* [PATCH] Illuminators and status LED controls
@ 2010-09-06 18:11 Jean-Francois Moine
  2010-09-07  7:16 ` Hans de Goede
                   ` (2 more replies)
  0 siblings, 3 replies; 56+ messages in thread
From: Jean-Francois Moine @ 2010-09-06 18:11 UTC (permalink / raw)
  To: linux-media

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

^ permalink raw reply related	[flat|nested] 56+ messages in thread
end of thread, other threads:[~2010-09-16 10:09 UTC | newest]

Thread overview: 56+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-07 20:33 [PATCH] Illuminators and status LED controls Andy Walls
2010-09-08  2:16 ` Eino-Ville Talvala
2010-09-08  7:59   ` Eduardo Valentin
2010-09-08 16:37     ` Andy Walls
2010-09-08 18:58       ` Peter Korsgaard
2010-09-08 19:27         ` Alex Deucher
2010-09-09  4:07           ` Andy Walls
2010-09-13  7:00           ` Laurent Pinchart
2010-09-09  6:07         ` Jean-Francois Moine
2010-09-09  6:25           ` Hans Verkuil
2010-09-09  6:55             ` Peter Korsgaard
2010-09-09 11:17               ` Hans de Goede
2010-09-09 13:29                 ` Hans Verkuil
2010-09-09 11:48                   ` Hans de Goede
2010-09-13  7:04                     ` Laurent Pinchart
2010-09-13  8:06                       ` Hans Verkuil
2010-09-13 11:45                         ` Mauro Carvalho Chehab
2010-09-13 13:49                           ` Andy Walls
2010-09-13 14:38                             ` Mauro Carvalho Chehab
2010-09-16 10:09                               ` Laurent Pinchart
2010-09-10 13:40           ` Andy Walls
  -- strict thread matches above, loose matches on Subject: below --
2010-09-09 14:41 Andy Walls
2010-09-09 13:17 ` Hans de Goede
2010-09-09 21:37   ` Andy Walls
2010-09-09 14:14 Andy Walls
2010-09-09 13:16 ` Hans de Goede
2010-09-09 14:01 Andy Walls
2010-09-09 14:17 ` Hans Verkuil
2010-09-09 19:26   ` Peter Korsgaard
2010-09-10  0:49   ` Andy Walls
2010-09-10  7:19     ` Peter Korsgaard
2010-09-10 13:30       ` Andy Walls
2010-09-07 16:35 Andy Walls
2010-09-06 18:11 Jean-Francois Moine
2010-09-07  7:16 ` Hans de Goede
2010-09-07  7:30 ` Hans Verkuil
2010-09-07  9:42   ` Hans de Goede
2010-09-07  9:44     ` Hans de Goede
2010-09-07  9:47       ` Hans Verkuil
2010-09-07 11:59         ` Hans de Goede
2010-09-07 14:50           ` Hans Verkuil
2010-09-07 13:04             ` Hans de Goede
2010-09-07 15:30               ` Hans Verkuil
2010-09-07 17:57                 ` Jean-Francois Moine
2010-09-07 18:42                   ` Hans Verkuil
2010-09-07 21:21                     ` Hans Verkuil
2010-09-07 22:29                       ` Theodore Kilgore
2010-09-08  5:17                         ` Hans Verkuil
2010-09-07 21:14                 ` Hans de Goede
2010-09-09  6:55                   ` Hans Verkuil
2010-09-09 11:15                     ` Hans de Goede
2010-09-09 13:38                       ` Hans Verkuil
2010-09-13  6:53                   ` Laurent Pinchart
2010-09-13  6:47         ` Laurent Pinchart
2010-09-13  6:59           ` Hans Verkuil
2010-09-07 19:12 ` Eduardo Valentin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).