Received-SPF: pass (google.com: domain of linux-kernel+bounces-118484-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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>
In-Reply-To: <CAMB8T1VWGaWtE0k5en4-xhE69G=OyFnhqJ3mexcgNSuvO_7uUQ@mail.gmail.com>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Tue, 26 Mar 2024 08:47:19 +0100
Message-ID: <CANiDSCvGfA=B5YCQhSMRW-0cTQNWKGytQF74z9F_x-77WFPHpw@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 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 checks=
 are done in the getter and setter for the pan/tilt/zoom controls so the on=
ly change needed is the 2 locations to get/check/set the minimum when neede=
d. 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! D=
igging into the UVC 1.5 spec I can see why this works, the first byte in ea=
ch 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     |                  Descr=
iption                   |
>> +--------+---------------+------+---------------+-----------------------=
-------------------------+
>> |      0 | bPanRelative  |    1 | Signed Number | 0: Stop, 1: clockwise,=
 0xFF: counter-clockwise |
>> |      1 | bPanSpeed     |    1 | Number        | Speed of the Pan movem=
ent                      |
>> |      2 | bTiltRelative |    1 | Signed Number | 0: Stop, 1: tilt up, 0=
xFF: tilt down           |
>> |      3 | bTiltSpeed    |    1 | Number        | Speed for the Tilt mov=
ement                    |
>> +--------+---------------+------+---------------+-----------------------=
-------------------------+
>>
>> 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 confo=
rms 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> wro=
te:
>>>
>>> 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 D=
irectShow implementation is off.
>>> I put this patch out there as I have encountered many Linux users who a=
re 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 y=
our patch is good for all UVC PTZ cameras, so you don't need to use UVC_QUI=
RK_OBSBOT_MIN_SETTINGS quirk entry, just apply the quirk changes to all cam=
eras.
>>>
>>> Thanks for doing this!
>>>
>>> Regards,
>>> Gergo
>>>


--
Ricardo Ribalda