u-blox / ubxlib

Portable C libraries which provide APIs to build applications with u-blox products and services. Delivered as add-on to existing microcontroller and RTOS SDKs.
Apache License 2.0
310 stars 94 forks source link

GNSS receiver as SPI slave #74

Closed bbyers-UBCO closed 1 year ago

bbyers-UBCO commented 2 years ago

With the SPI configuration available on several devices, but not being available in the ubxlib, could we get the SPI support in ubxlib?

RobMeades commented 2 years ago

Hi there and thanks for contacting.

An SPI driver wouldn't be a difficult thing to do but we would need to implement and test it across all our currently supported platforms before we could publish it properly here, which is where the time goes. First question: what platform is this on (I think NRFConnect/Zephyr but please confirm)?

Depending on your timescales, a few possibilities arise:

  1. If you need this ASAP, the simplest thing to do would be for you to write your own SPI driver that just meets the current uPortUart API: this would involve zero code changes in ubxlib itself, and you could likely copy/paste the event part of the code from the current UART driver; timescale for this would be entirely up to you as you would be doing the work.
  2. We could implement a "trial" SPI API on whichever platform it is you use and make this available as a "preview" branch with very basic testing; this would likely be rather flaky, just the nature of a rush job, and the API itself would be subject to change since we can't settle-down a final API without thorough testing of an implementation on all platforms; timescale for this 1 to 2 weeks?
  3. We could do a proper job of an SPI API, implemented across all platforms, but still make it available as a preview branch, before we've thrashed it for the usual few weeks. There would still be a small chance of an API change after the preview branch but much reduced over (2); timescale for this 2 to 3 weeks?
  4. We do the lot, proper thrash testing: timescale - it would [probably] all be over by Christmas.

@hkro, @manemannen (our product managers) probably have an opinion on how high up the priority-list this goes [only important for options 2, 3, and 4 of course], though I'd guess that customer-pull trumps most things :-).

bbyers-UBCO commented 2 years ago

@RobMeades thanks for the quick reply! We are using nRFConnect SDK with Zephyr. We will work on 1 for now, but if any of the other options become things you are actively working on, please let us know because they would much appreciated!

bbyers-UBCO commented 2 years ago

@RobMeades Option 1 is proving to be more of a burden than anticipated. There are apparent issues during startup, particularly in functions such as receiveUbxMessageStream in u_gnss_private.c where there is no stream to receive from because it needs to control the CS line of the SPI to even see data. How quickly do you think we could get a "trial" SPI API for Zephyr/NRF Connect SDK?

RobMeades commented 1 year ago

OK: I have a bit of a break in what I am meant to be doing at the moment and so I will see if I can do something between 2 and 3 and push out a preview branch that will at least get you working. There will still be a chance of API changes after the preview branch because I will not be able to test it properly on all platforms but such changes will be minor. I hope to have something for next week.

hkro commented 1 year ago

After the preview branch of this feature, we'll ship it in ubxlib v1.2. After UART, SPI is the 1st choice to attach a GNSS receiver as a slave to a ubxlib host (SPI master).

RobMeades commented 1 year ago

Update: the SPI port API is written and implemented/tested for all platforms that have SPI HW, am now going to integrate it with the GNSS code; hopefully something will be pushed here on a barely tested preview branch before the end of Wednesday.

RobMeades commented 1 year ago

I have now pushed a preview branch of SPI support here: https://github.com/u-blox/ubxlib/tree/preview_spi_rmea.

To repeat the dire warnings I've added there, this preview branch has NOT been heavily tested and the SPI-related contents of it (i.e. the most recent four commits of the branch) remain SUBJECT TO CHANGE. All of the examples/tests are updated to include use of SPI where relevant and I've run all of them manually, quite a few times, on Zephyr nRF52/nRF53, so I believe it is good, however it has not had the maturity/thrash testing I would normally like. In other words, option (3) above.

Let me know how you get on with it.

bbyers-UBCO commented 1 year ago

@RobMeades I'm trying to build it and getting a few errors:

