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
313 stars 98 forks source link

Zephyr SPI not communicating with device #91

Closed mtfurlan closed 1 year ago

mtfurlan commented 1 year ago

I'm trying to have ubxlib communicate with a ZED-F9R from a nRF5340 over SPI. I can show ubxlib successfully using I2C from a nRF52840 to a NEO-M8U.

I've done my best combining pos_main.c and the zephyr example application, you can see my demonstration here.

setup modifications

I didn't follow zephy integration note to append the path to ubloxlib to ZEPHYR_EXTRA_MODULES, I just specified ubxlib in the west.yml, this is much cleaner, and given that it compiles, I don't expect this to be a problem.

I'm also testing against actual zephyr, not Nordic's zephyr fork. I just used the zephyr example-application to demonstrate my issue and isolate the issue when I had this problem in my project repo that does use the nordic fork, and I didn't think about the fact that it's not the same zephyr till just now. I see the issue in both places though. If you want, I can rework the example repo to use the nordic fork.

I can talk to the ZED-F9R over spi without ubxlib

I have verified the SPI bus works and and that I can talk to the ublox(not using ubloxlib at all), that's app-spi. I have a ublox device in the devicetree on the spi bus that is compatible = "vnd,spi-device";.

actually using ubxlib

In trying to use ubxlib, I removed the ublox entry from the device from the devicetree, and set U_CFG_TEST_GNSS_MODULE_TYPE and U_CFG_APP_GNSS_SPI. This works for I2C on a nRF52840 to a ublox NEO-M8U(Just have to change the defines, see line 14).

I am using custom boards, but if you just wire up a ublox EVK to a nordic DK I expect that it will work the same.

I'm not super sure I did the transportCfg.cfgSpi.device correctly:

    .transportCfg = {
        .cfgSpi = {
            .spi = U_CFG_APP_GNSS_SPI,
            // pins *must* be negative, in ubxlib/port/platform/zephyr/src/u_port_spi.c uPortSpiOpen it says
            // > On Zephyr the pins are set at compile time so those passed
            // > into here must be non-valid
            .pinMosi = -1, // U_CFG_APP_PIN_GNSS_SPI_MOSI,
            .pinMiso = -1, // U_CFG_APP_PIN_GNSS_SPI_MISO,
            .pinClk = -1, // U_CFG_APP_PIN_GNSS_SPI_CLK,
            // I don't really know how this works
            .device = U_COMMON_SPI_CONTROLLER_DEVICE_DEFAULTS(-1) // U_CFG_APP_PIN_GNSS_SPI_SELECT
        },
    },

Output:

*** Booting Zephyr OS build zephyr-v3.3.0 ***
Zephyr Example Application 1.0.0
U_GNSS: initialising with ENABLE_POWER pin not connected, transport type SPI.
U_GNSS: sent command b5 62 0a 06 00 00 10 3a.
Opened device with return code -8.
Unable to open GNSS!
[00:00:10.617,645] <err> os: ***** BUS FAULT *****
[00:00:10.617,675] <err> os:   Precise data bus error
[00:00:10.617,706] <err> os:   BFAR Address: 0x2d402e8
[00:00:10.617,736] <err> os: r0/a1:  0x02d402e8  r1/a2:  0x20004304  r2/a3:  0x0ea7beef
[00:00:10.617,736] <err> os: r3/a4:  0x00000020 r12/ip:  0x0000000a r14/lr:  0x00022647
[00:00:10.617,767] <err> os:  xpsr:  0x29000000
[00:00:10.617,767] <err> os: Faulting instruction address (r15/pc): 0x00019c78
[00:00:10.617,797] <err> os: >>> ZEPHYR FATAL ERROR 25: Unknown error on CPU 0
[00:00:10.617,828] <err> os: Current thread: 0x20000860 (main)
[00:00:10.870,391] <err> os: Halting system

philwareublox commented 1 year ago

Hi. I've had a look at what you have in your example and I would say it is valid. You've set -1 for the MOSI pins, and have the overlay included. I have reached out to our engineer about this, although they are out-of-office currently.

I have looked at the test_farm database and we do test both Nordic boards with Zephyr on the M9 GNSS module over SPI. 15.1 and 17.

bbyers-UBCO commented 1 year ago

We also tried to implement the example found in "pos_main.c" recently on the v1.2.0 release using Zephyr and were getting, "Unable to open GNSS!" over SPI but with a different return code (-5). Not sure what is happening or how to go about debugging the UBXLIB side of this.

RobMeades commented 1 year ago

@mtfurlan: thanks for the detailed report. Going through your questions one by one:

Having to set defines like U_CFG_APP_GNSS_I2C=0 U_CFG_TEST_GNSS_MODULE_TYPE=U_GNSS_MODULE_TYPE_M8 instead of configuring in the devicetree is very confusing, and makes targeting multiple boards very difficult. Am I just doing this wrong?

ubxlib targets more than one OS and so is not "bound" to the device tree, if you see what I mean: it generally takes integer parameters as that is the simplest possible approach. In fact, our first platform was ESP-IDF, Zephyr only came along later, and while Zephyr is gaining a lot of traction, I have to say the device tree is the thing I dislike about it most: I can never, ever, find my way around it and it has changed syntax in a fairly fundamental way once a year so far! But enough of my belly-aching, that's the reason why.

In ubxlib/port/platform/zephyr/src/u_port_spi.c uPortSpiOpen it calls DEVICE_DT_GET_OR_NULL(DT_NODELABEL(spiX)) for spi0 to spi4, regardless of if those are defined. This causes build issues when not using spi.

That's true, we should fix that: we generally try to avoid conditional compilation inside the ubxlib code, it leads to uncompiled cruft, but Zephyr relies on it heavily so we should go with that flow in the Zephyr port layer.

How do I make ubxlib spi work?

Your error code -8 is U_ERROR_COMMON_PLATFORM, so my first thought was that the Zephyr functions that open and configure SPI might be returning an error, however your log shows:

U_GNSS: initialising with ENABLE_POWER pin not connected, transport type SPI.
U_GNSS: sent command b5 62 0a 06 00 00 10 3a.

...which means that uGnssAdd() has been called and has sent a command to the GNSS module over SPI. The command b5 62 0a 06 is just UBX-MON-MSGPP (ZED-F9R manual section 3.14.7), which ubxlib only sends as part of the function uGnssPrivateSendOnlyCheckStreamUbxMessage() so it seems likely we are in the function uGnssPwrOn() just here, where the code is first talking to the GNSS chip to make sure it is powered-on, but it has received no response.

So either (a) the GNSS chip is not on or (b) it is not listening to SPI or (c) the SPI is somehow not working correctly. You mention MOSI (P0.20), MISO (P0.31) and CLK (P0.19) and I see from your board file that CS is P0.17: do you have access to a Saleae probe or similar to take a plot, say triggered by CS going low, of these pins so that we could see the SPI comms?

RobMeades commented 1 year ago

@bbyers-UBCO: your error code, -5, is U_ERROR_COMMON_INVALID_PARAMETER, so that should be easier to get to the bottom of: can you paste in here what your configuration structure uDeviceCfg_t looks like?

bbyers-UBCO commented 1 year ago

