Re: [RFC PATCH v2 2/7] iio: core: Introduce IIO_ACTIVITY channel and TRANSITION event

[Daniel Baluta]

On Sat, Oct 25, 2014 at 10:01 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On 09/10/14 13:39, Daniel Baluta wrote:
>> This channel will be used for exposing information about
>> activity composite sensors. Activities supported so far:
>>       * running
>>       * jogging
>>       * walking
>>       * still
>>
>> TRANSITION event is used to signal a change in the activity
>> state.
>>
>> We associate a confidence interval for each activity expressed
>> as a percentage from 0 to 100.
>>   * 0, means the sensor IS NOT reporting that activity.
>>   * 100, means the sensor IS reporting that activity.
>>
>> Users of this interface have two possibile means to gather
>> information about the ongoing activities.
>>
>> 1. Event based, via event file descriptor
>>   * sensor may report an event when ENTERING an activity or LEAVING
>>     an activity based on a threshold value.
>>   * drivers will wake up applications waiting data on the event fd
>>
>> 2. Polling, by reading the sysfs associated attribute files:
>>   * /sys/bus/iio/devices/iio:device0/in_activity_running_input
>> expressed as percentage confidence value from 0 to 100.
>>
>> This will offer an interface for Android significant motion
>> composite sensor defined here:
>> http://source.android.com/devices/sensors/composite_sensors.html
>>
>> Activities listed above are supported by Freescale's MMA9553 sensor:
>> http://freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf
>>
>> Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
>> Signed-off-by: Daniel Baluta <daniel.baluta@xxxxxxxxx>
>
> I don't see why we need the transition event.  A transition is simply
> a threshold rising (or falling) event much like any other - just
> on a confidence interval.
>
> So it would be possible to be interested in the confidence that are not
> sitting rising above 20% on the basis that some other sensor needs enabling.
>

Using a THRESHOLD event type also work for us.

> See below for resulting suggestions.
>
> Other than that little point this looks good to me.
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio | 44 +++++++++++++++++++++++++++++++++
>>  drivers/iio/industrialio-core.c         |  5 ++++
>>  drivers/iio/industrialio-event.c        |  1 +
>>  include/linux/iio/types.h               |  8 +++++-
>>  4 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index d760b02..12d0385 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -776,6 +776,40 @@ Description:
>>               met before an event is generated. If direction is not
>>               specified then this period applies to both directions.
>>
>> +What:                /sys/.../events/in_activity_still_transition_rising_en
> What:   /sys/.../events/in_activity_threshold_rising_en

Shouldn't this be:

What:   /sys/.../events/in_activity_still_threshold_rising_en ?

Perhaps, one needs to enable events per activity type.

>> +What:                /sys/.../events/in_activity_still_transition_falling_en
> What:   /sys/.../events/in_activity_threshold_falling_en
Also, here:

What:   /sys/.../events/in_activity_still_threshold_falling_en

> etc
>> +What:                /sys/.../events/in_activity_walking_transition_rising_en
>> +What:                /sys/.../events/in_activity_walking_transition_falling_en
>> +What:                /sys/.../events/in_activity_jogging_transition_rising_en
>> +What:                /sys/.../events/in_activity_jogging_transition_falling_en
>> +What:                /sys/.../events/in_activity_running_transition_rising_en
>> +What:                /sys/.../events/in_activity_running_transition_falling_en
>> +KernelVersion:       3.19
>> +Contact:     linux-iio@xxxxxxxxxxxxxxx
>> +Description:
>> +             Enables or disables activitity events. Depending on direction
>> +             an event is generated when sensor ENTERS or LEAVES a given state.
>> +
>> +What:                /sys/.../events/in_activity_still_transition_rising_value
> What: /sys/.../events/in_activity_still_threshold_rising_value etc

This looks good to me :)

>> +What:                /sys/.../events/in_activity_still_transition_falling_value
>> +What:                /sys/.../events/in_activity_walking_transition_rising_value
>> +What:                /sys/.../events/in_activity_walking_transition_falling_value
>> +What:                /sys/.../events/in_activity_jogging_transition_rising_value
>> +What:                /sys/.../events/in_activity_jogging_transition_falling_value
>> +What:                /sys/.../events/in_activity_running_transition_rising_value
>> +What:                /sys/.../events/in_activity_running_transition_falling_value
>> +KernelVersion:       3.19
>> +Contact:     linux-iio@xxxxxxxxxxxxxxx
>> +Description:
>> +             Confidence value (in units as percentage) to be used
>> +             for deciding when an event should be generated. E.g for
>> +             running: If the confidence value reported by the sensor
>> +             is greater then in_activity_running_transition_rising_value
>> +             then the sensor ENTERS running state. Conversely, if the
>> +             confidence value reported by the sensor is lower than
>> +             in_activity_running_transition_rising_value then the sensor
>> +             is LEAVING running state.
>> +
>>  What:                /sys/.../iio:deviceX/events/in_accel_mag_en
>>  What:                /sys/.../iio:deviceX/events/in_accel_mag_rising_en
>>  What:                /sys/.../iio:deviceX/events/in_accel_mag_falling_en
>> @@ -942,6 +976,16 @@ Description:
>>               and the relevant _type attributes to establish the data storage
>>               format.
>>
>> +What:                /sys/.../iio:deviceX/in_activity_still_input
>> +What:                /sys/.../iio:deviceX/in_activity_walking_input
>> +What:                /sys/.../iio:deviceX/in_activity_jogging_input
>> +What:                /sys/.../iio:deviceX/in_activity_running_input
>> +KernelVersion:       3.19
>> +Contact:     linux-iio@xxxxxxxxxxxxxxx
>> +Description:
>> +             This attribute is used to read the confidence for an activity
>> +             expressed in units as percentage.
>> +
>>  What:                /sys/.../iio:deviceX/in_anglvel_z_quadrature_correction_raw
>>  KernelVersion:       2.6.38
>>  Contact:     linux-iio@xxxxxxxxxxxxxxx
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index af3e76d..e453ef9 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -70,6 +70,7 @@ static const char * const iio_chan_type_name_spec[] = {
>>       [IIO_CCT] = "cct",
>>       [IIO_PRESSURE] = "pressure",
>>       [IIO_HUMIDITYRELATIVE] = "humidityrelative",
>> +     [IIO_ACTIVITY] = "activity",
>>  };
>>
>>  static const char * const iio_modifier_names[] = {
>> @@ -91,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..afcf154 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_TRANSITION] = "transition",
>>  };
>>
>>  static const char * const iio_ev_dir_text[] = {
>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>> index 4a2af8a..a08ee6c 100644
>> --- a/include/linux/iio/types.h
>> +++ b/include/linux/iio/types.h
>> @@ -30,6 +30,7 @@ enum iio_chan_type {
>>       IIO_CCT,
>>       IIO_PRESSURE,
>>       IIO_HUMIDITYRELATIVE,
>> +     IIO_ACTIVITY,
>>  };
>>
>>  enum iio_modifier {
>> @@ -59,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 {
>> @@ -68,6 +73,7 @@ enum iio_event_type {
>>       IIO_EV_TYPE_ROC,
>>       IIO_EV_TYPE_THRESH_ADAPTIVE,
>>       IIO_EV_TYPE_MAG_ADAPTIVE,
>> +     IIO_EV_TYPE_TRANSITION,
>>  };
>>
>>  enum iio_event_info {
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/