Re: [RFC PATCHv2] fixp-arith: replace sin/cos table by a better precision one

["Prashant Laddha (prladdha)" <prladdha@cisco.com>]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

Hi Mauro,

I was able to test your patch with vivid sdr tone generation. It calls
sin/cos functions with radians as argument. I find that the sine wave
generated using fixp_sin32_rad() show discontinuities, especially around
90, 180 degrees. After debugging it further, these discontinuities seems
to be originating due to division of negative number. Please find it below

On 17/12/14 7:12 pm, "Mauro Carvalho Chehab" <mchehab@osg.samsung.com>
wrote:

>
>+ */
>+static inline s32 fixp_sin32_rad(u32 radians, u32 twopi)
> {
>+	int degrees;
>+	s32 v1, v2, dx, dy;
>+	s64 tmp;
>+	degrees = (radians * 360) / twopi;

Not sure if we should use higher precision here. But just a question - in
case, caller function uses higher precision for representing radians,
(radians * 360) can probably overflow, right ? So, could we possibly
specify on max precision for representing radian fraction cannot be more
than 18 bits. 

>+	v1 = fixp_sin32(degrees);
>+	v2 = fixp_sin32(degrees + 1);
>+	dx = twopi / 360;
>+	dy = v2 - v1;
>+	tmp = radians - (degrees * twopi) / 360;

Same as above.

>+	tmp *= dy;
>+	do_div(tmp, dx);

tmp can go negative. do_div() cannot handle negative number. We could
probably avoid do_div and do tmp / dx here. If we want to use do_div(), we
could still do it by modifying radian to degree calculation.

tmp goes negative when the slope sine waveform is negative, that is 2nd
and 3rd quadrant. We could avoid it by deciding the quadrant based on
radians and then calling fixp_sin32(). I modified the function on lines of
fixp_sin32() and tested. It works fine. Attaching a diff for fixp-arith.h
for with the this change to avoid negative values of tmp in
fixp_sin32_rad().

>2.1.0
>


[-- Attachment #2: fixp-arith.diff --]
[-- Type: application/octet-stream, Size: 5055 bytes --]

diff --git a/include/linux/fixp-arith.h b/include/linux/fixp-arith.h
index 3089d73..5973429 100644
--- a/include/linux/fixp-arith.h
+++ b/include/linux/fixp-arith.h
@@ -29,59 +29,116 @@
 
 #include <linux/types.h>
 
-/* The type representing fixed-point values */
-typedef s16 fixp_t;
-
-#define FRAC_N 8
-#define FRAC_MASK ((1<<FRAC_N)-1)
-
-/* Not to be used directly. Use fixp_{cos,sin} */
-static const fixp_t cos_table[46] = {
-	0x0100,	0x00FF,	0x00FF,	0x00FE,	0x00FD,	0x00FC,	0x00FA,	0x00F8,
-	0x00F6,	0x00F3,	0x00F0,	0x00ED,	0x00E9,	0x00E6,	0x00E2,	0x00DD,
-	0x00D9,	0x00D4,	0x00CF,	0x00C9,	0x00C4,	0x00BE,	0x00B8,	0x00B1,
-	0x00AB,	0x00A4,	0x009D,	0x0096,	0x008F,	0x0087,	0x0080,	0x0078,
-	0x0070,	0x0068,	0x005F,	0x0057,	0x004F,	0x0046,	0x003D,	0x0035,
-	0x002C,	0x0023,	0x001A,	0x0011,	0x0008, 0x0000
+static const s32 sin_table[] = {
+       0x00000000, 0x023be165, 0x04779632, 0x06b2f1d2, 0x08edc7b6, 0x0b27eb5c,
+       0x0d61304d, 0x0f996a26, 0x11d06c96, 0x14060b67, 0x163a1a7d, 0x186c6ddd,
+       0x1a9cd9ac, 0x1ccb3236, 0x1ef74bf2, 0x2120fb82, 0x234815ba, 0x256c6f9e,
+       0x278dde6e, 0x29ac379f, 0x2bc750e8, 0x2ddf003f, 0x2ff31bdd, 0x32037a44,
+       0x340ff241, 0x36185aee, 0x381c8bb5, 0x3a1c5c56, 0x3c17a4e7, 0x3e0e3ddb,
+       0x3fffffff, 0x41ecc483, 0x43d464fa, 0x45b6bb5d, 0x4793a20f, 0x496af3e1,
+       0x4b3c8c11, 0x4d084650, 0x4ecdfec6, 0x508d9210, 0x5246dd48, 0x53f9be04,
+       0x55a6125a, 0x574bb8e5, 0x58ea90c2, 0x5a827999, 0x5c135399, 0x5d9cff82,
+       0x5f1f5ea0, 0x609a52d1, 0x620dbe8a, 0x637984d3, 0x64dd894f, 0x6639b039,
+       0x678dde6d, 0x68d9f963, 0x6a1de735, 0x6b598ea1, 0x6c8cd70a, 0x6db7a879,
+       0x6ed9eba0, 0x6ff389de, 0x71046d3c, 0x720c8074, 0x730baeec, 0x7401e4bf,
+       0x74ef0ebb, 0x75d31a5f, 0x76adf5e5, 0x777f903b, 0x7847d908, 0x7906c0af,
+       0x79bc384c, 0x7a6831b8, 0x7b0a9f8c, 0x7ba3751c, 0x7c32a67c, 0x7cb82884,
+       0x7d33f0c8, 0x7da5f5a3, 0x7e0e2e31, 0x7e6c924f, 0x7ec11aa3, 0x7f0bc095,
+       0x7f4c7e52, 0x7f834ecf, 0x7fb02dc4, 0x7fd317b3, 0x7fec09e1, 0x7ffb025e,
+       0x7fffffff
 };
 
+/**
+ * fixp_sin32() returns the sin of an angle in degrees
+ *
+ * @degrees: angle, in degrees. It can be positive or negative
+ *
+ * The returned value ranges from -0x7fffffff to +0x7fffffff.
+ */
 
-/* a: 123 -> 123.0 */
-static inline fixp_t fixp_new(s16 a)
+static inline s32 fixp_sin32(int degrees)
 {
-	return a<<FRAC_N;
-}
+       s32 ret;
+       bool negative = false;
 
-/* a: 0xFFFF -> -1.0
-      0x8000 -> 1.0
-      0x0000 -> 0.0
-*/
-static inline fixp_t fixp_new16(s16 a)
-{
-	return ((s32)a)>>(16-FRAC_N);
-}
+       degrees = (degrees % 360 + 360) % 360;
+       if (degrees > 180) {
+               negative = true;
+               degrees -= 180;
+	   }
+       if (degrees > 90)
+               degrees = 180 - degrees;
 
-static inline fixp_t fixp_cos(unsigned int degrees)
-{
-	int quadrant = (degrees / 90) & 3;
-	unsigned int i = degrees % 90;
-
-	if (quadrant == 1 || quadrant == 3)
-		i = 90 - i;
+       ret = sin_table[degrees];
 
-	i >>= 1;
+       return negative ? -ret : ret;
 
-	return (quadrant == 1 || quadrant == 2)? -cos_table[i] : cos_table[i];
 }
 
-static inline fixp_t fixp_sin(unsigned int degrees)
-{
-	return -fixp_cos(degrees + 90);
-}
+/* cos(x) = sin(x + 90 degrees) */
+#define fixp_cos32(v) fixp_sin32((v) + 90)
 
-static inline fixp_t fixp_mult(fixp_t a, fixp_t b)
+/*
+ * 16 bits variants
+ *
+ * The returned value ranges from -0x7fff to 0x7fff
+ */
+
+#define fixp_sin16(v) (fixp_sin32(v) >> 16)
+#define fixp_cos16(v) (fixp_cos32(v) >> 16)
+
+/**
+ * fixp_sin32_rad() - calculates the sin of an angle in radians
+ *
+ * @radians: angle, in radians
+ * @twopi: value to be used for 2*pi
+ *
+ * Provides a variant for the cases where just 360
+ * values is not enough. This function uses linear
+ * interpolation to a wider range of values given by
+ * twopi var.
+ *
+ * Experimental tests gave a maximum difference of
+ * 0.000038 between the value calculated by sin() and
+ * the one produced by this function, when twopi is
+ * equal to 360000. That seems to be enough precision
+ * for practical purposes.
+ */
+static inline s32 fixp_sin32_rad(u32 radians, u32 twopi)
 {
-	return ((s32)(a*b))>>FRAC_N;
+       int degrees;
+       s32 v1, v2, dx, dy;
+       s64 tmp;
+       u32 pi = twopi / 2;
+       s32 ret;
+       bool negative = false;
+
+       radians = (radians % twopi + twopi) % twopi;
+       if (radians > pi) {
+               negative = true;
+               radians -= pi;
+	   }
+       if (radians > pi /2)
+               radians = pi - radians;
+
+       degrees = (radians * 360) / twopi;
+
+       v1 = fixp_sin32(degrees);
+       v2 = fixp_sin32(degrees + 1);
+
+       dx = twopi / 360;
+       dy = v2 - v1;
+
+       tmp = radians - (degrees * twopi) / 360;
+       do_div(tmp, dx);
+       ret = v1 + tmp;
+
+       return negative ? -ret : ret;
 }
 
+/* cos(x) = sin(x + pi / 2 radians) */
+
+#define fixp_cos32_rad(rad, twopi)     \
+       fixp_sin32_rad(rad + twopi / 4, twopi)
+
 #endif