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

AP_NavEKF2: clean up init failure handling #12799

Open
wants to merge 1 commit into
base: master
from

Conversation

@kd0aij
Copy link
Contributor

kd0aij commented Nov 8, 2019

If I am right, _enable was being cleared only to reduce noise from the gcs().sendtext messages which are deleted by this PR.
_enable is used nowhere else, except for being forced true for replay.

There is now only one sendtext remaining in InitialiseFilter(): "NavEKF2: core %d setup failed"
and it is also redundant to an initFailure message, although it does provide additional info as to which core setup failed.

@kd0aij

This comment has been minimized.

Copy link
Contributor Author

kd0aij commented Nov 9, 2019

I don't think there's any harm in repeated calls to malloc, at least while disarmed, and I believe @tridge said that _enable was cleared just to prevent spamming the gcs with test messages.

But wouldn't a check for (initFailure == NO_MEM) accomplish what you suggest without adding a new variable? Also, perhaps we should make sure InitialiseFilter is never called while armed?

@rmackay9

This comment has been minimized.

Copy link
Contributor

rmackay9 commented Nov 9, 2019

@kd0aij, I worry that if EKF2 and EKF3 were enabled that one of the two of them could fail to allocate and then we could get the constant malloc calls while flying.

yes, it's possible the initFailure state could be used instead of creating a new variable. I was just slightly concerned that it could be more fragile that way. I.e. someone could come along later and break it without realising it.

@rmackay9

This comment has been minimized.

Copy link
Contributor

rmackay9 commented Nov 9, 2019

LGTM, do you want to put the send_texts back? I don't mind too much either way.. but having them is more consistent with how we handle allocation failures in other places... but up to you.

only attempt to allocate memory once
@kd0aij kd0aij force-pushed the kd0aij:pr-fix-ekf-initfailure branch from ed464e3 to d545e9d Nov 9, 2019
@kd0aij

This comment has been minimized.

Copy link
Contributor Author

kd0aij commented Nov 9, 2019

restored the send_texts and tested in SITL
then squashed

Copy link
Contributor

rmackay9 left a comment

This looks good to me thanks. This will also resolve this issue #12712

@kd0aij kd0aij closed this Nov 12, 2019
@kd0aij kd0aij reopened this Nov 19, 2019
@rmackay9

This comment has been minimized.

Copy link
Contributor

rmackay9 commented Nov 19, 2019

@tridge, when you get a chance maybe you can have a peek at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.