On Mon, Aug 30, 2021 at 04:30:54PM +0200, Peter Rosin wrote:

On 2021-08-30 13:22, Jonathan Cameron wrote:

quoted

On Mon, 23 Aug 2021 00:18:55 +0200
Peter Rosin [off-list ref] wrote:

quoted

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

quoted

From: Liam Beguin <redacted>

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

No worries, I was also under the impression that the source channel
scale (and offset) could change.

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).
 */

If this is more of a general assumption, is there a place in the
Documentation/ that we could put this comment?

If not, I'll add it here.

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.

I agree with you, I'd much rather drop gcd() from rescale_process_scale()
altogether.

I was planning on keeping IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2
combined, but getting rid of the 100ppm condition in favor of a simple
if (rem).

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.

I was thinking of using check_mul_overflow() to do something about the
overflow, but I'm happy to leave it out for now.

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.

My original issue was with IIO_VAL_FRACTIONAL.
I'll look into what we can do for small IIO_VAL_FRACTIONAL_LOG2 values,
and will add more tests for that too.

Thanks,
Liam

Cheers,
Peter