Re: [PATCH v7 08/11] iio: accel: adxl345: add activity event feature

[Jonathan Cameron]

On Sun, 4 May 2025 19:47:55 +0200
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> On Sun, May 4, 2025 at 12:29 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Thu, 1 May 2025 00:53:32 +0200
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >  
> > > Hi Jonathan - Hi IIO list,
> > >
> > > Please, find some (many) questions inlined down below. Appologies for
> > > the separate
> > > channels last time and not right away fixing them up as array. I did
> > > not want to make extra work.
> > >
> > > On Sun, Apr 27, 2025 at 2:48 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:  
> > > >
> > > > On Mon, 21 Apr 2025 22:06:38 +0000
> > > > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> > > >  
> > > > > Make the sensor detect and issue interrupts at activity. Activity
> > > > > events are configured by a threshold stored in regmap cache. Initialize
> > > > > the activity threshold register to a reasonable default value in probe.
> > > > > The value is taken from the older ADXL345 input driver, to provide a
> > > > > similar behavior. Reset the activity/inactivity direction enabling
> > > > > register in probe. Reset and initialization shall bring the sensor in a
> > > > > defined initial state to prevent dangling settings when warm restarting
> > > > > the sensor.
> > > > >
> > > > > Activity, ODR configuration together with the range setting prepare the
> > > > > activity/inactivity hystersesis setup, implemented in a follow up patch.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/iio/accel/adxl345_core.c | 217 ++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 214 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > > index 80b5b8402ced..680981609d83 100644
> > > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > > @@ -36,11 +36,16 @@
> > > > >  #define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)
> > > > >  #define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
> > > > >  #define ADXL345_REG_TAP_SUPPRESS     BIT(3)
> > > > > +#define ADXL345_REG_ACT_AXIS_MSK     GENMASK(6, 4)
> > > > >
> > > > >  #define ADXL345_TAP_Z_EN             BIT(0)
> > > > >  #define ADXL345_TAP_Y_EN             BIT(1)
> > > > >  #define ADXL345_TAP_X_EN             BIT(2)
> > > > >
> > > > > +#define ADXL345_ACT_Z_EN             BIT(4)
> > > > > +#define ADXL345_ACT_Y_EN             BIT(5)
> > > > > +#define ADXL345_ACT_X_EN             BIT(6)
> > > > > +
> > > > >  /* single/double tap */
> > > > >  enum adxl345_tap_type {
> > > > >       ADXL345_SINGLE_TAP,
> > > > > @@ -64,6 +69,19 @@ static const unsigned int adxl345_tap_time_reg[] = {
> > > > >       [ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
> > > > >  };
> > > > >
> > > > > +/* activity/inactivity */
> > > > > +enum adxl345_activity_type {
> > > > > +     ADXL345_ACTIVITY,
> > > > > +};
> > > > > +
> > > > > +static const unsigned int adxl345_act_int_reg[] = {
> > > > > +     [ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
> > > > > +};
> > > > > +
> > > > > +static const unsigned int adxl345_act_thresh_reg[] = {
> > > > > +     [ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
> > > > > +};
> > > > > +
> > > > >  enum adxl345_odr {
> > > > >       ADXL345_ODR_0P10HZ = 0,
> > > > >       ADXL345_ODR_0P20HZ,
> > > > > @@ -154,6 +172,13 @@ struct adxl345_state {
> > > > >  };
> > > > >
> > > > >  static struct iio_event_spec adxl345_events[] = {
> > > > > +     {
> > > > > +             /* activity */
> > > > > +             .type = IIO_EV_TYPE_THRESH,  
> > > >
> > > > Is this a threshold, or a magnitude? I'd expect an activity detector
> > > > to be magnitude as it doesn't care which way up the sensor is.
> > > >  
> > >
> > > This is touching the main points still unclear to me. I tried to put
> > > this into the
> > > following questions. Could you please clarify?  
> >
> > There are some corners where it gets messy. When I have time
> > (not for a month or so) I'll try and write some proper docs for this.
> >  
> > >
> > > 1. Given a measurement "val", and a configured threshold "thr".
> > > A "rising" for IIO_EV_TYPE_THRESH means: val > thr
> > > where a "rising" for IIO_EV_TYPE_MAG means something like: val > |thr|
> > >
> > > Q: Do I understand this correctly now?  
> >
> > Yes that is the intended difference.
> >  
> > >
> > > Q: Is this documented somewhere (especially for reviewing further
> > > EV_TYPE fields)?  
> >
> > Only in the ABI documentation in
> > Documentation/ABI/testing/sysfs-bus-iio
> > This is definitely something we should look to improve with some
> > docs beyond simply what the ABI is.  That ABI is focused on
> > how the interrupt is triggered, not so much on what that means
> > wrt to freefall etc.
> >
> >  
> > >
> > > Q: I wonder if I missed this for the Tap events. Going by this
> > > definition, then actually the
> > > tap events should be rather MAG events, too. Right?  
> >
> > The tap events have their own type (gesture) because they are way
> > more complex than a simple threshold whether on magnitude or
> > the signed value.  So those should be fine as type GESTURE.
> >  
> 
> I was aware of that. Actually, the case of GESTURE is a bit
> particular. On the one side, I
> understand having a distinction between THRESH events and MAG events.
> Hence, a classification of the type of event in terms of a measurement
> value triggering event condition.
> This concept seems actually to be clear.
> 
> GESTURE to me then seems a bit like a "wildcard type covering all kind
> of tap events". I mean,
> saying tap detection, single tap, double tap, tripple tap, and so on
> tap go into category GESTURE - naively could also mean, then do a
> freefall  type as well (?).

Nope. Because freefall has a clear definition that aligns with
the events that we have for other types of sensor.

You are right that gesture is a wild card. I resisted it for a long
time but there is just no sane way to handle tap events alongside
the sort of things we get on any other sensor type.  Having looked
at a bunch of them they can be anything from straight magnitude thresholds
with time windows to things based on a mixture of jerk (rate of change
of acceleration) and other stuff.  The only thing that kind of close
to this is pedometer step events, but we handle those as a counting
channel rather than an event as time of each is less interesting than
how many have happened.  However as noted below we do have the CHANGE
type of event specifically to account for those (which is ugly).

> 
> [this is rather meant as a bit of a provocative rhetoric question than
> a proposal]
> 
> > >
> > >
> > > 2. I oriented myself mostly by reading other drivers, for instance the
> > > ADXL367, the ADXL372, or also the more recent ADXL380. I am aware that
> > > there might be differences among different
> > > (Analog) sensors. But all those sensors specify Inactivity (and Activity) as a
> > > IIO_EV_TYPE_THRESH with directions IIO_MOD_X_OR_Y_OR_Z.
> > > Given the above, I implemented Activity and Inactivity events as
> > > IIO_EV_TYPE_THRESH,
> > > now I'm a bit confused.  
> >
> > Hmm. This is one reason I think we need more documentation as those
> > seem to be wrong.  Clearly the event is a threshold on a magnitude of
> > the acceleration, not the signed value as it applies in both directions.
> >  
> > >
> > > Q: Why is this different for the ADXL345?  
> >
> > Because we got it wrong for these others it seems unless they genuinely
> > have directional events - which typically means separate positive and
> > negative thresholds.  Right now those events are strictly speaking
> > only apply to positive accelerations.
> >  
> > >
> > > Q: If I implement Activity / Inactivity analogous to the e.g. a
> > > ADXL380, then shouldn't it be IIO_EV_TYPE_THRESH with
> > > IIO_MOD_X_OR_Y_OR_Z? Why not?
> > >  
> >
> > I think we got it wrong for that part.  Going forwards we should work
> > on getting it (more) correct.
> >  
> 
> I understand the point better now.
> 
> > >
> > > 3. For the ADXL345, a Freefall signal is on all axis lower than
> > > threshold (magnitude). Thus I push a IIO_MOD_X_AND_Y_AND_Z to a
> > > separate
> > > fake channel. Inactivity will be like Freefall independent of the axis.
> > > The ADXL345 Activity can be configured by axis, as also the event will
> > > respect the axis information.
> > >
> > > Q: Setting up the "fake channel" to particuarly push to
> > > IIO_MOD_X_AND_Y_AND_Z, I probably better should also evaluate
> > > IIO_MOD_X_AND_Y_AND_Z in write_event_config(), write_event_value(),
> > > etc. rather than evaluating IIO_MOD_-types as I'm currently
> > > doing?  
> >
> > Yes. That sounds correct for events on these 'fake' channels.
> > The enable and the thresholds should all be on these fake channels
> > assuming they don't have different thresholds on a per axis basis
> > (if they do things get tricky to represent).
> >  
> > >
> > > Q: Activity probably remains in the regular channels for the corresponding axis?  
> >
> > Yes.  That is easier to handle as OR of channels is very similar
> > to separate interrupts etc.
> >  
> 
> I think I should definitely evaluate the IIO_MOD_X_AND_Y_AND_Z here,
> to take advantage
> of the fake channel.
> 
> > >
> > >
> > > 4. I implemented functions like adxl345_write_event_config(),
> > > adxl345_write_event_value() or corresponding
> > > readers, as follows
> > > - THRESH/rising: Activity
> > > - THRESH/falling: Inactivity
> > > - MAG/falling: Freefall
> > >
> > > If I change Activity and Inactivity to be both of type MAG, I will end
> > > up with MAG/falling to indicate Freefall or equally Inactivity.
> > > Both on the IIO_MOD_X_AND_Y_AND_Z channel. I admit (ab)using the
> > > IIO_EV_TYPEs to solve my combinatorial issues for event configuration
> > > is probably not as supposed to be.  
> >
> > Ah..  This I'd missed.  I'm fairly sure we didn't hit this for (some) previous
> > inactivity sensors because they were always rate of change based, (AC)
> > rather than DC. DC is relatively unlikely to be used in practice because
> > we can't set the threshold as less than 1G because of gravity.  It is a
> > bit odd that the device supports both DC and AC on this detector.
> >
> > I wonder why.... Might be to enable partial axis monitoring.  e.g.
> > If a device is flat on a table we only look for inactivity on the non
> > vertical axis when doing DC coupling. (as we have 1g always in the other
> > axis).
> >  
> 
> Thank you for clarifying your position in the other mail focussed on
> the AC- / DC-coupling
> topic. It helped me in better understanding what you actually expect
> here. Although I'll
> probably need to re-read it some times, before implementing something.

I definitely need to find time to write some docs on this.  Mad few
weeks coming up but maybe I'll get some time on a plane or at an airport
to try a first draft.

> 
> > > Given you still ask me to do Inactivity and Freefall as MAG/falling
> > > with IIO_MOD_X_AND_Y_AND_Z. The difference between both IMHO,
> > > is that Activity and Inactivity for the ADXL345 indicate sensor state
> > > changes, where Freefall indicates the particular event. The
> > > sensor is either in "active" or "standby/inactive", where Freefall
> > > just triggers and then retriggers and retriggers...  
> >
> > Maybe. The datasheet is annoyingly vague on these but indeed there
> > is no event for no longer falling.
> >  
> > >
> > > Q: What is the method to distinguish several similar IIO events, e.g.
> > > to tag them somehow one as Freefall, the other one as Inactivity?  
> >
> > In general we should not be able to do that.  Long ago we made the decision
> > to have compact event codes so they don't allow for an index on a particular
> > combination of channel number and modifier.  This is mainly because
> > there is limited purpose.   If one event is triggered, then we have
> > to process anyway so we can just look at the value for 'how far' it was
> > triggered.  I.e. if we thought DC inactivity was triggered, we can just
> > check free fall as well. (It gets a little more fiddly because of _period
> > etc which is why they may actually make sense here).
> >
> > The virtual (combination OR/AND) was added on top of that later and has
> > made the connection looser.
> >
> > In theory we could use labels + index for the virtual channels to achieve
> > separate control attributes and be able to tell which was which but
> > that would be new ABI.  I'm not sure how much use this stuff is already
> > getting from userspace applications and hence whether this would be
> > a big problem to get supported.
> >
> > That would give us something like
> >
> > iio\:device0/in_accel0_x&y&z_label   freefall
> > iio\:device0/in_accel1_x&y&z_label   inactivity
> > iio\:device0/events/in_accel0_x&y&z_en etc
> > iio\:device0/events/in_accel1_x&y&z_en etc
> >
> > I don't like it much because it then doesn't generalize to the case
> > of multiple sensors on each axis (there are multi range parts that do that).
> > That case is pretty rare though (I think we only have such sensor supported!)
> > However, it's currently the only option we have to fully represent this.
> >
> > An alternative here might be to assess if anyone is really going to use
> > DC coupled inactivity detection (because of the 1g problem) and hence whether
> > we want to support that at all?
> >
> > Yet another alternative might be to configure it purely based on the period
> > provided. If short use freefall, if long use inactivity. (I don't like this
> > one though as it doesn't really fit with usecase!)
> >
> > Sorry for lack of clarity on this. These events are tricky and
> > it takes me a while to get the whole situation back into my head (and I missing
> > things like inactivity and freefall being very similar here!)
> >
> > If you have time to take a look at what userspace is currently doing with
> > these events (iio_sensor_proxy etc) that might help us decide what works.
> >  
> 
> Just as a quick response here (or perhaps just to rule it out)..
> 
> Actually, I can spot as MAG-similar event types:
> - IIO_EV_TYPE_MAG
> - IIO_EV_TYPE_MAG_ADAPTIVE
> - IIO_EV_TYPE_CHANGE
> - IIO_EV_TYPE_MAG_REFERENCED
> 
> For instance the last one is only used in a single sensor. Is there a
> chance to put, say, freefall into one of the other "MAG-like" sensor
> types. Alternatively, what about putting Activity/Inactivity under
> say, "MAG_REFERENCED"? This might seem to be a stupid question, since
> I can imagine you have a clear definition of those in mind. But if
> this was possible. It would solve this problem easily.

The ABI docs do provide some definitions of these.

Free fall is definitely straight forward TYPE MAG.  It precisely aligns
with that definition as a threshold on the per axis magnitudes.

MAG_ADAPTIVE is meant for a case where the event is on the magnitude
relative to a slow moving adaptive baseline (usually a low pass filtered
version of the signal but can include corrective jumps - IIRC these turn
up for magnetic sensors).  This differs from a rate of change threshold
because it's not simply a difference between current and earlier signals
but rather current and some heavily filtered earlier signal.
These matter in cases where we have a slow changing baseline such as
coming into proximity with metal in the environment when using a magnetometer
for orientation detection.

MAG_REFERENCED is a weird one.  This was done for a nice IMU that had
the ability to estimate orientation and so remove the acceleration due
to gravity and then apply thresholds to the magnitude of the remaining
accelerations.  The AC filtering on your part is is a 'cheap' way
to achieve roughly the equivalent of that for the activity detection at least
where we are removing the 'nothing happening value'.
(it is less clear for the inactivity case though that will still include
 g, so maybe it is still somewhat valid).

CHANGE is IIRC only for counting channels (so far anyway). 

So none of the more esoteric forms of them fit for this DC coupled
inactivity monitor or freefall. Both of them are the same type of event
just differing in filters applied.

> 
> If not, then I'll need to think of it and come up with a more
> elaborate approach. The label + index approach seems to be a bit
> complex. Going somehow by the time constraints  in the event.. I need
> to play with that in the code to build up an oppinion, I guess.

The time constraints thing falls down on the basis that it would
be logical to have freefall enabled (for parking any moving parts - those
used to exist mainly to stop hard disks but maybe there are other use cases?)
and inactivity for power saving with a much longer timescale.

Freefall used to be fun because the aim was to get moving parts into
a safe state before the device hit the ground.  So that meant if you dropped
a device with a harddisk from higher up, it sometimes had a better chance
of surviving.  I'm not sure if anyone cares any more! Will be interesting
to see if that feature goes away on new devices.

Jonathan

> 
> Best,
> L
> 
> > Jonathan
> >  
> > >
> > > Best,
> > > L
> > >  
> > > > > +             .dir = IIO_EV_DIR_RISING,
> > > > > +             .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> > > > > +     },
> > > > >       {
> > > > >               /* single tap */
> > > > >               .type = IIO_EV_TYPE_GESTURE,
> > > > > @@ -265,6 +290,99 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > > > >       return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > > > >  }
> > > > >  
> > > > Jonathan
> > > >
> > > >  
> >