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

move baro altitude calculation to sensors #8466

Merged
merged 6 commits into from
Apr 12, 2018
Merged

move baro altitude calculation to sensors #8466

merged 6 commits into from
Apr 12, 2018

Conversation

dagar
Copy link
Member

@dagar dagar commented Dec 14, 2017

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?

@priseborough
Copy link
Contributor

priseborough commented Dec 14, 2017

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).

@priseborough
Copy link
Contributor

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.

@priseborough
Copy link
Contributor

priseborough commented Dec 15, 2017

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.

screen shot 2017-12-16 at 8 58 53 am

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.

@priseborough priseborough force-pushed the pr-baro_altitude branch 2 times, most recently from 30ed295 to f6c9652 Compare December 16, 2017 01:32
@dagar
Copy link
Member Author

dagar commented Dec 16, 2017

Updated to sitl_gazebo master.

@priseborough
Copy link
Contributor

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.

@dagar
Copy link
Member Author

dagar commented Dec 21, 2017

@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.

@priseborough
Copy link
Contributor

The method used by the old calibration helper was not valid. I'm happy for it to go in as is.

priseborough
priseborough previously approved these changes Dec 21, 2017
@dagar dagar changed the title [WIP] baro altitude calculation baro altitude calculation Dec 21, 2017
@dagar dagar changed the title baro altitude calculation move baro altitude calculation to sensors Dec 21, 2017
@santiago3dr
Copy link

couple flights on pixracer (V4)
https://logs.px4.io/plot_app?log=a1564af5-27ec-4aea-a9b6-c7a06699f272
https://logs.px4.io/plot_app?log=2b41b08c-fe6b-415d-8407-65e7a0fbedda

both indoor and full outdoor tests performed as expected; no issues

@dagar
Copy link
Member Author

dagar commented Dec 22, 2017

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.

@dagar dagar force-pushed the pr-baro_altitude branch from 629a123 to 06ec82e Compare March 26, 2018 14:20
@dagar dagar dismissed priseborough’s stale review March 26, 2018 14:20

ekf2_timestamps updated

@dagar dagar force-pushed the pr-baro_altitude branch 4 times, most recently from 7a12b8e to 67b376f Compare March 30, 2018 15:41
@dagar
Copy link
Member Author

dagar commented Mar 30, 2018

@MaEtUgR this PR saves ~5K of flash on px4fmu-v2.

@dagar dagar force-pushed the pr-baro_altitude branch 2 times, most recently from 78b1418 to 854dc01 Compare April 2, 2018 15:19
@mhkabir
Copy link
Member

mhkabir commented Apr 7, 2018

@priseborough Can you test replay functionality again? It would be great to get this in! Thanks!

@priseborough
Copy link
Contributor

OK, I will create some MC data and give it a test

@priseborough
Copy link
Contributor

Replay of data from a bench test is working.

priseborough
priseborough previously approved these changes Apr 9, 2018
@dagar
Copy link
Member Author

dagar commented Apr 9, 2018

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)?

@dagar dagar force-pushed the pr-baro_altitude branch from 727fdfa to 91c5a8f Compare April 12, 2018 02:18
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.

5 participants