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

Replace sine table with 5th order approximation #27

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

ndsl7109256
Copy link
Collaborator

@ndsl7109256 ndsl7109256 commented Aug 2, 2024

The previous implementation used the pre-calculated fixed-point
arithmetic table for sine values, which added over 2 KiB to the code
size.

To solve this issue, I implemented the 5th order polynomial
approximation for sine, which reduced the code size from 2556 bytes
to 494 bytes (by about 80%) while maintaining acceptable error (RMS
error is 8.866 observed with 0-90 degrees, 0-1024 in fixed-point
representation)

Original code size with sine table.

$ size .libtwin.a/src/trig.c.o
   text	   data	    bss	    dec	    hex	filename
   2556	      0	      0	   2556	    9fc	.libtwin.a/src/trig.c.o

Code size after using 5th order approximation

$ size .libtwin.a/src/trig.c.o
   text	   data	    bss	    dec	    hex	filename
   494	      0	      0	   494	    1ee	.libtwin.a/src/trig.c.o

I initially tried a 3rd order approximation, but the error was slightly large, causing the drawn circle to appear not perfectly round. Below is some analysis.

image

RMS Error Comparing with Sine table (valued from 0-65536, observed with 0-90 degrees, 0-1024 in fixed-point
representation)

3rd approximation 5th approximation
RMS 878.000907 8.8665040

image

ref:
https://www.nullhardware.com/blog/fixed-point-sine-and-cosine-for-embedded-systems/
https://hackmd.io/@rickywu0421/FinalProject#透過-3rd-order-sine-approximation-模擬-RSSI

@jserv jserv requested a review from jouae August 2, 2024 08:31
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can implement a new public function, twin_sincos(twin_angle_t a, twin_fixed_t *sin, twin_fixed_t *cos), to compute both sine and cosine at the same time and store the results in *sin and *cos, as several applications need the sine and cosine of the same angle a. Then, you can replace all occurrences of twin_sin and twin_cos with the shortcut twin_sincos, including the code in the files src/path.c and src/matrix.c.

Reference: https://man7.org/linux/man-pages/man3/sincos.3.html (GNU extension)

@jserv jserv requested a review from weihsinyeh August 2, 2024 08:42
@jserv
Copy link
Contributor

jserv commented Aug 2, 2024

@jouae
Copy link
Collaborator

jouae commented Aug 2, 2024

@ndsl7109256 Hey! You do a great job!

Here is a suggestion make your report getting better.

When mentioning RMS or MSE, be sure to include the number of observations.

@ndsl7109256 ndsl7109256 force-pushed the shrinkSineTable branch 2 times, most recently from 465635b to b250bc0 Compare August 2, 2024 10:38
Copy link
Collaborator

@jouae jouae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR, you wrote about the method used to compare code sizes, third-order and fifth-order approximation tests, and included references. However, in this commit, you did not mention them. Additionally, for the 80%, please add the actual data and the calculation process based on that actual data.

Remember, "writing a good commit" is an ongoing challenge for every engineer, including myself. Commits are written for others to read; they are not just explanations to convince others to accept a PR but also records of references, ways to solve problems, simple comparative data, and the reasons for adopting a particular solution at that time.

Once again, you did a great job, which includes code size comparisons, differences between third-order and fifth-order approximations, and providing data sources. Every piece of data and record is the result of your hard work and should be documented in the commit for future reference to see these contributions.

Thank you.

include/twin.h Outdated Show resolved Hide resolved
src/path.c Outdated Show resolved Hide resolved
src/trig.c Outdated Show resolved Hide resolved
src/trig.c Show resolved Hide resolved
src/trig.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jouae jouae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndsl7109256
Please include the number of observations on which the RMS error is based.

jserv

This comment was marked as outdated.

src/trig.c Outdated Show resolved Hide resolved
@jserv jserv requested a review from jouae August 5, 2024 10:59
jserv

This comment was marked as duplicate.

return twin_sin(a + TWIN_ANGLE_90);
twin_fixed_t cos_val = 0;
twin_sincos(a, NULL, &cos_val);
return cos_val;
}

twin_fixed_t twin_tan(twin_angle_t a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to calculate tan(x) without divisions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use Taylor series expansion to calculate tan(x) without using division, but as x approaches 90 degrees, the error would become very large. I'm not sure if this is suitable for use in this project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use Taylor series expansion to calculate tan(x) without using division, but as x approaches 90 degrees, the error would become very large. I'm not sure if this is suitable for use in this project.

TWIN_FIXED_MAX can be set for $tan(\pi/2)$ while TWIN_FIXED_MIN can be set for $tan(-\pi/2)$.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Though we could set $tan( \pi /2)$ to TWIN_FIXED_MAX directly. However, we still suffer significant error when angle is closed to $\pi/2$. Should we just neglect the error here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jouae, Can you comment this? Can CORDIC algorithm be applied to tan(x)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to spend some time studying the principles of CORDIC.

See https://github.com/Max-Gulda/Cordic-Math/blob/main/lib/cordicMath/src/cordic-math.c#L290-L332

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can improve the division about $\frac{x}{y}$

see https://hackmd.io/@sysprog/linux-intdiv

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can improve the division about x y

see https://hackmd.io/@sysprog/linux-intdiv

@jouae Thanks for the follow-up!
The divisor we use (x calculated in CORDIC or cos calculated in current way) varies for different angles. Is it possible to improve by the mentioned techniques?
Could you provide a more detailed explanation of your idea? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndsl7109256
Sure, leave it to me, dude.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jouae, Create a new GitHub issue when this pull request is merged.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Squash the commits and refine the git commit messages.
  2. Don't use markdown syntax in git commit messages. Instead, always use plain English.
  3. Append Close #13 at the end of the git commit message.
  4. Add comment about CORDIC based tangent function improvement.

@sysprog21 sysprog21 deleted a comment from jouae Aug 9, 2024
The previous implementation used the pre-calculated fixed-point
arithmetic table for sine values, which added over 2 KiB to the code
size.

To solve this issue, I implemented the 5th order polynomial
approximation for sine[1], which reduced the code size from 2556 bytes
to 918 bytes (by about 65%) while maintaining acceptable error (RMS
error is 8.866 observed with 0-90 degrees, 0-1024 in fixed-point
representation)

Besides, as several applications need the sine and cosine of the same
angle, I replace all occurrences of twin_sin and twin_cos with the
shortcut twin_sincos, including the code in the files src/path.c and
src/matrix.c.

[1]: https://www.nullhardware.com/blog/fixed-point-sine-and-cosine-for-embedded-systems/

Close sysprog21#13
@ndsl7109256
Copy link
Collaborator Author

Enhancement: Implement CORDIC algorithm for tan calculation

Currently, the twin_tan function uses division, which may be computationally expensive. A potential improvement would be to implement the CORDIC algorithm for calculating tan. This would avoid the use of the divider and potentially improve performance.

References:
https://github.com/Max-Gulda/Cordic-Math/blob/main/lib/cordicMath/src/cordic-math.c#L290-L332
https://hackmd.io/@sysprog/linux-intdiv

@jserv jserv merged commit d36535c into sysprog21:main Aug 9, 2024
3 checks passed
@jserv
Copy link
Contributor

jserv commented Aug 9, 2024

Thank @ndsl7109256 for contributing!

@ndsl7109256 ndsl7109256 deleted the shrinkSineTable branch December 24, 2024 07:33
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.

3 participants