Add NMEA-0183 Protocol GPS Driver

[bys1123]

Support standard NMEA messages.
Such as support more than 15 satellites which ashtech driver restricted in GGA message, and GSA also not working well.

Not implement baud rate detection yet.

Due to sensor_gps_s changes, not tested with current master.

[bkueng]

Thanks @bys1123

Such as support more than 15 satellites which ashtech driver restricted.

Where do you see that limitation?
On which device was this tested?

[bys1123]

@bkueng On M8N, you could see GGA message description in u-center.
And here is a community information.
https://portal.u-blox.com/s/question/0D52p00008RmGUMCA3/number-of-satellites-in-gga

[bys1123]

Test console output with this driver.

[bys1123]

Test only connect GPS2.

[bkueng]

We use the ubx driver for u-blox devices, so I don't see the use for this.

[bys1123]

Agree, never use NMEA on U-Blox GPS, I just say you could test with it, I know you must at least have one.

There is a batch of GPS brand is using NMEA protocol as it's a industrial standard, and their performance is enough for drones, some models even better than U-blox.

[bys1123]

And they all restricted by GGA message. But with this driver, they work perfect.

[bys1123]

Like NovAtel:

https://hexagondownloads.blob.core.windows.net/public/Novatel/assets/Documents/Papers/OEM7720-Product-Sheet/OEM7720-Product-Sheet.pdf

[bkueng]

And they all restricted by GGA message.

Ok, that is helpful to know. Can we extend the ashtech driver to support these? What difference is required?

[bys1123]

http://cetest01.ufile.ucloud.com.cn/fsite_1802020370/SinoGNSS_K726_OEM_BOARD-DS-ENG1561700969334.pdf
SinoGNSS

[bys1123]

1. Use ashtech name for general NMEA protocol is misleading.
2. Ashtech's baud rate is 115200, but others may not using this rate.
3. We don't have ashtech gps, it's hard to test if we break its driver, for other developers also have this concern.
4. From high-end to low-end GPS, they all support this NMEA-0183 protocol. We may make this driver more extendable and keep optimize compatibility.

[bkueng]

Ok that is fine. A few points:

• check if code sharing between the 2 drivers makes sense to reduce flash size (ashtech driver is 7KB)
• provide an exact list of tested and supported devices (also if that includes RTK)
• auto-detection: how do we make sure the nmea driver does not run on u-blox devices (for instance)? At this point we might have to abandon auto-detection and add a protocol parameter (as you already started with) and default to u-blox.

[lukegluke]

I do something similar for my own and it is not universal, I can't share it. But I have some thoughts for you, hope they would be useful.

1. To be truly universal, nmea driver should somehow autodetect what sentence is last in set to return 1 (it could easy be RMC or VTG for example) . For now it can be only GGA in suggested driver.
   The most difficult thing here I think will be not be broken by possible rare sentences.
2. "HDT" sentence should be also parsed in driver it is useful for two antenna RTK receivers with heading. Ashtech driver parse it already.
3. I would recommend to use uiCalcComma >= 14 when checking "GGA", because I encountered the receiver with extra "," in the end. Maybe this is just particular receiver bug, but it is not critical to check only min number of fields/commas going to parse in driver.
4. Missed checking number of commas when checking "RMC". Should be uiCalcComma >= 11
5. GPRMC message fields meaning comment is incorrect. It is copied from the trimble site I suppose? It omits "N or S", "E or W" indicators.
6. Mistake
   uint64_t usecs = static_cast<uint64_t>((nmea_sec - static_cast<uint64_t>(nmea_sec))) * 1000000;
   should be
   uint64_t usecs = static_cast<uint64_t>((nmea_sec - static_cast<uint64_t>(nmea_sec)) * 1000000);
   to fix missing parsed micro seconds. By the way this bug is from current ashtech driver.
7. I would recommend to fix parsing like this (here _bytes_readed is my name for bytes_count):

        /* pass received bytes to the packet decoder */
        while (_bytes_parsed < _bytes_readed) {
            int l = 0;

            if ((l = parseChar(_read_buf[_bytes_parsed++])) > 0) {
                /* return to configure during configuration or to the gps driver during normal work
                 * if a packet has arrived */
		int ret = handleMessage(l);

		if (ret > 0) {
			return ret;
	        }
            }
        }

        /* everything is read */
        _bytes_parsed = _bytes_readed = 0;

where _bytes_parsed is GPSDriverNmea member variable and is cleared in the same places as _bytes_readed. Or you will miss data on handling satellite info message in future. Take care to increase _bytes_parsed immediately to avoid sending same byte to parcer.
For current time this is more related to current ashtech driver I think.
8) I promote suggestion to include ashtech within nmea

[bys1123]

@lukegluke Thanks for review and these nice suggestions, I'll take research on those.

[bys1123]

