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_GPS: fixed the check for disabling CONFIG_RATE_SOL config #29320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Feb 18, 2025

we should stop looking for SOL as soon as we know we have PVT, instead of waiting for all others to be cleared

may help with
https://discuss.ardupilot.org/t/fc-tries-to-config-gps-constantly-when-trying-to-arm-in-loiter-mode/129930

@tridge tridge added the GPS label Feb 18, 2025
@rmackay9 rmackay9 added the BUG label Feb 18, 2025
@rmackay9 rmackay9 mentioned this pull request Feb 17, 2025
32 tasks
we should stop looking for SOL as soon as we know we have PVT, instead
of waiting for all others to be cleared

may help with
https://discuss.ardupilot.org/t/fc-tries-to-config-gps-constantly-when-trying-to-arm-in-loiter-mode/129930
@tridge tridge force-pushed the pr-fix-M10-GPS-config branch from e6b7865 to 6ade8a5 Compare February 19, 2025 09:21
@peterbarker
Copy link
Contributor

I bisected the bug, this was the result:

a9dc7b440f7a5b9418733c7b88743767c38623d4 is the first bad commit
commit a9dc7b440f7a5b9418733c7b88743767c38623d4
Author: Andy Piper <[email protected]>
Date:   Mon Apr 29 15:46:53 2024 +0100

    AP_GPS: support GPSx_GNSS_MODE for F9P
    
    support detecting F9P hardware variant
    fix bug in extension buffer management
    support NEO-F9P GNSS configuration
    allow multiple configuration values to be set in one go
    phase F9 configuration to account for GNSS reset

@andyp1per
Copy link
Collaborator

@peterbarker looks right but will need testing on F9P if you did not do that already

@@ -498,6 +498,10 @@ AP_GPS_UBLOX::_request_next_config(void)
const config_list *list = config_M10;
const uint8_t list_length = ARRAY_SIZE(config_M10);
Debug("Sending M10 settings");

// don't mix F9 and M10
_unconfigured_messages &= ~CONFIG_F9;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't help but wonder whether a tweak to the code affected by the patch which introduced the problems might be better. In particular:

@@ -334,6 +336,12 @@ AP_GPS_UBLOX::_request_next_config(void)
         }
         break;
     case STEP_POLL_GNSS:
+        if (supports_F9_config()) {
+            if (last_configured_gnss != params.gnss_mode) {
+                _unconfigured_messages |= CONFIG_F9;
+            }
+            break;
+        }
         if (!_send_message(CLASS_CFG, MSG_CFG_GNSS, nullptr, 0)) {
             _next_message--;
         }

Renaming "supports_f9_config" to "supports_valget_and_valset()` might stop this happening again?

@MallikarjunSE
Copy link
Contributor

@peterbarker do you suggest testing on F10N too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants