Skip to content

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

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

Closed
kopchik opened this issue Feb 21, 2021 · 6 comments
Closed

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

kopchik opened this issue Feb 21, 2021 · 6 comments
Assignees
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@kopchik
Copy link

kopchik commented Feb 21, 2021

Hi!

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

@kopchik
Copy link
Author

kopchik commented Feb 21, 2021

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

lurch commented Feb 22, 2021

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

kopchik commented Feb 24, 2021

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

@aallan aallan added the bug Something isn't working label May 28, 2021
@leycec
Copy link

leycec commented Dec 8, 2021

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 aallan added the wontfix This will not be worked on label Dec 8, 2021
@aallan
Copy link
Contributor

aallan commented Dec 8, 2021

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.

@aallan aallan closed this as completed Dec 8, 2021
@leycec
Copy link

leycec commented Dec 9, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants