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

Use attitude heading #3663

Merged
merged 5 commits into from
Dec 10, 2023
Merged

Use attitude heading #3663

merged 5 commits into from
Dec 10, 2023

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Dec 7, 2023

Summary from ctzsnooze:

Bugfix PR to:

  • stop the Map from jumping like crazy when a Mag is enabled, and
  • fix an issue where incorrect heading data would be shown in the Mag heading value in the GPS tab.

The PR displays the IMU heading value in the GPS tab, in place of the Mag X axis value, to the left of the GPS Heading value.

Previously the displayed Mag value was the Mag sensor X value converted to degrees. The X axis data alone returns a false heading value if the pitch or roll of the quad changes a tiny bit, and is, practically speaking, useless in the GPS tab.

Additionally, it is the IMU heading that we show in the Setup tab as the 'yaw attitude' of the quad (the 'heading' of the quad). It makes sense to use a consistent heading representation across Configurator.

Note that If a Mag is enabled, the IMU will derive heading from the Mag heading value, not the GPS value. If Mag is not enabled, the GPS heading will be shown.

Also note that the GPS Heading value will be incorrect (useless) unless the GPS is actually moving along the ground in a straight line at a velocity of at least 2m/s. Hence in most situations where both the computer that is running Configurator, and the quad itself, are stationary, the GPS value will not make sense.

That's why in most cases when a Mag is enabled, the IMU value will be 'correct', and the GPS value will be quite different. The only time both values will be the same is if the computer and quad are both walked, together, in the exact direction of the nose of the quad.

The 'madly jumping Map' issue was happening only when Mag was enabled. At present this is fixed by locking the Map stationary, as it should be (north always upwards). We are hoping to be able to rotate the icon, not the map, using the IMU value, but that may need to be in a different PR.

So this PR resolves the current problems with Mag display in the GPS tab. It has been tested and works.

I recommend we merge it now.

C

Original comments:

Also iProposal for renaming heading to clarify witch value is used.

  • setup tab uses MSP_ATTITUDE and FC.SENSOR_DATA.kinematics[X,Y,Z].
  • gps tab uses MSP_RAW_IMU and FC.SENSOR_DATA.magnetometer[X,Y,Z].

Heading on setup tab can be confused with magnetometer heading so like to rename it to Yaw.
For gps tab we rename heading text to Heading IMU / GPS:.

  • One problem found: Rotation using magHeading. @atomgomba may have some pointers. But did some investigation:

https://arduino.stackexchange.com/questions/18625/converting-three-axis-magnetometer-to-degrees

OpenLayers function setRotation(rotation) seems not working correctly with heading data.

All this assumes the XY plane is horizontal. Adding pitch or yaw movements would result in wrong calculation.

Here is a resource if we need a more accurate solution. https://www.mdpi.com/1424-8220/11/10/9182/pdf

@haslinghuis haslinghuis added this to the 10.10.0 milestone Dec 7, 2023
@haslinghuis haslinghuis self-assigned this Dec 7, 2023

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Dec 7, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> FAIL
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@haslinghuis haslinghuis changed the title Rename heading Rename attitude heading Dec 7, 2023
@ctzsnooze
Copy link
Member

When a properly calibrated Mag is active, the identified problem is:

Background:

In the main Setup tab, there is a yaw heading value that changes 'immediately' when the quad is quickly yawed, but about a half second later, a slower correction happens. I have previously assumed that this is an IMU-derived yaw heading value, and that the immediate change comes from gyro data, whereas a bit more slowly, the Mag information updates this value. Importantly, changing the CLI mag_declination value does NOT change this value. That's bad.

In the GPS Rescue tab, there is a 'raw Mag Heading' value. This changes immediately when the quad is yawed quickly. And, it responds to correctly to changes in the CLI mag_declination value. That's good.

We need to get consistent values in both places, and to be able to explain these differences.

Both values should respond to the CLI mag_declination value.

@haslinghuis haslinghuis changed the title Rename attitude heading Rename heading Dec 8, 2023
@haslinghuis haslinghuis changed the title Rename heading Rename attitude and mag heading Dec 8, 2023

This comment has been minimized.

This comment has been minimized.

@ctzsnooze
Copy link
Member

@haslinghuis Maybe we can simply use the MSP_ATTITUDE value for yaw when we want to show heading values from IMU.

MSP_ATTITUDE carries attitude.values.yaw from the flight controller. This is the IMU value for heading that the flight controller uses to control GPS Rescue. It incorporates Mag when Mag is enabled, otherwise comes from GPS.

So we should use the MSP_ATTITUDE value for yaw as the value to show on the main setup page, and for the value to be shown to the left of the GPS heading value in the GPS page. The label for these two values should be Heading IMU/GPS, not Heading Mag/GPS.

Regarding the map itself....

When GPS is 'stationary', its heading information is useless. Fortunately, the current Configurator leaves the map 'steady' when GPS alone is active.

But if Mag is active, the current Configurator code rotates the map, and makes it really shaky. And the rotation is affected by pitching the quad. This is ugly and needs fixing.

I would suggest that we do not rotate the map, but use the MSP_ATTITUDE value for yaw (which is smooth) to rotate the icon in the middle of the map and only when Mag is active.

Hopefully I've explained it clearly, looking forward to seeing how it works out. I can test easily.

@haslinghuis haslinghuis changed the title Rename attitude and mag heading Use attitude heading Dec 10, 2023

This comment has been minimized.

This comment has been minimized.

@ctzsnooze
Copy link
Member

Looking good. IMU values now on the GPS tab, updating properly, accounting for declination. Map is stable.
Thank you!

@ctzsnooze ctzsnooze self-requested a review December 10, 2023 13:32

This comment has been minimized.

Copy link

sonarcloud bot commented Dec 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit d23ea4b into betaflight:master Dec 10, 2023
9 checks passed
@haslinghuis haslinghuis deleted the fix-heading branch December 10, 2023 23:39
@HThuren
Copy link
Member

HThuren commented Dec 14, 2023

@haslinghuis @ctzsnooze but we loose the N symbol, pointing to North ?

@haslinghuis
Copy link
Member Author

Please check on master? As we have PR 3668 and 3669 on top of this.

chmelevskij pushed a commit to chmelevskij/betaflight-configurator that referenced this pull request Apr 27, 2024
* Rename heading

* Change gpsHeading message

* Use IMU heading on GPS tab

* Remove precision and rotation

* Code style fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

5 participants