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:25 +0200, Ricardo Ribalda wrote:
> On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@xxxxxx> wrote:
> > 
> > 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.
> 
> What I am saying, is that if
> CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> create the mapping to
> V4L2_CID_PAN_SPEED
> 
If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move at
maximum speed. This is what it should do according to description of
the V4L2_CID_PAN_SPEED.

My understanding is that, if the camera supports
CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that only
one speed is available not a range.



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