Skip to content

Remove barometer median filter#8201

Merged
DzikuVx merged 2 commits intomasterfrom
de_remove_baro_median
Jul 13, 2022
Merged

Remove barometer median filter#8201
DzikuVx merged 2 commits intomasterfrom
de_remove_baro_median

Conversation

@digitalentity
Copy link
Member

This is no longer useful for multiple reasons:

  • New baros are accurate enough and don't produce altitude glitches
  • Filter creates unnecessary delay which affect the estimation performace (minimally, but nevertheless not negligible)
  • Glitch prevention working together with median code is actually harmful (causes the barometer data to freeze when rapid altitude change is observed)

@digitalentity digitalentity added Cleanup/refactor Ready to merge Release Notes Add this when a PR needs to be mentioned in the release notes CLI Add this when new CLI commands needs to be added to docs labels Jul 9, 2022
@digitalentity digitalentity requested a review from DzikuVx July 9, 2022 11:40
@DzikuVx
Copy link
Member

DzikuVx commented Jul 9, 2022

@digitalentity do you have any flight logs?

@digitalentity
Copy link
Member Author

Not really. I don't have the solid data to back the "delay" claim, but I have seen the "glitch prevention" code causing issues (especially on the MSP barometer).

@JulioCesarMatias
Copy link
Collaborator

In the past, I've thought about removing this filter... But the tests told me that the baro gets quite noisy! I don't know if this step is correct, since the INS is quite limited... I've just tested your modifications and the baro is really noisy. I can't say if in flight there will be any difference... Let's test this! My Hardware: MatekF405SE with baro DPS310.

@digitalentity
Copy link
Member Author

Noise itself is not a problem and the median filter doesn't help reduce it much. Median filter is there because some baros were spotted in the past giving occasional extreme readings (plus/minus hundreds of kilometers).

Copy link
Collaborator

@JulioCesarMatias JulioCesarMatias left a comment

Choose a reason for hiding this comment

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

LGTM!

@digitalentity
Copy link
Member Author

Let's not merge this just yet, I'll replace median filter with something else that was tested on another code.

@JulioCesarMatias
Copy link
Collaborator

JulioCesarMatias commented Jul 9, 2022

Let's not merge this just yet, I'll replace median filter with something else that was tested on another code.

Could you give a spoiler of what it is? 🧐 😈

@digitalentity
Copy link
Member Author

digitalentity commented Jul 9, 2022

Please disregard my last comment. The thing I was referring to is the LPF on the barometer before feeding it into the INS. We have that (1 Hz LPF on baro) inside the INS (shame on me for forgetting).

@JulioCesarMatias
Copy link
Collaborator

Yes we do... Now it might be useful... Glad you didn't let me remove it in the past 😆 🙃

@digitalentity
Copy link
Member Author

@DzikuVx let's move this to 5.1?

@DzikuVx
Copy link
Member

DzikuVx commented Jul 11, 2022

@digitalentity ok, the release branch is there, please rebase

@DzikuVx
Copy link
Member

DzikuVx commented Jul 11, 2022

On other hand, this is a breaking change as it makes sense to put it in the next major, not minor

@digitalentity
Copy link
Member Author

Ok, let's keep it scheduled for 6 then. This will also fix #8224

@digitalentity digitalentity added this to the 6.0 milestone Jul 13, 2022
@DzikuVx DzikuVx merged commit 4d4e279 into master Jul 13, 2022
@DzikuVx DzikuVx deleted the de_remove_baro_median branch July 13, 2022 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cleanup/refactor CLI Add this when new CLI commands needs to be added to docs Ready to merge Release Notes Add this when a PR needs to be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants