Messages in this thread | | | Subject | Re: [PATCH] iio: cros_ec: Fix gyro scale calculation | From | Enric Balletbo i Serra <> | Date | Wed, 6 Mar 2019 12:16:18 +0100 |
| |
Hi Jonathan,
On 3/3/19 17:47, Jonathan Cameron wrote: > On Fri, 22 Feb 2019 11:24:24 +0100 > Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > >> Hi Jonathan, >> >> On 20/2/19 17:01, Jonathan Cameron wrote: >>> On Wed, 20 Feb 2019 16:03:00 +0100 >>> Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: >>> >>>> From: Gwendal Grignou <gwendal@chromium.org> >>>> >>>> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid >>>> rounding error is wrong. It should be only half of the divider. >>>> >>>> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver") >>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org> >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> >>> This one is kind of interesting. See below. >>> >>>> --- >>>> >>>> drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c >>>> index 89cb0066a6e0..600942af9f9c 100644 >>>> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c >>>> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c >>>> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev, >>>> * Do not use IIO_DEGREE_TO_RAD to avoid precision >>>> * loss. Round to the nearest integer. >>>> */ >>>> - *val = div_s64(val64 * 314159 + 9000000ULL, 1000); >>>> + *val = div_s64(val64 * 314159 + 500ULL, 1000); >>> That is only one of two divides going on. Firstly we divide by 1000 here, >>> then we provide it in fractional form which means that the actual value you get >>> from sysfs etc is >>> val/val2. It's this one we are protecting against rounding error on I guess. >>> Now this is even less obviously because it's not 18000 either, but >>> 18000 * 2^CROS_EC_SENSOR_BITS. >>> >>> Which ultimately means neither answer is correct. Hmm. >>> Not totally sure what the right answer actually is.. >>> >> >> If I understood well the Gwendal's patch the problem that we're trying to solve >> is that current calculation is not closer from the float calculation. >> >> For 1000dps, the result should be: >> >> (1000 * pi ) / 180 >> 15 ~= 0.000532632218 >> >> But with current calculation we get >> >> $ cat scale >> 0.000547890 >> >> With that patch (modifying the offset to avoid the rounding error) we get a >> closer result >> >> $ cat scale >> 0.000532631 >> >> So, what we're trying to do is have val/val2 closer to the real value. Makes >> this sense to you or I'm missing something? I can improve the commit message if >> it's not clear. > > I think we are in enough of a mess here with the different dividers that we > should just do the maths here, then we can avoid the bia. > > aiming for nano value. > val * pi * 10e12 / (180 * 2^15) > div_s64(val * 3141592653000 + 2949120, 5898240) = 532632 > vs 532632 for floating point division. > Then use IIO_INT_PLUS_NANO to return it. > > Even then I suspect the +2949120 is only effecting the last digit so > you could probably drop it safely enough. > > I'd certainly rather we had all the magic in one place rather than > trying to correct for divisions that aren't apparent here. >
Thanks for the clear explanation, yes, this looks better. I'll do some tests and submit a second version.
Thanks!
>> >> -- Enric >> >>> Jonathan >>> >>>> *val2 = 18000 << (CROS_EC_SENSOR_BITS - 1); >>>> ret = IIO_VAL_FRACTIONAL; >>>> break; >>> > >
| |