Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ricardo,
On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> wrote:
> > 
> > Hi Soyer
> > 
> > 
> > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@xxxxxx> wrote:
> > > 
> > > Hi Ricardo
> > > 
> > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > [Now in plain text mode]
> > > > 
> > > > Hi Gergo
> > > > 
> > > > Doesn't your patch affect pan and tilt for all the cameras, not
> > > > only
> > > > the BCC950?
> > > > 
> > > Yes, it affects all cameras that support
> > > CT_PANTILT_RELATIVE_CONTROL.
> > > 
> > > > Also it seems that 1 means that device does not support
> > > > programmable
> > > > speed. Is that correct?
> > > > 
> > > > ```
> > > > The bPanSpeed field is used to specify the speed of the
> > > > movement for
> > > > the Pan direction. A low
> > > > number indicates a slow speed and a high number indicates a
> > > > higher
> > > > speed. The GET_MIN,
> > > > GET_MAX and GET_RES requests are used to retrieve the range and
> > > > resolution for this field.
> > > > The GET_DEF request is used to retrieve the default value for
> > > > this
> > > > field. If the control does not
> > > > support speed control for the Pan control, it will return the
> > > > value 1
> > > > in this field for all these
> > > > requests.
> > > > ```
> > 
> > It seems to me that the module is compliant to the standard. It
> > returns 1 as GET_DEF because it does not support speed control.
> > 
> > Maybe you should ignore the speed control instead of changing its
> > default value?
> 
> ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> (Relative) Control
> 
>  https://www.usb.org/document-library/video-class-v15-document-set ;)
> > 
> > 

It's a different API. V4L2 control values are not the same as the UVC
standard control values.

Eg the V4L2_CID_PAN_SPEED control value calculated from 
CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.

It only bothers me that I have to handle these two controls
differently.

> > > > 
> > > I started from the V4L2 control description.
> > > 
> > > V4L2_CID_PAN_SPEED (integer)
> > > This control turns the camera horizontally at the specific speed.
> > > The
> > > unit is undefined. A positive value moves the camera to the right
> > > (clockwise when viewed from above), a negative value to the left.
> > > A
> > > value of zero stops the motion if one is in progress and has no
> > > effect
> > > otherwise.
> > > 
> > > And this is why I thought that 1 is not a good default value,
> > > because
> > > it moves the camera.
> > > The other V4L2 controls have a default value that I can safely
> > > set the
> > > controls to.
> > > 
> > > Are you using it to determine if the camera supports speed
> > > control?
> > > 
> > > > When you program that value do you see any difference on the
> > > > device?
> > > > What is max, min and res?
> > > > 
> > > 
> > > No, it works the same way.
> > > Only the default value changes (from 1 to 0)
> > > 
> > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0
> > > value=0
> > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0
> > > value=0
> > > 
> > > 
> > > 
> > > > 
> > > > Thanks!
> > > > 
> > > > Regards!
> > > > 
> > > > 
> > > 
> > > Thanks,
> > > Gergo
> > > 
> > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@xxxxxx>
> > > > wrote:
> > > > > 
> > > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > V4L2_CID_TILT_SPEED.
> > > > > 
> > > > > This patch sets them to 0, which is the stop value.
> > > > > 
> > > > > Previous discussion
> > > > > Link:
> > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@xxxxxxxxxxxxxx/
> > > > > 
> > > > > Signed-off-by: Gergo Koteles <soyer@xxxxxx>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > > uvc_control_mapping *mapping,
> > > > >                 return -data[first+1];
> > > > >         case UVC_GET_MAX:
> > > > >         case UVC_GET_RES:
> > > > > +               return data[first+1];
> > > > >         case UVC_GET_DEF:
> > > > >         default:
> > > > > -               return data[first+1];
> > > > > +               return 0;
> > > > >         }
> > > > >  }
> > > > > 
> > > > > 
> > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > --
> > > > > 2.40.1
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> > --
> > Ricardo Ribalda
> 
> 
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux