Re: [PATCH 2/2] docs: abi: iio: Add event when offset changes

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 31 Aug 2020 20:00:43 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> On Sun, Aug 30, 2020 at 6:11 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 28 Aug 2020 16:31:56 -0700
> > Gwendal Grignou <gwendal@chromium.org> wrote:
> >  
> > > Some sensors/sensorhubs can calculate drift or hard iron offsets to
> > > apply to raw data to get the true measure data.
> > > These offsets are applied by the user space application.
> > > When these offsets change, events are raised to tell the application
> > > to update the cached offset values.
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>  
> > Hi Gwendal
> >
> > This strikes me as rather prone to races.  I guess if the updates
> > tend to be fairly minor we will just have slightly less accurate data
> > until the update gets picked up by userspace.  
> In case of hard iron for instance, the sensorhub needs several samples
> to find out the current offset are now invalid. So it is likely the
> measurement of the geomagnetic field was incorrect for a while.
> For accelerometer online calibration, we don't expect vast changes
> when new offsets are available.
> >
> > We have had some discussions about how to handle meta data updates
> > in the past.  One option was to provide a metadata index channel
> > that could be used to indicate there was an update to something
> > sat in a separate fifo.  
> An extra sounds like a waste of space, as it will mostly be 0 most of
> the time, and edge to 1 once in a while. An event looks more
> appropriate.

Agreed for this case.
It would be a much more general interface with a lot of other
usecases. It is overkill here.  There isn't a huge burden in adding
support for your case in the meantime, even if we eventually end up
finding a better way of doing metadata in general.

> >  
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 33 +++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 47df16c87862d..40da602e7a555 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -1716,3 +1716,36 @@ Description:
> > >               Mass concentration reading of particulate matter in ug / m3.
> > >               pmX consists of particles with aerodynamic diameter less or
> > >               equal to X micrometers.
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_anglvel_x_offset
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_anglvel_y_offset
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_anglvel_z_offset
> > > +KernelVersion:       x.y
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Gyroscope drift calculated by the sensor. In addition to factory
> > > +             calibration, sensor or sensorhub can
> > > +             detect gyroscope drift and report it to userspace.  
> >
> > This looks like standard ABI that should probably already be documented,
> > unrelated to the controversial part of this patch. I would split it out into
> > it's own patch a I can pick that up much faster.  
> Done in v2.
> >
> >  
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_magn_x_offset
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_magn_y_offset
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_magn_z_offset
> > > +KernelVersion:       x.y
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Hard Iron bias calculated by the sensor or sensorhub. To be applied by
> > > +             user space application to the raw data to obtain the geomagnetic field.  
> >
> > Same as above.  
> Done in v2.
> >  
> > > +
> > > +What:                /sys/.../iio:deviceX/events/in_accel_offset_change_en
> > > +What:                /sys/.../iio:deviceX/events/in_magn_offset_change_en
> > > +What:                /sys/.../iio:deviceX/events/in_magn_scale_change_en
> > > +What:                /sys/.../iio:deviceX/events/in_anglvel_offset_change_en
> > > +KernelVersion:       x.y
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Some sensors internally calculate offset to apply to remove bias (for
> > > +             instance, hard/soft-iron bias for magnetometer, online calibration bias for
> > > +             gyroscope or accelerometer).
> > > +             When the sensor computes a new set of offset values, it generates an
> > > +             event for the userspace application to refresh the offsets to apply to raw
> > > +             data.  
> >
> > I'm not totally sold on this idea, though would like inputs from other people before
> > ruling it out.
> >
> > One minor change I'd make would be to have a single event to indicate that something
> > userspace might care about in the way of metadata for this channel has changed.
> > I guess something like  
> Make sense, scale and offset should be checked together if they both exists.
> Using vents/in_<type>_metadata_en in v2.
> >
> > in_accel_metachange_en etc with a single new event code.  For events, it's the event
> > code mapping that normally matters more than sysfs binding as that is much more
> > tightly restricted.
> >
> > Jonathan
> >