1. Automatic match the baud rate after detect NMEA-0183 protocol.
2. Add GPHDT message support.
3. Support satellite num display for modules only provide “GBGSV, BDGSV, GLGSV” message.
4. Optimized GGA message logic.
5. Reset FC clock after received ZDA or RMC message.
6. Optimized comma handling.

[lukegluke]

Hi @bys1123,

Thanks for your work! I don't have time to test your code, but I surely will (someday :).
I just took a look through code and I think this aspect I mentioned before is still in charge:

1. To be truly universal, nmea driver should somehow autodetect what sentence is last in set to return 1 (it could easy be RMC or VTG for example) .
The most difficult thing here I think will be not be broken by possible rare sentences.

For example as I see if HDT message is last in set (and it is easy can be) it will not be parsed within same block with lat and lon, so heading can be fall one message behind of coordinates - it's not good.

Another aspect:
Universally there can be used different messages sets:
For example GGA+RMC or GGA+ZDA+VTG for full date+time and velocity.
But in fact any of those message can be set with its own frequency not necessary equal to each other. Maybe it's too much to handle and just add a note to configure them with same frequency is enough.

But regarding to GSV message - first of all it is optional and also I saw real cases when GSV is sent rare than other messages (one time in several seconds) due to its size. Now it is necessity to have _SVINFO_received in your driver to get gps position at all.

[lukegluke]

missed
_TIME_received = false;

[lukegluke]

_gps_position->cog_rad = matrix::wrap_pi(track_rad);

because track_rad is [0, 2π], while cog_rad needs to be [-π, π]

[lukegluke]

same here

[moreba1]

https://github.com/ArduPilot/ardupilot/blob/997ffcf30d636c2236c21bc1bb1448643a938c2a/libraries/AP_GPS/AP_GPS_NMEA.cpp

does this help?

@lukegluke
@bkueng

[lukegluke]

does this help?

There no problems with current pull request, it is adopted for PX4 and works correctly, while the code you linked at is quite separate thing and also under GPL license. I don't get it why you mentioned it?

[moreba1]

@bys1123

can you correct this driver?

[bys1123]

@lukegluke I'm very busy on my work these days, I invited you to my project, can you help optimize the code, thanks!

[lukegluke]

@bys1123 Thanks, but unfortunately I'm also very busy :(, but at least on next week I will add those modifications I mentioned previously in comments.

[lukegluke]

I've pushed commit with fixes I mentioned and also several optimizations and fix code style. Also I tried to add same modifications as it was done in latest ashtech.

But I don't think this should be merged as separate driver.
Instead it should be GPSBaseStationSupport class and combined with ashtech.

I see this as general nmea driver with probably additional subsystem flag (ashtech or other receivers for future use). Also it occupy quite a space and needs more optimizations.
Sorry I don't have time for this, especially don't have opportunity and px4 hardware for tests.

[siddux]

Hi, I was trying the NMEA protocol implementation mentioned above on a Pixhawk cube black using a Swift Nav receiver. I can get coordinates info on the nutshell but the gps status still remains "No ok". Any idea on how to fix it?

Thanks.

[lukegluke]

VTG parsing has an issue, I forgot to fix it here. It should be fixed to 8 commas:
} else if ((memcmp(_rx_buffer + 3, "VTG,", 4) == 0) && (uiCalcComma >= 8)) {
because it looks like most of receivers don't have positioning system mode indicator
https://docs.novatel.com/oem7/Content/Logs/GPVTG.htm
https://www.trimble.com/OEM_ReceiverHelp/V4.44/en/NMEA-0183messages_VTG.html

@siddux try this

[jajberni]

I have been trying to test this pull request but unfortunately, we found some issues because of the new format in the constructor and the fact that messages don't necessarily have the same rates. The current implementation waits until all the messages arrive before returning OK, which causes the NOT OK status. In the Ashtech implementation, every time that a valid position or speed is received, it returns 1. I testes this approach and now we get 10Hz rates for speed and velocity. Satellite information and heading update the _gps_position as they arrive.

The implementation, based on DimianZhan:add_nmea_driver is at:

https://github.com/OpenAgriTech/PX4-GPSDrivers/tree/nmea_support.

However, due to the changes in the master branch, I had to fork it from the master and I am not sure how to reflect my changes from DimianZhan:add_nmea_driver.

This seems to work well and we have now RTK and heading without any issues using an RTKite with a dual antenna.

[bkueng]

@jajberni can you open a pull request?

[lukegluke]

I have been trying to test this pull request but unfortunately, we found some issues because of the new format in the constructor

I suppose you meant the configure() function, not the constructor. Yes, there is a minor change GPSConfig.

and the fact that messages don't necessarily have the same rates. The current implementation waits until all the messages arrive before returning OK, which causes the NOT OK status.

Yes, I mentioned this aspect here previously #58 (review) and it is not so easy to solve this correctly in general case.

In the Ashtech implementation, every time that a valid position or speed is received, it returns 1. I testes this approach and now we get 10Hz rates for speed and velocity. Satellite information and heading update the _gps_position as they arrive.

You make a workaround. By ignoring correct grouping of nmea messages, the heading and error information in vehicle_gps_position messages may be delayed each time by one period. While this delay is no so big, but still at least for my opinion it is not right to do this like you did.

Regarding this pull request to px4 as itself I would like to repeat myself:
For my POV, this driver should be GPSBaseStationSupport class for all nmea/nmea-like receivers including ashtech with possibility to send commands to different known receivers (in px4 currently only ashtech). But it needs someone's time who has px4 hardware and several receivers including ashtech. Sorry, for now can make only advises.

[jajberni]

@lukegluke very good points and thank you for your feedback!

You make a workaround. By ignoring correct grouping of nmea messages, the heading and error information in vehicle_gps_position messages may be delayed each time by one period. While this delay is no so big, but still at least for my opinion it is not right to do this like you did.

This is right and I am now observing this in the bag files where the GPS topics come grouped in two (I'm returning 1 on valid position and velocity messages):

I guess it makes more sense to wait until you have position, velocity and error before returning a valid position. Satellite information is not that critical and I don't think it would cause any trouble to have a delay in the number of satellites. Heading is trickier because it doesn't even have a timestamp.

I am not so sure about the GPSBaseStationSupport requirement because this is quite specific and I don't follow why this should be part of the PX4 driver. The base station support is meant to enable SurveryIn and transmission of RTCM messages for RTK correction. Why should this be implemented on the vehicle (rover) and not just on the base station? I haven't followed up on the discussions (I am sure there has to be a reason for it beyond the support of dual GNSS systems for heading).

In my opinion, the astech driver is quite specific to the astech/trimble dialect, with heaps of non-standard messages. Maybe what is required is just a command for custom initialization (very common on generic GPS software) that is passed to the GPS, but only read/interpret standard NMEA sentences to keep it generic. Despite all the limitations of NMEA, most high-end GPS units can provide NMEA sentences.

Also, how can you keep back compatibility with astech GPS type? The safer solution in the mid-term is to add a new type and eventually deprecate astech if both get merged.

[jajberni]

@jajberni can you open a pull request?

Let me do some more testing in the coming days and try to implement the comments from @lukegluke.

[lukegluke]

Satellite information is not that critical and I don't think it would cause any trouble to have a delay in the number of satellites.

I agree about number of sats. Probably about errors too. But I really suppose that their should not be delaying in heading information, because it is separately get fused in ekf2 filter.

I am not so sure about the GPSBaseStationSupport requirement because this is quite specific and I don't follow why this should be part of the PX4 driver.

Here we are in PX4-GPSDrivers repository that is used not only for PX4-Autopilot, but also in QGC. And it exactly uses this drivers for working with RTK receivers as SurveryIn/Fixed base station. Maybe for now px4 community used limited number of well known receivers, I would like to recommend to already make enough scalable solution for future.

But also speaking about implementing RTCM messages on the vehicle (rover). It obviously useful to make post processing (PPK). I even wonder why it is not implemented in px4, while architecturally there is already a GPSCallbackType::gotRTCMMessage? I thought that PPK is essential to aerophotography and aerophotogrammetry.
And there is no so much work to implement this: I see this like handling gotRTCMMessage callback, publishing orb topic gps_rtcm_data (same as gps_inject_data - or they even be combined in same way as gps_dump using MSB bit), recording topic in logger to sd card.

In my opinion, the astech driver is quite specific to the astech/trimble dialect, with heaps of non-standard messages.

Agree, but still there is quite of common code that can be combined. Code in this PR was quite copied from astech.

Also, how can you keep back compatibility with astech GPS type?

I suppose there can be implemented an autodetect by requesting particular astech get version command or something like this. But in first place if GPS_DRIVER_MODE_ASHTECH we can just pass additional specific nmea_flags in one general nmea driver (or specific type var) that is tell us that receiver is astech (or any other specific further).

The safer solution in the mid-term is to add a new type and eventually deprecate astech if both get merged.

Or like this, yes.

[jajberni]

I have created a pull request in #74

Now the code works nicely at 10Hz and a valid vehicle_gps_position is generated only when we got a valid VEL and POS. The code checks the UTC time from the NMEA sentence and the last received timestamp to avoid duplicated information until the next NMEA block.

Unfortunately, HDT and VTG don't have UTC information. I guess we'll need to live with that and update as they arrive. I am not sure if there are any NMEA order defined (I haven't found any clear indication) to determine if HDT is sent at the beginning or end of the NMEA block.

[lukegluke]

Unfortunately, HDT and VTG don't have UTC information.

Indeed, but they have gps_absolute_time on receive to distinguish the order (if assuming that output rate of these messages is the same as main ones GGA/RMC)

[bkueng]

Added in #87
Please submit a PR if you need any change.