-
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
move baro altitude calculation to sensors #8466
Conversation
d9de581
to
f391039
Compare
That depends on what we mean by 'calibration'. This could mean an 'auto-zero' so that the baro altitude correctly reports height above field level or height above sea level which is normally correction for changing atmospheric conditions, not a calibration of sensor error. At the moment we have the ability to set a SENS_BARO_QNH value, which is the way it is done in full sized aircraft, either by setting to QNH to get altitude above MSL or to QFE to get height above airfield. For our application where operators usually don't know/understand QFE and QNH, it could be achieved by setting SENS_BARO_QNH to a value that returned the specified height. I would also add a parameter to set the temperature offset applied to the international standard atmosphere when calculating the pressure altitude. Our calculation uses an ISA+5 temperature which is not compatible with the ISA convention used by manned aviation. Edit: Correction, it looks like we are using ISA for the height calculation. Calibration could also mean intrinsic calibration of sensor scale factor or offset. That can only be done against a known pressure. If you look at the data sheet for a MS5611, you will see that error varies across pressure range, even after an auto zero, so all we can achieve is an offset adjustment so that it reads the supplied pressure (this would be QFE if operating at an airfield). |
The sensor_combined topic description for baro_alt_meter does not specify whether it is altitude above MSL or above field elevation which requires the MSL altitude measured at startup to be subtracted. Current behaviour for PX4 master is to provide height above field elevation which requires the sensor module to subtract the average altitude measured at boot. The sensor_combined topic should also be updated to make it clear that this is measured relative to altitude at boot. Longer term this is not ideal because pressure altitude above MSL is a useful piece of information which is lost when we do an auto zero. The use of relative altitude above field level in aviation is typically reserved for local area work around an airfield but is not used when flying further or taking off from one airfield and landing at another. It also means the pressure altitude cannot be used to compensate for changes in aircraft performane with altitude. I think baro_alt_meter should always return absolute pressure altitude as would be returned by an aviation altimeter with the QNH set to SENS_BARO_QNH. If that is the case then no 'auto zero' on boot is required, however the estimators will need to auto-zero on initialisation so that local_position.z is relative to startup. |
I just found a bug in SITL when testing master using make posix_sitl_default gazebo that was misleading me. The sitl baro sensor is returning a sea level reading despite being at 488 metres. This means current master is returning an absolute pressure altitude relative to MSL. I will update the sensor_combined message description accordingly and also see if I can fix the SITL error. |
30ed295
to
f6c9652
Compare
f6c9652
to
16cfb73
Compare
Updated to sitl_gazebo master. |
16cfb73
to
663dc52
Compare
I've checked and the ecl/EKF is handling the auto-zero during filter alignment so no auto-zero in the sensor module is required. |
663dc52
to
603d950
Compare
@priseborough what else would you like to tackle in this PR? With this PR we lose the per sensor command line calibration helper, but I don't think they were used and some may have been broken. |
The method used by the old calibration helper was not valid. I'm happy for it to go in as is. |
couple flights on pixracer (V4) both indoor and full outdoor tests performed as expected; no issues |
603d950
to
6d7dc84
Compare
I don't believe it's related to anything you did, but something is wrong with sitl_gazebo. The version in master is 3 months behind. |
7a12b8e
to
67b376f
Compare
@MaEtUgR this PR saves ~5K of flash on px4fmu-v2. |
78b1418
to
854dc01
Compare
@priseborough Can you test replay functionality again? It would be great to get this in! Thanks! |
OK, I will create some MC data and give it a test |
Replay of data from a bench test is working. |
I may have gone overboard with the ekf2_timestamps additions. https://github.com/PX4/Firmware/blob/854dc0111c432482742b57fd3c4fb9aef1753d5a/msg/ekf2_timestamps.msg @priseborough / @bkueng - could you clarify when we need the additional ekf2_timestamps relative timestamp recorded? For example, in master why don't we need to record ekf2 relative timestamps for the supporting topics like vehicle_status, sensor_selection, etc? Is it only needed when the timestamp field doesn't correspond with when the message is actually published (available to copy in ekf2)? |
…old logs Firmware PR: PX4/PX4-Autopilot#8466
Centralizing the altitude pressure calculation removes a ton of duplication (5k of flash on fmu-v2 alone) and seems better architecturally.
What do we need for baro calibration? Should it happen every boot?