SPI bug?



  • 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:
    0_1495470811126_spi-write FF.png

    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:
    0_1495471080935_spi-read FF.png

    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?



  • 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.



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



  • 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,



  • @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.



  • @PaulM Neat!

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



  • 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
    


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



  • @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])



  • 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



  • 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).


Log in to reply
 

Pycom on Twitter