M10 ValSet support, GPS connection stability

[freasy]

Marked ready to merge; extensively bench tested, has been flight tested. All known functional issues have been addressed.

This PR is @freasy's work, primarily. With @ZzyzxTek's help, I've done much of the comment handling, some refactoring, and a lot of testing and documentation. However it was @freasy who single-handedly started this project. He set in place all the massive changes needed to properly support the new ValGetSet type M10 UBlox commands, to identify the type of module we are working with and configure it appropriately, and essentially all the underpinnings that make this PR work so well.

Users should now expect a reliable and solid hardware connection from nearly all modern GPS modules, leading to smoother and better GPS rescue control.

UBLox is strongly recommended. NMEA 'sort of'l works, but simply isn't well supported both in our software and in the modules themselves.

The recommended baud rate is 57600. There is little to be gained by using 115200 with the recommended 10hz data rate. Lower baud rates reduce the CPU time required for parsing the incoming data and are more resistant to electrical noise.

The recommended data rate is 10Hz for GPS Rescue purposes. 5Hz or less is recommended if the GPS is only for fixes on the radio in case the quad is lost. If an M8 module is set at 20Hz, it will revert to its own internal default rate, which could be very slow. 20Hz requires 115200 and M9 or higher, and is not recommended as it may be susceptible to electrical interference.

Data Rate	Baud Rate	CPU cost	Comments
20hz	11500	Highest	M10 only; test carefully!
10hz	38400 or higher	Medium	57600 and 10hz is recommended for GPS Rescue
5hz	19200 or higher	Medium	For GPS Rescue or general use
1-2Hz	9600 or higher	Leasts	Too slow for smooth GPS rescue, OK for simple position/speed info

The recommended PID loop rate while using GPS is 8k4k. 8k8k may simply not have enough CPU time available in the loop to support higher baud rates. GPS code requires almost as much time as the Rx or OSD code. It is quite expensive. Check the tasks CLI command and the percentage CPU usage when using 8k8k.

ToDo's:

• ack-nack code: find out why sometimes asks appear to be 'ignored' while in waiting state at low baud rate, starting at UBLOX_MSG_DISABLE_NMEA for the next 11 Configure states; decide the best action if wait time expires; decide what to do for nack in different situations.
• fix a possible issue identified by PL, where, in the phase of disabling NMEA messages for M9 or higher, we might go back to the wrong place in the case list if waiting for ack.
• finalise which things should be included in the GPS_CONNECTION debug once PR is complete, update BB log viewer accordingly
• Many NMEA fixes are required: proper baud rate negotiation, switch/case structure for connection, proper NMEA standard messages to change update rate, requested update rate to match user config in CLI, etc etc - much to do here

Note 1: NMEA may work, but with the following caveats:

• Baud rate auto-configuration may not work. If so, use uCenter to permanently save the power-on baud rate to a suitable rate (eg 38400 or 57600)
• your configured data update rate setting may be ignored. Many NMEA units only send data at 1s intervals on power up; use uCenter to permanently configure a higher update rate, eg 5hz or 10hz, on power up, and use the debug to check that you're getting the update rate you intended.
• Note: NMEA only uses the 12 best sats in a Nav solution, so that number on the right, in Configurator, never goes above 12. NMEA also reports signal quality only on 12 sats. If you see 12, probably many more are 'in view', and the best 12 are bring used in the solution.
• the following NMEA improvements have been implemented in this PR:
• UBX NAV_PVT and NAV-SAT messages are disabled on power up, reducing incoming message traffic.
• On power up,without Configurator connected, NMEA GSV (sat info) messages are not requested, reducing incoming message traffic
• The NMEA GGA message time stamp is used to calculate the Nav Solution interval

Note 2: The GPS 'AutoBaud' switch, in Configurator, has been disabled in this PR. It now achieves nothing, either way; leave it off.

Note 3: When more than about 25 sats are part of the Nav solution, some UBLox modules randomly drop Nav packets. This appears in the debug as sample intervals of 200 and occasionally 300ms. This seems to be a problem with some GPS hardware modules, it's not a Betaflight problem.

Update 2023-08-12

• Dashboard / OLED code defined out; USE_DASHBOARD would be required in a build to enable that code. Saves flash space and identifies which variables are purely for this functionality. Thanks @ZzyzxTek
• Re-named parameters and improved comments, to facilitate future code review - thanks @ZzyzxTek
• Addresses issues with the Scheduler peak detection algorithm to better handle isolated CPU time peaks that only happen during the connection phase and were pushing the gps task allocation excessively high
• Added delays to reduce the likelihood of CPU spikes.
• Fixed the problem causing the M8 to not connect properly by adding a LONG (one second) delay to settle the module's serial port after changing its baud rate. M10's don't have this problem so perhaps in future we could reduce this time for M10s to give a faster connect phase.
• Fixes DOP shown as zero by fixing the source value, thanks @ZzyzxTek
• Improves handling of a potential issue with excessively long input strings by aborting any that exceed UBLOX_MAX_PAYLOAD_SANITY_SIZE, thanks @ZzyzxTek
• Improves corrupted message handling by aborting early when bad sync bytes, sizes or checksums are found, thanks @ZzyzxTek

Update 2023-08-08

