On Mon, Aug 30, 2021 at 04:30:54PM +0200, Peter Rosin wrote: > On 2021-08-30 13:22, Jonathan Cameron wrote: > > On Mon, 23 Aug 2021 00:18:55 +0200 > > Peter Rosin <peda@xxxxxxxxxx> 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@xxxxxxxxxx> > >>> > >>> 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
