pio/pio_spi.py: methods of PIOSPI are missing `self`

[kopchik]

Hi!

It seems methods here are missing self:
https://github.com/raspberrypi/pico-micropython-examples/blob/master/pio/pio_spi.py

[kopchik]

I think the pio_spi.py has a bug: the PIO code expects SS pin which it toggles (line 11, the .side(0x2) part), but this pin is not passed here: https://github.com/raspberrypi/pico-micropython-examples/blob/master/pio/pio_spi.py#L30 . So, I think, the MOSI pin acts as SS pin.

[lurch]

It seems methods here are missing self:

Yup, seems like you're right. AFAICT the method-declarations should be:

• def write_blocking(self, wdata):
• def read_blocking(self, n):
• def write_read_blocking(self, wdata):

I think the pio_spi.py has a bug: the PIO code expects SS pin which it toggle

I'm not very familiar with the low-level details of SPI or PIO, so I suspect this is one for @Wren6991 - but I think that as sideset_base=Pin(pin_sck) and there are 2 items in the sideset_init=, then setting .side(0x2) would actually set Pin(pin_sck+1) high.

I also see that https://github.com/raspberrypi/pico-micropython-examples/blob/master/pio/pio_spi.py#L30 has sideset_base=Pin(pin_sck), out_base=Pin(pin_mosi), in_base=Pin(pin_sck) whereas I suspect that in_base ought to be in_base=Pin(pin_miso)? It's possible that this is early-version micropython-example code which slipped under the radar in our frenzy to get everything released 😆

There's a more complete example of SPI-implemented-with-PIO at https://github.com/raspberrypi/pico-examples/blob/master/pio/spi/spi.pio

[kopchik]

There's a more complete example of SPI-implemented-with-PIO at https://github.com/raspberrypi/pico-examples/blob/master/pio/spi/spi.pio

Great link, this is where I took "inspiration" for my own version of SPI that just does sending, without MISO or CS pins (because I didn't need them for my application). This saved me two pins :).

[leycec]

Necro-bump. We just hit this ourselves! In theory, the self portion of this issue should be a trivial fix. Likewise, in_base=Pin(pin_miso) is almost certainly also needed but requires actual hands-on QA. Kinda seems like someone just theory-crafted this entire example in their heads without testing anything. It is what it is. 😞

To compound matters, PIOSPI fails to comply with the standard APIs defined by the stock hardware-based machine.SPI and software-based machine.SoftSPI classes. That unnecessarily complicates comparison and especially profiling between the three. Here's our go at entirely refactoring PIOSPI into a drop-in machine.{Soft,}SPI replacement:

from rp2 import StateMachine

class SPI:
    """
    Raspberry Pi Pico-specific drop-in replacement for Micropython's stock
    :class:`machine.SPI` class, implemented as a Programmable Input/Output
    (PIO)-optimized class.
    
    Attributes
    --------------
    _sm : StateMachine
        Finite state machine (FSM) underlying the PIO performed by this class.
    """
    
    #FIXME: Generalize to support enabling the "cpha" and "cpol" parameters.
    def __init__(
        self,
        pio_statemachine_id,
        mosi,
        miso,
        sck,
        baudrate,
        cpha=False,
        cpol=False,
    ):
        """
        Create this PIO-optimized SPI interface.

        Parameters
        --------------
        pio_statemachine_id : int
            0-based index of the PIO-based finite state machine (FSM) to run
            this SPI implementation on. Since the Pico supports 8 PIO FSMs,
            this *must* be an integer in ``[0, 7]``.
        """
        
        # If this SPI baudrate exceeds the Pico's system clock frequency,
        # raise an exception.
        if baudrate > 125000000:
            raise ValueError(
                f'SPI baudrate {baudrate} > system clock frequency {BAUDRATE_MAX}.')
        
        # If the caller attempted to enable either CPHA or CPOL, raise an
        # exception. This run_spi_cpol_0_cpha_0() assembly deferred to below
        # currently assumes this to be the case.
        if cpha:
            raise ValueError('Enabling CPHA currently unsupported.')
        if cpol:
            raise ValueError('Enabling CPOL currently unsupported.')
        
        # Create, classify, and enable a new FSM running this PIO assembly.
        self._sm = StateMachine(
            # These first two parameters *MUST* be passed positionally.
            pio_statemachine_id,
            spi_cpha0,
            # All remaining parameters *MAY* be passed by keyword.
            #FIXME: Why the "4" multiplier magic number here? *sigh*
            freq=4 * baudrate,
            sideset_base=sck,
            out_base=mosi,
            in_base=miso,
        )
        self._sm.active(1)


    def read(self, buffer_out):
        for byte_index in range(len(buffer_out)):
            buffer_out[byte_index] = self._sm.get() & 0xff


    def write(self, buffer_in):        
        for byte_in in buffer_in:
            self._sm.put(byte_in << 24)
            
            # Drain the RX FIFO to avoid catastrophic memory leaks.
            self._sm.get() & 0xff


    def write_readinto(self, buffer_in, buffer_out):        
        for byte_index in range(len(buffer_out)):
            self._sm.put(buffer_in[byte_index] << 24)
            buffer_out[byte_index] = self._sm.get() & 0xff

Great, right? Nope. This is profiling as approximately four times slower than machine.SPI for us. Admittedly, that's without @viper-izing any of the pure-Python methods – which is still unlikely to overcome the four-fold slowdown. The only potential advantage of a PIO-based SPI approach that we can see is in refactoring the entire thing from its current synchronous blocking behaviour into an asynchronous IRQ callback structure – which is highly non-trivial to the extreme. Why? Because the RX and TX FIFOs on the PIO FSM need to be drained in time for the next SPI cycle, so your IRQ callbacks need to copy between those FIFOs and thread-safe queues in MicroPython space. 😮‍💨

We suspect this is all probably intended and unexpected, but thought it worth mentioning for everyone watching back home.

tl;dr

Just use machine.SPI. Everything else is too busted and/or slow to meaningfully bother with.

[aallan]

The RP2040 MicroPython port has been merged upstream to the main MicroPython repo, see micropython/micropython#6791. Going forward you should report problems with the port there, https://github.com/micropython/micropython/. Technical support for MicroPython problems can be found on the MicroPython forums, https://forum.micropython.org/viewforum.php.

[leycec]

Oh. You're quite right. I'll copy-pasta myself over there, then. Thanks for the heads up, Alasdair!

Interestingly, MicroPython's RP2040 examples/ subdirectory entirely omits this example and all reference to SPI. There's a reason for that. Welp, alrighty then. machine.SPI it is.