Skip to content
This repository was archived by the owner on Jan 28, 2021. It is now read-only.

Add support for ublox 7 series chips #113

Closed
blazczak opened this issue Jun 2, 2020 · 7 comments
Closed

Add support for ublox 7 series chips #113

blazczak opened this issue Jun 2, 2020 · 7 comments

Comments

@blazczak
Copy link

blazczak commented Jun 2, 2020

First of all, thanks to everyone who has contributed to this library thus far. Thanks @nseidle for doing a good job responding to requests and supporting the current code base.

I've been a SparkFun customer since at least 2014 and I have made more than a dozen orders with your company to date. I've also visited your campus in 2015. I say this not to throw my weight around, but simply to show that I'm a supporter.

Onto the issue: I have a 7 series chip and the current Ublox library doesn't support it - I'm not getting any valid data using the get methods on SFE_UBLOX_GPS. I noticed in #80 that @geeksville has done the preliminary investigation into the issue (his chip is a bit older though: neo6) and has made a good first stab at a proof of concept.

Can you guys leverage the findings and code in #80 to add support for older chips into your library? I know you said that if it's a reasonable merge, you can do it.

I agree that the candidate code needs be cleaned up a bit first before inclusion in the library code, to remove hardcoded stuff etc. Ideally, the target solution should check the capabilities of the chip and keep that state to be used in the execution of getPVT() or a similar strategy. Looks like the equivalent of the call SFE_UBLOX_GPS.minfo.hwVersion (anything below "00080000" for non-PVT solution) or something leveraging the value in SFE_UBLOX_GPS.getProtocolVersionHigh() would be good candidates for such capabilities determination - I don't have the u-blox specs in front of me. Adding this sort of strategy now may end up being needed in the future anyway.

Lastly, you're probably already aware of this, but your company and specifically your u-blox library are already trending on the u-blox.com website, giving you the well deserved credit and exposure. It may be a matter of time before more requests like this start coming in..

https://portal.u-blox.com/s/question/0D52p00008HKD5jCAH/i2c-configuration-using-arduino

https://portal.u-blox.com/s/question/0D52p000090aP1bCAE/maxm8-linux-cc-i2cddc-example-source-code

https://www.u-blox.com/en/blogs/stories/sparkfun%E2%80%99s-list-u-blox-shields-just-got-longer

@nseidle
Copy link
Member

nseidle commented Jun 8, 2020

I'm all for a PR that demos compatibility with series 7 (and/or 6 as well) but I can't dedicate time to it right now.

@blazczak
Copy link
Author

Based on the code in meshtastic@512833e I've created a patch (attached) which works against current SparkFun_Ublox_Arduino_Library and which could be used to create a PR. The only thing I've added was a fix to parse horizontal and vertical accuracies from POSLLH:

--- a/src/SparkFun_Ublox_Arduino_Library.cpp
+++ b/src/SparkFun_Ublox_Arduino_Library.cpp
@@ -796,12 +796,16 @@ void SFE_UBLOX_GPS::processUBXpacket(ubxPacket *msg)
       latitude = extractLong(8);
       altitude = extractLong(12);
       altitudeMSL = extractLong(16);
+      horizontalAccuracy = extractLong(20);
+      verticalAccuracy = extractLong(24);
 
       moduleQueried.gpsiTOW = true;
       moduleQueried.longitude = true;
       moduleQueried.latitude = true;
       moduleQueried.altitude = true;
       moduleQueried.altitudeMSL = true;
+      highResModuleQueried.horizontalAccuracy = true;
+      highResModuleQueried.verticalAccuracy = true;
     }
     else if (msg->id == UBX_NAV_HPPOSLLH && msg->len == 36)
     {

@geeksville do you want to create a PR with this updated patch so that you can get credit for it? The only thing remaining to be done is to remove hardcoding and to use the hardware capability determination I suggested above to invoke the proper parsing routine based on chip series.

patch.txt

@geeksville
Copy link

thank you for your nice note. I don't need credit though a mention in the commit is nice.

@blazczak
Copy link
Author

Done, @nseidle pls take it from here.

@letranloc
Copy link

The u-blox 7 chip has NAV_PVT command but the message length is just 84 bytes
Screen Shot 2020-06-15 at 13 12 59
How about this approach?
Screen Shot 2020-06-15 at 13 14 06

I modified that line and it's working fine

@blazczak
Copy link
Author

@letranloc that may work fine for you locally but with the above suggestion in library code you could get an out-of-bounds read with certain messages (and modules). The current code doesn't even reach offset 83 in that message payload (skips headVeh, magDec and magAcc among others) so the issue in the above approach is masked and the code silently works.

That's why parsing based on chip capabilities is the correct approach - the maintainers should double-check the protocol versions that trigger the particular invocation. An elegant way to map parsing code to the evolving message payload contents could be a further improvement; so not to use too many switches and/or ifs.

Also, the above approach doesn't address the lack of support for UBX-NAV-HPPOSLLH in 7 (and 6) which is handled by @geeksville's code.

@PaulZC
Copy link
Collaborator

PaulZC commented Jun 16, 2020

Thanks again for the contribution!
We have added a stand-alone example for series 6 and 7. Please see PR #115 and #114 for further details. I'll issue a new release which includes the changes later today.
Best wishes,
Paul

@PaulZC PaulZC closed this as completed Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants