-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[ECL] Moving towards multi-sensor-per-same-measurement fusion #13127
Conversation
346b7d3
to
cd92cc8
Compare
0c67074
to
11a8d41
Compare
@dagar This is running out of space on fmu-v2 :) |
Looks good |
@kamilritz you are also missing adding the RTPS ID's: http://ci.px4.io:8080/blue/organizations/jenkins/PX4_misc%2FFirmware-compile/detail/PR-13127/8/pipeline/60#step-619-log-45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamilritz is there any reason ware creating 2 new messages for this? Why not just use ekf2_innovations.msg
and add #TOPICS
aliases for the other two, as they seem to have the exact same fields?
This would be something like: (
Also, please add those aliases to the RTPS ID's list. |
@TSC21 That is a nice idea. The innovation_test_ratios fields are not exactly the same. But we can do this for innovation and innovation variances. |
@kamilritz the only difference I see is on the arrays. The same structure can still be used, but instead of using the entire array, just used the first array index to propagate the test ratio (this can be clarified on a comment). |
I am not sure how much short we can get on fmu-v2 to get this in :S |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for me now
db0a2d2
to
ea339c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break all post-flight tooling due to the uorb format changes - we need to take a more compliant approach for message changes. It is fine to break uORB to some degree, but ulog needs to be a more steady format.
Tested on pixhawk4 v5 f-450 Modes Tested: Procedure: Notes: Logs: Modes Tested Procedure Note: No issues were noted, good flight in general. Logs: https://review.px4.io/plot_app?log=38bf258c-29c1-43c2-ba2e-2f1df34e5af4 Flight Card 3 Procedure Note: Logs: https://review.px4.io/plot_app?log=a2b30f1d-1a01-40a6-b305-69ca7692d8cf Flight Card 4 Logs: Tested on PixRacer V4 f-450 Flight Card 1 Modes Tested: Procedure: Notes: Logs: https://review.px4.io/plot_app?log=1cfb8413-51a8-4493-97c1-61cbdc58048c Flight Card 2 Modes Tested Procedure Logs: Flight Card 3 Procedure Note: Logs: Flight Card 4 Logs: Software Crash |
Tested on NXP FMUK66 v3Flight Card 1
Flight Card 2
Flight Card 3
Tested on Pixhawk Pro v4Flight Card 1
Flight Card 2
Flight Card 3
Flight Card 4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some tiny changes, otherwise this side of the PR LGTM
{ | ||
const float vel_d_innov = innov.vel_pos_innov[2]; | ||
const float vel_d_innov = fmaxf(fabsf(innov.gps_vvel), fabs(innov.ev_vvel)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: longer term the preflight checks shouldn't use innovations at all , but rather the covariances of the states. In the mean time this is a conservative check that can be performed, but I think we all agree it isn't optimal.
_pub_innov.get().vel_pos_innov_var[i] = S(i, i); | ||
} | ||
|
||
for (size_t i = 3; i < 6; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a note, this was broken before
@Junkim3DR To see if the reported problems in landing detection are for sure related to this pr and what exactly was different on that particular setup it would help a lot to have the log when you compared with master and had no issues as well. Do you still have this log? Could you upload it? |
Hi @MaEtUgR, I believe this are the 2 logs I tested with master to see if the issue persisted. Logs |
@MaEtUgR it's the same setup nothing was changed. |
@Junkim3DR can you please fly the NXP FMUK66 v3 with flight card 2&3 again. Once with this PR and once with current master. Please perform the same flights with both firmware version. Thank you very much. |
Remaining things before we merge this (we're very close!):
|
@mhkabir any ideas how I can investigate this? |
Tested on NXP FMUK66 v3Flight Card 2
Flight Card 3
|
9428d48
to
8c105d2
Compare
@Junkim3DR Thanks for the new log files. There is definitively something odd. We would need another log file that we can use for EKF replays. Can you flash the newest version of this PR, set SDLOG_PROFILE=2, set SDLOG_MODE="from boot to disarm", fly Flight Card 2 again and upload the log. |
@Junkim3DR Which master firmware were you running as reference? |
8c105d2
to
48f892a
Compare
Tested on NXP FMUK66 v3Flight Card 2
Master used was latest form QGC the 12th of December 2019, commit: 9b66cbd |
@Junkim3DR Thank you very much! @mhkabir @dagar Together with @priseborough we tracked down the issue in the first test flights to extensively filtered baro data on the I argue that it is okay to merge this now. LGTM!!!! (Reminder: ECL PR has to go first: PX4/PX4-ECL#668) |
I've checked the logs from Mexico testers who were using the standard GPS sensor fit and daa looked OK. |
addressed issues
This ECL PR aims to provide unambiguous logging of velocity and position measurement updates in the EKF2 estimator.
To enable this, we need to provide the right uorb messages on the Firmware side. Making the measurements sensor specific would increase the number of fields in
ekf2_innovations.msg
.To avoid this we split the message in to sensor specific innovations, innovation variances and innovation test ratios.
These messages have different use-cases and can be enabled accordingly.
Logs of several test flights can be found in the ECL PR.
The
ekf2_innovations.msg
is marked as deprecated. The fields are replaced according to the following scheme:ekf2_innovations.vel_pos_innov -> estimator_innovations.[gps_hvel, gps_vvel, gps_hpos, gps_vpos, ev_hvel, ev_vvel, ev_hpos, ev_vpos, fake_hvel, fake_vvel, fake_hpos, fake_vpos ]
ekf2_innovations.mag_innov -> estimator_innovations.mag
ekf2_innovations.heading_innov -> estimator_innovations.heading
ekf2_innovations.airspeed_innov -> estimator_innovations.airspeed
ekf2_innovations.beta_innov -> estimator_innovations.beta
ekf2_innovations.flow_innov -> estimator_innovations.flow
ekf2_innovations.hagl_innov -> estimator_innovations.hagl
ekf2_innovations.vel_pos_innov_var -> estimator_innovation_variances.[gps_hvel, gps_vvel, gps_hpos, gps_vpos, ev_hvel, ev_vvel, ev_hpos, ev_vpos, fake_hvel, fake_vvel, fake_hpos, fake_vpos ]
ekf2_innovations.mag_innov_var -> estimator_innovation_variances.mag
ekf2_innovations.heading_innov_var -> estimator_innovation_variances.heading
ekf2_innovations.airspeed_innov_var -> estimator_innovation_variances.airspeed
ekf2_innovations.beta_innov_var -> estimator_innovation_variances.beta
ekf2_innovations.flow_innov_var -> estimator_innovation_variances.flow
ekf2_innovations.hagl_innov_var -> estimator_innovation_variances.hagl
ekf2_innovations.output_tracking_error -> estimator_status.output_tracking_error
ekf2_innovations.drag_innov -> estimator_innovations.drag
ekf2_innovations.drag_innov_var -> estimator_innovation_variances.drag
ekf2_innovations.aux_vel_innov -> estimator_innovations.[aux_hvel, aux_vvel]
not existing -> estimator_innovation_variances.[aux_hvel, aux_vvel]