-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
AP_Mission: do not use float functions on integers #27788
Conversation
pitch is int8_t, yaw is int16_t
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 must be correct. But I do wonder if this means were missing the NaN check on the command write. The incomming MAVLink could be NaN here:
ardupilot/libraries/AP_Mission/AP_Mission.cpp
Lines 1360 to 1367 in 748fe53
case MAV_CMD_DO_GIMBAL_MANAGER_PITCHYAW: | |
cmd.content.gimbal_manager_pitchyaw.pitch_angle_deg = packet.param1; | |
cmd.content.gimbal_manager_pitchyaw.yaw_angle_deg = packet.param2; | |
cmd.content.gimbal_manager_pitchyaw.pitch_rate_degs = packet.param3; | |
cmd.content.gimbal_manager_pitchyaw.yaw_rate_degs = packet.param4; | |
cmd.content.gimbal_manager_pitchyaw.flags = packet.x; | |
cmd.content.gimbal_manager_pitchyaw.gimbal_id = packet.z; | |
break; |
Presumably if NaN the intended behavior would be that it is not used. So we could set to a out of range value so it is ignored via the abs. I guess we would also have to return a NaN in the MAVLink read....
edit: It looks like NaNs are not in the spec, so we might be OK. https://mavlink.io/en/messages/common.html#MAV_CMD_DO_GIMBAL_MANAGER_PITCHYAW
This is probably the wrong way to fix this. We should probably add a We have the same problem with the handle-rate-commands block below the one I've modified here. |
I'm pretty sure that the caller is meant to set the unused fields to NaN |
@@ -330,8 +330,8 @@ bool AP_Mission::start_command_do_gimbal_manager_pitchyaw(const AP_Mission::Miss | |||
} | |||
|
|||
// handle angle target | |||
const bool pitch_angle_valid = !isnan(cmd.content.gimbal_manager_pitchyaw.pitch_angle_deg) && (fabsF(cmd.content.gimbal_manager_pitchyaw.pitch_angle_deg) <= 90); | |||
const bool yaw_angle_valid = !isnan(cmd.content.gimbal_manager_pitchyaw.yaw_angle_deg) && (fabsF(cmd.content.gimbal_manager_pitchyaw.yaw_angle_deg) <= 360); | |||
const bool pitch_angle_valid = abs(cmd.content.gimbal_manager_pitchyaw.pitch_angle_deg) <= 90; |
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.
a simple change would be to use -128 as a magic value meaning invalid for int8 rates, and set to -128 for NaN in the mavlink msg. If someone asks for -128 then instead set -127
It's in the message description, not the field descriptions.
|
We currently bounce NaN values in mission upload. The Mount backend doesn't cope if either value is NaN. I've created a branch which preserves NaN values in storage using INT8_MIN and INT16_MIN, but I reckon that's probably just a waste of flash space at this point. |
pitch is int8_t, yaw is int16_t
Completely untested. Well, CI will make sure it compiles.