Received-SPF: pass (google.com: domain of linux-kernel+bounces-118523-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240325-obsbot-quirk-fix-relative-ptz-speed-v1-1-0eb1387d98c7@securitylive.com>
 <6e6b75a15cdc6a1239edc4d49b927b187ed20054.camel@irl.hu> <CAMB8T1ULcfBOB5VwZzUtvRnp4FvtBCFWxxTdb+OJK8FOpjKCXA@mail.gmail.com>
 <ec0430f6e687fc5e1a19338e381804b9d6aeabba.camel@irl.hu> <CAMB8T1Vv7CMqhH1ZY6fouxsE6r+6JbmeLnuFma_0_de814UoMA@mail.gmail.com>
 <CAMB8T1VWGaWtE0k5en4-xhE69G=OyFnhqJ3mexcgNSuvO_7uUQ@mail.gmail.com> <CANiDSCvGfA=B5YCQhSMRW-0cTQNWKGytQF74z9F_x-77WFPHpw@mail.gmail.com>
In-Reply-To: <CANiDSCvGfA=B5YCQhSMRW-0cTQNWKGytQF74z9F_x-77WFPHpw@mail.gmail.com>
From: John Bauer <john@oxt.co>
Date: Tue, 26 Mar 2024 03:15:09 -0500
Message-ID: <CAMB8T1W5K68fX4jw=V8+kqc8eT2HGCv75PAidO0Nkzy-A1jFAQ@mail.gmail.com>
Subject: Re: [PATCH] uvcvideo: Remo OBSBOT quirk fix for incorrect relative
 min pan/tilt/zoom speeds
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Gergo Koteles <soyer@irl.hu>, johnebgood@securitylive.com, 
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linh.tp.vu@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=3D1XqWPROw49s

Thanks,
John

On Tue, Mar 26, 2024 at 2:47=E2=80=AFAM Ricardo Ribalda <ribalda@chromium.o=
rg> wrote:
>
> Hi Jon, Hi Gergo
>
> On Tue, 26 Mar 2024 at 07:23, John Bauer <john@oxt.co> wrote:
> >
> > After looking through the current implementation all of the proper chec=
ks 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 nee=
ded. 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=E2=80=AFPM John Bauer <john@oxt.co> 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 se=
t when a negative value is applied to both bytes so there should probably b=
e some checks.
> >>
> >> Here from the UVC 1.5 spec:  CT_PANTILT_RELATIVE_CONTROL
> >> +--------+---------------+------+---------------+---------------------=
---------------------------+
> >> | Offset |     Field     | Size |     Value     |                  Des=
cription                   |
> >> +--------+---------------+------+---------------+---------------------=
---------------------------+
> >> |      0 | bPanRelative  |    1 | Signed Number | 0: Stop, 1: clockwis=
e, 0xFF: counter-clockwise |
> >> |      1 | bPanSpeed     |    1 | Number        | Speed of the Pan mov=
ement                      |
> >> |      2 | bTiltRelative |    1 | Signed Number | 0: Stop, 1: tilt up,=
 0xFF: tilt down           |
> >> |      3 | bTiltSpeed    |    1 | Number        | Speed for the Tilt m=
ovement                    |
> >> +--------+---------------+------+---------------+---------------------=
---------------------------+
> >>
> >> I think it is the direction of the original implementation which is wa=
y 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 negativ=
e value in the direction byte we should probably check and make sure it con=
forms to spec with 0xFF for counter clockwise and down.
> >>
> >> In the current implementation both pan and tilt each use 2 bytes:
> >>
> >> {
> >> .id =3D V4L2_CID_PAN_SPEED,
> >> .entity =3D UVC_GUID_UVC_CAMERA,
> >> .selector =3D UVC_CT_PANTILT_RELATIVE_CONTROL,
> >> .size =3D 16,
> >> .offset =3D 0,
> >> .v4l2_type =3D V4L2_CTRL_TYPE_INTEGER,
> >> .data_type =3D UVC_CTRL_DATA_TYPE_SIGNED,
> >> .get =3D uvc_ctrl_get_rel_speed,
> >> .set =3D uvc_ctrl_set_rel_speed,
> >> },
> >> {
> >> .id =3D V4L2_CID_TILT_SPEED,
> >> .entity =3D UVC_GUID_UVC_CAMERA,
> >> .selector =3D UVC_CT_PANTILT_RELATIVE_CONTROL,
> >> .size =3D 16,
> >> .offset =3D 16,
> >> .v4l2_type =3D V4L2_CTRL_TYPE_INTEGER,
> >> .data_type =3D UVC_CTRL_DATA_TYPE_SIGNED,
> >> .get =3D uvc_ctrl_get_rel_speed,
> >> .set =3D uvc_ctrl_set_rel_speed,
> >> },
> >>
> >> Going to do some testing and report back.
> >>
> >> Thanks,
> >> John
> >>
> >>
> >>
> >> On Mon, Mar 25, 2024 at 9:23=E2=80=AFPM Gergo Koteles <soyer@irl.hu> w=
rote:
> >>>
> >>> 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; bu=
t 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, tha=
t'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_Q=
UIRK_OBSBOT_MIN_SETTINGS quirk entry, just apply the quirk changes to all c=
ameras.
> >>>
> >>> Thanks for doing this!
> >>>
> >>> Regards,
> >>> Gergo
> >>>
>
>
> --
> Ricardo Ribalda