GPS Rescue using MSP is not getting Velocity to Home

[AZDane]

When doing GPS Rescue using MSP, the quad was acting very erratically. Looking at the Blackbox data it was discovered that Velocity to home was not updating.

Examining the code it was discovered that calculateNavInterval() was not called when setting MSP for GPS.

Solution:
Added calculateNavInterval() function call into MSP GPS logic in src/main/io/gps.c and moved function calculateNavInterval() to above the call. Needed to update gpsSol.navIntervalMs so velocity to home can get calculated.

New code has been tested on quad with successful rescue.

code correction in collaboration with @ctzsnooze

[github-actions]

Do you want to test this code? You can flash it directly from Betaflight Configurator:

• Simply put #13639 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

[ctzsnooze]

Thanks @AZDane, great to work with you on this. Fantastic to find people who can find a problem, make a series of tests, find for a solution, and implement it via a PR.

Ps - I can’t figure how an MSP connected GPS rescue could have worked before this change?

[haslinghuis]

Should we remove calculateNavInterval() from line 2199 as now it is called twice in same function (2204 in this PR)?

[AZDane]

Should we remove calculateNavInterval() from line 2199 as now it is called twice in same function (2204 in this PR)?

The calls are in different cases of the switch statement where gpsSol.time is updated. If you put it outside the switch it will run even when gpsSol.time has not been updated. Don't know if there are unintended consequences by doing so.

[haslinghuis]

Yes might no longer needed in second [switch] case - avoiding duplication and reducing overhead.

[AZDane]

Yes might no longer needed in second [switch] case - avoiding duplication and reducing overhead.

But if you put it outside the switch it runs in every case, even when not needed, so what is the best approach? 2 of the cases renders different results of calculateNavInterval() because one case is based on gpsSol.time = ubxRcvMsgPayload.ubxNavPosllh.time and the other gpsSol.time = ubxRcvMsgPayload.ubxNavPvt.time

[ctzsnooze]

There could be some refactoring to put the call to calculate the time delta at the very end of the sequence, after new data, MSP or otherwise, is received.

With this PR we put the call to calculate the time delta in each place where we receive a new time stamp with a new packet of data, ie:

• line 2204 when we parse a new MSG_NAV_PVT packet (m10 type modules)
• line 2152 when we parse a new MSG_NAV_POSLLH packet (earlier modules that don't support Nav_pvt)
• and now at 1386 when we parse a new MSP packet.

Since we only get time updates with these specific elements, it seems to me that this is the correct approach.

A more substantial refactoring would require significant testing, and I think this bug fix should not be delayed by that.