-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
px4iofirmware use std NAN instead of undefined 0.0f/0.0f #9280
Conversation
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.
see my one comment, otherwise it looks good to me. I will try test it on my platform this weekend
@@ -604,7 +591,7 @@ mixer_callback(uintptr_t handle, uint8_t control_group, uint8_t control_index, f | |||
|
|||
if (should_prearm && control_group == actuator_controls_s::GROUP_INDEX_ATTITUDE && | |||
control_index == actuator_controls_s::INDEX_THROTTLE) { | |||
control = NAN_VALUE; | |||
control = std::numeric_limits<float>::quiet_NaN(); |
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.
My one concern about this line is that std::numeric_limits<float>::quiet_NaN()
can actually return a number that is not Nan if they are unsupported for the datatype. You can check their support by putting an assert on std::numeric_limits<float>::has_quiet_NaN
to make sure. Alternatively you could also use the NAN macro here as was done in the mixer.cpp as I believe this macro is only defined if the NAN exists.
@@ -37,21 +37,6 @@ | |||
* Simple summing mixer. | |||
*/ | |||
|
|||
#include <px4_config.h> |
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.
I am not sure I am up to reviewing the parts of the code where you have removed a large number of includes, however if it still compiles I guess they were not needed
@ZacharyTaylor please review.