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

Add public GNSS API for obtaining and processing navigation data #61073

Merged

Conversation

bjarki-trackunit
Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit commented Aug 2, 2023

Introduction

This PR adds a device driver API for GNSS modems.

Navigation data

The first commit adds a header with a proposed data type and a set of utilities to lay the foundation for working with navigation data in a manner abstracted away from the specific source of said data. Navigation data can be obtained from different sources, such as GNSS, WiFi location, cell tower triangulation etc. Processing the data once obtained is identical, which is the reason the navigation data type and utilities are split apart from the GNSS API.

GNSS API

The second commit contains the GNSS API itself. This API provides a function to obtain navigation data as described in the first commit, along with a bunch of GNSS specific features, like setting fix intervals, getting GNSS status etc, which can be used to determine and configure the accuracy of the supplied navigation data for example.

Relevant information before reviewing

The API has been designed to accommodate the common features of the following GNSS modems:

  • Quectel LC76G
  • TELIT SE868
  • Quectel BG96 (internal GNSS)
  • Quectel L76LL
  • UBLOX NEO M8

This commit contains a driver for the Quectel LCX6G GNSS modem, along with a collection of libraries for parsing NMEA messages and string formatted integers.

@bjarki-trackunit bjarki-trackunit added DNM This PR should not be merged (Do Not Merge) area: API Changes to public APIs labels Aug 2, 2023
@bjarki-trackunit bjarki-trackunit self-assigned this Aug 2, 2023
@bjarki-trackunit bjarki-trackunit marked this pull request as ready for review August 2, 2023 12:37
@fabiobaltieri
Copy link
Member

fabiobaltieri commented Aug 2, 2023

Thanks for working on this! Love the navigation utils library idea!

For the API, how about:

  • instead of gnss_get_nav_data(), have an iterable section to register callbacks at build time that would get a pointer of the data structure automatically every time it's updated (thinking like the input one, then it'd be very easy to integrate with zbus down the road)
  • in struct gnss_driver_api, all the other APIs are set/get pairs, how about just having two generic set_parameter, get_parameter pair? If anything that would keep the API compact and extensible without having to have so many pointers in the structure, thinking you may still do inline wrapper for standard ones so you still have type checking and validation.

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Aug 2, 2023

cc @brec-u-blox @jakkra any interest from u-blox folks on this?

@jakkra
Copy link
Contributor

jakkra commented Aug 2, 2023

cc @brec-u-blox @jakkra any interest from u-blox folks on this?

Yes probably @RobMeades as he is responsible for our ubxlib GNSS library
Maybe also FYI @plerup

@rodrigopex
Copy link
Contributor

rodrigopex commented Aug 2, 2023

Great work! Integrating with ZBus (in the future) is an excellent way to go. Some ZBus channels could share the events and nav data. Agree with @fabiobaltieri about iterable sections. However, I imagine the gnss_get_nav_data would still be necessary for some cases.

@bjarki-trackunit
Copy link
Collaborator Author

instead of gnss_get_nav_data(), have an iterable section to register callbacks at build time that would get a pointer of the data structure automatically every time it's updated (thinking like the input one, then it'd be very easy to integrate with zbus down the road)

The navigation data provided by an instance of a GNSS device driver is not necessarily completely abstract. An application wanting to share navigation data should do something like the following:

  • Get GNSS info (number of sats, hdop etc.)
  • Validate quality of fix (subjective to the application)
  • Get navigation data from GNSS
  • Process navigation data (publish on ZBUS for example)

The GNSS API presents the capabilities of the GNSS (getting the data), processing the data is the responsibility of the application.

in struct gnss_driver_api, all the other APIs are set/get pairs, how about just having two generic set_parameter, get_parameter pair? If anything that would keep the API compact and extensible without having to have so many pointers in the structure, thinking you may still do inline wrapper for standard ones so you still have type checking and validation.

The types passed to the functions are not identical, like they are in an API like the sensor API, leading to void * and typecasting on both sides of the API. In this case, having fewer pointers in the API struct will be at the cost of type safety, which I value over saving ROM.

@fabiobaltieri
Copy link
Member