• implemented a CONFIG_COMPLETE final state; much neater at that point now; thanks @ledvinap
• thoroughly investigated and tested CPU usage. Improved and tested the scheduler 'peak load' tracking. Much gratitude to @SteveCEvans for a lot of advice and guidance. The CPU peak tracking mechanism handles transients during Config stage a bit better. The CLI 'tasks' command now shows very few late commands. Reliable scheduler timing confirmed with typical max CPU of around 33-38us at 38400, 57600 and 115200 baud. - CPU usage is logged, other changes to the debugs.
• rearranged the CONFIG sequence so that we send messages to prevent unwanted incoming packets as early as possible. However, they keep coming in for several seconds, and handling them while sending config messages is why we get CPU spikes during the config period. The proper solution is to send the commands to block incoming data and then pause, but it's not essential I think.
• restructured the ACK code a little and checked that ACK_WAITING is handled properly. Logging shows the ACK command being received some time after a command, and the ACK_WAITING state then clears and we move on to the next config stage. Clearly visible in logs. One residual strange thing to resolve is that it ends up in ACK_WAITING after configuring the flight mode, not quite sure why that is.
• removed unnecessary ubloxSendMessage() call
• flxed longstanding indentation issue
• removed ubloxSendNAV5Message() elements which only re-stated UBlox defaults, in order to reduce CPU cost of that message.
• re-named some states to better reflect their intent

Update 2023-08-04

