Message-ID: <CAFXKEHb8+76WiiTCQbRa9v6gAicT0wnua6vCn-NxCfgNiuud2g@mail.gmail.com>
Date: Sun, 4 May 2025 19:47:55 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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 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 (?).

[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.

> > 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.

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.

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