The navigation data provided by an instance of a GNSS device driver is not necessarily completely abstract. An application wanting to share navigation data should do something like the following:

* Get GNSS info (number of sats, hdop etc.)

* Validate quality of fix (subjective to the application)

* Get navigation data from GNSS

* Process navigation data (publish on ZBUS for example)

The GNSS API presents the capabilities of the GNSS (getting the data), processing the data is the responsibility of the application.

Right but it's an inherently event-driven type of device (new set of data available), thinking that a callback based API would suit it well, in this case it would notify a of new data available (navigation, fix quality etc... guess it makes sense to split them out since they can arrive at different frequency).

The types passed to the functions are not identical, like they are in an API like the sensor API, leading to void * and typecasting on both sides of the API. In this case, having fewer pointers in the API struct will be at the cost of type safety, which I value over saving ROM.

Yeah that's fair, though from what I recall API proposals with an extensive set of functions are usually hard to justify. Anyway, worth trying.

@bjarki-trackunit
Copy link
Collaborator Author

Right but it's an inherently event-driven type of device (new set of data available), thinking that a callback based API would suit it well, in this case it would notify a of new data available (navigation, fix quality etc... guess it makes sense to split them out since they can arrive at different frequency).

Do you have an example of an existing event-driven device driver API? I am not entirely sure what an event driven device driver API would look like, it sounds closer to a subsystem to me? Like the navigation header could become a subsystem to which applications and GNSS modem could register as listener/publisher?

@fabiobaltieri
Copy link
Member

Do you have an example of an existing event-driven device driver API? I am not entirely sure what an event driven device driver API would look like, it sounds closer to a subsystem to me? Like the navigation header could become a subsystem to which applications and GNSS modem could register as listener/publisher?

Yeah, input has that as the only API:

#define INPUT_LISTENER_CB_DEFINE(_dev, _callback) \
static const STRUCT_SECTION_ITERABLE(input_listener, \
_input_listener__##_callback) = { \
.dev = _dev, \
.callback = _callback, \
}

but there's plenty of those, bluetooth uses a similar pattern for most of the callbacks.

@cfriedt
Copy link
Member

cfriedt commented Aug 3, 2023

dev-review: discussed runtime callback registration, vs iterable section callback registration, sensing subsystem comparison (distributing information, events). Maybe make some of those options available via Kconfig?

important: GNSS uses fixed-point math for high-precision calculations (used extensively as well by MISB). Is there a standard library that can be used? Can we implement something for Zephyr? Do we already have something in a module that can be used?

@bjarki-trackunit
Copy link
Collaborator Author

Update

After dev-review, I will update the callback mechanism for navigation_data, gnss info and gnss time to work similar to the INPUT_LISTENER_CB_DEFINE, and continue work on the fixed point navigation utilities :)

@nashif nashif added the Architecture Review Discussion in the Architecture WG required label Aug 5, 2023
This commit adds dumping of GNSS data and satellites to
the log if CONFIG_GNSS_DUMP_TO_LOG is selected

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
This commit adds parsing utilites for common string
representations of values contained in GNSS messages.

These utilites both parse and validate the integrity of
the data.

Unit tests are also added to validate the parsing
utilities.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
This commit adds utilites to parse the RMC and GGA
NMEA0183 messages, which contain all data which shall be
published using the struct gnss_data.

It also adds a test suite for the added utilities.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
This commit adds generic matches and handlers for the RMC,
GGA and GSV messages to be implemented as part of all
NMEA0183 based GNSS modems.

NMEA0183 based GNSS modems must place the
struct gnss_nmea0183_match_data struct as the first struct
in their data struct. Their data struct shall then be set
as the user_data for the modem_chat instance.

Lastly, the gnss_nmea0183_match callbacks must be included
in the unsolicited matches for the modem_chat instance.

