Clarify EKF3 Affinity#7499
Conversation
|
Pre-commit errors fixed in: |
Cool! But... this is entirely independent from this PR, right? (Fixing typos in files this doesn't touch.) @cclauss tagging you just in case there's a typo in a PR number or something. 🤷 |
No worries for minor turbulence. Hurray for automation! Should the instructions mention pre-commit? If they do, I missed them. If not, no worries, I imagine that will come in time. |
Suggestions made in PR form 😄 |
| If affinity for that sensor-type is disabled, each lane uses the system's "primary" instance of the sensor. | ||
| (Affinity being disabled for every sensor-type is the conventional/default choice.) | ||
| The initial primary sensor instance is selected by a user-modifiable parameter. | ||
| The system may change which sensor instance is its primary, even in-flight, for example in case of a sensor fault. |
There was a problem hiding this comment.
This line is a bit confusing in this context. Yes, the lane can possibly change its primary sensor for some sensors mid-flight, for example, a compass. But I don't think that's true for all sensors.. for example a barometer
There was a problem hiding this comment.
I had details like this in mind with the note-box "This page [...] is not perfect on the details."
Do you think it's worth describing and then maintaining all the details on this page? If yes, additional commit(s) to fix them & remove the "this page is not perfect on the details" are welcome.
On the flip side: Do you think the page is better as it was before? That path forward also makes sense: Reject this PR and keep the content as it was.
There was a problem hiding this comment.
I think it would be better if you could say something along the lines of "There are some sensor types where the primary instance for the lane may change in flight, for example, the compass, which are not related to affinity but other health checks inside the EK3 code"
There was a problem hiding this comment.
I'm trying a reword, let's see if that addresses your concern.
| The system may change which sensor instance is its primary, even in-flight, for example in case of a sensor fault. | ||
|
|
||
| Because modern-day vehicles may have redundant good quality sensors installed, "affinity" provides a way to have some EKF lanes prefer sensor instances which are not the system-wide primary. | ||
| For sensors with affinity enabled, lane-switching should be thought of as sensor-switching. |
There was a problem hiding this comment.
I think this is over-simplification. It's possible that not all IMUs present in a flight controller perform equally well. Which means lane-switching with affinity enabled isn't just "sensor-switching". If a user hasn't tested all lanes (or at least checked logs for all lanes), they might switch to a worse lane.
There was a problem hiding this comment.
I agree it is a simplification. (I hoped "should be thought of" would communicate that... maybe it did?)
Over-simplification suggests it might be worse than some alternative... help me understand what you have in mind?
Also, I think there's no gap in my understanding of the topic, it's merely a discussion about what wording is appropriate. This PR is my attempt to find the right balance, but I'm glad to take it in the direction(s) you'd like!
Do you know github's "Suggest Changes" feature? That would be a great way to specifically communicate what differences you'd like to see, and my "Accept Suggestion" would confirm my agreement/understanding.
There was a problem hiding this comment.
Maybe you could just remove that last line?
There was a problem hiding this comment.
If I'm understanding correctly, I think it makes explicit a key concept: The system has 2 ways to avoid a "bad" sensor instance.
To confirm my own understanding, I'll state the 2 ways:
- Detect that it is faulted, and select a different instance as primary.
- Detect that it contributes to a high error-score, and lane-switch away from it.
And also to check my own understanding, each way has at least one downside:
- We never primary-switch just for high noise, right? Only for sensor "faults".
- Affinity 'hard codes' an instance to a lane. So for a 2-lane setup, if IMU1 has high noise and Airspeed2 has high noise, we're stuck. (The desired behavior would be to use IMU2 + Airspeed1, but that's not available. Thankfully, we always get the 'less bad' lane.)
Getting my mind around the fact that primary-switching and lane-switching-with-affinity were distinct but related was the key to my understanding affinity. (Assuming, of course, that I do now understand it... 😅)
|
Please rebase to pick up the typo fixes. |
543c994 to
61a4457
Compare
|
@rishabsingh3003 ready for your re-review, please |
61a4457 to
5215009
Compare
|
@rishabsingh3003 ping |
(I sent a discord DM, but we aren't yet friends, so that may not help... I'll wait a few days to see.) |
5215009 to
8c302bb
Compare
EKF3 affinity is difficult to understand from our wiki docs. This (hopefully!) improves it's clarity.
Reviewers: I determined this from source-diving. Please carefully examine this PR's content for mistakes.
Summary of significant points:
The first commit switches to one-sentence-per-line. I find this style much easier for future reviews/changes. (I did not fix the whole file. That has the downside of leaving us with mixed style in a single file. IMO, any movement towards one-sentence-per-line is improvement.) I will revert this to preserve the "one paragraph per line" style if any reviewer objects.
The second commit fixes some trailing whitespace. I'll drop it if any reviewer objects.
Testing:

Built locally, renders as expected. (The image may be outdated as we wordsmith via the review process.)