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

[Saul Wold]

On Mon, 2016-06-13 at 10:00 -0700, Jianxun Zhang wrote:
> > 
> > On Jun 13, 2016, at 8:25 AM, Saul Wold <sgw at linux.intel.com> 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 <jianxun.zhang at linux.intel.com>
> > > ---
> > > 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 0000000..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.
> 
Jianxun,

While I understand the logic, I think it's would be cleaner to have the
actual test separated out that way it's obvious why the script is
bailing out.

We look forward to v4.

Thanks
Sau!

> 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 0000000..72d991c
> > > --- /dev/null
> > > +++ b/common/recipes-bsp/formfactor/formfactor_0.0.bbappend
> > > @@ -0,0 +1 @@
> > > +FILESEXTRAPATHS_prepend := "${THISDIR}/${PN}:"