Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: gnss: add a generic NMEA GNSS driver #65422

Merged
merged 2 commits into from Nov 28, 2023

Conversation

fabiobaltieri
Copy link
Member

@fabiobaltieri fabiobaltieri commented Nov 17, 2023

Minimal generic NMEA based UART GNSS driver, basically the Quectel driver without any specific stuff or power management. Prob worth adding PM at a later stage, a sleep pin + regulator or something, but let's start minimal for now.

Tested against an Antenova M10578-A3 and a random ebay ublox counterfeit module.

Copy link
Member Author

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there, selfie review, I would nack and request changes if I could

drivers/gnss/gnss_nmea_generic.c Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
@fabiobaltieri fabiobaltieri force-pushed the gnss-generic branch 2 times, most recently from e88efe6 to 239a336 Compare November 22, 2023 23:23
@fabiobaltieri
Copy link
Member Author

Skimmed it down a bit more and tested again, added a build_all test as well. Should be good enough for a start.

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but would be magical even without magic numbers ✨

drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
drivers/gnss/gnss_nmea_generic.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes an we are good to go! I tested the driver on a Quectel L76LB as well, worked immediately :)

MAINTAINERS.yml Outdated Show resolved Hide resolved
tests/drivers/build_all/gnss/testcase.yaml Outdated Show resolved Hide resolved
@fabiobaltieri
Copy link
Member Author

Update:

  • moved few magics (✨) in defines and Kconfig
  • fixed the native_sim and maintainer path
  • also realized that few buffers were using ARRAY_SIZE, switched to sizeof, I think that would be correct one since that's used as bytes
  • moved the delimiters on a file wide variable, this allowed dropping the initializer from the data structure which should save few bytes, could not make it const though, I think there's few fields in struct modem_chat_config that could use a const and then we could tweak some drivers to save some RAM

@fabiobaltieri fabiobaltieri force-pushed the gnss-generic branch 3 times, most recently from 6d99fd2 to 6781264 Compare November 23, 2023 19:03
@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Nov 23, 2023

I tweaked the default nmea-timeout-ms up to 500, this is a sneaky one to get right, we could use a dedicated debugging option and some strategic debug print. Or maybe just a well placed warning if sentences are dropped continuously. Basically what I bumped into was that everything was working, then I disabled satellite support, that dropped the handler for GSV which sent the timing off and caused it to not work until I tweaked it up again.

Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Quectel driver, I could use proprietary NMEA commands to disable GSV and anything else I was not listening for to have a constant timeout, of course this can not be done for the generic one :)

One more nit though

Synchronization timeout for NMEA sentences. The NMEA parser is expecting
to receive a GGA and RMC sentences within this time frame to publish a
location data. Set accordingly to the UART datarate and location
reporting frequency. Defaults to 200ms if unspecified.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

500ms :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Do you think it would be possible to have the driver measure few messages and tweak this automatically?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, if we track the max delta between messages we are listening to, then we could set the timeout to half that delta.

0.10s RMC
0.15s GGA +0.05s
0.35s GSV +0.20s
0.36s GSV +0.01s
0.37s GSV +0.01s
0.38s GSV +0.01s
0.39s GSV +0.01s
1.00s RMC +0.61s

Max delta / 2 = 305ms

0.10s RMC
0.15s GGA +0.05s
0.35s GSV +0.20s
0.36s GSV +0.01s
0.37s GSV +0.01s
0.38s GSV +0.01s
0.39s GSV +0.01s
10.00s RMC +9.61s

Max delta / 2 = 4805ms

We can use RMC to recalculate the automatic delta calculation, to handle the user changing the fix rate. The first 1 to 2 data points would be discarded since the delta is not known

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm looking at it, it can just be fix rate / 2 :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then if the link is slow enough (guess 4800 is still common on these) then a transmission I think could use well over 50% of the uart bandwidth and get close to the 1s fix. Maybe it could use like 90% fix rate and then reset once the message is published?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the order, we don't know which message comes first, RMC, GGA or GSV, if we don't get the timing right, we could be publishing an RMC message which contains data which does not reflect GGA or GSV. The timeout is for re-synchronization, so we know when to publish

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we knew which came first, we could forego the timeout completely

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but then it sounds like the timeout option would only be usable on fast channels, if more than 50% of the uart time is utilized you'd end up never timing out and always reporting one of the two out of sync I imagine. But then the only solution would be to manually specify which one comes first... wondering how gps handles that.

Add a generic NMEA GNSS driver.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Add a generic test for GNSS devices.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Comment on lines +26 to +28
#define UART_RECV_BUF_SZ 128
#define CHAT_RECV_BUF_SZ 256
#define CHAT_ARGV_SZ 32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could always convert to Kconfig, but not blocking

@carlescufi carlescufi merged commit 407e9a4 into zephyrproject-rtos:main Nov 28, 2023
18 checks passed
@fabiobaltieri fabiobaltieri deleted the gnss-generic branch November 28, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: GNSS area: Process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants