Message-ID: <20250507204847.01807b28@jic23-huawei>
Date: Wed, 7 May 2025 20:48:47 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 eraretuya@...il.com
Subject: Re: [PATCH v7 08/11] iio: accel: adxl345: add activity event
 feature

On Wed, 7 May 2025 00:37:43 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Hi Jonathan,
> Still some questions and thoughts down below.
> 
> On Mon, May 5, 2025 at 2:37 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Sun, 4 May 2025 19:47:55 +0200
> > Lothar Rubusch <l.rubusch@...il.com> wrote:
> >  
> > > On Sun, May 4, 2025 at 12:29 PM Jonathan Cameron <jic23@...nel.org> wrote:  
> > > >
> > > > On Thu, 1 May 2025 00:53:32 +0200
> > > > Lothar Rubusch <l.rubusch@...il.com> 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@...nel.org> wrote:  
> > > > > >
> > > > > > On Mon, 21 Apr 2025 22:06:38 +0000
> > > > > > Lothar Rubusch <l.rubusch@...il.com> 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@...il.com>
> > > > > > > ---
> > > > > > >  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.
> > > >  
> 
> Kind of the naive approach to allow for extending everything, but it probably
> does not fit together with the existing sensor ABI.

Agreed.

> 
> > > > 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?
> > > >  
> 
> I'd rather drop free fall then. Activity and inactivity, linked to
> gether work well for the power saving
> thing. I guess that's probably also more of general use. Not sure how
> usefull free fall is here.
> 
> > > > 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!)
> > > >  
> 
> That's either free fall or the other? Ideally, I'd like to represent
> what's possible with this sensor.
That's the only option if using the period to control it.   You can't have
one event with two periods.

> Since I'm not really familiar with how those sensors are used, for me
> all features are equally
> important. I mean, the sensor offers enabling act/inact and enabling
> free fall if one likes.

Sure.

> 
> > > > 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!)
> > > >  
> 
> Ideally I'd like to implement support for all features of the sensor.
> I can understand if some corner case features are too individual to
> support them. So, at least the main features.
> 
> From what I see in the code, configuring threshold and period, there
> might be differences and it can be easy to distinguish in
> read/write_event_value() like functions. Where
> read/write_event_config() will be more difficult.
> So, for the event, when I receive it I have knowledge of the exact
> event type, free fall or inactivity. I'm losing this information since
> it can currently not be conveyed over the channel. Might be that the
> ADXL345 is a very particular case. Might also be, that this shows a
> more general shortcoming which should be addressed.

I'm sure there will be other sensors that have similar events.
How common is hard to say though.

> 
> Equally I could say, there is just a MAG event, so turning on
> inactivity or free fall would result in the same event. The tricky
> case then is if someone wants inactivity AND free fall. In such case
> it would be a inact_or_free_fall event. Not sure about. Probably
> rather not.

It's just a threshold on falling magnitude of signal. What that means depends
on the sensor type and other settings (here the period).  Event codes
are the same, but without the settings those are pretty meaningless.

> 
> Q: Still, what about the direction - I see, where activity is using
> IIO_EV_DIR_RISING, inactivity uses _DIR_FALLING. Free fall now uses
> _DIR_FALLING. I'm a bit unsure if this is just a choice. Could I use
> _DIR_EITHER here? Or why not? I mean, actually it's not possible to
> detect fall into one direction, it's simply "in a fall". We cannot
> tell which axis we cannot tell which direction.

Not about direction in sense of x/y/z, but direction of the signal.

They are definitely triggered when the |X| > thresh transitions
to |X| < thresh.  So direction is falling as value |X| is getting smaller.

The choice reflects the physical thing being measured.

> 
> > > > 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.
> > > >  
> 
> This might be generally a good option. Honestly, I'd like to mess
> still with some other sensors,
> first.

Make sense.

J

> 
> Best,
> L
> 
> > >
> > > 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
> > > > > >
> > > > > >  
> > > >  
> >  
> 

Powered by blists - more mailing lists