Re: [meta-intel] [PATCH V3 1/1] formfactor: detect USB HID keyboard and touch screen

2016-06-13 Thread Jianxun Zhang

> On Jun 13, 2016, at 8:25 AM, Saul Wold  wrote:
> 
> On Fri, 2016-06-10 at 13:17 -0700, Jianxun Zhang wrote:
>> The new machconfig probes USB keyboard and touch screen, and
>> then sets HAVE_* variables according to detection.
>> 
>> Detectable devices:
>> USB HID keyboards (Generic Desktop)
>> USB HID touch screens (Digitizer)
>> 
>> Note:
>> The intention is to have a way to provide initial formfactor
>> settings in a boot procedure. That means supported keyboard
>> and touch screen must be connected before machconfig runs.
>> Any new connection or disconnection won't be detected until
>> machconfig is executed again.
>> 
>> Limitation:
>> There could be some USB HID devices presents more than one
>> usage in a single descriptor. We will add support once such
>> device emerges.
>> 
>> Some platforms may have _virtual_ devices provided by BIOS.
>> It will cause false detection when they are presented as
>> types we supported. We can add black list logic when it
>> becomes a big concern.
>> 
>> Fixes [YOCTO #9205]
>> 
>> Signed-off-by: Jianxun Zhang 
>> ---
>> Tom & Saul,
>> V3 puts entire logic in machconfig into a new if block, so
>> that detection will be skipped when any error occurs at the
>> first step. Same basic test is done as before.
>> 
>> Let me know if this version makes more sense.
>> 
>> Thanks
>> 
>>  .../recipes-bsp/formfactor/formfactor/machconfig   | 37
>> ++
>>  .../recipes-bsp/formfactor/formfactor_0.0.bbappend |  1 +
>>  2 files changed, 38 insertions(+)
>>  create mode 100644 common/recipes-
>> bsp/formfactor/formfactor/machconfig
>>  create mode 100644 common/recipes-
>> bsp/formfactor/formfactor_0.0.bbappend
>> 
>> diff --git a/common/recipes-bsp/formfactor/formfactor/machconfig
>> b/common/recipes-bsp/formfactor/formfactor/machconfig
>> new file mode 100644
>> index 000..22d3112
>> --- /dev/null
>> +++ b/common/recipes-bsp/formfactor/formfactor/machconfig
>> @@ -0,0 +1,37 @@
>> +# Note: super user permission is required to run usbhid-dump
>> +# successfully.
>> +
>> +# HEX keys are according to USB HID spec and USB HID usage table
>> +# We could add more keys as needed in the future.
>> +
>> +# It may not be very accurate. Here we only look for first two lines
>> +# of a descriptor section. Example:
>> +#
>> +# 001:003:000:DESCRIPTOR 1460501386.337809
>> +#  05 01 09 02 A1 01 09 01 A1 00 05 09 19 01 29 03
>> +#  15 00 25 01 95 03 75 01 81 02 .. .. .. .. .. ..
>> +#  .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
>> +#
>> +# By doing so we elimiate false matches when HEX keys are in the
>> lines
>> +# in the middle of whole descriptor section.
>> +
>> +if USBHID_DUMP_OUTPUT=$(usbhid-dump -e descriptor 2>/dev/null|grep
>> -A1 DESCRIPTOR); then
> Jianxun, 
> 
> I think you missed the intent of the if test, while this could work
> it's not clear from this line why the "if" succeeds or fails.  Both Tom
> and I suggested checking for the existence of usbhid-dump first, that
> way it's clear why the rest of the code is not used.
> 

Saul,
I understand you want to check usbhid-dump’ s existence first, but then I 
realize I should skip whole detection for any other errors too (a bigger issue 
in V1&2). That’s why I combined errors into single command in my mind. The 
logic (well, implicitly) covers that error case already so I don’t need another 
check. What we missed when cmd doesn’t exist, is the error code which I thought 
we don’t really care.

I will revise it with a check of cmd existence first, just let you know my 
thoughts behind this V3.

Thanks

> Thanks
>Sau!
> 
>> +# checker for generic USB HID keyboard
>> +USBHID_KBD_CMD="grep -E '^ 05 01 09 06'"
>> +
>> +# checker for touch screen
>> +USBHID_TS_CMD="grep -E '^ 05 0D 09 04'"
>> +
>> +if echo "$USBHID_DUMP_OUTPUT"|eval $USBHID_TS_CMD &>/dev/null;
>> then
>> +HAVE_TOUCHSCREEN=1
>> +fi
>> +
>> +if echo "$USBHID_DUMP_OUTPUT"|eval $USBHID_KBD_CMD &>/dev/null;
>> then
>> +HAVE_KEYBOARD=1
>> +else
>> +# config script in OE will set HAVE_KEYBOARD=1
>> +# if we don't set any value. We have to explicitly
>> +# tell it when keyboard is not detected.
>> +HAVE_KEYBOARD=0
>> +fi
>> +fi
>> diff --git a/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
>> b/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
>> new file mode 100644
>> index 000..72d991c
>> --- /dev/null
>> +++ b/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
>> @@ -0,0 +1 @@
>> +FILESEXTRAPATHS_prepend := "${THISDIR}/${PN}:"

-- 
___
meta-intel mailing list
meta-intel@yoctoproject.org
https://lists.yoctoproject.org/listinfo/meta-intel


Re: [meta-intel] [PATCH V3 1/1] formfactor: detect USB HID keyboard and touch screen

2016-06-13 Thread Saul Wold
On Fri, 2016-06-10 at 13:17 -0700, Jianxun Zhang wrote:
> The new machconfig probes USB keyboard and touch screen, and
> then sets HAVE_* variables according to detection.
> 
> Detectable devices:
> USB HID keyboards (Generic Desktop)
> USB HID touch screens (Digitizer)
> 
> Note:
> The intention is to have a way to provide initial formfactor
> settings in a boot procedure. That means supported keyboard
> and touch screen must be connected before machconfig runs.
> Any new connection or disconnection won't be detected until
> machconfig is executed again.
> 
> Limitation:
> There could be some USB HID devices presents more than one
> usage in a single descriptor. We will add support once such
> device emerges.
> 
> Some platforms may have _virtual_ devices provided by BIOS.
> It will cause false detection when they are presented as
> types we supported. We can add black list logic when it
> becomes a big concern.
> 
> Fixes [YOCTO #9205]
> 
> Signed-off-by: Jianxun Zhang 
> ---
> Tom & Saul,
> V3 puts entire logic in machconfig into a new if block, so
> that detection will be skipped when any error occurs at the
> first step. Same basic test is done as before.
> 
> Let me know if this version makes more sense.
> 
> Thanks
> 
>  .../recipes-bsp/formfactor/formfactor/machconfig   | 37
> ++
>  .../recipes-bsp/formfactor/formfactor_0.0.bbappend |  1 +
>  2 files changed, 38 insertions(+)
>  create mode 100644 common/recipes-
> bsp/formfactor/formfactor/machconfig
>  create mode 100644 common/recipes-
> bsp/formfactor/formfactor_0.0.bbappend
> 
> diff --git a/common/recipes-bsp/formfactor/formfactor/machconfig
> b/common/recipes-bsp/formfactor/formfactor/machconfig
> new file mode 100644
> index 000..22d3112
> --- /dev/null
> +++ b/common/recipes-bsp/formfactor/formfactor/machconfig
> @@ -0,0 +1,37 @@
> +# Note: super user permission is required to run usbhid-dump
> +# successfully.
> +
> +# HEX keys are according to USB HID spec and USB HID usage table
> +# We could add more keys as needed in the future.
> +
> +# It may not be very accurate. Here we only look for first two lines
> +# of a descriptor section. Example:
> +#
> +# 001:003:000:DESCRIPTOR 1460501386.337809
> +#  05 01 09 02 A1 01 09 01 A1 00 05 09 19 01 29 03
> +#  15 00 25 01 95 03 75 01 81 02 .. .. .. .. .. ..
> +#  .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
> +#
> +# By doing so we elimiate false matches when HEX keys are in the
> lines
> +# in the middle of whole descriptor section.
> +
> +if USBHID_DUMP_OUTPUT=$(usbhid-dump -e descriptor 2>/dev/null|grep
> -A1 DESCRIPTOR); then
Jianxun, 

I think you missed the intent of the if test, while this could work
it's not clear from this line why the "if" succeeds or fails.  Both Tom
and I suggested checking for the existence of usbhid-dump first, that
way it's clear why the rest of the code is not used.

Thanks
   Sau!

> +# checker for generic USB HID keyboard
> +USBHID_KBD_CMD="grep -E '^ 05 01 09 06'"
> +
> +# checker for touch screen
> +USBHID_TS_CMD="grep -E '^ 05 0D 09 04'"
> +
> +if echo "$USBHID_DUMP_OUTPUT"|eval $USBHID_TS_CMD &>/dev/null;
> then
> + HAVE_TOUCHSCREEN=1
> +fi
> +
> +if echo "$USBHID_DUMP_OUTPUT"|eval $USBHID_KBD_CMD &>/dev/null;
> then
> + HAVE_KEYBOARD=1
> +else
> + # config script in OE will set HAVE_KEYBOARD=1
> + # if we don't set any value. We have to explicitly
> + # tell it when keyboard is not detected.
> + HAVE_KEYBOARD=0
> +fi
> +fi
> diff --git a/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
> b/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
> new file mode 100644
> index 000..72d991c
> --- /dev/null
> +++ b/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
> @@ -0,0 +1 @@
> +FILESEXTRAPATHS_prepend := "${THISDIR}/${PN}:"
-- 
___
meta-intel mailing list
meta-intel@yoctoproject.org
https://lists.yoctoproject.org/listinfo/meta-intel