#713 Improve SPI set-up: Don't change the config whilst it is enabled (not merged) #1158

Merged

daveythacher merged 2 commits from develop into develop

2023-02-06 08:49:38 -06:00

daveythacher commented

2022-12-26 04:36:19 -06:00

(Migrated from github.com)

Tested with the following code: (Quick and dirty)

#include <stdio.h>
#include "pico/stdio.h"
#include "hardware/gpio.h"
#include "hardware/spi.h"

int main() {
    // Setup USB Serial
    stdio_init_all();

    // Setup SPI0 Pins
    for (int i = 0; i < 4; i++) {
        gpio_init(i + 2);
        gpio_set_function(i + 2, GPIO_FUNC_SPI);
    }
    gpio_set_dir(3, true);

    // Setup and enable SPI0-slave
    spi_init(spi0, 1000000);
    spi_set_format(spi0, 8, SPI_CPOL_0, SPI_CPHA_1, SPI_MSB_FIRST);
    spi_set_slave(spi0, true);

    while(1) {
        // Test SPI0-slave function
        if (spi_is_readable(spi0)) {
            uint8_t c;
            spi_read_blocking(spi0, 0, &c, 1);
            printf("%c ", c);
        }
    }
}

Three SDK functions potentially attempt to change the SPI configuration while it is enabled. I hacked it to disable the module then change the configuration. It will attempt to re-enable the SPI if enabled. I do not save and restore other settings. I am not sure if this is required or not. Tested with the following code: (Quick and dirty) ```C++ #include <stdio.h> #include "pico/stdio.h" #include "hardware/gpio.h" #include "hardware/spi.h" int main() { // Setup USB Serial stdio_init_all(); // Setup SPI0 Pins for (int i = 0; i < 4; i++) { gpio_init(i + 2); gpio_set_function(i + 2, GPIO_FUNC_SPI); } gpio_set_dir(3, true); // Setup and enable SPI0-slave spi_init(spi0, 1000000); spi_set_format(spi0, 8, SPI_CPOL_0, SPI_CPHA_1, SPI_MSB_FIRST); spi_set_slave(spi0, true); while(1) { // Test SPI0-slave function if (spi_is_readable(spi0)) { uint8_t c; spi_read_blocking(spi0, 0, &c, 1); printf("%c ", c); } } } ```

daveythacher commented

2022-12-28 04:14:04 -06:00

(Migrated from github.com)

FYI, do not use more the 1Mbps with that test. It can be modified to support 10Mbps.

daveythacher commented

2023-01-11 11:05:02 -06:00

(Migrated from github.com)

@kilograham Any chance of this on 1.4.1?

daveythacher commented

2023-01-24 19:55:43 -06:00

(Migrated from github.com)

This is one approach to solving this. This is the best path without increasing the major version?

andygpz11 (Migrated from github.com) approved these changes

2023-02-06 07:16:00 -06:00

andygpz11 (Migrated from github.com) left a comment

Thanks - looks good to me.

kilograham commented

2023-02-06 09:14:44 -06:00

(Migrated from github.com)

re-opened as #1227

andygpz11 commented

2023-02-09 11:00:14 -06:00

(Migrated from github.com)

Hi @daveythacher I think your change looks plausible. But, I do have to ask:

If you saw problems provably caused by not taking the extra steps you have added here?
That those issues were then fixed by adding your extra steps?
Thanks.

Hi @daveythacher I think your change looks plausible. But, I do have to ask: 1. If you saw problems provably caused by **_not_** taking the extra steps you have added here? 2. That those issues were then fixed by adding your extra steps? Thanks.

daveythacher commented

2023-02-09 13:45:36 -06:00

(Migrated from github.com)

There are few possible issues which could happen depending on the SPI implementation. However I am assuming that this is not the case. This is not mentioned in the ARM documentation and I do not recall it being an issue in other SPI implementations. Switching from master to slave while enabled is almost always forbidden.

In a version 2.0 of the SDK, the SPI API may need more work. However for the time being this is likely fine, in my opinion. Addressing this with documentation should also work. Not all features are exposed by the SPI API but that is not a major issue. My personal preference is for a disable, configure and enable workflow. However this likely is not required and would actually be annoying if required. With this approach this can be left to the programmer to decide/handle.

One thing that my change does not do and this probably should be documented, is waiting for idle. If the SPI port needs to change roles, a pending transfer could be aborted. However this should be handled in application logic as the SDK should not be expected to handle DMA. A note for the programmer to see in the SDK, should handle this.

I based this on documentation from ARM SPI, which stated explicitly not to do this. This change allows the API to be used without register programming. In #713 I found a way to use the SDK to implement this with register programming. (Not documented in SDK to my knowledge.) There are few possible issues which could happen depending on the SPI implementation. However I am assuming that this is not the case. This is not mentioned in the ARM documentation and I do not recall it being an issue in other SPI implementations. Switching from master to slave while enabled is almost always forbidden. In a version 2.0 of the SDK, the SPI API may need more work. However for the time being this is likely fine, in my opinion. Addressing this with documentation should also work. Not all features are exposed by the SPI API but that is not a major issue. My personal preference is for a disable, configure and enable workflow. However this likely is not required and would actually be annoying if required. With this approach this can be left to the programmer to decide/handle. One thing that my change does not do and this probably should be documented, is waiting for idle. If the SPI port needs to change roles, a pending transfer could be aborted. However this should be handled in application logic as the SDK should not be expected to handle DMA. A note for the programmer to see in the SDK, should handle this.