Re: [PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 26 Jun 2022 18:51:54 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Jonathan,
> 
> Sorry for the delay in replying.
> 
> On Sun, Jun 19, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 14 Jun 2022 00:47:05 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > Add new event type for tap called gesture and the direction can be used
> > > to differentiate single and double tap. This may be used by accelerometer
> > > sensors to express single and double tap events. For directional tap,
> > > modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
> > > doubletap direction.
> > >
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> >
> > Hi,
> >
> > With fresh eyes I think we need to rethink the use of _period to make sure
> > we have 'space' for another very like ABI element which is the maximum
> > time between events for them to be considered a double tap.
> >
> > Jonathan
> >  
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
> > >  drivers/iio/industrialio-event.c        |  5 ++++-
> > >  include/uapi/linux/iio/types.h          |  3 +++
> > >  tools/iio/iio_event_monitor.c           |  8 +++++++-
> > >  4 files changed, 38 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 3e00d7f7ee22..4eaf85e01911 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -2035,3 +2035,27 @@ Description:
> > >               Available range for the forced calibration value, expressed as:
> > >
> > >               - a range specified as "[min step max]"
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_singletap_en
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_en
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Device generates an event on a single or double tap.
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_singletap_value
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_value
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Specifies the threshold value that the device is comparing
> > > +             against to generate the tap gesture event. Units and exact
> > > +             meaning of value are device specific.  
> >
> > I'm fine with this one being device specific, as likely a complex alg
> > involved.
> >  
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_period
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Minimum time period between before and after the double tap
> > > +             event. Units and exact meaning of period value are device
> > > +             specific.  
> >
> > I think the units need to be standard.  Also would this work better
> > as a description?
> >
> >         Minimum time in seconds between the two taps making up a double
> >         tap event.  
> 
> The values for the time between the two taps are not in terms of seconds,
> here period value is in terms of data samples which depends on
> the data rate.

These time based controls in IIO are always defined in seconds. You'll have
to correct for the data rate in the driver.

Either just make the available options change with data rate, or have the
driver do a 'nearest possible' choice based on what was requested and what
is possible at the current data rate.

It's more complex but it gives a consistent ABI across devices that work
in many different ways.

Jonathan

> 
> >
> > Raises a question though. How would we specify the maximum time? I.e.
> > if taps are further apart than Xseconds, they are reported as two single
> > taps.
> >
> > Maybe reusing period here isn't a good idea and we need to have new ABI for
> > this?
> >
> >
> >
> >