Re: [PATCH] uvcvideo: Remo OBSBOT quirk fix for incorrect relative min pan/tilt/zoom speeds

[Ricardo Ribalda]

Hi John

On Tue, 26 Mar 2024 at 09:15, John Bauer <john@xxxxxx> wrote:
>
> According to the spec bPanRelative and bTiltRelative are of "Signed
> Number" but bPanSpeed and bTiltSpeed are of "Number". I think this
> implies that a negative number is not possible for a minimum here.
>
> It is very beneficial that the direction and speed are condensed into
> one value, it greatly simplifies control as shown in a test I just did
> below.
>
> Here is a quick test I just did with the patch I'll be sending
> shortly: https://www.youtube.com/watch?v=1XqWPROw49s

That looks pretty cool :)

Thanks!

>
> Thanks,
> John
>
> On Tue, Mar 26, 2024 at 2:47 AM Ricardo Ribalda <ribalda@chromiumorg> wrote:
> >
> > Hi Jon, Hi Gergo
> >
> > On Tue, 26 Mar 2024 at 07:23, John Bauer <john@xxxxxx> wrote:
> > >
> > > After looking through the current implementation all of the proper checks are done in the getter and setter for the pan/tilt/zoom controls so the only change needed is the 2 locations to get/check/set the minimum when needed. Thankfully all the code that does the hard work is already implemented. I'll be submitting another patch that summarizes our findings.
> >
> >
> > My issue with the spec is that it is not clear about what GET_MIN
> > should return.  Is it the minimum absolute value for that control, or
> > the minimum value in that direction?
> >
> > In other words, can we have a device with a range (-10,20) (-A,B), or
> > only (-20,20) (-A,A) is allowed.
> >
> > If there is no device that supports (-A,B), then we do not need a quirk.
> >
> > Regards!
> >
> >
> > >
> > > Thanks,
> > > John
> > >
> > >
> > >
> > > On Mon, Mar 25, 2024 at 10:42 PM John Bauer <john@xxxxxx> wrote:
> > >>
> > >> Ok, I get you now Gergo, I think I got lucky and I think you're right! Digging into the UVC 1.5 spec I can see why this works, the first byte in each 2 byte pair signifying the direction is just getting the signed bit set when a negative value is applied to both bytes so there should probably be some checks.
> > >>
> > >> Here from the UVC 1.5 spec:  CT_PANTILT_RELATIVE_CONTROL
> > >> +--------+---------------+------+---------------+------------------------------------------------+
> > >> | Offset |     Field     | Size |     Value     |                  Description                   |
> > >> +--------+---------------+------+---------------+------------------------------------------------+
> > >> |      0 | bPanRelative  |    1 | Signed Number | 0: Stop, 1: clockwise, 0xFF: counter-clockwise |
> > >> |      1 | bPanSpeed     |    1 | Number        | Speed of the Pan movement                      |
> > >> |      2 | bTiltRelative |    1 | Signed Number | 0: Stop, 1: tilt up, 0xFF: tilt down           |
> > >> |      3 | bTiltSpeed    |    1 | Number        | Speed for the Tilt movement                    |
> > >> +--------+---------------+------+---------------+------------------------------------------------+
> > >>
> > >> I think it is the direction of the original implementation which is way easier to use than having 2 controls anyway, I would say it's preferred, it's how I have all my analog stick controls mappings.
> > >>
> > >> While the OBSBOT firmware implementation may handle any signed negative value in the direction byte we should probably check and make sure it conforms to spec with 0xFF for counter clockwise and down.
> > >>
> > >> In the current implementation both pan and tilt each use 2 bytes:
> > >>
> > >> {
> > >> .id = V4L2_CID_PAN_SPEED,
> > >> .entity = UVC_GUID_UVC_CAMERA,
> > >> .selector = UVC_CT_PANTILT_RELATIVE_CONTROL,
> > >> .size = 16,
> > >> .offset = 0,
> > >> .v4l2_type = V4L2_CTRL_TYPE_INTEGER,
> > >> .data_type = UVC_CTRL_DATA_TYPE_SIGNED,
> > >> .get = uvc_ctrl_get_rel_speed,
> > >> .set = uvc_ctrl_set_rel_speed,
> > >> },
> > >> {
> > >> .id = V4L2_CID_TILT_SPEED,
> > >> .entity = UVC_GUID_UVC_CAMERA,
> > >> .selector = UVC_CT_PANTILT_RELATIVE_CONTROL,
> > >> .size = 16,
> > >> .offset = 16,
> > >> .v4l2_type = V4L2_CTRL_TYPE_INTEGER,
> > >> .data_type = UVC_CTRL_DATA_TYPE_SIGNED,
> > >> .get = uvc_ctrl_get_rel_speed,
> > >> .set = uvc_ctrl_set_rel_speed,
> > >> },
> > >>
> > >> Going to do some testing and report back.
> > >>
> > >> Thanks,
> > >> John
> > >>
> > >>
> > >>
> > >> On Mon, Mar 25, 2024 at 9:23 PM Gergo Koteles <soyer@xxxxxx> wrote:
> > >>>
> > >>> Hi John,
> > >>>
> > >>> On Mon, 2024-03-25 at 20:51 -0500, John Bauer wrote:
> > >>>
> > >>> I understand this patch might not be the ideal or proper solution; but it works. I don't think the UVC
> > >>> implementation can be trusted on these cameras, just like the Windows DirectShow implementation is off.
> > >>> I put this patch out there as I have encountered many Linux users who are struggling to get proper
> > >>> control of these awesome cameras. If the patch dies here for now, that's OK, at least there's a possible
> > >>> patch for those in need.
> > >>>
> > >>>
> > >>> Sorry, maybe I didn't phrase it well. Based on the UVC specs, I think your patch is good for all UVC PTZ cameras, so you don't need to use UVC_QUIRK_OBSBOT_MIN_SETTINGS quirk entry, just apply the quirk changes to all cameras.
> > >>>
> > >>> Thanks for doing this!
> > >>>
> > >>> Regards,
> > >>> Gergo
> > >>>
> >
> >
> > --
> > Ricardo Ribalda



-- 
Ricardo Ribalda