Received-SPF: pass (google.com: domain of linux-kernel+bounces-119175-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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> <CAMB8T1W5K68fX4jw=V8+kqc8eT2HGCv75PAidO0Nkzy-A1jFAQ@mail.gmail.com>
In-Reply-To: <CAMB8T1W5K68fX4jw=V8+kqc8eT2HGCv75PAidO0Nkzy-A1jFAQ@mail.gmail.com>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Tue, 26 Mar 2024 15:27:25 +0100
Message-ID: <CANiDSCu7qRnySQn=YJ=pkOOYe50H89CDw_AGu287LfjWebnnYQ@mail.gmail.com>
Subject: Re: [PATCH] uvcvideo: Remo OBSBOT quirk fix for incorrect relative
 min pan/tilt/zoom speeds
To: John Bauer <john@oxt.co>
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

Hi John

On Tue, 26 Mar 2024 at 09:15, John Bauer <john@oxt.co> 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=3D1XqWPROw49s

That looks pretty cool :)

Thanks!

>
> Thanks,
> John
>
> On Tue, Mar 26, 2024 at 2:47=E2=80=AFAM Ricardo Ribalda <ribalda@chromium=
org> 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 ch=
ecks are done in the getter and setter for the pan/tilt/zoom controls so th=
e only change needed is the 2 locations to get/check/set the minimum when n=
eeded. Thankfully all the code that does the hard work is already implement=
ed. 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> wro=
te:
> > >>
> > >> Ok, I get you now Gergo, I think I got lucky and I think you're righ=
t! Digging into the UVC 1.5 spec I can see why this works, the first byte i=
n 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     |                  D=
escription                   |
> > >> +--------+---------------+------+---------------+-------------------=
-----------------------------+
> > >> |      0 | bPanRelative  |    1 | Signed Number | 0: Stop, 1: clockw=
ise, 0xFF: counter-clockwise |
> > >> |      1 | bPanSpeed     |    1 | Number        | Speed of the Pan m=
ovement                      |
> > >> |      2 | bTiltRelative |    1 | Signed Number | 0: Stop, 1: tilt u=
p, 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 negat=
ive value in the direction byte we should probably check and make sure it c=
onforms 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>=
 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 Windo=
ws DirectShow implementation is off.
> > >>> I put this patch out there as I have encountered many Linux users w=
ho are struggling to get proper
> > >>> control of these awesome cameras. If the patch dies here for now, t=
hat'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 thi=
nk 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


--=20
Ricardo Ribalda