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

Using asinf may leed to nan (not a number) #25

Closed
the-m-master opened this issue Jun 9, 2022 · 3 comments
Closed

Using asinf may leed to nan (not a number) #25

the-m-master opened this issue Jun 9, 2022 · 3 comments

Comments

@the-m-master
Copy link

Using 'asinf()' may lead to nan. In the function 'FusionQuaternionToEuler' a call is done to 'asinf', if the input of this is larger then 1 or less then -1, the result will be nan. All calculations using this pitch value (as a nan) will fail as well.

A potential solution might be something like:

float asinf_safe(float val) {
if (val >= 1.0f) { return (M_PI / 2.0f); }
if (val <= -1.0f) { return (M_PI / -2.0f); }
return asinf(val);
}

@xioTechnologies
Copy link
Owner

Thank you for raising the issue. The problem can be demonstrated using the following code. The plot ends after 45 samples because the remaining 55 samples are all NaN.

import imufusion
import matplotlib.pyplot as pyplot
import numpy

numberOfSamples = 100

ahrs = imufusion.Ahrs()
pitch = numpy.empty(numberOfSamples)

for index in range(numberOfSamples):
    ahrs.update_no_magnetometer(numpy.array([0, 0, 0]), numpy.array([1, 0, 0]), 1 / numberOfSamples)
    pitch[index] = ahrs.quaternion.to_euler()[1]

pyplot.plot(pitch)
pyplot.show()

image

After implementing the fix you have suggested, the plot shows all 100 samples as shown below.

image

As an aside, it is interesting to note that the transition from the final asinf result (-89.5) to the hardcoded -90.0f is visible in the above plot as step change. The transition becomes smoother when FUSION_USE_NORMAL_SQRT is used as shown below.

image

@xioTechnologies
Copy link
Owner

The fix has been applied as commit d7788de.

@the-m-master
Copy link
Author

Thanks! We are going to use it in the next release :)

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

No branches or pull requests

2 participants