doceme / py-spidev

MIT License
461 stars 203 forks source link

RX buffer overwrites the TX buffer in 'xfer2' #119

Open classbproject opened 3 years ago

classbproject commented 3 years ago

Hi, I'm setting up a comms link between an STM32F407 MCU and the Pi3B+. The code shown below works but when the data is printed out, it looks like the RX buffer overwrites the TX one. How do I stop that? The console output is shown below the code.

import spidev
from time import sleep
from tabulate import tabulate  # pip3 install tabulate

ARRAY_SIZE = 56

spi = spidev.SpiDev()
spi.open(0, 0)
spi.mode = 0
spi.max_speed_hz = 10000000
spi.bits_per_word = 8

tx1 = [0xCA, 0xFE, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0xff,
       0xff, 0x00, 0x00, 0xff, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00]

# Do an SPI transaction
# rx1 is the receive buffer
rx1 = spi.xfer2(tx1)

# Print the RX and TX arrays
headers = ['array #', 'TX', 'RX']
idx = range(0, ARRAY_SIZE, 1)
table = zip(idx, tx1, rx1)

print(tabulate(table, headers=headers))

# close the SPI port
spi.close()

The console output looks like this,

  array #    TX    RX
---------  ----  ----
        0     0     0
        1     0     0
        2     0     0
        3     0     0
        4     0     0
        5     0     0
        6     0     0
        7     0     0
        8     0     0
        9     0     0
       10     0     0
       11     0     0
       12     0     0
       13     0     0
       14     0     0
       15     0     0
       16     0     0
       17     0     0
       18     0     0
       19     0     0
       20     0     0
       21     0     0
       22     0     0
       23     0     0
       24     0     0
       25     0     0
       26     0     0
       27     0     0
       28     0     0
       29   254   254
       30   255   255
       31   134   134
       32   254   254
       33   204   204
       34   254   254
       35   253   253
       36     0     0
       37     0     0
       38     0     0
       39     0     0
       40     0     0
       41     0     0
       42     0     0
       43     0     0
       44     0     0
       45     0     0
       46     0     0
       47    54    54
       48    84    84
       49    52    52
       50   108   108
       51     0     0
       52     0     0
       53     0     0
       54     0     0
       55     0     0
Gadgetoid commented 3 years ago

For better or worse, this is intentional- potentially since in-place modification of the TX/RX buffer isn't entirely uncommon in SPI communications. However I didn't write this code originally so I can only rationalize what's there in hindsight.

If you supply your input as a list it will be modified in-place. However if you supply your input as an tuple (you can trivially convert a list to a tuple by just calling rx1 = spi.xfer2(tuple(tx1))) then the tuple will remain unchanged (since they are immutable) and a new list will be returned.

You can see the relevant lines for conversion from a tuple to a new list here:

https://github.com/doceme/py-spidev/blob/a5d82b88bd1c95c9f9ec46e9f7afe08c3323e625/spidev_module.c#L696-L699

and back again here:

https://github.com/doceme/py-spidev/blob/a5d82b88bd1c95c9f9ec46e9f7afe08c3323e625/spidev_module.c#L737-L741

Outside the context of a microcontroller this behavior is weird, surprising, very unpythonic, and somewhat conservative given I suspect most systems running this code could spare some memory for separate TX/RX buffers in all cases.... but it's a widely used library and I'm not convinced it's worth fixing when you can toss a tuple() in there.

Or, in your case, just use:

tx1 = (0xCA, 0xFE, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0xff,
       0xff, 0x00, 0x00, 0xff, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00,
       0x00)
TheMatt2 commented 1 year ago

This is a duplicate of #61

Though, having spent a good amount of time debugging due to this behavior, I disagree with the assertion this should be considered a feature and not a bug.

While the input list to xfer() and xfer2() is modified in place, the input to xfer3() is not, making this behavior inconsistent across spidev. None of these functions have anything in their documentation to indicate they modify their input, which seems to be a recipe for a code bug that hides quite well in plain sight.

If you really wanted to copy data to and from SPI without allocating the additional memory, you could use a writebytes() call followed by readbytes() to accomplish the same thing.

While converting the input to a tuple gets the desired behavior, I think this is not a particularly elegant solution in a duck-typed language, where precise typing should not cause a behavioral change. Having read the code, I'm still not sure how the code ends up correctly handling a bytes object.

I think this behavior needs to be fixed, or at least enabled by an inplace flag or similar. I do not think this undocumented, inconsistent, un-pythonic behavior should be kept as a feature.

Edit: Just wanted to say this library has worked fine overall. Don't want to come off as too negative.