-
Notifications
You must be signed in to change notification settings - Fork 19
Replace FP division with fixed-point shift in atan2 #110
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
base: main
Are you sure you want to change the base?
Conversation
The double-precision division was replaced with bit shift operation in twin_atan2_first_quadrant(), using right shift by 15 bits equivalent to division by 32768.
@@ -148,7 +148,8 @@ static twin_angle_t twin_atan2_first_quadrant(twin_fixed_t y, twin_fixed_t x) | |||
} | |||
} | |||
|
|||
return (twin_angle_t) (double) angle / (32768.0) * TWIN_ANGLE_360; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this commit also aim to fix the incorrect casting of the variable angle
from twin_angle_t
to double
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this commit also aim to fix the incorrect casting of the variable
angle
fromtwin_angle_t
todouble
?
Yes, intentionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, why didn't you write this purpose on this commit log ?
@@ -148,7 +148,8 @@ static twin_angle_t twin_atan2_first_quadrant(twin_fixed_t y, twin_fixed_t x) | |||
} | |||
} | |||
|
|||
return (twin_angle_t) (double) angle / (32768.0) * TWIN_ANGLE_360; | |||
/* Fixed-point conversion: angle * TWIN_ANGLE_360 / 32768 */ | |||
return (twin_angle_t) ((angle * TWIN_ANGLE_360) >> 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since TWIN_ANGLE_360=4096 = 2^12
by simple simplification, the original expression can be represented as
which is equivalent to
+/* Fixed-point conversion: angle * TWIN_ANGLE_360 / 32768 */
+ return (twin_angle_t) (angle >> 3);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the value range of angle
is
(angle * TWIN_ANGLE_360) >> 15
with TWIN_ANGLE_360 = 4096
can cause overflow. The maximum possible result of the multiplication is:
This value exceeds the limits of 16-bit, it may result in integer overflow and unpredictable behavior.
To avoid this issue, the multiplication and division can be replaced with a simple bit shift:
angle >> 3
The double-precision division was replaced with bit shift operation in twin_atan2_first_quadrant(), using right shift by 15 bits equivalent to division by 32768.
Summary by Bito
This pull request optimizes the `twin_atan2_first_quadrant` function by replacing a double-precision division with a fixed-point bit shift operation, enhancing performance for angle calculations in the twin angle system.