SPI bug?

[Eric24]

I'm seeing some very strange behavior on the SPI inteface on a LoPy.

The SPI class is instantiated as so:
from machine import SPI
spi = SPI(0, mode=SPI.MASTER, baudrate=250000, polarity=0, phase=0, bits=8, firstbit=SPI.MSB)

Using a very simple test loop:
while True:
cs(0)
spi.write(b'\xFF')
cs(1)

This works as expected:

In the screenshot:

• Yellow - SPI Clock
• Light Blue - CS line (P9 in this example, although here it's only used to trigger the o'scope)
• Dark Blue - MISO
• Pink - MOSI

Note that there is nothing connected to the SPI pins (using defaults P10/11/12), except a pull-up on MISO. Also, I've tested other values in the write function, including a bytes array, and what I see on the o'scope matches what I'm writing exactly, as expected.

But when I use this test loop:
while True:
cs(0)
spi.read(1, write=0xFF)
cs(1)

I see this:

Note that the LoPy is driving MOSI (with what appears to be 0x55). What's even more interesting is that this is the pattern I see regardless of the value supplied to the 'write' argument).

I couldn't find the source code for the SPI class (not sure if it's published), so I wasn't able to look at the read code. Could this be a bug or am I doing something wrong?

[Eric24]

Additional info...

I'm pretty well convinced that none of the SPI functions that take a 'write' argument pay any attention to that argument--no matter what, only 0x55 is sent during the operation. So I tried rewriting my SPI driver to use only the write_readinto function, passing in a write buffer full of 0xFF. This worked as expected (and made my driver work as well--unfortunately, it makes for some pretty gross code to use only write_readinto).

[throwaway]

Came across someone's post from Nov, 2018 and it looks like you're right.

See https://github.com/pycom/pycom-micropython-sigfox/blob/master/esp32/mods/machspi.c#L402 and https://github.com/pycom/pycom-micropython-sigfox/blob/master/esp32/mods/machspi.c#L190. The former actually considers the write argument but it's completely ignored in pybspi_transfer(). Instead the code uses 0x55555555 because there's a NULL TX buffer in the call that pyb_spi_read() makes.

STATIC mp_obj_t pyb_spi_read(mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
    static const mp_arg_t allowed_args[] = {
        { MP_QSTR_nbytes,    MP_ARG_REQUIRED | MP_ARG_OBJ, },
        { MP_QSTR_write,     MP_ARG_KW_ONLY  | MP_ARG_INT, {.u_int = 0x00} },
    };

    // parse args
    mach_spi_obj_t *self = pos_args[0];
    mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
    mp_arg_parse_all(n_args - 1, pos_args + 1, kw_args, MP_ARRAY_SIZE(args), allowed_args, args);

    // get the buffer to receive into
    vstr_t vstr;
    pyb_buf_get_for_recv(args[0].u_obj, &vstr);

    // just receive
    uint32_t write = args[1].u_int;
    pybspi_transfer(self, NULL, vstr.buf, vstr.len, &write);

    // return the received data
    return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr);
}

and:

STATIC void pybspi_transfer (mach_spi_obj_t *self, const char *txdata, char *rxdata, uint32_t len, uint32_t *txchar) {
    if (!self->baudrate) {
        nlr_raise(mp_obj_new_exception_msg(&mp_type_OSError, mpexception_os_request_not_possible));
    }
    // send and receive the data
    for (int i = 0; i < len; i += self->wlen) {
        uint32_t _rxdata = 0;
        uint32_t _txdata;
        if (txdata) {
            memcpy(&_txdata, &txdata[i], self->wlen);
        } else {
            _txdata = 0x55555555;
        }
        spi_data_t spidata = {.cmd = 0, .cmdLen = 0, .addr = NULL, .addrLen = 0,
                                .txData = &_txdata, .txDataLen = self->wlen,
                                .rxData = &_rxdata, .rxDataLen = self->wlen};

        spi_master_send_recv_data(self->spi_num, &spidata);
        if (rxdata) {
            memcpy(&rxdata[i], &_rxdata, self->wlen);
        }
    }
}

The upcoming v1.20 RC might have a different implementation that writes out whatever appears in the input buffer before updating it with whatever comes in -- although I'm not sure when "soft" SPI is used. See here:
https://github.com/pycom/pycom-micropython-sigfox/blob/release-candidate/drivers/bus/softspi.c

This might also be relevant:
https://github.com/pycom/pycom-micropython-sigfox/blob/release-candidate/extmod/machine_spi.c

[robert-hh]

@throwaway Thanks for pointing at the interesting places. It looks like not all cases of spi_transfer had been considered. Maybe it should be:

        if (txdata) {
            memcpy(&_txdata, &txdata[i], self->wlen);
        } else {
            if (txchar) {
                _txdata = *txchar;
            } else {
                _txdata = 0x55555555; // Just some dummy value
            }
        }

Edit: Tested the change; it seems to work.
Edit2: There is still a glitch: when setting bits=32, bit 31 of the word at write=xxxx must be 0, so the largest value is 0x7fffffff.
Edit3: OK. Using negative numbers you can set bit31. Conversion of a hex pattern could be done with struct.unpack, like:
spi.read(4, write=struct.unpack("<i", b'\x80\x80\x80\x80')[0])

[Paul Thornton]

Good diagnosis. Ill talk to the team and make sure they are aware of this and it get's looked into.

[PaulM]

This was the simple way I worked around it, until fixed in native API:

def spiread(spiObj, nbytes, write=0x00):
        wb = bytearray()
        wb.extend(bytearray([write]*nbytes))
        rb = bytearray(len(wb))
        spiObj.write_readinto(wb,rb)
        return rb

[throwaway]

@PaulM Neat!

I noticed that @robert-hh's patch accidently snuck into this PR: https://github.com/pycom/pycom-micropython-sigfox/pull/241/files

[robert-hh]

@throwaway Actually, to commits to make it complete. The commit text mentions the analysis done by you, folks.
Accidentally, since it should have been a separate PR, but git merged it all together.

[Paul Thornton]

Just to follow up on this.

@robert-hh has done some wonderfull work. His fix has been merged into the next RC candidate. Thanks again @robert-hh, @paulM and anyone else who's been involved.

Edit: Additional thanks to @throwaway for his work in debugging this,

[robert-hh]

@paul-thornton It should be mentioned that this was based on @throwaway's analysis of the place of failure. So kudos to him.

[noviniumJeef]

Is there a fix to this? Im running the latest 1.18.2.r2 and im having this exact issue only the write_readinto() work around doesnt seem to work.