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

Add support for older u-blox chips (7 series, 6 series). Fixes https:… #114

Closed
wants to merge 1 commit into from

Conversation

blazczak
Copy link

…//github.com//issues/113

Based on meshtastic@512833e by @geeksville

Includes my suggestion to check hardware capability before invoking the proper parsing routine from #113 and a fix for obtaining accuracies from UBX-NAV-POSLLH.

getProtocolVersionHigh() short-circuits after the 1st invocation.

Based on meshtastic@512833e by @geeksville

Includes my suggestion to check hardware capability before invoking the proper parsing routine from #113 and a fix for obtaining accuracies from UBX-NAV-POSLLH.

getProtocolVersionHigh() short-circuits after the 1st invocation.
@PaulZC
Copy link
Collaborator

PaulZC commented Jun 15, 2020

Hi @blazczak ,
Tagging @nseidle .
Many thanks for submitting this PR. I have to say that it looks very neat and tidy. I was worried that adding support for the series 6 and 7 chips could turn into a nightmare, but your and @geeksville's solution is quite elegant.
Unfortunately I don't have any series 6 or 7 hardware to try this on. And I'm assuming you have tried it on M8 and haven't noticed any problems? Maybe F9 too?
I'm glad you spotted that getProtocolVersionHigh() does indeed short-circuit after the 1st invocation, so we don't need to worry about excess I2C traffic.
Here's my concern: if we implement this, will users of the series 6 and 7 expect all of the other library features to work for them? I'm thinking of autoPVT in particular which will fail on series 6. It could work on Series 7 as @letranloc points out.
So I'm wondering if we need to add a whole bunch of protocol warnings like we do for powerSaveMode.
Thoughts please.
This reminds me that I should add protocol warnings for the the getVal and setVal functions that are only supported on protocols >= 27.
Very best wishes,
Paul

@blazczak
Copy link
Author

Thanks @PaulZC.

I don't have an M8 available to test with, I'm assuming/hoping Sparkfun has an automated deployment/testing rig set up to validate PRs against the supported hardware list (from README.md). 😃

In #113 (comment) I mention as one of the potential future steps abstracting out the parsing code a bit to help handle evolving message payloads, again keyed off protocol versions, which may become necessary with the introduction of protocol changes in module versions past M8, F9.

To answer your question, I think it's a reasonable expectation for the library to support, on best effort basis, (features present in) older modules. If for example autoPVT doesn't work in 6 then the library could gracefully fail-over to the functionality already present - the approach this code tries to follow for some related features.

There conceivably exist only two ways a library evolves: 1) it comes to life with the earliest models and in time it is augmented to include functionality added in subsequent hardware revisions or 2) (as the case is here) it starts somewhere in the middle of the product line and still needs to evolve to include future and possibly past models. Those two ways are really not that much different, they will fundamentally only differ in when certain features for particular hardware revisions become available in the library.

My hope is that with little effort on the part of the contributors we can have this library support all of the module series in use today with a reasonable feature set for each. The refactoring I mentioned above can help keep things "lower maintenance" and make it easier to add functionality. As I've mentioned in #113, this library has (deservedly) a lot of visibility and usage already. To add a specific example for why 6-series u-blox chip support is valuable, those NEO6 modules are still present in some not so-old copter GPS hardware that people are still tinkering with.

PaulZC added a commit that referenced this pull request Jun 16, 2020
PaulZC added a commit that referenced this pull request Jun 16, 2020
Support for series 6 and 7: incorporating the changes from PR #114
@PaulZC
Copy link
Collaborator

PaulZC commented Jun 16, 2020

Hi @blazczak ,
Thanks again for submitting this PR.
After further discussion, we have decided to include your changes - but only as a stand-alone example (please see examples/Series_6_7).
Fundamentally, we don't have any series 6 or 7 hardware we can test the changes on and we're not willing to incorporate the changes unless we can demonstrate that series 6 and 7 can work happily alongside series 8 and 9. Like we say in the change notes, your changes should work, they look like they will work, but we have no way of proving this.
I hope you understand.
Very best wishes,
Paul & Nathan

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants