Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORDIC sincos implementation from http://www.dcs.gla.ac.uk/~jhw/cordic/ #117

Closed
wants to merge 7 commits into from

Conversation

westonal
Copy link
Contributor

@westonal westonal commented Jun 9, 2015

SinCos implementation with CORDIC from http://www.dcs.gla.ac.uk/~jhw/cordic/

Uses only int maths, addition/subtraction xor.

Used 17 iterations which gives error of less than 0.00001549 compared to sinf/cosf, takes about 3x as long on I7 desktop, which means about 21 microsconds from about 7 for native.

Takes any float angle, normalizes to range [-pi..pi] see discussion here #118.

@westonal
Copy link
Contributor Author

westonal commented Jun 9, 2015

I should point out that I don't think M_NORMALIZE_RADIANS works which is why I didn't use it.

Example: M_NORMALIZE_RADIANS(3.141593)=-21.991148

@westonal
Copy link
Contributor Author

westonal commented Jun 9, 2015

It's exactly what I want to do, normalize to -pi..pi by adding or subtracting a multiple of 2*pi

@westonal
Copy link
Contributor Author

westonal commented Jun 9, 2015

I also wasn't able to fix it by properly bracketing the x argument in the macro M_FLOOR:

#define M_FLOOR(x) ((int)((int)(x) - ((int)(x) > (x))))

@westonal
Copy link
Contributor Author

westonal commented Jun 9, 2015

No while loop now. My function could replace the broken macro.

@westonal
Copy link
Contributor Author

westonal commented Jun 9, 2015

I've fully tested this for large range -720..720 degrees https://gist.github.com/westonized/15722186f947be191d73 Just don't know where to put such a test.

@mateunho
Copy link
Contributor

mateunho commented Jun 9, 2015

I think that you can put it in pal/examples or rewrite it into testcase for tests/math.

@kfitch
Copy link
Contributor

kfitch commented Jun 13, 2015

Personally I don't think this should be the default implementation for sincos (or sin or cos). As mentioned earlier, this is slower on a machine with actual floating point. Having this CORDIC implementation available for those situations where it is faster (I assume primarily when you don't have actual floating point hardware) is nice, but the rest of this library pretty much assumes floating point hardware at the moment.

@westonal
Copy link
Contributor Author

@kfitch

As mentioned earlier, this is slower on a machine with actual floating point.

Where is it mentioned? Only I mentioned timings and I was talking about vs calling hardware implemented sin/cos. Floating point is still slower even if you have floating point hardware.

That's not to say the existing sin cos code isn't faster. But as it's completely inaccurate ATM what's to compare.

@kfitch
Copy link
Contributor

kfitch commented Jun 13, 2015

I just redid some quick/dirty benchmarks on my desktop box (Athon64X2 4600+) and I find that p_sin_f is very roughly twice as fast a calling sinf from -lm. That being said, it turns out that p_cos_f32 is not faster, I think the difference mainly comes down to the M_NORMALIZE_RADIAN macro which p_cos uses, but p_sin doesn't (as of right now).

The end result is that I was a little too quick to dismiss your code, but I am not yet convinced there is a clear winner in performance between the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants