Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
classbproject opened this issue Mar 21, 2021 · 2 comments
Open

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

classbproject opened this issue Mar 21, 2021 · 2 comments

Comments

@classbproject
Copy link

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
Copy link
Contributor

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:

py-spidev/spidev_module.c

Lines 696 to 699 in a5d82b8

if (PyTuple_Check(obj)) {
Py_DECREF(seq);
seq = PySequence_List(obj);
}

and back again here:

py-spidev/spidev_module.c

Lines 737 to 741 in a5d82b8

if (PyTuple_Check(obj)) {
PyObject *old = seq;
seq = PySequence_Tuple(seq);
Py_DECREF(old);
}

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
Copy link

TheMatt2 commented Dec 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants