Math: Library: Added cordic sine function

[ShriramShastry]

Implement cordic sine algorithm with angle in the range [-pi/2, pi/2] having total harmonic distortion (THD) of (-170) dBc for SNR

Signed-off-by: ShriramShastry malladi.sastry@intel.com

** figure -1 cordic sine performance**

** figure -2 Computed time per sample derived using x86 performance count**

** figure -3 Difference between floating point and fixed point out**

[paulstelian97]

I'd expect the commit message to contain a description on what this improves.

[lgirdwood]

Indentation and content LGTM. Cant comment on the maths though.

[marc-hb]

Github Actions hang fixed by #3992 . @ShriramShastry you must:

git stash
git fetch <origin> pull/3990/head
git checkout FETCH_HEAD
git commit --amend # fake commit to change the date and SHA
git push <your remote> HEAD:refs/heads/shastry_cordicsin_dev

This will restart Github Actions. Sorry for the inconvenience.

Of course you could also address review comments and push, this would also trigger the same thing.

[lgirdwood]

@ShriramShastry are you able to rebase on top of latest main branch to fix the CI report. We are good to merge, we just need to make sure your shastry_cordicsin_dev branch has the CI fix from the main branch.

[marc-hb]

we just need to make sure your shastry_cordicsin_dev branch has the CI fix from the main branch.

Rebasing is always possible but very rarely useful because Github Actions (and Travis, and Quickbuild, and our Jenkins, and others,...) always test the constantly moving pull/12345/merge by default anyway, they do not test pull/12345/head by default. This is a very common misconception which hides why CI results can change.

Rebasing when no conflict also creates pointless git range-diff noise, example: https://github.com/thesofproject/sof/compare/8fbac256c0683171297d1b36f12a65c5651f0bf6..fcbf08d40dcba9dbd7423dd7def6ba23af1e6679

NOT rebasing also provides more test coverage: 1. without the moving target locally, 2. with the moving target in CI.

[lgirdwood]

@zrombel can you comment on UT here, as this should be unrelated ?

[marc-hb]

In https://sof-ci.01.org/sof-pr-viewer/#/build/PR3990/build6249622 all platforms fail with something like this so it does seem related

39/39 Testing: sin_fixed
39/39 Test: sin_fixed
Command: "/localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-run" "--exit_with_target_code" "sin_fixed"
Directory: /quickbuild/workspace1/24733/SOF_FW/build_ut/test/cmocka/src/math/trig
"sin_fixed" start time: Apr 01 15:39 CEST
Output:
----------------------------------------------------------
1..1
test_math_trig_sin_fixed: diff for 1 deg = 0.0087261982
not ok 1 - test_math_trig_sin_fixed
# diff <= CMP_TOLERANCE
# /quickbuild/workspace1/24733/SOF_FW/test/cmocka/src/math/trig/sin_fixed.c:132: error: Failure!
# not ok - tests
<end of output>
Test time =   3.55 sec
----------------------------------------------------------
Test Failed.
"sin_fixed" end time: Apr 01 15:39 CEST
"sin_fixed" time elapsed: 00:00:03

97% tests passed, 1 tests failed out of 39

Total Test time (real) =  12.24 sec

The following tests FAILED:
	 39 - sin_fixed (Failed)
Errors while running CTest

[ShriramShastry]

SOFCI TEST

Thanks @aiChaoSONG for helping me with triggering the tests.

[lgirdwood]

@cujomalainey please merge if you are happy.

[keyonjie]

Thanks for the optimization @ShriramShastry , do you intend to add support of input formats Q2.30, Q3.29 (besides today's Q4.28) for sin_fixed()? If so, better to add another param to denote this and avoid the checking logic. The variables declaration and comments need follow the guide and remained code also.

[ShriramShastry]

Thanks for the optimization @ShriramShastry , do you intend to add support of input formats Q2.30, Q3.29 (besides today's Q4.28) for sin_fixed()? If so, better to add another param to denote this and avoid the checking logic. The variables declaration and comments need follow the guide and remained code also.

do you intend to add support of input formats Q2.30, Q3.29 (besides today's Q4.28) for sin_fixed()

The cordic sine algorithm only converges when the angle is in the range [-pi/2, pi/2). If an angle is outside of this range, then a multiple of pi/2 is added or subtracted from the angle until it is within the range [-pi/2,pi/2). The cordicsin function do addition or subtraction by a multiple of pi/2 in the data type of the input. So, when the fraction length is 30 or 29 , then the quantization error introduced by the addition or subtraction of pi/2 is done on input fraction bits of precision.

I checked the implementation with cmocka which is pretty good #3990 (comment)
figure -3 in the draft explains

In case it is desired, subsequent adaptation is needed outside the cordicsine algorithm ( For example DRC fix)

[singalsu]

Look pretty good, can you check the things mentioned by me and Keyon!

[paulstelian97]

Still cannot fully review algorithm (my own theory is lacking).

Marking this as comment rather than approve due to issues with the commit message itself (for example, you need to fix the range in the commit message -- should be [-2*pi to 2*pi] -- and don't insert spaces where you shouldn't -- should be sizeof(int32) without extra spaces between the parentheses).

Other than that, as far as I could tell this looks good. I assume tests confirm that this works correctly.

[ShriramShastry]

Still cannot fully review algorithm (my own theory is lacking).

Marking this as comment rather than approve due to issues with the commit message itself (for example, you need to fix the range in the commit message -- should be [-2*pi to 2*pi] -- and don't insert spaces where you shouldn't -- should be sizeof(int32) without extra spaces between the parentheses).

Other than that, as far as I could tell this looks good. I assume tests confirm that this works correctly.

Thanks you.
I will include your suggested change in my next commit message.
A description of cordicsine is available in https://en.wikipedia.org/wiki/CORDIC) and http://www.andraka.com/files/crdcsrvy.pdf

[singalsu]

@ShriramShastry Also please mark as resolved the issues those you have addressed.

[singalsu]

Looks good to me, thanks!

[lgirdwood]

CML showing unrelated kernel resume issue.