/ublox/port/platform/zephyr/src/u_port_spi.c: In function 'setSpiConfig': /ublox/port/platform/zephyr/src/u_port_spi.c:111:26: error: 'struct spi_cs_control' has no member named 'gpio_dev' 111 | pSpiCfg->spiCsControl.gpio_dev = NULL; | ^ /ublox/port/platform/zephyr/src/u_port_spi.c:116:30: error: 'struct spi_cs_control' has no member named 'gpio_dev' 116 | pSpiCfg->spiCsControl.gpio_dev = pUPortPrivateGetGpioDevice(pinSelect); | ^ /ublox/port/platform/zephyr/src/u_port_spi.c:117:34: error: 'struct spi_cs_control' has no member named 'gpio_dev' 117 | if (pSpiCfg->spiCsControl.gpio_dev != NULL) { | ^ /ublox/port/platform/zephyr/src/u_port_spi.c:126:34: error: 'struct spi_cs_control' has no member named 'gpio_pin' 126 | pSpiCfg->spiCsControl.gpio_pin = pinSelect % GPIO_MAX_PINS_PER_PORT; | ^ /ublox/port/platform/zephyr/src/u_port_spi.c:128:38: error: 'struct spi_cs_control' has no member named 'gpio_dt_flags' 128 | pSpiCfg->spiCsControl.gpio_dt_flags = GPIO_ACTIVE_LOW; | ^ /ublox/port/platform/zephyr/src/u_port_spi.c: In function 'uPortSpiControllerGetDevice': /ublox/port/platform/zephyr/src/u_port_spi.c:303:38: error: 'struct spi_cs_control' has no member named 'gpio_dev' 303 | if (pSpiCfg->spiCsControl.gpio_dev != NULL) { | ^ /ublox/port/platform/zephyr/src/u_port_spi.c:305:43: error: 'struct spi_cs_control' has no member named 'gpio_dt_flags' 305 | if ((pSpiCfg->spiCsControl.gpio_dt_flags & GPIO_ACTIVE_LOW) != GPIO_ACTIVE_LOW) {

What version of Zephyr are you using for this to make sure I'm on the same or can find some differences?

RobMeades commented 1 year ago

https://github.com/u-blox/ubxlib/tree/master/port/platform/zephyr#sdk-installation

bbyers-UBCO commented 1 year ago

We are using NCS 2.1.0 and can't backport to 1.6.1 right now, so that's likely our issue. I may dig into how we have to modify that for 2.1.0.

RobMeades commented 1 year ago

That's strange, @plerup has tested compilation with NCS 2.1.0, see this commit 484eaf408a7f1293c18c803b78a0b0be482b7b44, so it should work...?

RobMeades commented 1 year ago

Of course, this is probably some other Zephyr 3.x change. Gah. Maybe I should go get a copy of NCS 2.1.0...

RobMeades commented 1 year ago

Double gah! The Zephyr version we are using allows you to just specify a chip select GPIO number and it will work; the later versions of Zephyr has drawn the chip select into the horrible mess of the device tree and removed the flexibility. Gawd, I hate the device tree.

bbyers-UBCO commented 1 year ago

Is that something you'll be able to look into? Or should I?

RobMeades commented 1 year ago

Don't worry, I'll fix it tomorrow.

RobMeades commented 1 year ago

Update: though I have what might be a fix, Nordic have moved where they store their toolchains after version 2 and our installation scripts can no longer find them. If you want to have a go at using the fix while I hack the hack of the hack to try to persuade the right tools to install, find attached the relevant file, which needs to go in the <ubxlib_root>/port/platform/zephyr/src directory (renamed to .c).

Amazing how running the tiniest embedded system somehow needs three PC applications and more than a gigabyte of download to install it :-|.

u_port_spi.c.txt

RobMeades commented 1 year ago

Update: I've built with NRF Connect 2.1.0 now but they've changed the debugger side as well so I will need to do battle with that on Monday. In the meantime an updated file with compilation errors fixed is attached in case you feel the urge.

u_port_spi.c.txt

RobMeades commented 1 year ago

Update: I have now verified that our Zephyr-platform SPI implementation supports the version 3 Zephyr structure as used in NRF Connect 2.1.0 and the preview branch is updated to reflect this.

bbyers-UBCO commented 1 year ago

@RobMeades I'll give that a shot today. Thank you!

bbyers-UBCO commented 1 year ago

Having issues with the device tree not finding my device. I'm fairly familiar with the device tree these days, so I'm a little baffled. How do you have the SPI device setup in your device tree file?

ubxlib commented 1 year ago

[from Rob, logged in as ubxlib at the moment]

I tested on an NRF53 DK board, for which I took the standard Zephyr nrf5340dk_nrf5340_cpuapp setup as comes with NRFConnect 2.1.0, unmodified, and used SPI4; cutting and pasting their file into here for clarity:

arduino_spi: &spi4 {
    compatible = "nordic,nrf-spim";
    status = "okay";
    cs-gpios = <&arduino_header 16 GPIO_ACTIVE_LOW>; /* D10 */
    pinctrl-0 = <&spi4_default>;
    pinctrl-1 = <&spi4_sleep>;
    pinctrl-names = "default", "sleep";
};

...where:

    spi4_default: spi4_default {
        group1 {
            psels = <NRF_PSEL(SPIM_SCK, 1, 15)>,
                <NRF_PSEL(SPIM_MISO, 1, 14)>,
                <NRF_PSEL(SPIM_MOSI, 1, 13)>;
        };
    };

    spi4_sleep: spi4_sleep {
        group1 {
            psels = <NRF_PSEL(SPIM_SCK, 1, 15)>,
                <NRF_PSEL(SPIM_MISO, 1, 14)>,
                <NRF_PSEL(SPIM_MOSI, 1, 13)>;
            low-power-enable;
        };
    };

...and:

    arduino_header: connector {
        compatible = "arduino-header-r3";
        #gpio-cells = <2>;
        gpio-map-mask = <0xffffffff 0xffffffc0>;
        gpio-map-pass-thru = <0 0x3f>;
        gpio-map = <0 0 &gpio0 4 0>,    /* A0 */
               ...
               <16 0 &gpio1 12 0>,  /* D10 */
               ...
    };

[Just as an aside, our standard test runner build, if you're using it, has an overlay file which I had to modify to disable our definition of uart2 since that uses those same pins; will need to reorganise this to avoid a collision when we publish properly].

Then, for the pins passed into the ubxlib functions, I used -1 for the MOSI, MISO and CLK pins, since they are defined at compile-time in the Zephyr case, but I wanted to test that I could pass to the ubxlib functions a real GPIO pin for SELECT, rather than using -1, so for that pin I passed in GPIO 44 (32 + 12, pin 12 in port 1, AKA Arduino pin 16/D10, pick your numbering scheme), i.e. using the pin already selected in the default NRFConnect 2.1.0 .dts file as above, for this, since I'm fresh out of pins.

This led to one dodge: even though I'm passing the SELECT pin in explicitly to the Zephyr driver (their struct spi_cs_control), Zephyr has, at compile time through their default .dts file, already been told to have that GPIO pin GPIO_ACTIVE_LOW (see above). Hence when I pass in my own explicit SELECT pin I need to say it is inverted, 'cos the Zephyr code will then invert it again and it will be the right way up, if you see what I mean :-) [really hate this in-the-RTOS compile-time configuration stuff; I mean, I can see the advantage of a way to define a board but hard coding it in the RTOS is too limiting, anyway...].

So in my test code the SELECT pin passed into the ubxlib functions was 44 | U_COMMON_SPI_PIN_SELECT_INVERTED, but this is ONLY BECAUSE I had to chose pin 44, which was already "inverted" in the .dts file; if the .dts file didn't mention, or at least didn't invert, the pin I had chosen, I would have passed in just 44. If you pass -1 in for the SELECT pin the ubxlib code will do nothing with it and so whatever Zephyr does automagically with that pin should "just work" (tm) :-).

bbyers-UBCO commented 1 year ago

Ok, digging into this again today. Forgot to mention, uPortSpiInit() wasn't being called anywhere so I kept getting an error because the gMutex was NULL.

bbyers-UBCO commented 1 year ago

Receiving data now. Still seem to be a few issues, but they may be on my end, so investigating why. Keep receiving 0xFF's after the initial message I receive. Trying to just follow the example to filter for "GPGGA" messages right now.

RobMeades commented 1 year ago

The SPI support code has now been pushed to master here, see commit 337c408bac08e84791134fca038859983b77374c. I will close this issue and delete the preview branch in a few weeks.