@RobMeades


    .deviceType = U_DEVICE_TYPE_GNSS,
    .deviceCfg = {
        .cfgGnss = {
            .moduleType = U_CFG_TEST_GNSS_MODULE_TYPE,
            .pinEnablePower = GNSS_EN_PIN,
            .pinDataReady = -1, // Not used
            // There is an additional field here:
            // "i2cAddress", which we do NOT set,
            // we allow the compiler to set it to 0
            // and all will be fine. You may set the
            // field to the I2C address of your GNSS
            // device if you have modified the I2C
            // address of your GNSS device to something
            // other than the default value of 0x42,
            // for example:
            // .i2cAddress = 0x43
        },
    },
    .transportType = U_DEVICE_TRANSPORT_TYPE_SPI,
    .transportCfg = {
        .cfgSpi = {
            .spi = U_CFG_APP_GNSS_SPI,
            .pinMosi = U_CFG_APP_PIN_GNSS_SPI_MOSI,
            .pinMiso = U_CFG_APP_PIN_GNSS_SPI_MISO,
            .pinClk = U_CFG_APP_PIN_GNSS_SPI_CLK,
            .device = U_COMMON_SPI_CONTROLLER_DEVICE_DEFAULTS(U_CFG_APP_PIN_GNSS_SPI_SELECT)
        },
    },
};```
RobMeades commented 1 year ago

To you both: I'm not in the office right now and don't have access to work e-mail on my phone but I will power the laptop up at least once a day to see if you have added information here.

RobMeades commented 1 year ago

@bbyers-UBCO: great, thanks.

I guess that U_CFG_APP_PIN_GNSS_SPI_MOSI, U_CFG_APP_PIN_GNSS_SPI_MISO, U_CFG_APP_PIN_GNSS_SPI_CLK and U_CFG_APP_PIN_GNSS_SPI_SELECT are all -1? Can you confirm what you have set U_CFG_TEST_GNSS_MODULE_TYPE and GNSS_EN_PIN to?

EDIT: also what U_CFG_APP_GNSS_SPI is set to, for completeness.

RobMeades commented 1 year ago

@mtfurlan, one question: I guess you are setting U_CFG_APP_GNSS_SPI to 4 to match your choice of SPI 4 in the board file, but I can't immediately see where you're doing that; just thought I'd mention it.

RobMeades commented 1 year ago

@mtfurlan:

Having to set defines like U_CFG_APP_GNSS_I2C=0 U_CFG_TEST_GNSS_MODULE_TYPE=U_GNSS_MODULE_TYPE_M8 instead of configuring in the devicetree is very confusing, and makes targeting multiple boards very difficult.

We're open to suggestions here of course; not being Zephyr experts, is there some custom metadata we could require a user to insert in the devicetree which would tell us the transport a given module is connected on and the module type, so that Zephyr user's could work that way?

mtfurlan commented 1 year ago

@RobMeades Thanks for the detailed reply!

I'm setting U_CFG_APP_GNSS_SPI in app/CMakeLists.txt, I didn't really see any other way to do it.

On the devicetree stuff, I'll look at it a bit and see if I can think of anything that doesn't break the other platforms. I'll open a different issue about ubxlib pull from devicetree not defines to not clutter up this one next week after I investigate.

On the spi bus, the app-spi has a nearly identical devicetree overlay to app(it defines a vnd,spi-device on the spi bus), but uses zephyr spi stuff to successfully talk to the ublox. I believe that demonstrates that the spi bus is functional?

Anyway, I'll get a logic analyzer on it, but that might not happen till next week because we forgot to put test pads on the spi line so breaking it out is going to suck a little.

RobMeades commented 1 year ago

Ah, yes, I see it now, that should be fine. What the devil is the difference between the two? I will keep looking, maybe something will occur and you won't have to butcher your board.

Good idea to open a new issue for the "canonical" Zephyr way of doing it, that would be very useful if there were a way.

Just FYI, here's the logging output from our test instance 17, which, uses this overlay file to talk SPI (SPI 2, SCK 0.27, MISO 0.25, MOSI 0.26, CS 1.14) with an M9 module (the "M" is not relevant here, it is the digit which matters).

14:30:15,214 U_APP: Running exampleGnssPos...
14:30:15,215 progress update - item exampleGnssPos() started on 14:30:15.215643.
14:30:15,226 U_GNSS: initialising with ENABLE_POWER pin not connected, transport type SPI.
14:30:15,261 U_GNSS: sent command b5 62 0a 06 00 00 10 3a.
14:30:16,176 U_GNSS: decoded UBX response 0x0a 0x06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 16 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d1 06 00 00 00 00 00 00 [body 120 byte(s)].
14:30:16,189 U_GNSS: sent command b5 62 06 04 04 00 00 00 09 00 17 76.
14:30:16,200 U_GNSS: sent command b5 62 0a 06 00 00 10 3a.
14:30:17,181 U_GNSS: decoded UBX response 0x0a 0x06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 18 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 63 07 00 00 00 00 00 00 [body 120 byte(s)].
14:30:17,194 Opened device with return code 0.
14:30:17,205 Starting position stream.
14:30:17,273 U_GNSS: sent command b5 62 06 8b 08 00 00 00 00 00 ff ff 21 00 b8 cc.
14:30:17,285 U_GNSS: decoded UBX response 0x06 0x8b: 01 00 00 00 03 00 21 20 01 01 00 21 30 e8 03 02 00 21 30 01 00 [body 21 byte(s)].
14:30:17,373 U_GNSS: sent command b5 62 06 01 02 00 01 07 11 3a.
14:30:17,385 U_GNSS: decoded UBX response 0x06 0x01: 01 07 00 00 00 00 00 00 [body 8 byte(s)].
14:30:17,475 U_GNSS: sent command b5 62 06 01 08 00 01 07 00 00 00 00 01 00 18 de.
14:30:17,576 U_GNSS: decoded UBX response 0x05 0x01: 06 01 [body 2 byte(s)].
14:30:17,587 Waiting up to 60 seconds for 5 position fixes.
14:30:18,280 I am here: https://maps.google.com/?q=+52.2227470,-0.0748310
14:30:19,486 I am here: https://maps.google.com/?q=+52.2227471,-0.0748314
14:30:20,689 I am here: https://maps.google.com/?q=+52.2227471,-0.0748317
14:30:21,993 I am here: https://maps.google.com/?q=+52.2227472,-0.0748317
14:30:23,197 I am here: https://maps.google.com/?q=+52.2227472,-0.0748319
14:30:23,598 U_GNSS: sent command b5 62 06 01 08 00 01 07 00 00 00 00 00 00 17 dc.
14:30:23,609 U_GNSS: decoded UBX response 0x05 0x01: 06 01 [body 2 byte(s)].
14:30:23,620 Done.
14:30:23,630 /home/ubxlib/workspace/ubxlib_master@4/ubxlib/example/gnss/pos_main.c:227:exampleGnssPos:PASS
RobMeades commented 1 year ago

@mtfurlan :something you might try if you have time, before you do any butchering: also define U_GNSS_PRIVATE_DEBUG_PARSING.

This should not normally be switched on, as it might cause too much logging, but it would tell us if we are somehow receiving a partial or damaged response or the like that is being discarded.

mtfurlan commented 1 year ago
U_GNSS: initialising with ENABLE_POWER pin not connected, transport type SPI.
U_GNSS: sent command b5 62 0a 06 00 00 10 3a.
** DISCARD: wanted UBX 0a06, got UNKNOWN, 8 byte(s)
** Discarded contents: ff ff ff ff ff ff ff ff
Opened device with return code -8.
Unable to open GNSS!
RobMeades commented 1 year ago

Looks like just fill then, nothing of consequence. I'll keep thinking.

mtfurlan commented 1 year ago

So I don't 100% trust the cheap LA104 logic analyzer I have right now, but it seems to suggest that ubxlib isn't asserting CS. I can get it to trigger on the app-spi zephyr spi, but not with ubxlib.

RobMeades commented 1 year ago

Oh, interesting. We shouldn't be doing anything with CS, should be all automatic Zephyr stuff since you are passing in -1.

RobMeades commented 1 year ago

Look around here:

https://github.com/u-blox/ubxlib/blob/master/port/platform/zephyr/src/u_port_spi.c#L116

pinSelect should be -1, given what you are setting, and so that chunk of code should be doing nothing, leaving the CSness to Zephyr.

bbyers-UBCO commented 1 year ago

U_CFG_APP_PIN_GNSS_SPI_SELECT isn't set to -1. Sounds like we should try setting it to -1 to let Zephyr handle the CS? U_CFG_TEST_GNSS_MODULE_TYPE=1 GNSS_EN_PIN=5 U_CFG_APP_GNSS_SPI=3

mtfurlan commented 1 year ago
.device = U_COMMON_SPI_CONTROLLER_DEVICE_DEFAULTS(-1) // U_CFG_APP_PIN_GNSS_SPI_SELECT

Isn't that the same thing as setting U_CFG_APP_PIN_GNSS_SPI_SELECT to -1? Without code highlighting that really isn't easy to see it's a comment, sorry.

Also I don't have an GNSS enable pin, the ublox is just always powered. Is having an enable pin recommended?

RobMeades commented 1 year ago

To avoid my brain melting (I'm in a pub Tromso :-)) could I suggest that @bbyers-UBCO we open a new issue for your problem? Otherwise we might all get very confused.

RobMeades commented 1 year ago

@bbyers-UBCO: thanks!

@mtfurlan; yes indeed, that should set U_CFG_APP_PIN_GNSS_SPI_SELECT to -1: can I suggest you add a line after this one with something like:

uPortLog("pSpiCfg->pinSelect %d.\n", pSpiCfg->pinSelect);

...just to be completely sure the compiler is not having a laugh with us somehow?

Having GNSS always powered is fine, assuming your application is not power-limited.

mtfurlan commented 1 year ago

The log doesn't work for some reason, but I confirmed that it's -1 with gdb.

RobMeades commented 1 year ago

Hmph. So we have:

Not sure what to suggest next. Maybe you could examine the configuration structure passed to Zephyr from gdb and see if anything in it looks wrong?

mtfurlan commented 1 year ago

const struct device a bunch of void* so I can't inspect very well. I did a bit of looking around, there might be something weird happening in _spi_context_cs_contro. I get to line 239, and then leave the function. But all of the things it checks are valid pointers.

I haven't had time to compare this to the working spi, I'll do that tomorrow.

mtfurlan commented 1 year ago

Yeah it acts different when not using ubxlib, it actually goes through stuff in _spi_context_cs_control. I'll keep looking at this.

mtfurlan commented 1 year ago

I got a nRF5340 dk and a C102-F9R, and connected it using the pins you said your test instance 17 uses, and got the same result. So I feel like it must be something with how I'm using ubxlib?

I'm working through some issues building the examples in port/platform/zephyr/runner now.

RobMeades commented 1 year ago

Sorry you're having all this hassle, it is really rather mysterious. Just to confirm, when you say "got the same result", do you mean that, with the nRF5340 DK board and an F9R connected to it via SPI, the chip select pin is still not toggling?

Also to confirm, in our test system, for the NRF5340 DK setup, we use SPI as follows:

    spi2_default: spi2_default {
        group1 {
            psels = <NRF_PSEL(SPIM_SCK, 0, 27)>,
                    <NRF_PSEL(SPIM_MISO, 0, 25)>,
                    <NRF_PSEL(SPIM_MOSI, 0, 26)>;
        };
    };

...

&spi2 {
    compatible = "nordic,nrf-spim";
    status = "okay";
    cs-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
    pinctrl-0 = <&spi2_default>;
    pinctrl-1 = <&spi2_sleep>;
    pinctrl-names = "default", "sleep";
};

...so the chip select pin is 14 and active low; you are using these pins?

mtfurlan commented 1 year ago

Yeah, I used this overlay file that you linked earlier.

By "got the same result" I just mean that if I use zephyr spi driectly with a slightly modified overlay the ublox responds, but if I use ubxlib with your overlay, the ublox doesn't respond. I didn't actually confirm that CS wasn't doing anything though.

So my current guess is that I'm setting up ubxlib wrong somehow, so I'm trying to build the port/platform/zephyr/runner stuff.

RobMeades commented 1 year ago

Understood, that seems sensible. I'm trying to think what could go wrong at compile time.

The trace output you put in your initial post indicates that ubxlib is running and trying to send something over SPI, I guess SPI 2 from your linked CMakeLists.txt, I can't think of anything you could do at compile time for that to go wrong; we generally avoid using build flags in the ubxlib core code if we can, and aside from a Zephyr version 3 switch, which would likely fail to compile if it were wrong, there's nothing like that around here.

All the pins are being handled inside Zephyr itself.

If at all possible, it would be good to get visibility of what the pins are doing; it is a bit black and white as to what's going wrong otherwise, mostly black :-).

mtfurlan commented 1 year ago

Yep, looks like it's still a CS issue. Working by using zephyr spi stuff directly: app-spi working using ubxlib: app not working


I got the ubxlib/port/platform/zephyr/runner to build, but it's also failing.

I added this to the CMakelists.txt:

target_compile_definitions(app PRIVATE U_CFG_APP_GNSS_SPI=2 U_CFG_TEST_GNSS_MODULE_TYPE=U_GNSS_MODULE_TYPE_M9 U_GNSS_PRIVATE_DEBUG_PARSING U_CFG_APP_FILTER=exampleGnss)

It print some normal looking stuff about what functions are available, and then

** DISCARD: wanted UBX 0a06, got UNKNOWN, 2047 byte(s)
** Discarded contents: 00 00 00 00...

with (presumably) 2047 bytes of 00, a bunch of times, and then eventually

Opened device with return code -8.
Unable to open GNSS!

ubxlib runner This one looks pretty weird to me, here is the first tiny bit: what?

I'll verify the overlays didn't change or something and that I have the pins right, that seems wrong.

I'll also try doing i2c with the ubxlib runner, that should be fine but I want to prove it.

RobMeades commented 1 year ago

That's excellent, full visibility.

One experiment worth trying is to also add to target_compile_definitions() U_CFG_APP_PIN_GNSS_SPI_SELECT=xx, where xx is the CS pin you are using (e.g. for pin 3 on port 1 this would be 3 + 32). Doing that would cause the code here to be executed, which shouldn't be necessary but it might be worth checking.

mtfurlan commented 1 year ago

That made it work!

RobMeades commented 1 year ago

Aha, so I have some work to do then. Basically, that should not be necessary.

Our test system has always set the CS pin, just so that we test that one can set it "manually" that way. When we moved from Zephyr 2 to Zephyr 3, my colleague who did the change found that he had to specify the CS pin in the Zephyr device tree for things to work.

I thought nothing of it at the time but, checking the test system again, it still has the "manual" setting of the CS pin in it, so it looks like we've somehow or other got ourselves into a situation where, by default, our code manages to kind of disable the natural Zephy CS pin selection, always requiring it to be overridden.

Of course, you can progress setting the pin "manually" like this for now, but I will see if I can find out what's upsetting the natural Zephyr way of doing things.

RobMeades commented 1 year ago

FYI, I have consulted on the Zephyr Discord forums, SPI topic, and it seems that I misunderstood the spi_transcieve() command: the config field passed into it has to match the data from the device tree, it kind of goes hand-in-hand with the device tree, it is not a mechanism to supply additional configuration information or override the device tree settings as I thought it was (and, I think, as it used to be until Zephyr 3 came along).

I will modify our code to read the device tree for SPI and pass that into spi_transcieve(), which should have the same effect as setting the CS pin in our API as well as having it specified in the device tree. When I do that I will change the code to check that the CS pin passed into our API is -1 (or at least negative), like all the other pins for Zephyr, so that it is very obvious that we have made this change.

I will let you know when I have made that change and pushed it here.

RobMeades commented 1 year ago

I've just had a moment of realization. spi_transceive(), the Zephyr function which does SPI transfers, takes a pointer to an spi_config structure, which contains things like the CS pin to use, and in Zephyr speak, can be taken from the device tree. But this is the SPI configuration for a device (e.g. it might be called spidevgnss or some such in the device tree), it is NOT the SPI configuration for the controller (which might be spi3).

This makes sense as things like frequency, clock polarity, and of course the CS pin, are device specific, and it is in fact how the ubxlib APIs work: the CS pin, frequency, etc. are passed in via uPortSpiControllerSetDevice(), I'd just not realized that was how Zephyr was looking at the world also.

With that understanding it makes sense that pinSelect passed to uPortSpiControllerSetDevice() must be the same as the one in the cs-gpios field of spi3; if it isn't then the CS pins are not in the same "set" and nothing will work; the cs-gpios field can include multiple CS pins, uPortSpiControllerSetDevice() (and hence spi_transceive()) just picks one of them.

With this thinking, I should allow the CS pin passed to uPortSpiControllerSetDevice() to be a real pin: if it is, then I drive CS "manually" from within the ubxlib Zephyr SPI driver code; otherwise if it is -1 then I could assume that the first pin in cs-gpios for that controller (e.g. the first item in the cs-gpios field of spi3) is to be used, if I can find a way of reading that value out of the Zephyr device tree, the part that is proving problematic...

RobMeades commented 1 year ago

I have pushed to this branch a preview of the fix for this CS issue on Zephyr SPI.

Basically, there is a new field in the uCommonSpiControllerDevice_t structure that is passed to uPortSpiControllerSetDevice() called indexSelect, and there is a new initialisation macro, U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(indexSelect) to go with it.

Zephyr users can use this new macro when initialising the device variable that they will pass to uPortSpiControllerSetDevice(), the indexSelect value being the offset of the cs-gpios entry in the SPI controller device tree.

For instance, given a device tree entry:

&spi2 {
    compatible = "nordic,nrf-spim";
    status = "okay";
    cs-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>,
               <&gpio1 14 GPIO_ACTIVE_LOW>;
    pinctrl-0 = <&spi2_default>;
    pinctrl-1 = <&spi2_sleep>;
    pinctrl-names = "default", "sleep";
};

...then the SPI configuration in ubxlib to use pin 14 of GPIO 1 as the chip select (i.e. the cs-gpios entry at index 1) would be:

.transportType = U_DEVICE_TRANSPORT_TYPE_SPI,
.transportCfg = {
    .cfgSpi = {
        .spi = 2,
        .pinMosi = -1,
        .pinMiso = -1,
        .pinClk = -1,
        .device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(1)
    },
}

FTAOD, it is still possible to set a pin number directly as before, that works as it always did, and in fact there is no need to set the CS pin in the device tree; just passing a pin into the device structure will now work, simply to cover all bases.

@mtfurlan: when you have some time, could you maybe give this a try?

mtfurlan commented 1 year ago

Using port/platform/zephyr/runner to run the gnss examples doesn't work, but it looks like you didn't change example/gnss/pos_main.c to use U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS so I think that's expected?

If I use ubxlib in my own code: test result
.device = U_COMMON_SPI_CONTROLLER_DEVICE_DEFAULTS(U_CFG_APP_PIN_GNSS_SPI_SELECT) with U_CFG_APP_PIN_GNSS_SPI_SELECT defined to be 32+14 works
.device = U_COMMON_SPI_CONTROLLER_DEVICE_DEFAULTS(32+14) works
.device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(0) with cs-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>; Unable to open GNSS!

This is using a stock nRF5340 dk and the overlay from ubxlib.

RobMeades commented 1 year ago

Correct, I did not modify the examples (though in the version that I eventually committed I put a big note in each of them about the option of using U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS() instead).

Thanks for trying it: I think I might see the problem - in making the change I modified the nRF5340DK overlay, so that the correct CS pin is in position 1, just so that I can test that the indexing is doing something (see below). Sorry, I should have mentioned this!

If you have time, could you possibly try again with U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(1)? Fingers crossed that will work.

/* Can't use the default SPI pins as they overlap with the flow control pins we use for the UART.
   Note that we set gpio1 15 below as well as the real gpio1 14 simply to test the CS selection
   logic */
&spi2 {
    compatible = "nordic,nrf-spim";
    status = "okay";
    cs-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>,
               <&gpio1 14 GPIO_ACTIVE_LOW>;
    pinctrl-0 = <&spi2_default>;
    pinctrl-1 = <&spi2_sleep>;
    pinctrl-names = "default", "sleep";
};
mtfurlan commented 1 year ago

I didn't actually use the updated overlay which is why I did 0.

Tried a few more things, and it's weird:

test result
.device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(0) with cs-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>; Unable to open GNSS!
.device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(0) with cs-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>, <&gpio1 15 GPIO_ACTIVE_LOW>; Unable to open GNSS!
.device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(1) with cs-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>, <&gpio1 14 GPIO_ACTIVE_LOW>; works
RobMeades commented 1 year ago

Gah, sorry, turns out I wasn't testing what I thought I was testing and I had a schoolboy error (if (index > 0) rather than if (index >= 0)); will post a fixed version here tomorrow.

RobMeades commented 1 year ago

https://github.com/u-blox/ubxlib/tree/preview_fix_zephyr_spi_cs_rmea now updated, hopefully more likely to work with an index of 0. Apologies for the screw-up.

mtfurlan commented 1 year ago

That works!

RobMeades commented 1 year ago

Phew! One down, I will push it here in the next few days.

RobMeades commented 1 year ago

Fix is now pushed here, see 9acf3ff6db64ec37b9681df967ca0e727257762e. I will close this: please re-open it if you think there are things we need to discuss here, things that are not covered by our other threads.