LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Peter Rosin <peda@axentia.se>
Cc: Liam Beguin <liambeguin@gmail.com>,
	lars@metafoo.de, linux-kernel@vger.kernel.org,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	robh+dt@kernel.org
Subject: Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales
Date: Mon, 30 Aug 2021 18:03:48 +0100	[thread overview]
Message-ID: <20210830180348.032b217f@jic23-huawei> (raw)
In-Reply-To: <e1542f14-f271-a0a3-eaa1-092e12c3ed6c@axentia.se>

On Mon, 30 Aug 2021 16:30:54 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2021-08-30 13:22, Jonathan Cameron wrote:
> > On Mon, 23 Aug 2021 00:18:55 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> [I started to write an answer to your plans in the v7 thread, but didn't
> >> have time to finish before v8 appeared...]
> >>
> >> On 2021-08-20 21:17, Liam Beguin wrote:  
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> The approximation caused by integer divisions can be costly on smaller
> >>> scale values since the decimal part is significant compared to the
> >>> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
> >>> cases to maintain accuracy.    
> >>
> >> The conversion to int-plus-nano may also carry a cost of accuracy.
> >>
> >> 90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8,
> >> but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more
> >> digits). So, in this case you lose precision with the new code.
> >>
> >> Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old
> >> code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.
> >>
> >> I'm also wondering if it is wise to not always return the same scale type?
> >> What happens if we want to extend this driver to scale a buffered channel?
> >> Honest question! I don't know, but I fear that this patch may make that
> >> step more difficult to take??
> >>
> >> Jonathan, do you have any input on that?  
> > 
> > I'm a bit lost.  As things currently stand IIO buffered data flows all use
> > _RAW.  It's either up to userspace or the inkernel user to query scale
> > and use that to compute the appropriate _processed values.  There have been
> > various discussions over the years on how to add metadata but it's tricky
> > without adding significant complexity and for vast majority of usecases not
> > necessary.  Given the rescaler copes with _raw and _processed inputs, we
> > would only support buffered flows if using the _raw ones.
> > 
> > If nothing changes in configuration of the rescaler, the scale should be
> > static for a given device.  What format that 'scale' takes is something
> > that userspace code or inkernel users should cope fine with given they
> > need to do that anyway for different devices.  
> 
> Ok, if 'scale' (and 'offset') of the source channel is to be considered
> static, then it is much safer to ignore the "island problem" and rescale
> each value independently on a case-by-case basis. We should add an
> explicit comment somewhere that we make this assumption.
> 
> Sorry for wasting time and effort by not realizing by myself (and earlier).
> 
> Maybe something like this?
> 
> /*
>  * The rescaler assumes that the 'scale' and 'offset' properties of
>  * the source channel are static. If they are not, there exist some
>  * corner cases where rounding/truncating might cause confusing
>  * mathematical properties (non-linearity).
>  */
Looks good to me.

There are some really obscure corner cases in which it is theoretically possible
to have scale change autonomously. Reality is they mostly don't happen in reality
and are on strange sensors you couldn't stick an AFE on anyway ;)
IIRC hid-sensors in theory allow this, but as far as I know, no device actually does
it...  I'm not sure the driver takes any notice if they do!

Jonathan

> 
> I then propose that we rescale IIO_VAL_FRACTIONAL as before if that works,
> thus preserving any previous exact rescaling, but if there is an overflow
> while doing that, then we fall back to new code that rescales to a
> IIO_VAL_INT_PLUS_NANO value. Trying the gcd-thing as it ended up in v7 still
> seems expensive to me, but maybe I overestimate the cost of gcd? Anyway, my
> guts vote for completely skipping gcd and that we aim directly for
> IIO_VAL_INT_PLUS_NANO in case of overflow while doing the old thing.
> 
> Having said that, if 'scale' and 'offset' indeed are static, then the gcd
> cost can be mitigated by caching the result. Exact rescaling is always
> nice...
> 
> If IIO_VAL_INT overflows while rescaling, we are SOL whichever way we turn,
> so ignore doing anything about that.
> 
> Liam, was it IIO_VAL_FRACTIONAL or IIO_VAL_FRACTIONAL_LOG2 that was your
> real use case? Anyway, your 100 ppm threshold is probably as good a
> compromise as any for this case. However, that threshold does nothing for
> the case where the conversion to IIO_VAL_INT_PLUS_NANO throws away way
> more precision than the existing operations. It would probably be good
> to somehow stay with the old method for the really tiny values, if there
> is a viable test/threshold for when to do what.
> 
> Cheers,
> Peter


  reply	other threads:[~2021-08-30 17:00 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 19:17 [PATCH v8 00/14] iio: afe: add temperature rescaling support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 01/14] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-08-20 19:17 ` [PATCH v8 02/14] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-08-20 19:17 ` [PATCH v8 03/14] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-08-20 19:17 ` [PATCH v8 04/14] iio: afe: rescale: expose scale processing function Liam Beguin
2021-08-20 19:17 ` [PATCH v8 05/14] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-08-26  8:11   ` Peter Rosin
2021-08-29  2:50     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 06/14] iio: afe: rescale: add offset support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 07/14] iio: afe: rescale: use s64 for temporary scale calculations Liam Beguin
2021-08-20 19:17 ` [PATCH v8 08/14] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-08-26  9:13   ` Peter Rosin
2021-08-29  4:01     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Liam Beguin
2021-08-20 23:37   ` kernel test robot
2021-08-21  1:33     ` Liam Beguin
2021-08-23  6:53       ` Peter Rosin
2021-08-21  2:00   ` kernel test robot
2021-08-21  7:21   ` kernel test robot
2021-08-22 22:18   ` Peter Rosin
2021-08-24 20:28     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small Liam Beguin
2021-08-26  9:53       ` Peter Rosin
2021-08-29  4:41         ` Liam Beguin
2021-08-30 11:27           ` Jonathan Cameron
2021-09-11 23:31             ` Liam Beguin
2021-08-30 13:03           ` Peter Rosin
2021-09-11 23:20             ` Liam Beguin
2021-08-30 11:22     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Jonathan Cameron
2021-08-30 14:30       ` Peter Rosin
2021-08-30 17:03         ` Jonathan Cameron [this message]
2021-09-02  2:27         ` Liam Beguin
2021-09-02 21:52           ` Peter Rosin
2021-08-20 19:17 ` [PATCH v8 10/14] iio: test: add basic tests for the iio-rescale driver Liam Beguin
2021-08-20 19:17 ` [PATCH v8 11/14] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 12/14] iio: afe: rescale: add temperature transducers Liam Beguin
2021-08-26  8:56   ` Peter Rosin
2021-08-29  2:33     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 13/14] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-08-20 19:17 ` [PATCH v8 14/14] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210830180348.032b217f@jic23-huawei \
    --to=jic23@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lars@metafoo.de \
    --cc=liambeguin@gmail.com \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peda@axentia.se \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).