[BUG] sin/cos integer overflow on 16-bit CPUs

[dekutree64]

Thanks to ChrisMicro on the forum for finding this bug in the fast sin/cos added last year #285

Forum thread https://community.simplefoc.com/t/simplefoc-sin-calculation/5131

There are two potential solutions:
Solution 1 is to change the lookup table to signed 16-bit and fudge the last entry to 32767 so it doesn't overflow, and add some casts to do part of the interpolation calculation in 32-bit:
return (1.0f/32768.0f) * (t1 + (int)((((int32_t)t2 - t1) * frac) >> 8));

Solution 2 is to change t1 and t2 to int32_t. This will be a few cycles slower on 16-bit platforms, but retain the behavior of returning exactly 1.0 at pi/2 and -1.0 at 3pi/2.

I would be fine with either. I think 1 was my original intent, but after all the iterations testing the speed and accuracy of the calculations, I forgot to make the final edits.

EDIT: No measured speed difference when testing, so I went with solution 2. Pull request #416

[ChrisMicro]

The maximum value of an int16 is 32767. For this reason I would expect for a int16 sin calculation the maximum value in the table should be this one. To achieve the highest accuracy the whole table should be recalculated for this value.

[dekutree64]

The table is unsigned, so it can go up to 65535. I went with 32768 so the quadrant points would return exactly 1.0 and -1.0, but thinking about it now, the return has a float multiplier so it could use (1.0f/65535.0f) and return the correct amplitude anyway.

This is the table generation code: for(int i = 0; i <= 64; i++) fprintf(file, "%i,", (int)(sin((i * M_PI/2) / 64) * 32768 + 0.5));

And here is the result with the constant changed to 65535:
0,1608,3216,4821,6424,8022,9616,11204,12785,14359,15924,17479,19024,20557,22078,23586,25079,26557,28020,29465,30893,32302,33692,35061,36409,37736,39039,40319,41575,42806,44011,45189,46340,47464,48558,49624,50659,51664,52638,53580,54490,55367,56211,57021,57797,58537,59243,59913,60546,61144,61704,62227,62713,63161,63571,63943,64276,64570,64826,65042,65219,65357,65456,65515,65535

I tested it and the speed is no different, but average error and RMS error relative to stdlib sin are also almost no different (about one millionth) so I don't know if we should bother changing it. Theoretically the (1.0f/32768.0f) multiplier should be possible to do faster on FPU-less platforms using integer math on the exponent of the float value, but we were never able to make the compiler do it. ldexp was the best bet, but for some reason it's even slower than a regular multiply.

On another note, I was reading through the thread where we developed this code, and runger had already spotted and fixed the 16-bit overflow bug before we ever introduced it to the actual codebase. I don't remember which of us did the initial commit, but apparently whoever it was copy/pasted the wrong version from the thread.
https://community.simplefoc.com/t/embedded-world-2023-stm32-cordic-co-processor/3107/94

[askuric]

In the new release v2.3.4