Re: [RFC PATCH 3/8] iio: core: Introduce new MOTION event

[Jonathan Cameron <jic23@kernel.org>]

On 07/10/14 11:48, Daniel Baluta wrote:
> On Sat, Oct 4, 2014 at 4:12 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/10/14 14:43, Daniel Baluta wrote:
>>> This is to be used by drivers to signal detection of motion. We also
>>> add some possible values for motion as IIO events modifiers:
>>>       * running
>>>       * jogging
>>>       * walking
>>>       * still
>>>
>>> These values are supported by Frescale's MMA9553 sensor:
>>>
>>> http://freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>> Hmm.. This is the interesting one.
>> Not immediately obvious how best to represent this stuff.
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
>>>  drivers/iio/industrialio-core.c         | 4 ++++
>>>  drivers/iio/industrialio-event.c        | 1 +
>>>  include/linux/iio/types.h               | 7 ++++++-
>>>  4 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index d760b02..070346d 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -808,6 +808,13 @@ Description:
>>>               number or direction is not specified, applies to all channels of
>>>               this type.
>>>
>>> +What:                /sys/.../events/in_activity_motion_either_en
>>> +KernelVersion:       3.17
>>> +Contact:     linux-iio@vger.kernel.org
>>> +Description:
>>> +             Enables or disables motion detection. Each time motion is detected an
>>> +             event of this type will be generated.
>>> +
>> The either bit seems a bit random but I can see there is no particularly obvious
>> alternative.
>>
>> We really need a clean way of representing a multilevel 'state change' like this.
>>
>> Looking at the event code, I almost wonder if we would be better using the
>> direction element for running, walking etc rather than a modifier.
>>
>> Having said that we will probably also get devices where this is polled rather than
>> event.  'What activity is currently going on?'
>> If we take that view modifiers make sense as it becomes
>> 'Is the user running?'  Perhaps even offering a confidence interval, e.g units as
>> percentage
>> in_activity_running_input 0..100
>> in_activity_walking_input 0..100
>> etc
>>
> 
> This is clear.
> 
>> Then our event becomes a state change event (yup we'll need to add that)
> 
> So this means we will have a single event type named "state_change"?
> Or better "transition".
I was being rather incoherent here.  I think we are mostly covered
simple rising type events for the confidence interval measurements for
each of the activity_running, activity_walking etc 'modified channels'
we have above.

Don't need a state change.  If we actually have a device that interrupts on
simply the event has change, then the event handling will need to check what
the new state is and fire the relevant event.
> 
>>
>> /events/in_activity_walking_rising_en  will then cause events when the percentage
>> confidence on a state rises above the provided threshold or goes above it
>> (default of 50% perhaps on devices which only report one state).
> 
> Then here the attribute name will be:
> /events/in_activity_state_change_walking_rising_en
> 
> or if we call the event type "transition" this will be:
> /events/in_activity_transition_walking_rising_en
> 
> I am a little bit confused :).
I confused myself as well.

we should end up with
/events/in_activity_walking_rising_en
/events/in_activity_running_rising_en
/events/in_activity_hopping_rising_en
(I'll stop there before things get silly)

Each of which claims to based on the confidence level of that state, even if that
only ever takes values of 0 and 100%
> 
>>
>> /events/in_activity_walking_falling_en will do the leaving case.
>>
>> Note these are just some quick initial thoughts on alternative methods.
>> I'll want to think on this more and get responses from more interested
>> parties!
>>
>>>  What:                /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger
>>>  KernelVersion:       2.6.35
>>>  Contact:     linux-iio@vger.kernel.org
>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>> index 67e8561..e453ef9 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -92,6 +92,10 @@ static const char * const iio_modifier_names[] = {
>>>       [IIO_MOD_NORTH_TRUE] = "from_north_true",
>>>       [IIO_MOD_NORTH_MAGN_TILT_COMP] = "from_north_magnetic_tilt_comp",
>>>       [IIO_MOD_NORTH_TRUE_TILT_COMP] = "from_north_true_tilt_comp",
>>> +     [IIO_MOD_RUNNING] = "running",
>>> +     [IIO_MOD_JOGGING] = "jogging",
>>> +     [IIO_MOD_WALKING] = "walking",
>>> +     [IIO_MOD_STILL] = "still",
>>>  };
>>>
>>>  /* relies on pairs of these shared then separate */
>>> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
>>> index 0c1e37e..eca5af2 100644
>>> --- a/drivers/iio/industrialio-event.c
>>> +++ b/drivers/iio/industrialio-event.c
>>> @@ -197,6 +197,7 @@ static const char * const iio_ev_type_text[] = {
>>>       [IIO_EV_TYPE_ROC] = "roc",
>>>       [IIO_EV_TYPE_THRESH_ADAPTIVE] = "thresh_adaptive",
>>>       [IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
>>> +     [IIO_EV_TYPE_MOTION] = "motion",
>>>  };
>>>
>>>  static const char * const iio_ev_dir_text[] = {
>>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>>> index d58769a..003638d 100644
>>> --- a/include/linux/iio/types.h
>>> +++ b/include/linux/iio/types.h
>>> @@ -60,7 +60,11 @@ enum iio_modifier {
>>>       IIO_MOD_NORTH_MAGN,
>>>       IIO_MOD_NORTH_TRUE,
>>>       IIO_MOD_NORTH_MAGN_TILT_COMP,
>>> -     IIO_MOD_NORTH_TRUE_TILT_COMP
>>> +     IIO_MOD_NORTH_TRUE_TILT_COMP,
>>> +     IIO_MOD_RUNNING,
>>> +     IIO_MOD_JOGGING,
>>> +     IIO_MOD_WALKING,
>>> +     IIO_MOD_STILL,
>>>  };
>>>
>>>  enum iio_event_type {
>>> @@ -69,6 +73,7 @@ enum iio_event_type {
>>>       IIO_EV_TYPE_ROC,
>>>       IIO_EV_TYPE_THRESH_ADAPTIVE,
>>>       IIO_EV_TYPE_MAG_ADAPTIVE,
>>> +     IIO_EV_TYPE_MOTION,
>>>  };
>>>
>>>  enum iio_event_info {
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html