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

Wind & airspeed scale estimator #6386

Closed
wants to merge 18 commits into from
Closed

Conversation

RomanBapst
Copy link
Contributor

No description provided.

@dagar
Copy link
Member

dagar commented Jan 19, 2017

Validate with flights logging this, but not relying on it?

Do you think it should be in ekf2 like this, or could it live in sensors/airspeed getting the velocity variance from the uorb message?

@dagar dagar self-requested a review January 19, 2017 15:43
@dagar
Copy link
Member

dagar commented Jan 19, 2017

Adding myself as a reviewer for testing, but someone that better understands the derivation should actually review.

@priseborough would you mind reviewing?

@priseborough
Copy link
Contributor

I would prefer for this to live in sensors/airspeed and include the differential pressure scale factor error as an additional state. It would then solve the calibration problem we have and also mean that the estimates are available if ecl is not loaded.

@RomanBapst
Copy link
Contributor Author

@dagar @priseborough Sounds like a good idea. What do you think about the parameters? Do we need to share noise parameters with ekf2 or should we have separate params? (wind process noise, beta noise, airspeed noise)

@LorenzMeier
Copy link
Member

@Tumbili Can you fix CI / style failures?

"""

from sympy import *

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see the use of sympy. Any chance you can include an ipython notebook with the rendered equations? I think it would be worthwhile since github renders them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgoppert I'll see what I can do. I never used ipython notebook before but I know it's cool.

@RomanBapst
Copy link
Contributor Author

@LorenzMeier @dagar @priseborough I'm happy to help fully integrate this based on the above comments. @priseborough mentioned that it should not depend on ekf2 (or ecl) which makes sense.
Does it fit into the sensors module? Or should it rather go into a separate module / work queue?

The estimator needs the following data to run:

  • dt for integration
  • ground velocity
  • attitude estimate
  • measured airspeed
  • parameters

@dagar
Copy link
Member

dagar commented Jan 31, 2017

I think it would fit cleanly into a standalone module that also incorporates the true and indicated calculations. Then it could be the only publisher of airspeed.
https://github.com/PX4/Firmware/blob/master/src/modules/sensors/sensors.cpp#L384

@RomanBapst
Copy link
Contributor Author

@dagar So if I understand you correctly you would move the entire diff pressure stuff out of the sensors module into a new one? Or are you rather talking about a class residing in the sensors module?

@dagar
Copy link
Member

dagar commented Feb 2, 2017

Either way sounds acceptable to me, unless anyone objects to a scale estimator living in the sensors module?

@LorenzMeier
Copy link
Member

@priseborough Can you chime in here?

@priseborough
Copy link
Contributor

I'm happy for it to go in the sensors module. If it is executed after the publishing of the sensors_combined topic, then it won't impact on latency for the attitude loops.

@RomanBapst
Copy link
Contributor Author

@priseborough ok, I'll move it there then!

@RomanBapst
Copy link
Contributor Author

Airspeed scale and wind states
states

Airspeed innovation + airspeed innovation sigma
airspeed_innov

Sideslip innovation + sideslip innovation sigma
beta_innov

@@ -964,19 +964,19 @@ void Ekf2::task_main()
}

// Publish wind estimate
struct wind_estimate_s wind_estimate = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dagar @priseborough @LorenzMeier Need a solution for this.

@RomanBapst
Copy link
Contributor Author

@priseborough @dagar I moved the wind estimator to the sensors module. Above, you can see some plots from the three states and innovations + variances. This was tested in SITL when the plane was flying a circle.

if (fuse_airspeed) {
matrix::Vector3f vel_var(_control_state.vel_variance);
vel_var = R_to_earth * vel_var;
_wind_estimator.fuse_airspeed(_airspeed.indicated_airspeed_m_s, &vI._data[0][0], &vel_var._data[0][0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dagar @priseborough I used indicated airspeed here because "true airspeed" seems to be rubbish in SITL, very small values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
needs a solution

Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
Signed-off-by: Roman <bapstroman@gmail.com>
@LorenzMeier
Copy link
Member

@Tumbili Can you rebase and update this?

@LorenzMeier
Copy link
Member

@Tumbili @dagar Can we push this forward?

@LorenzMeier LorenzMeier added this to the Release v1.7.0 milestone Jun 15, 2017
@bresch
Copy link
Member

bresch commented Jun 15, 2017

I tried this PR about two weeks ago and I wasn't able to calibrate the airspeed sensor anymore. Not completely sure if it was because of this PR, I only tried once.

@LorenzMeier
Copy link
Member

Closing in favor of rebased version: #7424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants