Re: [PATCH 1/1] iio: core: Improve precision of __iio_format_value for FRACTIONAL values

[Phil Reid <preid@electromag.com.au>]

On 31/01/2019 9:35 pm, Jonathan Cameron wrote:
> On Tue, 29 Jan 2019 17:11:25 +0800
> Phil Reid <preid@electromag.com.au> wrote:
> 
>> G'day Alex,
>>
>> On 29/01/2019 4:32 pm, Alexandru Ardelean wrote:
>>> On Tue, Jan 29, 2019 at 8:28 AM Phil Reid <preid@electromag.com.au> wrote:
>>>>
>>>> Currently FRACTIONAL values are outputed with 9 digits after the decimal
>>>> place. This is not always sufficient to resolve the raw value with 1 bit.
>>>> Output FRACTIONAL values to 15 decimal places of precision, regardless
>>>> of the number of leading zeros.
>>>>
>>>> Currently for a 2.5V ref with 24 bits of precision the code outputs only
>>>> to 9 decimal places.
>>>>
>>>> Cur: 0.00014901100000000000 * 16777216 = 2499.989733
>>>> New: 0.00014901161193847600 * 16777216 = 2500.000000
>>>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>>>> ---
>>>>
>>>> Notes:
>>>>       Alternatively I could add additonal FRACTIONAL types that select the new
>>>>       behaviour to prevent any possible regressions.
>>>>
>>>>    drivers/iio/industrialio-core.c | 55 ++++++++++++++++++++++++++++++++++-------
>>>>    1 file changed, 46 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>>> index a062cfd..bd9da64 100644
>>>> --- a/drivers/iio/industrialio-core.c
>>>> +++ b/drivers/iio/industrialio-core.c
>>>> @@ -571,11 +571,53 @@ int of_iio_read_mount_matrix(const struct device *dev,
>>>>    #endif
>>>>    EXPORT_SYMBOL(of_iio_read_mount_matrix);
>>>>
>>>> +static ssize_t __iio_format_div_prec(char *buf, unsigned int len, s64 x, s32 y)
>>>> +{
>>>> +       unsigned int prec = 0;
>>>> +       unsigned int idx = 0;
>>>> +       s64 d;
>>>> +
>>>> +       if (!len)
>>>> +               return 0;
>>>> +
>>>> +       if (!y)
>>>> +               return snprintf(buf, len, "inf");
>>>> +
>>>> +       if (!x)
>>>> +               return snprintf(buf, len, "0");
>>>> +
>>>> +       if (((x > 0) && (y < 0)) || ((x < 0) && (y > 0))) {
>>>> +               buf[idx++] = '-';
>>>> +               x = x > 0 ? x : -x;
>>>> +               y = y > 0 ? y : -y;
>>>> +       }
>>>> +
>>>> +       d = div64_s64(x, y);
>>>> +       idx += snprintf(buf+idx, len-idx, "%d", (int)d);
>>>> +       x = x - (y * d);
>>>> +       if ((x != 0) && (idx < len-1)) {
>>>> +               buf[idx++] = '.';
>>>> +               x = x * 10;
>>>> +               d = div64_s64(x, y);
>>>> +
>>>> +               while ((idx < len-1) && (prec < 15)) {
>>>> +                       if (d || prec)
>>>> +                               prec++;
>>>> +                       buf[idx++] = '0' + (char)d;
>>>> +                       x = x - (y * d);
>>>> +                       if (!x)
>>>> +                               break;
>>>> +                       x = x * 10;
>>>> +                       d = div64_s64(x, y);
>>>> +               }
>>>> +               buf[idx] = 0;
>>>> +       }
>>>> +       return idx;
>>>> +}
>>>> +
>>>>    static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
>>>>                                     int size, const int *vals)
>>>>    {
>>>> -       unsigned long long tmp;
>>>> -       int tmp0, tmp1;
>>>>           bool scale_db = false;
>>>>
>>>>           switch (type) {
>>>> @@ -598,14 +640,9 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
>>>>                   else
>>>>                           return snprintf(buf, len, "%d.%09u", vals[0], vals[1]);
>>>>           case IIO_VAL_FRACTIONAL:
>>>> -               tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
>>>> -               tmp1 = vals[1];
>>>> -               tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1);
>>>> -               return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
>>>> +               return __iio_format_div_prec(buf, len, vals[0], vals[1]);
>>>
>>> Maybe I'm a bit naive, but I'm also a bit curious.
>>> If you just bump the numbers here, would it work the same ?
>>>
>>> i.e.   10^9 -> 10^15 and "snprintf(buf, len, "%d.%15u", tmp0, abs(tmp1));"
>> I did look at that solution.
>>
>> But I was running into overflow issues (even with 64 bit numbers).
>>
>> eg: with a 2500 reference and 32 bits.
>>
>> 2500 * 10^15 = 2e+18 = 61 bits
>> And the result of
>> 2500 / 2^32 =  0.000000582076609
>> Only provides 9 significant digits with 15 decimal places.
>>
>> I was looking to provide 15 significant digits to match a standard double
>> precision floating point value.
> 
> I'll ask the awkward engineering question.  Is this precision actually valid?
> I.e. typical voltage references are +- 0.0x %
> 
> The fact we have a 32 bit ADC means you'll detect small changes, but I'm
> dubious about whether the absolute value will ever be 'that good'.
> 
> If we are going to go out of way to support greater precision we need
> a strong justification of why.
> To take advantage of these high precision devices you need to take into
> account non linear effects, temperature etc.  These will swamp (I think)
> any effect of a lack of precision the scale value.
> 

All valid points.

9 signification digits is probably fine.
However the current formatting doesn't always provide 9 significant digits.
So I believe this can start to add significant error.


Some typical ref voltages using 32 bit scale.
         scale          iio scale    err               err%
2500  5.820766091E-07  0.000000582  -7.660913467E-11  -0.0132%
3000  6.984919310E-07  0.000000698  -4.919309616E-10  -0.0705%
3300  7.683411241E-07  0.000000768  -3.411240578E-10  -0.0444%
5000  1.164153218E-06  0.000001164  -1.532182693E-10  -0.0132%

Looking at other drivers they seem to adjust the scale figure based on gain
selection as well. Is this expected?

If so when adding gain eg: x100

       scale            iio scale     err              err%
2500  5.820766091E-09  0.000000005  -8.207660913E-10  -16%
3000  6.984919310E-09  0.000000006  -9.849193096E-10  -16%
3300  7.683411241E-09  0.000000007  -6.834112406E-10  -10%
5000  1.164153218E-08  0.000000011  -6.415321827E-10  -6%

The limited number of significant digits is swamping everything else.

even at 24bit with gain 1x00

       scale            iio scale     err              err%
2500  1.490116119E-06  0.000001490  -1.161193848E-10  -0.01%
3000  1.788139343E-06  0.000001788  -1.393432617E-10  -0.01%
3300  1.966953278E-06  0.000001966  -9.532775879E-10  -0.05%
5000  2.980232239E-06  0.000002980  -2.322387695E-10  -0.01%


Similarly this while also affect the accuracy of values mapped thru the
rescale driver.



-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au