search

doceme / py-spidev

MIT License
461 stars 203 forks source link

Proposal: deprecate xfer2 #99

Open Gadgetoid opened 4 years ago

Gadgetoid commented 4 years ago

This library has suffered somewhat from function creep caused by useful features being added in conjunction with a fear of breaking the existing API.

I think it's worth reviewing what functionality an SPI library should constitute- taking into account https://github.com/doceme/py-spidev/pull/83 and the possibility of reading/writing bytearrays. But before we dive into what could be a time consuming nightmare it migh tbe worth reviewing the current functionality and attempting to whittle the codebase down to a maintainable core.

Right now as near as I can tell from code-review xfer and xfer2 are functionality identical unless the library is compiled with -DSPIDEV_SINGLE. I'm unaware of how this library is packaged for RPi (I have asked who I believe to be the relevant individual) but this flag is not referenced or set anywhere in the packaging visible in this repository. The library is shipped on PyPi without this flag set. The lack of any functional difference between xfer and xfer2 was noticed in #35 and #25 and has remained unchanged since.

I don't believe there's any reason to have a first-class method that handles re-asserting chip-select between "blocks" (in this case, we mean bytes).

I propose we quietly roll xfer and xfer2 into a single function and drop the -DSPIDEV_SINGLE code path altogether. Along with dropping some of the Python version checks this constitutes a significant reduction in code that needs maintained and testing in future.

Right now there are also inexplicable minor differences (allowing/disallowing threads) between xfer and xfer2 that should be ironed out, so this would be a good opportunity to reconcile those.

Gadgetoid commented 4 years ago

Have confirmation that -DSPIDEV_SINGLE is never set- at least in the Raspberry Pi distribution.

Are there any other binary distributions that might set this flag and suffer from that code being removed?

I supose at the very least if the bugfixes are rolled out a release before this code is killed, then it will always be possible for anyone who needs this functionality to rely on a previous release (everyone pins their dependencies right, right!?).

Elektrozeugs commented 3 years ago

Hi, I tried to use xfer because I have a chip that needs a CE spike between any 8bit transaction. With my Logic analyzer I can see that this is not working with the function. Is there any easy way to get the "old" xfer function as described on the main page? (PS: sending xfer byte-by-byte is no option for me, because I need a fast transaction without delay) Best wishes

semininja commented 3 years ago

In your application, is the byte-rate for byte-by-byte so much slower than with xfer()?

On Sun, Jan 17, 2021, 19:42 Elektrozeugs notifications@github.com wrote:

Hi, I tried to use xfer because I have a chip that needs a CE spike between any 8bit transaction. With my Logic analyzer I can see that this is not working with the function. Is there any easy way to get the "old" xfer function as described on the main page? (PS: sending xfer byte-by-byte is no option for me, because I need a fast transaction without delay) Best wishes

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/doceme/py-spidev/issues/99#issuecomment-761912656, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSUTL3EEXYDQP47PIENZFLS2N7YZANCNFSM4NOACAGQ .

Elektrozeugs commented 3 years ago

I am sending RGB data to a display, so I need a fast transmission. Here is my test code

import spidev import time spi = spidev.SpiDev() spi.open(0, 0)

spi.xfer([0xFF, 0xFF, 0xFF],1000000, 0, 8) spi.xfer([0xFF],1000000, 0, 8) spi.xfer([0xFF],1000000, 0, 8) spi.xfer([0xFF],1000000, 0, 8)

spi.close()

SPItest

As you can see the first trasmission is a lot faster. You can also see a bug that the CE is dropped twice which is mentioned here: https://www.raspberrypi.org/forums/viewtopic.php?t=73323 but sadly it wasnt solved

AI0867 commented 4 months ago

I realize this issue has been silent for a while, but to me, your usecase suggests the addition of a new xfer4, which takes a list of lists of values, running each list in a separate transaction.

Your test behaviour (which I know isn't the behaviour you want) could then be reproduced, (but faster, and without the spurious CE0 transitions) with:

spi.xfer4([
    [0xFF, 0xFF, 0xFF],
    [0xFF],
    [0xFF],
], 1000000, 0, 8)

As for deprecating xfer or xfer2, given that xfer2 is the function that actually does what its documentation says it does, I would suggest keeping that one, so that people who use xfer, and might expect the documented behaviour, get a heads up and get to choose which semantics they want.