Critical BUG: serial.gps may induce unwanted commands -FIXED

[Hamburger]

Bug: an attached (nmea) gps can induce data chars which may get interpreted either as MSP commands or via evaluateOtherData() as serial commands.

Symptom: this may drop user into configuration.loop or trigger other functionality. What is even more critical/confusing: once the gps is physically attached the symptom cannot be avoided with disabling gps in config.h.

Reason: in v2.3+ even on gps port, incoming data get scanned for msp and other command data. (as a nicety for promini and prmicro users, I presume; since r1715)

Partial Fix: in evaluateOtherData() do nothing if currentport == gps.serial port. But this would leave wrongly possible MSP interpretation of gps data stream intact.

Discussion: Again, why was msp interpretation on gps serial port introduced in the first place? Is this still desirable? I would prefer to skip all interpretation other than for gps on gps port.

Comments, please.

[Hamburger]

parital fix to Protocol..cpp in r1737

[PatrikE]

parital fix to Protocol..cpp in r1737

It will kill the Promini FC and GPS as a option..

It is still possible to run Pre2.4 on a ProMini FC.
With proper #if defined(USE_MSP_WP) around the WPNavigation code
In fact Non of the GPS code is behind defines in gps.cpp only in protocl.

A better soulution would be to disable MSP data if GPS data have been identified.
At least on serialport 0.
Otherwise it's not possible to run MSP if
#define GPS_SERIAL 0 is selected

It would extend the Life on the small FC's

[Leo]

Reading this makes me realize that maybe the issue I had a couple of days ago might have to do with this.

I was flying a way point when all of a sudden EZ-GUI "shouted" GPS signal lost. This was only for a second and then GPS was active again.

Just guessing here...

Does this issue concern all MW flyers with GPS active?

[Hamburger]

parital fix to Protocol..cpp in r1737

It will kill the Promini FC and GPS as a option..

why so?
my partial fix only prevents handling of "other serial commands" but not of the MSP commands on the gps.serial port.

True, I am still not convinced having both MSP and GPS on same port is guaranteed to be free of unwanted side effects.
Maybe if gps.port !=0 then skip MSP handling on the gps.serial port ?

[Alexinparis]

Hi,

The probability to grasp a valid MSP in a GPS data stream is extremely low.
The GPS stream should contain:
[M] [$] [<] size [an existing message number] data... [a valid CRC]
So a match of 4 bytes at specific position + a valid message number.
I think it's enough secure.

ok, it's another story for evaluateOtherData() which is right protected now

If you want a better protection MSP vs GPS, something based on no GPS frame seen during a period to activate MSP evaluation

Code: Select all

  static uint8_t GPSDetected[UART_NUMBER];
...
            if (checksum[port] == c && GPSDetected[port] == 0) // compare calculated and transferred checksum
              evaluateCommand(cmdMSP[port]); // we got a valid packet, evaluate it
...
          if ((timeMax - GPS_last_frame_seen) > 1200000) {
            //No update since 1200ms clear fix...
            f.GPS_FIX = 0;
            GPS_numSat = 0;
            GPSDetected[port] = 0;
          } else
            GPSDetected[port] = 1;

@Leo, your issue can't be tied as the GPS stream is always captured

[Leo]

@Leo, your issue can't be tied as the GPS stream is always captured

Thank you. This software is complex and for an outsider it's difficult to understand the relationship between each module code.