The GNSS modems will initialize the NMEA0183 match instance
using gnss_nmea0183_match_init.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
This commit adds a GNSS driver for the Quectel LCX6G
series of GNSS modems (LC26G, LC76G, LC86G). It is
based on the modem subsystem, and the GNSS utilities
added in the two previous commits.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
This commit adds a sample which configures a GNSS modem
to enable all available systems, registers callbacks to
the GNSS data and satellites callbacks, and prints the
GNSS data and satellites from the callbacks when invoked
using printk.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
- Added GNSS documentation entry to peripherals
- Added GNSS API entry to the API overview as 3.6 experimental

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
@cfriedt cfriedt merged commit 42676e3 into zephyrproject-rtos:main Oct 30, 2023
20 of 21 checks passed
@fabiobaltieri
Copy link
Member

Great job @bjarki-trackunit!

@bjarki-trackunit
Copy link
Collaborator Author

@fabiobaltieri Thanks for the help!

Comment on lines +49 to +63
int navigation_distance(uint64_t *distance, const struct navigation_data *p1,
const struct navigation_data *p2);

/**
* @brief Calculate the bearing from one navigation point to another
*
* @param bearing Destination for calculated bearing angle in millidegrees
* @param from First navigation point
* @param to Second navigation point
*
* @return 0 if successful
* @return -EINVAL if either navigation point is invalid
*/
int navigation_bearing(uint32_t *bearing, const struct navigation_data *from,
const struct navigation_data *to);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bjarki-trackunit quick Q: was this intentional that these don't seem to have an implementation just yet? Just asking in case this was meant to be included in the PR and was somehow missed :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I completely forgot about those! They still need an implementation

int64_t increment;

__ASSERT(str != NULL, "str argument must be provided");
__ASSERT(str != NULL, "nano argument must be provided");
Copy link
Contributor

Choose a reason for hiding this comment

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

str is being checked instead of nano

Copy link
Member

Choose a reason for hiding this comment

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

could you send a PR with the fix?

@Oleh-Kravchenko
Copy link

@bjarki-trackunit,
Where is generic driver for simple NMEA over UART?

@bjarki-trackunit
Copy link
Collaborator Author

@bjarki-trackunit,
Where is generic driver for simple NMEA over UART?

All generic NMEA parsing is part of the gnss_nmea0183_match library, a driver will have to implement initialization and power management on top of this. See the quectel one for inspiration :)

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Nov 17, 2023

All generic NMEA parsing is part of the gnss_nmea0183_match library, a driver will have to implement initialization and power management on top of this. See the quectel one for inspiration :)

I have one in draft, may send it out over the weekend, did it while this was still in review and never got around to test it again.

@Oleh-Kravchenko
Copy link

@bjarki-trackunit,
Where is generic driver for simple NMEA over UART?

All generic NMEA parsing is part of the gnss_nmea0183_match library, a driver will have to implement initialization and power management on top of this. See the quectel one for inspiration :)

I've already did, but it doesn't work :)
It is also very hard to debug, no logs :(

@bjarki-trackunit
Copy link
Collaborator Author

@bjarki-trackunit,
Where is generic driver for simple NMEA over UART?

All generic NMEA parsing is part of the gnss_nmea0183_match library, a driver will have to implement initialization and power management on top of this. See the quectel one for inspiration :)

I've already did, but it doesn't work :)
It is also very hard to debug, no logs :(

CONFIG_MODEM_MODULES_LOG_LEVEL_DBG=y
CONFIG_LOG=y

@Oleh-Kravchenko
Copy link

@bjarki-trackunit,
Where is generic driver for simple NMEA over UART?

All generic NMEA parsing is part of the gnss_nmea0183_match library, a driver will have to implement initialization and power management on top of this. See the quectel one for inspiration :)

I've already did, but it doesn't work :)
It is also very hard to debug, no logs :(

CONFIG_MODEM_MODULES_LOG_LEVEL_DBG=y CONFIG_LOG=y

It's generate raw input from UART, but doesn't give any clue why it doesn't accept input:

D: $GNGGA, 151513.00 5020.00000 N 02020.0000 E 1 12 0.68 167.5 M 25.7 M 43

@fabiobaltieri
Copy link
Member

@bjarki-trackunit @Oleh-Kravchenko #65422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required area: API Changes to public APIs area: Build System area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation area: JSON area: Samples Samples Feature Request A request for a new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet