-
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
AP_OpticalFlow: fix for msp opflow sensor alignment #16226
Conversation
Txs for this. So there are no changes to the flow rates (the changes to these lines is just cosmetic) but the body rates are reversed? Seems OK to me. |
@rmackay9 yes, the bug was reversing only the flow, by installing the sensor as suggested by matek one could align body and flow but imu was reversed and had to apply a flow orient of 180°, by inverting both flow and body before applying flow orient you get aligned flow, body and imu with flow_orient = 0 as expected |
71da6d2
to
a1625cc
Compare
@rmackay9 now I got it right, but I think there's a bug in the mavlink opflow driver I used as a reference when writing the MSP one, the state.bodyRate vector comes from the gyros and we should not apply the FLOW_ORIENT_YAW compensation since gyro info comes from ahrs and not from onboard the flow sensor, what do you think? mavlink opflow bugged code is here ardupilot/libraries/AP_OpticalFlow/AP_OpticalFlow_MAV.cpp Lines 68 to 69 in b0fa5fa
I tested the MSP code against a Matek 3901 sensor pointing forwards sensor pointing right |
@yaapu, yes, I think you are right. The issue exists in the CXOF driver as well. This would mean that these drivers wouldn't work correctly unless the sensor was placed in its default position (i.e. FLOW_ORIENT_YAW = 0). Maybe create a separate PR to fix the other drivers too? We will have a bit of an issue because we will need to find someone to test them probably. |
@rmackay9 ok, I might have a CXOF sensor somewhere and should be able to test by myself but we'll need help for the mavlink one, thanks |
related PR #16256 for MAVLink and CXOF |
a1625cc
to
877586b
Compare
Merged, thanks! |
This fixes a bug in the MSP opflow driver.
It has been tested by @tenten8401, waiting for more testers to confirm the issue is solved.