Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
On Sun, 2010-09-05 at 10:24 +0200, Hans de Goede wrote: > Hi, > > p.s. (forgot to mention this in my previous mail) > > On 09/03/2010 03:09 AM, Andy Walls wrote: > > > > > @@ -447,6 +449,20 @@ > > .set = sd_setcomptarget, > > .get = sd_getcomptarget, > > }, > > + { > > + { > > +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) > > + .id = V4L2_CID_LAMPS, > > + .type= V4L2_CTRL_TYPE_MENU, > > + .name= "Lamps", > > + .minimum = 0, > > + .maximum = 3, > > + .step= 1, > > + .default_value = 0, > > + }, > > + .set = sd_setlamps, > > + .get = sd_getlamps, > > + }, > > }; > > > > static const struct v4l2_pix_format mode[] = { > > We only want this control to be available on the qx3 and not on > all cpia1 devices, Yes, I though about that, but couldn't think up a clean way of doing it in the short amount of time I had available. I did know that the control was essentially a NoOp, so I wasn't too concerned at the time. > so you need to add something like the following to > sd_config: > > if (!(id->idVendor == 0x0813 && id->idProduct == 0x0001)) > gspca_dev->ctrl_dis = 1 << LAMPS_IDX; > > Where LAMPS_IDX is a define giving the index of V4L2_CID_LAMPS in the > sd_ctrls array, see the ov519 gspca driver for example. Thanks for the pointer, I'll have a look. Regards, Andy > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
On Sun, 2010-09-05 at 10:03 +0200, Hans de Goede wrote: > Hi, > > On 09/03/2010 03:09 AM, Andy Walls wrote: > > # HG changeset patch > > # User Andy Walls > > # Date 1283475832 14400 > > # Node ID 0d251a2976b46e11cc817207de191896718b93a3 > > # Parent a4c762698bcb138982b81cf59e5bc4b7155866a9 > > gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope > > > > From: Andy Walls > > > > Add a v4l2 control to get the lamp control code working for the Intel Play > > QX3 microscope. My daughter in middle school thought it was cool, and is > > now > > examining the grossest specimens she can find. > > > > Hehe, cool I'm very happy to hear the cpia1 driver actually being used in a > "productive" manner, that shows it is worth all the time and effort I've put > into > cleaning up / rewriting old v4l1 drivers :) Yes, thank you. Now I have 10x, 60x, and 200x images of pencils, hair, dead skin, fingernails, and insects. As soon as my kids figure out the microscope body still works when removed from the base, I'm sure to have 10x, 60x, and 200x images of eyes, mouths, ears, and nostrils... :P > About the patch: first of all thanks. wrt lamps versus lights I'm indifferent. I don't care much either. Illuminator appears to be the correct term for microscopy. The cpia2 (QX5) and gspca_cpia1 (QX3) drivers should probably try to match on any user visible terminology such as the string presented as the name of the control. The cpia2 driver has presented the user with a "Lights" control. I don't know much about photography or film, so I don't know what artificial light sources that illuminate subject matter are called in those fields. > The only thing I've notices is that you've made the controls instand apply. > Normally > controls setting changes when not streaming are just remembered and then > applied > when the stream is initialized. > > However your code sends the lamp settings to the device as soon as they are > changed, and does not send them on sd_start. The sending as soon as changes > makes sense. But did you check that this actually works, After a few minutes of playing with the microscope one realizes that one must be able to change the illumination on-the-fly. Trying to get the best image possible from a microscope is easiest when one can tweak as much as possible in real-time: lighting, positioning, focus, etc. Changing the illuminator settings does work during a capture, with no apparent ill effects. Then again, one is usually starting from bad illumination conditions searching for good illumination conditions, so who cares about glitching a frame that doesn't look good. > iow did you play with > the lamps control while not streaming ? and then tried to stream and see if > the settings stuck. Yes they do. 'v4l2-ctl -c lamps=n' was used to manipulate the control, Cheese was use for streaming. The lamps will stay in the last state commanded, regardless of when streaming is started and stopped. > Also the not sending at sd_start, nor sd_init means that you assume that the > defaults in the driver (both lamps off) ar the same as of the device as you > never force that the device <-> driver settings are synced on driver load > or stream start. This may not be the case when resuming from suspend or > the driver is rmmod-ed insmod-ed. 1. manual `modprobe -r` followed by `modprobe` is not a case that the normal end user cares about. 2. when this USB device is unplugged and plugged back in, its lights are always off and the driver, as you note, always assumes they are off as well. 3. suspend/resume: well, yes that case probably matters. :) > So assuming that the instant apply > of this control does not cause issues, you should add a call to > command_setlamps(gspca_dev); at the end of sd_init. Easy enough. Regards, Andy > Regards, > > Hans > > > > Priority: normal > > > > Signed-off-by: Andy Walls > > > > diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c > > --- a/linux/drivers/media/video/gspca/cpia1.c Wed Aug 25 16:13:54 > > 2010 -0300 > > +++ b/linux/drivers/media/video/gspca/cpia1.c Thu Sep 02 21:03:52 > > 2010 -0400 > > @@ -333,8 +333,8 @@ > > } format; > > struct {/* Intel QX3 specific data */ > > u8 qx3_detected;/* a QX3 is present */ > > - u8 toplight;/* top light lit , R/W */ > > - u8 bottomlight; /* bottom light lit, R/W */ > > + u8 toplamp; /* top lamp lit , R/W */ > > + u8 bottomlamp; /* bottom lamp lit, R/W */ > > u8 button; /* snapshot button pressed (R/O) */ > > u8 cradled; /* microscope is in cradle (R/O) */ > > } qx3; > > @@ -373,6 +373,8 @@ > > static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val); > > static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val); > > static int sd_getcomptarget(struct gspca_
Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
Hi, p.s. (forgot to mention this in my previous mail) On 09/03/2010 03:09 AM, Andy Walls wrote: @@ -447,6 +449,20 @@ .set = sd_setcomptarget, .get = sd_getcomptarget, }, + { + { +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) + .id = V4L2_CID_LAMPS, + .type= V4L2_CTRL_TYPE_MENU, + .name= "Lamps", + .minimum = 0, + .maximum = 3, + .step= 1, + .default_value = 0, + }, + .set = sd_setlamps, + .get = sd_getlamps, + }, }; static const struct v4l2_pix_format mode[] = { We only want this control to be available on the qx3 and not on all cpia1 devices, so you need to add something like the following to sd_config: if (!(id->idVendor == 0x0813 && id->idProduct == 0x0001)) gspca_dev->ctrl_dis = 1 << LAMPS_IDX; Where LAMPS_IDX is a define giving the index of V4L2_CID_LAMPS in the sd_ctrls array, see the ov519 gspca driver for example. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
Hi, On 09/03/2010 03:09 AM, Andy Walls wrote: # HG changeset patch # User Andy Walls # Date 1283475832 14400 # Node ID 0d251a2976b46e11cc817207de191896718b93a3 # Parent a4c762698bcb138982b81cf59e5bc4b7155866a9 gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope From: Andy Walls Add a v4l2 control to get the lamp control code working for the Intel Play QX3 microscope. My daughter in middle school thought it was cool, and is now examining the grossest specimens she can find. Hehe, cool I'm very happy to hear the cpia1 driver actually being used in a "productive" manner, that shows it is worth all the time and effort I've put into cleaning up / rewriting old v4l1 drivers :) About the patch: first of all thanks. wrt lamps versus lights I'm indifferent. The only thing I've notices is that you've made the controls instand apply. Normally controls setting changes when not streaming are just remembered and then applied when the stream is initialized. However your code sends the lamp settings to the device as soon as they are changed, and does not send them on sd_start. The sending as soon as changes makes sense. But did you check that this actually works, iow did you play with the lamps control while not streaming ? and then tried to stream and see if the settings stuck. Also the not sending at sd_start, nor sd_init means that you assume that the defaults in the driver (both lamps off) ar the same as of the device as you never force that the device <-> driver settings are synced on driver load or stream start. This may not be the case when resuming from suspend or the driver is rmmod-ed insmod-ed. So assuming that the instant apply of this control does not cause issues, you should add a call to command_setlamps(gspca_dev); at the end of sd_init. Regards, Hans Priority: normal Signed-off-by: Andy Walls diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c --- a/linux/drivers/media/video/gspca/cpia1.c Wed Aug 25 16:13:54 2010 -0300 +++ b/linux/drivers/media/video/gspca/cpia1.c Thu Sep 02 21:03:52 2010 -0400 @@ -333,8 +333,8 @@ } format; struct {/* Intel QX3 specific data */ u8 qx3_detected;/* a QX3 is present */ - u8 toplight;/* top light lit , R/W */ - u8 bottomlight; /* bottom light lit, R/W */ + u8 toplamp; /* top lamp lit , R/W */ + u8 bottomlamp; /* bottom lamp lit, R/W */ u8 button; /* snapshot button pressed (R/O) */ u8 cradled; /* microscope is in cradle (R/O) */ } qx3; @@ -373,6 +373,8 @@ static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val); static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val); static int sd_getcomptarget(struct gspca_dev *gspca_dev, __s32 *val); +static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val); +static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val); static const struct ctrl sd_ctrls[] = { { @@ -447,6 +449,20 @@ .set = sd_setcomptarget, .get = sd_getcomptarget, }, + { + { +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) + .id = V4L2_CID_LAMPS, + .type= V4L2_CTRL_TYPE_MENU, + .name= "Lamps", + .minimum = 0, + .maximum = 3, + .step= 1, + .default_value = 0, + }, + .set = sd_setlamps, + .get = sd_getlamps, + }, }; static const struct v4l2_pix_format mode[] = { @@ -766,8 +782,8 @@ params->compressionTarget.targetQ = 5; /* From windows driver */ params->qx3.qx3_detected = 0; - params->qx3.toplight = 0; - params->qx3.bottomlight = 0; + params->qx3.toplamp = 0; + params->qx3.bottomlamp = 0; params->qx3.button = 0; params->qx3.cradled = 0; } @@ -1059,17 +1075,16 @@ 0, sd->params.streamStartLine, 0, 0); } -#if 0 /* Currently unused */ /* keep */ -static int command_setlights(struct gspca_dev *gspca_dev) +static int command_setlamps(struct gspca_dev *gspca_dev) { struct sd *sd = (struct sd *) gspca_dev; - int ret, p1, p2; + int ret, p; if (!sd->params.qx3.qx3_detected) return 0; - p1 = (sd->params.qx3.bottomlight == 0)<< 1; - p2 = (sd->params.qx3.toplight == 0)<< 3; + p = (sd->params.qx3.toplamp== 0) ? 0x8 : 0; + p |= (sd->params.qx3.bottomlamp == 0) ? 0x2 : 0; ret = do_command(gspca_dev, CPIA_COMMAND_WriteVCReg, 0x90, 0x8F, 0x50, 0); @@ -1077,9 +1092,8 @@ return ret; return do_command(gspca_dev, CPIA_COMMAND_WriteMCPort, 2, 0, - p1
Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
On Fri, 2010-09-03 at 10:38 +0200, Jean-Francois Moine wrote: > On Thu, 02 Sep 2010 21:09:42 -0400 > Andy Walls wrote: > [snip] > > Add a v4l2 control to get the lamp control code working for the Intel > > Play QX3 microscope. My daughter in middle school thought it was > > cool, and is now examining the grossest specimens she can find. > [snip] > > - u8 toplight;/* top light lit , R/W */ > > - u8 bottomlight; /* bottom light lit, R/W */ > > + u8 toplamp; /* top lamp lit , R/W */ > > + u8 bottomlamp; /* bottom lamp lit, R/W */ > [snip] > > +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) > [snip] > > Hi Andy, > > First, I do not see why you changed the name 'light' to 'lamp' while > 'light' is used in the other cpia driver (cpia2). Hi Jean-Francois, My primary reason was slightly easier maintenance of gspca_cpia1. A case-insensitive search for "lamp" will find only the code related to controlling the lamps. The string "light" is used in at least one other context in the driver. Other reasons for using "lamp": The QX3 actually uses incandescent bulbs with slow turn on circuitry to meet USB surge limit requirements. The light sources really are lamps in that sense, not just lights. "Illuminator" seems to be the proper microscopy term for the assembly that provides light from below/behind the specimen, but it is harder to type than "lamp". ;) I'm not sure what term applies to light sources above/afore the specimen. > Then, you used a private control ID, and linux-media people don't like > that. I knew that going in, however: The gspca_cpia module already uses a private control. The cpia2 driver uses 7 private controls, one of which is CPIA2_CID_LIGHTS for controlling the lamps, er, lights. ;) The cpia2 driver used the same menu labels I did - "Off", "Top", "Bottom", "Both" - just in a slightly different order. So the QX5 microscope can use a private control, but the QX3 microscope can't use a private control? It seems like there's a strong precedent here, since such an API is already presented to user-space for controlling a device, the QX5, that is functionally and physically nearly identical to the QX3. I don't quite understand the aversion to well written private controls. A private control under V4L2 appears to mean "not used by another bridge driver", but what end user cares about that? The V4L2 Control API is so well specified, that user space needs no apriori knowledge of a driver's controls, and yet applications can still present a GUI for every control useful for a user to manipulate. I put forward 'v4l2-ctl -L' and 'qv4l2' as examples of how users need not ever care if a control is private or not. V4L2 applications that would be capturing video or stills from microscopes already know how to manipulate V4L2 controls, so why not let them control the illumination with the same interface. Why make them rummage around with some other API? Using V4L2 controls to control the image processing, but prohibiting applications from using them to control the subject matter environment, doesn't make a lot of sense from an application writing perspective. We're tying our hands behind our back for the sake of "API idealism". So we have a flexible, well specified V4L2 control API and then don't let people use it, because we haven't figured out how to make lighting controls common across 200 video capture implementations and a crappy LED interface exists somewhere else in the kernel? Does that make sense to anyone? What are the benefits? > As many gspca users are waiting for a light/LED/illuminator/lamp > control, I tried to define a standard one in March 2009: > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3095 > > A second, but more restrictive, attempt was done by Németh Márton in > February 2010: > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16705 > > The main objection to that proposals was that the sysfs LED interface > should be used instead: > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3114 > > A patch in this way was done by Németh Márton in February 2010: > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16670 > > but it was rather complex, and there was no consensus > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/17111 I see I'm preaching to the choir then. I do not have time, nor desire to play the "bring m a rock" game for the API for controlling lights - especially when multiple people have been playing it already. It's a shame. The Intel Play QX3 is a nice microscope and is readily found on eBay. My daughter absolutely loves it. However, the unit is close to useless without control of the lights. If users are losing out because of inability to agree on an implementation, then something is wrong with the process. Re
Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
On Thu, 02 Sep 2010 21:09:42 -0400 Andy Walls wrote: [snip] > Add a v4l2 control to get the lamp control code working for the Intel > Play QX3 microscope. My daughter in middle school thought it was > cool, and is now examining the grossest specimens she can find. [snip] > - u8 toplight;/* top light lit , R/W */ > - u8 bottomlight; /* bottom light lit, R/W */ > + u8 toplamp; /* top lamp lit , R/W */ > + u8 bottomlamp; /* bottom lamp lit, R/W */ [snip] > +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) [snip] Hi Andy, First, I do not see why you changed the name 'light' to 'lamp' while 'light' is used in the other cpia driver (cpia2). Then, you used a private control ID, and linux-media people don't like that. As many gspca users are waiting for a light/LED/illuminator/lamp control, I tried to define a standard one in March 2009: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3095 A second, but more restrictive, attempt was done by Németh Márton in February 2010: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16705 The main objection to that proposals was that the sysfs LED interface should be used instead: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3114 A patch in this way was done by Németh Márton in February 2010: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16670 but it was rather complex, and there was no consensus http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/17111 So, I don't think that your patch could be accepted... Best regards. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope
# HG changeset patch # User Andy Walls # Date 1283475832 14400 # Node ID 0d251a2976b46e11cc817207de191896718b93a3 # Parent a4c762698bcb138982b81cf59e5bc4b7155866a9 gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope From: Andy Walls Add a v4l2 control to get the lamp control code working for the Intel Play QX3 microscope. My daughter in middle school thought it was cool, and is now examining the grossest specimens she can find. Priority: normal Signed-off-by: Andy Walls diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c --- a/linux/drivers/media/video/gspca/cpia1.c Wed Aug 25 16:13:54 2010 -0300 +++ b/linux/drivers/media/video/gspca/cpia1.c Thu Sep 02 21:03:52 2010 -0400 @@ -333,8 +333,8 @@ } format; struct {/* Intel QX3 specific data */ u8 qx3_detected;/* a QX3 is present */ - u8 toplight;/* top light lit , R/W */ - u8 bottomlight; /* bottom light lit, R/W */ + u8 toplamp; /* top lamp lit , R/W */ + u8 bottomlamp; /* bottom lamp lit, R/W */ u8 button; /* snapshot button pressed (R/O) */ u8 cradled; /* microscope is in cradle (R/O) */ } qx3; @@ -373,6 +373,8 @@ static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val); static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val); static int sd_getcomptarget(struct gspca_dev *gspca_dev, __s32 *val); +static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val); +static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val); static const struct ctrl sd_ctrls[] = { { @@ -447,6 +449,20 @@ .set = sd_setcomptarget, .get = sd_getcomptarget, }, + { + { +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1) + .id = V4L2_CID_LAMPS, + .type= V4L2_CTRL_TYPE_MENU, + .name= "Lamps", + .minimum = 0, + .maximum = 3, + .step= 1, + .default_value = 0, + }, + .set = sd_setlamps, + .get = sd_getlamps, + }, }; static const struct v4l2_pix_format mode[] = { @@ -766,8 +782,8 @@ params->compressionTarget.targetQ = 5; /* From windows driver */ params->qx3.qx3_detected = 0; - params->qx3.toplight = 0; - params->qx3.bottomlight = 0; + params->qx3.toplamp = 0; + params->qx3.bottomlamp = 0; params->qx3.button = 0; params->qx3.cradled = 0; } @@ -1059,17 +1075,16 @@ 0, sd->params.streamStartLine, 0, 0); } -#if 0 /* Currently unused */ /* keep */ -static int command_setlights(struct gspca_dev *gspca_dev) +static int command_setlamps(struct gspca_dev *gspca_dev) { struct sd *sd = (struct sd *) gspca_dev; - int ret, p1, p2; + int ret, p; if (!sd->params.qx3.qx3_detected) return 0; - p1 = (sd->params.qx3.bottomlight == 0) << 1; - p2 = (sd->params.qx3.toplight == 0) << 3; + p = (sd->params.qx3.toplamp== 0) ? 0x8 : 0; + p |= (sd->params.qx3.bottomlamp == 0) ? 0x2 : 0; ret = do_command(gspca_dev, CPIA_COMMAND_WriteVCReg, 0x90, 0x8F, 0x50, 0); @@ -1077,9 +1092,8 @@ return ret; return do_command(gspca_dev, CPIA_COMMAND_WriteMCPort, 2, 0, - p1 | p2 | 0xE0, 0); + p | 0xE0, 0); } -#endif static int set_flicker(struct gspca_dev *gspca_dev, int on, int apply) { @@ -1932,6 +1946,27 @@ return 0; } +static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val) +{ + struct sd *sd = (struct sd *) gspca_dev; + + sd->params.qx3.toplamp= (val & 0x2) ? 1 : 0; + sd->params.qx3.bottomlamp = (val & 0x1) ? 1 : 0; + + if (sd->params.qx3.qx3_detected) + return command_setlamps(gspca_dev); + + return 0; +} + +static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val) +{ + struct sd *sd = (struct sd *) gspca_dev; + + *val = (sd->params.qx3.toplamp << 1) | (sd->params.qx3.bottomlamp << 0); + return 0; +} + static int sd_querymenu(struct gspca_dev *gspca_dev, struct v4l2_querymenu *menu) { @@ -1959,6 +1994,22 @@ return 0; } break; + case V4L2_CID_LAMPS: + switch (menu->index) { + case 0: + strcpy((char *) menu->name, "Off"); + return 0; + case 1: + strcpy((char *) menu->name, "Bottom"); + return 0; + case 2: + strcpy((char *) menu->name, "Top")