Rounding broken for negative numbers

[GoogleCodeExporter]

Rounding seems to be broken in all implementations of fix16_(s)mul, 
fix16_(s)div for negative numbers:

#include <stdio.h>
#include <fix16.h>

int main()
{
        fix16_t a = fix16_from_int(-1);
        fix16_t b = fix16_from_int(2);
        fix16_t result = fix16_div(a, b);
        printf("-1/2 = %0.6f\n", fix16_to_float(result));
}

-1/2 = -0.499985

Furthermore, the ARM and non-64bit implementations of fix16_mul do not 
propagate the carry from adding the rounding constant. This could cause 
significant errors when the lowest word overflows.

Original issue reported on code.google.com by Petteri.Aimonen on 29 Nov 2011 at 8:02

[GoogleCodeExporter]

Fix for the general 64bit cases:

Index: fix16.c
===================================================================
--- fix16.c (revision 51)
+++ fix16.c (working copy)
@@ -35,7 +35,7 @@
    #ifndef FIXMATH_NO_64BIT
    int64_t tempResult = ((int64_t)inArg0 * (int64_t)inArg1);
    #ifndef FIXMATH_NO_ROUNDING
-   tempResult += (fix16_one >> 1);
+   tempResult += (tempResult > 0) ? (fix16_one >> 1) : -(fix16_one >> 1);
    #endif
    tempResult >>= 16;
    return tempResult;
@@ -97,7 +97,7 @@
    int64_t tempResult = inArg0;
    tempResult <<= 16;
    #ifndef FIXMATH_NO_ROUNDING
-   tempResult += (inArg1 >> 1);
+   tempResult += (tempResult > 0) ? (inArg1 >> 1) : -(inArg1 >> 1);
    #endif
    tempResult /= inArg1;
    return tempResult;

Original comment by Petteri.Aimonen on 29 Nov 2011 at 8:24

[GoogleCodeExporter]

Actually, for it to work also for result = 0:

Index: fix16.c
===================================================================
--- fix16.c (revision 51)
+++ fix16.c (working copy)
@@ -35,7 +35,7 @@
    #ifndef FIXMATH_NO_64BIT
    int64_t tempResult = ((int64_t)inArg0 * (int64_t)inArg1);
    #ifndef FIXMATH_NO_ROUNDING
-   tempResult += (fix16_one >> 1);
+   tempResult += (tempResult >= 0) ? (fix16_one >> 1) : -(fix16_one >> 1);
    #endif
    tempResult >>= 16;
    return tempResult;
@@ -97,7 +97,7 @@
    int64_t tempResult = inArg0;
    tempResult <<= 16;
    #ifndef FIXMATH_NO_ROUNDING
-   tempResult += (inArg1 >> 1);
+   tempResult += (tempResult >= 0) ? (inArg1 >> 1) : -(inArg1 >> 1);
    #endif
    tempResult /= inArg1;
    return tempResult;

Looks like we could use some validation framework :)

Original comment by Petteri.Aimonen on 29 Nov 2011 at 8:36

[GoogleCodeExporter]

Agreed, we have some tests but they clearly aren't even close to exhaustive. 
Would you be interested in joining the project? There are no expectations of 
work but it means you can make any reasonable changes you'd like to improve the 
codebase.

Original comment by Flatmush@googlemail.com on 30 Nov 2011 at 10:02

[GoogleCodeExporter]

I'm currently working on a fix16_t based matrix algebra library, so yes, I 
would be interested.

Original comment by Petteri.Aimonen on 30 Nov 2011 at 10:17

[GoogleCodeExporter]

This issue was closed by revision r52.

Original comment by Petteri.Aimonen on 26 Jan 2012 at 3:43

• Changed state: Fixed