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

Fix GPS Rescue parameters confusion #3554

Merged
merged 12 commits into from
Aug 31, 2023
Merged

Conversation

haslinghuis
Copy link
Member

  • Fix confusing parameter naming
  • Add initialClimb parameter
  • Add course over ground to heading
  • Fix range for some parameters

Firmware PR: betaflight/betaflight#13047

@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Aug 22, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • 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

Copy link
Member

@asizon asizon left a comment

Choose a reason for hiding this comment

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

Looks good!

@haslinghuis haslinghuis changed the title Fix GPS Rescue parameters Fix GPS Rescue parameters confusion Aug 22, 2023
@sugaarK sugaarK requested a review from ctzsnooze August 23, 2023 05:45
@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

Thanks @haslinghuis for these changes!

This is a screen dump showing both pyGPSclient and Betaflight Configurator handling the same data coming from a GPS in parallel.

Note that the GPS heading is shown in degrees, and matches the 'track' value in pyGPSclient. All other values match up also.

Screen Shot 2023-08-24 at 09 31 21

@ctzsnooze
Copy link
Member

When I go to check the Failsafe tab, the labels for the GPS rescue settings are now correct, and the message warning that return altitude only applies in FIXED altitude mode is very clear.

I tested using the Betaflight PR #13047, and it seems are some issues to fix (not sure which end):

  • The gps_rescue_initial_climb field is blank, it doesn't appear to read the value from the CLI
  • If I adjust gps_rescue_initial_climb to a value, and save, the value is not saved
  • The gps_rescue_return_alt range is 2-255m, default of 30m. In Configurator, with this PR, the range is 20-100m. Now frankly 20m return height is probably a safer minimum than 2m, but the user should be able to configure it down to say 5m, which should still be safe. Most likely we should set minimum to 5m in both places. The maximum height could be whatever the user wanted, within reason, and the two maximum values should match. Looks like we now have 16 bits, the old maximum of 255 was limited by 8 bits. In some mountain surfing situations I guess the user could want to set the height to 1000m; perhaps then the range should be 5-1000m in both firmware and configurator.

Screen Shot 2023-08-24 at 09 43 36

@ctzsnooze
Copy link
Member

One minor detail that I noticed, in relation to our 3D fix.

In Configurator we have an icon next to the text '3D fix' on the right.
I noticed that pyGPSclient reports a 3D fix sometimes well before we light up our 3D fix icon in green.
To get a 3D fix requires, for most modules, a minimum of 4 sats.
A 2D fix can be achieved with just 3 sats.

Here is an image showing a 2D fix with an erroneous altitude value, early on in a connection phase, with pyGPSclient reporting on the raw GPS values at the top:
Screen Shot 2023-08-24 at 10 14 34
Note that Betaflight shows the GPS icon, at the top, as being Red still.

Here is the situation where we get our 3D fix, with 5 sats in the solution. The altitude value is a bit different now.
Screen Shot 2023-08-24 at 10 17 57

A bit later on we get 3D fix with 11 sats in the solution. Horizontal accuracy (hAcc) is 1.29m and vertical accuracy (vAcc) is 2.27m. This is quite a lot better than before, but there isn't a clear graphical indication that we now have the 8 sats required for us to set a home point.

Screen Shot 2023-08-24 at 10 21 20

I wonder if we could change the display of the satellite number on the right so that it is shown like the 3D fix icon, ie like an icon or button, and make it:

  • red when there are not enough sats to have a 3D fix
  • orange when we have a 3D fix but less sats than the value of gps_rescue_min_sats
  • green when we have a 3D fix and more sats than gps_rescue_min_sats (ie, 8 or more sats, by default)

I wonder also if we could display the map as soon as we get any coordinates at all, rather than waiting for the 3D fix. Currently no map is shown until we get a 3D fix.

But it might be quite nice to see the map as soon as we receive latitude and longitude values. My reasoning is that the position indicated on the map would then run around all over the place initially, and as the number of sats increased, so would its accuracy. This might help people who choose the 'arm without home point' option visualise just how dangerous this is.

src/tabs/failsafe.html Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the fix-gps branch 4 times, most recently from b9deb0e to f9c1bfd Compare August 24, 2023 00:54
@haslinghuis
Copy link
Member Author

The gps_rescue_initial_climb field is blank, it doesn't appear to read the value from the CLI

Sorted now

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

Hmm, no, it seems like the gps_rescue_initial_climb field is still blank, and doesn't appear to read the value from the CLI. If I up-arrow it then shows '5', even if the value is 7 in the CLI.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the fix-gps branch 2 times, most recently from e530723 to 87f48e5 Compare August 26, 2023 23:59
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
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 172cb0b into betaflight:master Aug 31, 2023
9 checks passed
@haslinghuis haslinghuis deleted the fix-gps branch August 31, 2023 09:50
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.

None yet

5 participants