• Fixed OSD display of DOP; it shows PDOP, not HDOP, since NAV-PVT only gives us PDOP
• 'now' is set as gpsData.now = millis() once only, when gpsUpdate() is called, avoiding multiple recalculations of `millis()
• addressed comments

Update 2023-08-01

• No longer spamming the UARTs with test messages at rates that could be the fast GPS task rate (1000hz), instead send test messages only at 120ms intervals, and wait 500ms for an answer, to avoid issues with overloading the data input buffers.
• modify the Rxbuffer input methods using suggestions from @ledvinap and @adam-ah which collect characters in groups of up to 20 before doing the timeout test, speed up the GPS task as soon as we receive a single character, so that we will quickly come back to get whatever comes in next, and finally wait for one clear buffer before slowing back down again.

Update 2023-07-29

• Removed the gps_info code that, if enabled in a custom build, would show the software version details in the CLI, after many comments about its utility. This can be the subject of a new PR.
• Thanks to @ZzyzxTek, who found, and fixed, the issue where an excessively large sat string would fail to be analysed. This allows GPS_SV_MAXSATS_M8N to be set back to 32 sats, which is the maximum that can be viewed in Configurator.
• The interval between successive GPS Nav solutions is now calculated within the Nav Data code, and is made public as gpsSol.navIntervalMs. This works very well for both NMEA and UBlox, providing an accurate time interval that is consistent with the reported change in position, leading to more stable velocity estimates.
• Removed the 'hack' that 'rounded' the reported GPS data interval to approximate values, so that if the GPS decides to run at 5hz, not 10hz, the exact time interval between received data values will be reported to GPS rescue.
• Removed some unnecessary timing functions.
• Renamed the debug to GPS_CONNECTION
• Addressed some previous comments.

Update 2023-07-26

• Significant NMEA code improvements - UBlox PVT and SAT messages are prevented when NMEA is selected, NMEA state engine cleaned up a bit, and NMEA GSV (sat info) messages are prevented when Configurator is not connected. These changes have been tested.
• Addressed some of the more straightforward comments.

Update 2023-07-25

• increased the UART Rx buffer size to 256 bytes, up from 128bytes. Previously some modules were associated with an Rx buffer overflow issue that causes loss of satellite info display - no signal strength or health data, and no satellite list, in Configurator. NAV-PVT packets (100bytes) can be promptly followed by NAV_SAT packets of between 8 and 600 bytes, depending on the number of connected sats. Increasing the Rx buffer to 256 bytes should allow Configurator to display Sat data, even if up to 40 Sats are connected and the Sat info message immediately follows a NAV-PVT packet. Note that this data overflow does not cause any problem in flight, because sat data is neither requested, nor sent, unless the FC was connected to Configurator at power-up time.
• Debug 7 now logs the serial Rx buffer size. The serial buffer size should not exceed 256 bytes, or it will overflow and the checksum will fail. Faster baud rates fill the buffer more quickly than slower baud rates, but slower baud rates take longer to get through the data.
• reduced flash space by 612 bytes by excluding all code required for the CLI gps_info command unless the code is built with -DUSE_GPS_DEBUG
• learned that some M10 modules start dropping NAV packets, randomly, once they get more than about 20-25 sats. This is just something they do. It's not a betaflight problem. This can be seen by just connecting the module up with uCenter or pyGPSclient and closely inspecting the message time-stamps as satellite count increases. TO DO: (1) update GPS Rescue code to use the actual GPS solution interval value for velocity calculation and (2) see if constraining the number of sats used in the nav solution will prevent this.
• Investigated NMEA behaviour

Update 2023-07-22

• addressed many comments, mostly just tidying up
• marked as resolved all comments that appears to be resolved, to reduce clutter
• revised the time stamps so that we log the gps nav solution interval using time from the GPS packets. These are currently used to detect nav solution packet dropouts, but will, in future, be used by GPS rescue as the time interval for vertical and to home velocity estimation.
• confirmed that satellite info requests are not being sent, and are being blocked, when booted up without Configurator connected
• confirmed that at 115200, while connected to Configurator, we get packet dropout when there are a lot of sats connected (and a 3D fix). Whenever a sat info packet and a nav info solution packet are sent at the same time, ie at 2Hz, we lose both of them. The other 8 nav packets per second get through. This is not a problem when sat packets are not requested. Further testing is required to clarify exactly how this overload occurs.
• hence the recommendation is to use 57600 for the most reliable overall experience.
• the code should be flight ready now

Update 2023-07-22

• big THANK YOU to many testers
• baud rate configuration now seems 100% reliable
• navigation solution data is being received very reliably
• configuration code seems very reliable in enabling and disabling the various commands
• an issue where the Configurator check was happening too early, causing satInfo to not appear, has been fixed, in this commit.
• The AssistNow Autonomous code has been commented out for now, agreeing with comments that this is a new feature and should be part of a separate PR, once the benefit is ascertained.

Update 2023-07_20

• fixes an issue from a previous commit in this PR that prevented switching to the user's gps_ublox_flight_model once the 3D fix was acquired.

Update 2023_07_19 : lots of changes from @ctzsnooze and @ZzyzxTek:

• Both M8 and M10 units connect quickly and work correctly, please test with your module.
• Sat info messages are only requested while connected to configurator, not in flight
• Debugs modified
• After powering up, the FC cycles through the allowed GPS baud rates, attempting to connect to the GPS module. Once connected, the module is requested to receive at the configured serial port rate, and the serial port is switched to the same rate.

@freasy earlier extensive work included:

• support for the new ValSet API for M10 units 'out of the box'
• fixes for connection logic
• user configuration of the UBlox UTC standard, which can be auto, or one of the four GNSS system UTC times. If set manually to the dominant GNSS system in your area, the reported iTow UTC timestamps should not suffer occasional sub-microsecond shifts if the GPS module automatically switches to a different UTC system mid-flight.
• Enabled AssistNow Offline for M7 and later modules, but this has been commented out and will be the subject of a separate PR
• the CLI command gps_info, which, in a build compiled with -DUSE_GPS_DEBUG, will show the UBlox module software and hardware versions, and the Ublox software protocol version and hardware ID.
• adding a new debug mode to monitor GPS module connect/reconnect processes

The debug mode is GPS_CONNECTION, and can be viewed with this Blackbox Log viewer PR, and 'live' in the sensors tab. It currently displays:

Debug	While connecting	While receiving data	Comments
0	GPS din model	GPS dyn model	1 at the start (acquire model ID), 7 once 3D fix arrives (flight model ID)
1	GPS Nav data interval	GPS Nav data interval	GPS module interval between nav data packets
2	time since last Nav data	time since last Nav data	each GPS iteration, 100 or 500Hz, resets on new Nav data
3	Baud rate	GPS data calculated, interval	FC time interval between Nav data output
4	gpsData.state	gpsData.state	main State * 100 + sub-State, eg 413 is CONFIGURE at step 13
5	executeTimeUs	executeTimeUs	CPU time required for code to run
6	gpsData.ackState	gpsData.ackState	0 = idle, 1 = waiting, 2 = ack, 3 = mack
7	serialRxBytesWaiting	serialRxBytesWaiting	incoming serial buffer size in bytes

For testing:

1. Set the GPS communication rate in the ports tab to the default of 57600, and the gps_update_rate to 10hz

2. Set the GPS tab AutoConfig to ON and choose Ublox. Auto-baud does nothing, ignore it.

3. Go to cli and type:

   set debug_mode = GPS_CONNECTION
   save

4. Power cycle....

5. Check that the GPS indicator light at the top of the Configurator screen comes on quickly

6. Check that satellites register, and the map updates

7. Use to the CLI 'status' command to confirm that the GPS is 'connected' at the intended baud rate..

8. In the sensors tab, include 'debug', check the baud rate, current nav packet interval (100 for 10hz), flight model value (7 is normal), etc

9. confirm normal behaviour in the OSD

10. Go for a fly, make a log, check it all works, try a rescue!

Known Issues:

• NMEA doesn't correctly change baud rate, and may not change data rate. If this is an issue, set it to power on with data rate of 5hz and baud rate of 57600 and see how that goes.
• Baud rates of 19200 are too slow and will clog up the input buffer; 38400 is alright for long cable runs, but 57600 is strongly recommended, regardless of data rate.

Feedback would be gratefully appreciated!

[blckmn]

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 -> FAIL
• approver count at least three -> FAIL

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

[jsk2084]

gps_info

MON-VER acquired: true

swVersion: ROM SPG 5.10 (7b202e) hwVersion: 000A0000 (M10) extension: FWVER=SPG 5.10 PROTVER=34.10 GPS;GLO;GAL;BDS SBAS;QZSS

[haslinghuis]

@freasy can we have this on one line:

GPS;GLO;GAL;BDS SBAS;QZSS

[freasy]

@freasy can we have this on one line:

GPS;GLO;GAL;BDS SBAS;QZSS

No, this are 2 lines in the unit, which have no id or anything, so a line merge would be fuzzy :/ i would like to leave it like that as it is the raw response from the unit

[KarateBrot]

The PR adds an absolute freight train of additional flash. Can you try to make the code more memory efficient? For example I can see at least one function which is not needed (ubloxUTCStandardConfigToUnitValue()). The file structure also might need some work. We often split up init code if the c-file gets too bloated, so rather than gps_config.h and .c, just make a single gps_init.c and put all GPS init code in there. Also, splitting up NMEA and Ublox code and putting them into their respective files might be a good idea.

Does gps_info work for NMEA as well? (We support NMEA and Ublox) My suggestion is to focus on one goal at a time, either ublox M10 support OR the new GPS info command.

[SteveCEvans]

gps_info

MON-VER acquired: true

swVersion: ROM SPG 5.10 (7b202e) hwVersion: 000A0000 (M10) extension: FWVER=SPG 5.10 PROTVER=34.10 GPS;GLO;GAL;BDS SBAS;QZSS

The swVersion, hwVersion and extension strings seem surplus to requirements and a waste of FLASH. Also, why report 000A0000 when M10 is all we need.

[ctzsnooze]

Rather than moving gps.c and gps.h into a new directory, maybe simplify the PR by leaving them where they are, and adding gps_init.c and gps_init.h into the same io directory.

[freasy]

@SteveCEvans if there is a version string, that is unknown to the parse function, i want to display it

[ZzyzxTek]

What does adding the UTC standard settings fix for us? Previously the standard was set to 0 in UBX-CFG-NAV5, which corresponds to AUTO. It seems like in the new code it also defaults to AUTO. Does AUTO break something with M10s and it needs to be set to one of the other explicit standards?

[freasy]

What does adding the UTC standard settings fix for us? Previously the standard was set to 0 in UBX-CFG-NAV5, which corresponds to AUTO. It seems like in the new code it also defaults to AUTO. Does AUTO break something with M10s and it needs to be set to one of the other explicit standards?

It speeds up the sat find and fix if you set your corresponding time zone here

[ZzyzxTek]

It speeds up the sat find and fix if you set your corresponding time zone here

But is that broken for M10s? Not trying to be difficult, but expanding the scope of this PR to other improvements makes it hard to see what exactly we're doing to address the M10 problems.

The new code still sets it to AUTO by default. Does the user need to know to set it to their local UTC standard?

[freasy]

No auto is fine and it just expands the options to configure the unit

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[freasy]

@ledvinap thank you for all your reviews and input! That really helps alot! Are you on the bf discord? Would help in some cases :)

[ledvinap]

@freasy : You can contact me on Gmail

[freasy]

@freasy : You can contact me on Gmail

i cant find it :(

[ledvinap]

@gmail.com ;-)

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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]

Tested with an M8 unit that is configured in the GPS hardware to link up at 115200 baud.

When set in Ports tab to manual, connection is directly at 115200 in status window = good!

When set in Ports tab to AUTO, the connection can transiently be 9600 (and show not configured), but then, and most of the time, shows 57600 (set to57600)

I think this is a Configurator error, since I notice that if I save AUTO in Configurator, then exit and come back the value gets saved as a manual value of 57600.

@haslinghuis, once this PR is finalised, can we get Configurator to trigger this GPS auto-connect code when the GPS UART in the Ports tab is set to Auto?

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[ZzyzxTek]

When set in Ports tab to manual, connection is directly at 115200 in status window = good!

When set in Ports tab to AUTO, the connection can transiently be 9600 (and show not configured), but then, and most of the time, shows 57600 (set to57600)

I think this is a Configurator error, since I notice that if I save AUTO in Configurator, then exit and come back the value gets saved as a manual value of 57600.

Setting AUTO on the Ports tab does not actually set anything to AUTO, it sets the port baud to 57600. This seems intentional.

I mentioned this in the IMU dev channel, I'll copy it here for doc...

In Configurator, when setting the GPS port to AUTO, Configurator maps it to 57600 (port.js):

            let gpsBaudrate = $(portConfig).find('.gps_baudrate').val();
            if (gpsBaudrate === 'AUTO') {
                gpsBaudrate = '57600';
            }

In CLI, when entering a serial config with GPS baud rate of 0 (AUTO), it rejects any baud not 9600 to 115200 (doesn't map it to anything) (cli.c):

        case 1:
            if (baudRateIndex < BAUD_9600 || baudRateIndex > BAUD_115200) {
                continue;
            }
            portConfig.gps_baudrateIndex = baudRateIndex;
            break;

In MSP, it takes whatever is submitted, no validation (msp.c):

            portConfig->gps_baudrateIndex = sbufReadU8(src);

So, it seems at some point the decision was made not to let users set it to AUTO. The doc page for Serial under GPS Baud Rate says:

Also has a boolean AUTOBAUD. It is recommended to use a fixed baudrate. Configure GPS baudrate according to device documentation.

The doc page for GPS doesn't say anything about support for AUTO. The default baud rate for a GPS serial port is 57600.

[haslinghuis]

Flashed MambaG4 and set port to 115200 -> does not detect GPS (icon stays grey) and in GPS tabs true / false and sat count are being toggled constantly.

Setting speed to 57600 everything seems to work.

# gps_info
# MON-VER acquired: true
swVersion: 
 FW=5.10, PROTO=34.10
hwVersion: 
 M10 (000A0000)
extension:

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets

• See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
• An easier way to test this, using the Configurator cloud build, is simply put #12799 (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!

[FrenzyIncarnate]

gps_info

MON-VER acquired: true

swVersion: FW=5.10, PROTO=34.10 hwVersion: M10 (000A0000) extension: This is the result I got. Is this ok? I dont see extensions as I saw at some of the commenters

[FrenzyIncarnate]

Just installed 4.5 zulu with this pr. Previously it was loosing sats after takeoff . Did a small flight outside at 11pm :) it did not lose any . Good work @freasy . Attached the log file LOG00001.zip

[TheIsotopes]

@freasy tested on latest master. The extension from gps_info is empty. I seem to remember that something was listed there.

using: MATEK GNSS SAM-M10Q

# gps_info
# MON-VER acquired: true
swVersion: 
 FW=5.10, PROTO=34.10
hwVersion: 
 M10 (000A0000)
extension: 

[TheIsotopes]

imo all looks good.

[ctzsnooze]

@adam-ah please join us on discord - ping me there

[adam-ah]

@adam-ah please join us on discord - ping me there

@ctzsnooze I don't use discord - am I missing out on much? :)

[ctzsnooze]

@adam-ah yes ;-)

[adam-ah]

Wow, first of all, thank you so much for all the hard work you've put into this PR since May 12! Your dedication is truly commendable, and I'm excited to see the progress made so far.

However, as much as I appreciate the effort, I must admit that the current size of the PR has become quite challenging to review meaningfully. There are hundreds of unanswered comments and new commits, making it a bit overwhelming to handle in one go.

To ensure that everyone can give your work the attention it deserves, I would really suggest considering splitting this PR into three smaller ones, as discussed earlier. This way, it will be easier for us to review and thoroughly test each portion both on unit tests and actual hardware.

Again, I want to thank the team for the efforts, and I'm looking forward to your understanding in breaking down the PR to help us proceed with the review and merge process more efficiently. Let's collaborate on this, and I'm confident we'll get it done in no time! 🙏😊

[haslinghuis]

@ctzsnooze please apply following diff. GPS Signal Strength is updated in configurator.

mark@pc:~/dev/pr/firmware/test/betaflight$ git diff
diff --git a/src/main/drivers/stm32/serial_uart_stm32f4xx.c b/src/main/drivers/stm32/serial_uart_stm32f4xx.c
index 1f5dbf5b7..d24ba47c7 100644
--- a/src/main/drivers/stm32/serial_uart_stm32f4xx.c
+++ b/src/main/drivers/stm32/serial_uart_stm32f4xx.c
@@ -392,7 +392,7 @@ void uartIrqHandler(uartPort_t *s)
             USART_ITConfig(s->USARTx, USART_IT_TXE, DISABLE);

             // Switch TX to an input with pullup so it's state can be monitored
-            uartTxMonitor(s);
+            // uartTxMonitor(s);
         }
     }

diff --git a/src/main/io/gps.c b/src/main/io/gps.c
index f4539c1cb..fccb08450 100644
--- a/src/main/io/gps.c
+++ b/src/main/io/gps.c
@@ -1357,6 +1357,7 @@ void gpsUpdate(timeUs_t currentTimeUs)
         GPS_update &= ~GPS_MSP_UPDATE;
     }

+    DEBUG_SET(DEBUG_GPS_UNIT_CONNECTION, 4, gpsData.state);
     DEBUG_SET(DEBUG_GPS_UNIT_CONNECTION, 7, gpsData.satInfoRequired);

     switch (gpsData.state) {
@@ -1391,14 +1392,18 @@ void gpsUpdate(timeUs_t currentTimeUs)

 #ifdef USE_GPS_UBLOX
             } else {
-                if (gpsConfig()->autoConfig == GPS_AUTOCONFIG_ON) {
-                    if (isConfiguratorConnected()) {
-                        if (gpsData.satInfoRequired) {
-                            requestSatelliteInfo();
-                            gpsData.satInfoRequired = false;
-                        }
+                if (isConfiguratorConnected()) {
+                    DEBUG_SET(DEBUG_GPS_UNIT_CONNECTION, 1, gpsPackageCounter);
+                    gpsPackageCounter++;
+                    // set the condition for requesting new satellite info while connected to Configurator
+                    // We only should call this once, so we set the flag and call it once
+                    if (!gpsData.satInfoRequired) {
+                        gpsData.satInfoRequired = true;
+                        gpsPackageCounter = 0;
+                        requestSatelliteInfo();
                     }
                 }
+                // THIS HAS NOTHING TO DO WITH RECEIVING DATA
                 if (gpsData.ubloxUsingFlightModel == false && STATE(GPS_FIX)) {
                     ubloxSendNAV5Message(gpsConfig()->gps_ublox_flight_model);
                     gpsData.ubloxUsingFlightModel = true;
@@ -1448,20 +1453,10 @@ static void gpsNewData(uint16_t c)
         return;
     }

-    DEBUG_SET(DEBUG_GPS_UNIT_CONNECTION, 1, gpsPackageCounter);
-    gpsPackageCounter++;
-    if (gpsData.state == GPS_STATE_RECEIVING_DATA) {
-        // count the number of packets received in one second
-        if (cmp32(millis(), gpsData.lastNavMessage) >= 1000) {
-            // set the condition for requesting new satellite info while connected to Configurator
-            gpsData.satInfoRequired = true;
-            gpsPackageCounter = 0;
-        }
-        // new speed and position data received, and successfully parsed.  Other commands ignored.
-        gpsData.lastLastNavMessage = gpsData.lastNavMessage; // used only for a delta time in dashboard.c
-        gpsData.lastNavMessage = millis();
-        sensorsSet(SENSOR_GPS);
-    }
+    // new speed and position data received, and successfully parsed.  Other commands ignored.
+    gpsData.lastLastNavMessage = gpsData.lastNavMessage; // used only for a delta time in dashboard.c
+    gpsData.lastNavMessage = millis();
+    sensorsSet(SENSOR_GPS);

     GPS_update ^= GPS_DIRECT_TICK;

BTFL_BLACKBOX_LOG_20230721_214450_FOSSF4ELRS.zip

Full 16MB bench log: BTFL_BLACKBOX_LOG_20230722_012121_FOSSF4ELRS.zip

[adam-ah]

What incredible work being poured into this project! It's great to see how much the community will benefit from these efforts.

However, I must admit that when it comes to gps.c, I have some concerns. The code's complexity, length, and difficulty to read and test give me a bit of unease.

It feels like we might be throwing a ticking time bomb into a room (release), not only hoping the occupants (pilots) won't be harmed too much by any shrapnel (bugs), but that the room (codebase) stays well-organized after the explosion (code maintainability).

Breaking down the code into separate files, such as (gps.c, gps_init.c, gps_ublox.c, and others), and refactoring the monolithic methods would be immensely helpful, if I may suggest. Also, whenever possible, simplifying nested structures would make the code much more approachable (both readability, maintainability, and testability).

I understand that everyone's efforts are invaluable especially as people are spending their free time on this, and I'm truly grateful for everyone's dedication to this project.

[ZzyzxTek]

Yes, we all share your concerns about the state of the code in the GPS driver files. This code has been around in this condition for a long time, it was not written for this PR. And for the most part, has worked well enough for the users of GPS.

The intent of this PR was only to address a couple of immediate issues:

• Connecting and reconnecting to the module at the configured baud rate was not reliable, Auto baud did not work.
• Support for the newer M10 modules was insufficient. BF did not use the new u-blox config commands and the risk of not properly configuring M10 modules was high.

That's what was embarked on, along the way a few other things crept in:

• Added configurable UTC standard.
• Added enabling of AssistNow Autonomous.
• Found some other bugs that were considered too significant to leave alone.
• Cleaned up some things, mostly those that touched the above, some that just bugged us too much.

This ended up being a bigger PR with the above than I think some of us expected or wanted. un!t did a huge amount of work to get it started and mostly finished up before his new baby arrived (congrats!). ctzsnooze and I have been working to get it finished up to where it's releasable. It's what we've got right now, maybe not everything we wish we had.

GPS Rescue testing has run into the connect/reconnect issues, as have some users. There are a lot of users adopting the new M10 modules, most are unaware of the risks of them not being fully supported. We feel it would be very valuable to get these changes released as soon as we can (with hopefully a 4.4 retrofit). Breaking up this PR and especially embarking on larger refactorings now would delay that. Keep in mind we have a non-trivial testing effort in front of us to make sure this works across a variety of modules.

There is an active discussion on Discord about larger refactoring or rewrite plans. You are encouraged to join that. I think you'll find the folks there understand the principles of good design & coding and want to apply them to the GPS module.

Perhaps as a suggestion, we can focus the review comments for this PR on what's minimally necessary to get it releasable? If there are changes made for this PR that smell, let's clean them up, but perhaps limit the clean up to that?

What incredible work being poured into this project! It's great to see how much the community will benefit from these efforts.

However, I must admit that when it comes to gps.c, I have some concerns. The code's complexity, length, and difficulty to read and test give me a bit of unease.

It feels like we might be throwing a ticking time bomb into a room (release), not only hoping the occupants (pilots) won't be harmed too much by any shrapnel (bugs), but that the room (codebase) stays well-organized after the explosion (code maintainability).

Breaking down the code into separate files, such as (gps.c, gps_init.c, gps_ublox.c, and others), and refactoring the monolithic methods would be immensely helpful, if I may suggest. Also, whenever possible, simplifying nested structures would make the code much more approachable (both readability, maintainability, and testability).

I understand that everyone's efforts are invaluable especially as people are spending their free time on this, and I'm truly grateful for everyone's dedication to this project.

[adam-ah]

@ZzyzxTek, thanks for taking the time to fill in the history or this PR.

I'll do my best to address these as objectively as possible, and apologies if any of these are contradictory, I truly appreciate all the effort on BF

That's what was embarked on, along the way a few other things crept in:

It is more important to focus on what matters now: quality code that is readable, testable, maintainable, and bug-free. We (hoomans) are easily distracted by emotions and history, but it is our role to make the right decisions - what matters is where we are at now.

GPS Rescue testing has run into the connect/reconnect issues, as have some users.

• I set up my M10 GPS manually after reviewing the GPS code. In my case (and I suspect in most cases), the too high baud rate caused drops which eventually caused disconnects / re-inits. Working fine. BF4.4 code is not compatible with M10 chips
• I can't see how this PR, without knowing what exactly the problem is for users, would solve any of the problems (we can't fix what we don't understand)
• Most of the changes in this PR have little to do with GPS connection stability

Breaking up this PR and especially embarking on larger refactorings now would delay that. Keep in mind we have a non-trivial testing effort in front of us to make sure this works across a variety of modules.

If we can't review and test the code properly, it leads to endless changes without meaningful review and testing. Delaying this PR is necessary due to numerous unaddressed comments and proposed additions to already lengthy files.

Splitting up the PR would actually make it easier to release the fixes and would give us time to mull over the less critical parts. Given that the PR started in May, the argument has never been stronger to split up this PR.

I do hear your argument and desire to help users. I have had these discussion countless times in professional contexts, and it is never an easy one.

folks there understand the principles of good design & coding and want to apply them to the GPS module.

It would be good to see these being applied more.

This code has been around in this condition for a long time, it was not written for this PR.

While in isolation it might be true,

• it does not mean we should keep adding to it without refactoring first
• it certainly does not justify adding another 1k lines to an already hard to read file (an no reviewer should just accept that)

Perhaps as a suggestion, we can focus the review comments for this PR on what's minimally necessary to get it releasable?

If I summarise your arguments, I see the key points as follow

• the PR should be smaller and more focused, but it's not
• the PR is important as it might fix some bugs
• while people know about coding guidelines, the code is legacy code, and therefore guidelines do not apply (?)
• we should not refactor because that would take some time
• reviewers should only make comments if the code has a trivial bug, otherwise approve

If I understood you correctly, I'm afraid I cannot support this, for the following reasons

• if the PR was smaller, it could be live already - this is the best time to isolate what is a fix and what is an in-flux idea (such as temporarily disabled for testing)
• it is unknown if the PR actually addresses the GPS rescue problems
• given the huge amounts of changes, it is guaranteed there are new bugs being introduced (needing to roll back a released binary on non-connected devices is nothing but a nightmare for everyone)
• not being able to meaningfully review the code makes it near impossible to catch bugs either now or based on user feedback
• it is never okay to ignore coding guidelines, especially when working with legacy code
• refactoring takes relatively little time, especially compared how long this PR has been in flight for (refactoring is not as fun as adding AssistNow, but much more important)
• reviewers should be able to meaningfully review functionality, not just a mere line-by-line review - the code not being readable is a valid concern and must not be brushed over

As for the above, I still support my previous suggestions, in line with @haslinghuis

• All review comments need to be resolved
• Split up code into smaller files
• Separate bug-fixes into small PRs

In any case, thanks again for all the hard work!

[ctzsnooze]

@adam-ah

If you would re-write the code from scratch, test it, and make a PR - that would be fabulous. We all agree, and we all already knew, that the code requires a total refactor. So we are 100% on the same wavelength there.

But you won't. And at present, no-body will.

So we have a situation where M10 units require manual re-programming for them to work, and in fact a lot of M8 units also don't work.

Now you may not understand the code, and I agree that it is extremely tough to follow. But since working on it, we have at least 3 people who understand it very, very well. And those people are slowly, and effectively, solving the issues, one by one. This has been a team effort, involving changes to the debugging tools, to Configurator, and to the code.

This is the current situation:

• Before, setting the baud rate to match the user's preference failed badly. Now it works extremely reliably. Whether the module only connects at 9600 or only at 38400, or only at 115200, if you want 57600, you will get 57600, reliably.
• Before, the 'auto-baud' function in the GPS configuration tab resulted in bad things. Now it does nothing at all. That is actually a meaningful improvement :-)
• Before, if the module was configured to send NMEA messages all the time, we were not always reliably disabling them during the auto-configuration stage. Now, this is done reliably.
• Before, often the GPS light would be unreliable, or flicker, indicating an unreliable link to the module. This is now much less common because the link to the module is now far more reliable. Mostly this is because we are now much more effectively controlling which messages should be enabled and which should not.
• Before, if the module was configured to send sat info messages all the time, they kept being sent all the time, causing issues with data reliability as the detected satellite count increased. In the last few commits this issue should be now properly resolved.

There are a lot more improvements like that. And with each improvement, our understanding of the code improves, and it gets a little bit more transparent, better commented, and easier to follow.

So I am going to be blunt, this PR will continue its incremental improvements and will be merged, fundamentally in the current form and structure, no matter how strongly I agree that it is in need of a massive re-write.

I would be grateful if you would take the time to actually test the PR, use the blackbox debug tool provided, and help make the code better by providing clear, tested, code improvement suggestions. Otherwise, I accept your views, and indeed agree, and hope that you agree that you don't need to keep re-stating the same views over and over.

[adam-ah]

So I am going to be blunt, this PR will continue its incremental improvements and will be merged, fundamentally in the current form and structure, no matter how strongly I agree that it is in need of a massive re-write.

Considering that many of these changes have already been implemented elsewhere with improved structure and readability, I'm not entirely convinced of the necessity for this level of arrogance. @ctzsnooze https://github.com/iNavFlight/inav/blob/a528a6dfcac275666b8d22dcebfa6d756be8985a/src/main/io/gps_ublox.c

In any case, I understand that feedback can be challenging to handle at times. I'll leave it at that and wish you the best of luck!

[ctzsnooze]

@adam-ah I too very much appreciate how nicely GPS connections are handled in iNav. And I agreed with you that this code requires refactoring, extensively, or rewriting in full. However that is not the purpose of this PR. It is important, right now, to focus on what is achievable and necessary. Right now the most useful thing you could do, if you want to assist with code development in a productive and positive way, is to test the code and provide solutions to any issues that you identify. Later on we will look at refactoring, at which time your PR's will be duly considered, if you provide any.

[freasy]

So I am going to be blunt, this PR will continue its incremental improvements and will be merged, fundamentally in the current form and structure, no matter how strongly I agree that it is in need of a massive re-write.

Considering that many of these changes have already been implemented elsewhere with improved structure and readability, I'm not entirely convinced of the necessity for this level of arrogance. @ctzsnooze https://github.com/iNavFlight/inav/blob/a528a6dfcac275666b8d22dcebfa6d756be8985a/src/main/io/gps_ublox.c

In any case, I understand that feedback can be challenging to handle at times. I'll leave it at that and wish you the best of luck!

Hey, even most of the work here is from me, I was absent, because my second child arrived. That's why I handed this PR over to @ctzsnooze

In discord I already stated, that the whole gps code will be deleted and rewritten at some point. Because fixing this mess is useless hours. On the other hand this PR brings stuff to the table, we don't want to miss, thus the continued work here.

I very much appreciate your contribution! If possible, could you join discord, I am more familiar with communication there?

When I rewrite the whole gps code, I need as much help and brain power as needed, if you want to join then, I would be happy :)

[ctzsnooze]

This is a log image showing nav packet loss at 115200 when Configurator is connected and we get enough sats for a 3D fix. At the point we change to flight model 7 (red debug 0 trace) we start getting dropouts (debug 1 and 3, and the yellow timer climbing higher). The dropouts occur every 500ms, which is the interval at which sat info is requested.

Here is the identical circumstance when the board is booted up without Configurator connected. There is no dropout problem, because sat info is not requested after booting up without Configurator being connected.

It's also clear that GPS modules 'remember' the number of sats recently acquired, and when hot-swapped, the NAV-SAT commands are as big as the max number of possible satellites recently identified. So while a module may connect and show sats reliably when first cold connected, because the number of sats, and the size of the NAV-SAT data messages are small, once it acquires enough sats to start failing, it will always fail after a hot swap. This explains some of the randomness with 115200 baud under the current code; the module may show sats early on, then stubbornly refuse to, yet work perfectly when cold-connected the next day.

[haslinghuis]

BTFL_BLACKBOX_LOG_COMMIT59.zip

70 minutes on 115K2 without loosing gpx fix. Gps signal data is hardly updated.

16:50 After powering off for 10 minutes or so with cold start it took 10 minutes to gain a fix.

[haslinghuis]

BTFL_BLACKBOX_LOG_20230725_163058_FOSSF4ELRS(256 buffer).zip

Maybe we need 256+128 bytes buffer but latest tests are showing much better results!!!

Really like to implement @adam-ah suggestions after this gets done

https://github.com/betaflight/betaflight/blob/293e14e8bd4b87019780ed0598cb7cfaa636415a/src/main/io/gps.c#L1535-L1545

[ctzsnooze]

this image is outdated; it shows earlier versions of the code failing to speed up the task at the start of a packet and also at the end. This has been fixed in later commits. This image shows first a NAV-PVT message (first red line), and then a NAV-SAT message (second red line), being cleared from the serial buffer at 1kHz. This was at 115200 baud, a 10hz update rate, a 256byte serial buffer, and about 30 sats in view.  Note that the peak serial buffer value while parsing this particular NAV-SAT packet was 147bytes, which would have overloaded the old 128 byte buffer, leading to loss of this packet. If the baud rate was 57600, the number of bytes entering the buffer in a given time period would be halved, and the old buffer would have been OK - but the downside would have been that clearing the buffer would take twice as long. It looks like 34800 baud could lead to the NAV-PVT buffer being incompletely cleared before the NAV-SAT data started arriving, which could also lead to loss of the NAV-SAT packet. Note that in-flight, no SAT messages are sent. The FC only has to decode NAV-PVT messages. It is likely that 115200 will have more than enough buffer space in-flight for NAV-PVT messages only, and that 57600 will also be fine. If the user wishes to experiment with higher than 10hz update rates, higher baud rates will result in the buffer being cleared faster. I anticipate that for reliable 20hz update rates in-flight, 57600 may be OK, but that 115200 would be a better bet. At this point I don't recommend 20hz update rates.

[ctzsnooze]

@haslinghuis - despite the slow log sample rate, your log shows that sometimes there is sufficient time between NAV-PVT and NAV-SAT messages for them to be distinct, and both kinds of message would reliably get through in these cases.

At other times, the two appear to coincide. However, the NAV-PVT message appears to always be handled, since the time interval between received NAV packets steps up when that happens, and appears to happen normally.