drivers/soft_spi: fix soft spi transfer bug

[kex2017]

fix:fix drivers/soft_spi transfer one byte without read back bug
test:test read w25q64 id success

[kfessel]

I am no reviewer but to interested by the Tietel of your PR

The following might be early unnecessary optimization and i currently have no spi device to check but i would go with this:

out as a transfer register; shifting only by one (some archs (e.g.:AVR) a bad at shifting more than on step at once)

static inline uint8_t _transfer_one_byte(soft_spi_t bus, uint8_t out)
{
    int8_t bit = 0, i = 8;
    if (SOFT_SPI_MODE_1 == soft_spi_config[bus].soft_spi_mode ||
        SOFT_SPI_MODE_3 == soft_spi_config[bus].soft_spi_mode) {
        /* CPHA = 1*/
        gpio_toggle(soft_spi_config[bus].clk_pin);
    }

    do{
        bit = (out & (1 << 7)) >> 7;
        gpio_write(soft_spi_config[bus].mosi_pin, bit);

        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        gpio_toggle(soft_spi_config[bus].clk_pin);

        out <<= 1; /*shift tranfer register*/

        bit = gpio_read(soft_spi_config[bus].miso_pin);
        out = bit ? out | 0x01 : out & 0xfe; /*set or delete bit 0*/

        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        --i;
        if (i > 0) gpio_toggle(soft_spi_config[bus].clk_pin);
    }while(i > 0)

    if (SOFT_SPI_MODE_0 == soft_spi_config[bus].soft_spi_mode ||
        SOFT_SPI_MODE_2 == soft_spi_config[bus].soft_spi_mode) {
        /* CPHA = 0 */
        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        gpio_toggle(soft_spi_config[bus].clk_pin);
    }

    return out;
}

For the read function i would just transfer 0x00 as done for hardware SPI.

[kex2017]

I am no reviewer but to interested by the Tietel of your PR

The following might be early unnecessary optimization and i currently have no spi device to check but i would go with this:

out as a transfer register; shifting only by one (some archs (e.g.:AVR) a bad at shifting more than on step at once)

static inline uint8_t _transfer_one_byte(soft_spi_t bus, uint8_t out)
{
    int8_t bit = 0, i = 8;
    if (SOFT_SPI_MODE_1 == soft_spi_config[bus].soft_spi_mode ||
        SOFT_SPI_MODE_3 == soft_spi_config[bus].soft_spi_mode) {
        /* CPHA = 1*/
        gpio_toggle(soft_spi_config[bus].clk_pin);
    }

    do{
        bit = (out & (1 << 7)) >> 7;
        gpio_write(soft_spi_config[bus].mosi_pin, bit);

        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        gpio_toggle(soft_spi_config[bus].clk_pin);

        out <<= 1; /*shift tranfer register*/

        bit = gpio_read(soft_spi_config[bus].miso_pin);
        out = bit ? out | 0x01 : out & 0xfe; /*set or delete bit 0*/

        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        --i;
        if (i > 0) gpio_toggle(soft_spi_config[bus].clk_pin);
    }while(i > 0)

    if (SOFT_SPI_MODE_0 == soft_spi_config[bus].soft_spi_mode ||
        SOFT_SPI_MODE_2 == soft_spi_config[bus].soft_spi_mode) {
        /* CPHA = 0 */
        xtimer_nanosleep(soft_spi_config[bus].soft_spi_clk);
        gpio_toggle(soft_spi_config[bus].clk_pin);
    }

    return out;
}

For the read function i would just transfer 0x00 as done for hardware SPI.

test on w25q64 ok

[kfessel]

@kex2017: Thanks for testing

Even though I am not a reviewer, i would suggest:

• do not PR from your master branch, create a new one prior PR git checkout -b <my new branch> and git push -u <your_repo> <my_new_branch>
• keep the Headlines that you got when you started your PR (try to fit your information within them)
• do not merge master into your PR instead someone may ask you to git rebase on <RIOT/origin>/master
• the static test already have some issues with spelling and code formating (sorry for that; these tests don't like my writing style)
  you might want to git commit --fixup
• If you want to restart the PR please put this one into references

[kex2017]

Thank you for your advice

[kfessel]

@basilfx and @maribu might have interest at this PR that basicaly adds read support to soft_spi

[maribu]

Thanks for this fix. I wonder how this ever got merged in this state.

I have some comments regarding code style.

[maribu]

Code looks good to me. Let's see if the CI has some comments. Feel free to squash.

It might be that now some boards fail to build test applications or similar due to the increased ROM size. If that happens, we need to disable the final linking stage in the affected application by adding the affected board to the applications Makefile.ci - but maybe we're like and everything still builds fine.

[kex2017]

adding the affected board to the applications Makefile.ci

Maybe add the affected board to the applications Makefile.ci is a good way

[maribu]

adding the affected board to the applications Makefile.ci

Maybe add the affected board to the applications Makefile.ci is a good way

Not needed, all builds succeeded :-)

[maribu]

Tested using #16196. Please squash!

[maribu]

@kex2017 Could you combine all commits into the first using git rebase -i master? Then change pick to fixup for all but the first commit. Here is a random search hit for a guide on squashing for more details.

[kfessel]

@kex2017 maybe its not so easy for you since this is your master branch you need to rebase it on upstream/master

git rebase -i origin/master if origin is the upstream RIOT remote

if you do not yet have an upstream RIOT remote you might need to create one

git remote add up https://github.com/kex2017/RIOT.git
git fetch up

and rebase on its master

git rebase -i up/master

and then mark your commits as @maribu described

[maribu]

ACK. Thanks for the fix. A very nice first contribution, btw!