-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
GCS_MAVLink: Do not operate on variables with NAN set #17865
GCS_MAVLink: Do not operate on variables with NAN set #17865
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.
it isn't clear that silencing these is the right thing to do given QGC now may send these. I think this PR would stop the SITL fault, but wouldn't leave us with right behaviour
@@ -2569,6 +2569,9 @@ uint8_t GCS::get_channel_from_port_number(uint8_t port_num) | |||
|
|||
MAV_RESULT GCS_MAVLINK::handle_command_request_message(const mavlink_command_long_t &packet) | |||
{ | |||
if (isnan(packet.param1)) { |
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.
this would deny on commands that don't use any of the parameters, that doesn't seem reasonable
out.param2 = in.param2; | ||
out.param3 = in.param3; | ||
out.param4 = in.param4; | ||
out.param1 = isnan(in.param1)?std::numeric_limits<double>::quiet_NaN():in.param1; |
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.
what impact does this change make?
and why double?
if (command_long_stores_location((MAV_CMD)in.command)) { | ||
out.x = in.param5 *1e7; | ||
out.y = in.param6 *1e7; | ||
} else { | ||
out.x = in.param5; | ||
out.y = in.param6; | ||
out.x = isnan(in.param5)?std::numeric_limits<double>::quiet_NaN():in.param5; |
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.
this is putting a nan into an int?
A discussion about AP's handle of NANs came up here in the 4.1.0 beta testing category https://discuss.ardupilot.org/t/copter-4-1-0-rc1-released-for-beta-testing/75878/32 |
NeedRework since 2021; please re-open if you want to pursue, having addressed the review comments. |
1 similar comment
NeedRework since 2021; please re-open if you want to pursue, having addressed the review comments. |
The variable value that NAN was set to was set to an integer type variable.
This setting caused SITL to stop processing.
I expect ArduPilot to work properly even when NAN is set in GCS.
I know that the GCS developers do not understand the source of ArduPilot.