ports/rp2: Prevent machine.SPI settings from persisting through a soft reset #16917

Gadgetoid · 2025-03-12T11:56:30Z

Replaced by, original info has moved there: #16922

This has gone off the rails somewhat into a discussion about whether RP2'S SPI (and other peripherals on RP2 and other devices) should be fully reset upon a "soft reset" (in MicroPython terms this is basically just bailing out of user code, doing some teardown and set up and returning back to user code/REPL again)

To state my case in brief: A "reset" should, you know, reset things and an "init" should... "init" things.

For my part, I represent a vendor of "educational" electronics targeted at beginners/hobbyists. We have many MicroPython boards out in the wild and most of these have some self evident physical "default" bus connector: QwSt/Quiic/Stemma for I2C or SP/CE for SPI. Unsurprising and consistent behaviour of these default instances is therefore in my best interests.

In Detail

SPI Settings Persisting Across Soft Resets

Running SPI(mosi=23) will change the behaviour of SPI(), which is roughly what the documentation confirms- however this change persists across soft resets. IE: SPI() will use pin 23 for MOSI until a new pin is specified, or until the board is hard reset. This happens because the current SPI pin configuration is saved into static RAM (not managed by MicroPython) and not re-initialised on soft reset.

It's not obvious (read: not sufficiently documented) that this is the case. Anyone idly tinkering on the REPL or writing a script could find themselves unable to talk to their device because they've inadvertently made a persistent pin change.

I would be interested in feedback from anyone who relies on the persistence of SPI settings between soft resets since I'm struggling to find many reasons for it (not glitching sensitive external peripherals on reset?) but that may just be a failure of imagination on my part.

As a side-effect of this, SPI claims a small amount of RAM for these persistent instances whether you use it or not. That may not be a big deal on RP2 but reserving RAM for a peripheral that a user might not even use is an odd choice, especially in the absence of any good argumentation for why someone would want SPI settings to be persistent.

SPI().init() Does Not Initialise Anything

SPI().init() (and other hardware init functions) have been somewhat twisted in meaning. Despite the claims of the documentation SPI().init() does not "init" the SPI.

It seems "init" functions across various hardware peripherals have been misappropriated to mean "make some change to a user-supplied bus." Examples include changing the baud rate on SPI or UART (perhaps supplied by a user into a driver instance) without resetting ("initialising") other values. Again the documentation doesn't sufficiently explain this practice.

As a tertiary consequence of this, SPI's "init" on RP2 specifically doesn't contain all the parameters that SPI() has available; chiefly you cannot configure pins with it. As the docs suggest, SPI(id, *args, *kwargs) should be equivalent to SPI(id).init(*args, *kwargs). It is not.

If the "spi_id" arg is not supplied and then the board default specified by PICO_DEFAULT_SPI will be used. If this is not set, it's defaulted to 0. Note: spi_id is still require when MICROPY_HW_SPI_NO_DEFAULT_PINS is set. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid · 2025-03-12T12:01:44Z

8000

While testing this I realised that configured SPI pins survive a soft reset. That's quite a pitfall for beginners doing on device tinkering. I will add a commit to fix that, might as well go the distance with breaking machine.SPI on RP2 if we're going to change it at all.

Eg: here's me changing to a custom mosi pin, hitting "Stop" in Thonny and then trying to get the default SPI0 instance:

>>> machine.SPI(0, mosi=23)
SPI(0, baudrate=1000000, polarity=0, phase=0, bits=8, sck=18, mosi=23, miso=16)

MPY: soft reboot

MicroPython v1.25.0-preview.366.g0a46b6ca38.dirty on 2025-03-12; Raspberry Pi Pico 2 W with RP2350
Type "help()" for more information.
>>> machine.SPI(0)
SPI(0, baudrate=1000000, polarity=0, phase=0, bits=8, sck=18, mosi=23, miso=16)

This bug also affects machine.I2C():

>>> machine.I2C()
I2C(0, freq=400000, scl=5, sda=4, timeout=50000)
>>> machine.I2C(scl=9)
I2C(0, freq=400000, scl=9, sda=4, timeout=50000)

MPY: soft reboot

MicroPython v1.25.0-preview.366.g0a46b6ca38.dirty on 2025-03-12; Raspberry Pi Pico 2 W with RP2350
Type "help()" for more information.
>>> machine.I2C()
I2C(0, freq=400000, scl=9, sda=4, timeout=50000)

This also means that since no arguments are being supplied, and since freq is already set neither of these instances would be configured even if the pins were set "correctly".

This is weird. Is anyone really using machine.SPI(0) to mean "return me SPI(0) with the last pins I configured?"

github-actions · 2025-03-12T12:04:18Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +48 +0.005% RPI_PICO_W[incl +8(bss)]
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

It's common with write-only SPI displays for MISO to be repurposed as a register select or data/command pin. While that was possible by setting up the pin after a call to `machine.SPI()` this change makes `machine.SPI(miso=None)` explicit. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid · 2025-03-12T14:18:53Z

Okay after way too much looking into this and feeling like I'm losing my mind

For SPI(n) the docs say:

With no additional parameters, the SPI object is created but not initialised (it has the settings from the last initialisation of the bus, if any). If extra arguments are given, the bus is initialised. See init for parameters of initialisation.

This is not strictly true. SPI is initialised if it was not already initialised since the last hard reset.

If extra parameters are given you will get a mix of default values, your custom values, and whatever state the SPI instance was last in.

Additionally SPI(n) accepts pin parameters that SPI(n).init() does not.

For SPI(n).init() the docs say:

Initialise the SPI bus

It does not under any circumstances initialise the bus, though it does change any parameters that are passed to it and has a sort of defacto shorthand where you can change any property without affecting the other... this is somewhat antithetical to what people would expect from an init function.

The docs also suggest init accepts sck, mosi, miso. It does not.

Right now there is a significant portion of repeated c 8000 ode between SPI(n) and SPI(n).init

In keeping with the lean toward shorthand methods machine.SPI on RP2 also accepts baudrate as a positional argument. The docs don't really suggest the arguments should be positional or otherwise, but on RP2 the baudrate argument is positional to allow for things like machine.SPI(0, 8_000_000) .

I think that:

Soft reset needs to - somehow - clear the SPI instances back to their default values in all cases. Possibly using MicroPython to create a machine_spi_obj with default or normal values, and hang it off a root pointer.
machine.SPI(n) - should return the SPI instance in whatever state the user last configured it
machine.SPI(n).init() - should initialise any unspecified value back to its default. If the user wishes to change the baudrate on the fly we really ought to have some more formal means for this.
machine.SPI(n, *args, **kwargs) - should function exactly like machine.SPI(n).init(*args, **kwargs)

peterhinch · 2025-03-12T14:48:01Z

Re 3. There is a need to be able to change the baudrate without affecting other properties. An example is touch displays where an SPI bus is shared between a display and a touch controller, where these have radically different maximum baudrates. My library uses

spi.init(baudrate=new_rate)  # spi is an SPI instance

Make `init()` init the SPI. Add pins arguments to `PI(n).init()`. Make `SPI(n, *args, *kwargs)` defer to (and match) `init(*args, *kwargs)`. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid · 2025-03-12T15:17:10Z

My library uses

spi.init(baudrate=new_rate)  # spi is an SPI instance

Thanks for chiming in, I knew there would be an edge case.

My latest commit explicitly and intentionally breaks this behaviour. I guess in cases like yours the user is supplying an SPI instance and you don't necessarily know what the other settings are?

It feels like init should be renamed to configure (or configure added) and the documentation updated accordingly. The way it works across SPI and UART would suggest it doesn't "init" anything at all. I suspect I2C is probably similar.

Some code sharing between init and configure behind the scenes would be nice, too, but that's somewhat secondary to getting the API to feel less bizarre - my primary concern being to make this all less weird for beginners, and give them an easy road back to default values without having to specify them manually.

FWIW I don't expect any of this to be merged unchallenged 😆

Edit:

It's clear this lies somewhere at the intersection between a code issue and a documentation issue and warrants further discussion about the shape of these functions so I should probably dial back my ambitions slightly. I've added another commit to revert init back to mostly it's previous behaviour, with the exceptions:

You can init(8_000_000) - baudrate is an - optionally - positional argument, a vestigial remainder of the need to be back-compatible with SPI(n, baudrate).
You can change pins with init() (which should always have been the case according to the docs, since SPI(n, ...) is supposed to be a superset of init() aiui.

Hopefully I've managed to retain some of my code size and clarity improvements. Chiefly not having two completely different code paths for changing peripheral settings when one should do.

Contrary to the documentation, `SPI(n).init()` is used widely for modifying an SPI instance passed in from user code to a Python driver. Drivers may depend upon the ability to change a single parameter while retaining other unknown parameters. Signed-off-by: Phil Howard <github@gadgetoid.com>

robert-hh · 2025-03-12T16:10:00Z

If you look at the various implementations in the ports of supporting devices like UART, SPI, ... you surely have noticed that it is consistently inconsistent. But there is a preference for a certain structure:
There are functions like _new(), _init_helper() and _init.

_new() implements the constructor. It allocated the device, creates the Python object and either sets all device parameters to the respective default values or flags them as "uninitialized". Then it calls _init_helper().
_init() just calls _init_helper().
_init_helper() processes the remaining arguments of the constructor or <object.init() call. In general it will only set or change a parameter which is mentions in the Python call's argument list. Not mentioned parameters are not changed, unless they are in the state "uninitialized". Then they can be set to a default value or an error can be raised. The option of being able to omit mandatory parameters excludes using the ARG_REQUIRED in the functions argument list. That's a pity. That would only be possible then with the constructor function.

Gadgetoid · 2025-03-12T16:58:28Z

That's pretty much the structure I've been moving toward in my PR, with the exception that _new() still doesn't properly initialise the hardware. I'm not sure we have any consensus on whether it should yet?

Oh and that I am being weird and passing MicroPython argument collections instead of regular C types around 😬

For a succinct example of where this goes awry consider the following:

from machine import SPI
test = machine.Pin(16, machine.Pin.OUT)
test.value(0)
spi = SPI(0)
test.value(1)        # Should not work because pin is now SPI
print(test.value())  # Shoul be 0

Edit: I have changed the above proof to add a test.value(0). Shows how insidious unpredictable hardware state across soft resets is... I had a bug in my proof 😆

test.value() will return 0 on the first run because SPI(0) has mux'd GPIO 16 into SPI mode.

All subsequent runs will return 1 because SPI(0) will no longer initialise anything.

Quoting you from the issue:

My approach would be that Soft Reset would reset the peripherals as well and the device is in the state it was after hard reset and setting up clocks and memory devices.

I would tend to agree.

With my current code that means adding machine_spi_init which would be called in main.c like:

micropython/ports/rp2/main.c

Lines 168 to 171 in 3823aeb

    
           machine_pin_init(); 
        
           rp2_pio_init(); 
        
           rp2_dma_init(); 
        
           machine_i2s_init0();

And it would just need to roll through the machine_spi_obj[] array and set initialised (which I have added versus using baudrate == 0) to false to make sure the next SPI() call triggers a full initialisation.... I hope 😆

robert-hh · 2025-03-12T17:16:19Z

With my current code that means adding machine_spi_init which would be called in main.c like

Init is one aspect, 8000 deinit the other. If you look at the code in main after the label soft_reset_exit: you see quite a few deinit calls. These are required for devices which use memory allocated from the Python heap, interrupts and DMA. That all has to be done before the call to gc_sweep_all(), which resets the Python heap. A long as SPI does not use DMA or interrupts there seems nothing that need to be deinit'ed.

Add a simple init handler that marks the SPI instances as uninitialised so they are reconfigured on soft reset. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid · 2025-03-12T17:43:21Z

I think it's conceptually an init since SPI isn't doing any allocation, but because it uses a static array of SPI instances that don't live on gc_heap and thus aren't reset to their default values it needs a little gentle reminder to do so.

The culprit is here:

micropython/ports/rp2/machine_spi.c

Lines 115 to 128 in 58f8229

    
           static machine_spi_obj_t machine_spi_obj[] = { 
        
               { 
        
                   {&machine_spi_type}, spi0, 0, 
        
                   DEFAULT_SPI_POLARITY, DEFAULT_SPI_PHASE, DEFAULT_SPI_BITS, DEFAULT_SPI_FIRSTBIT, 
        
                   MICROPY_HW_SPI0_SCK, MICROPY_HW_SPI0_MOSI, MICROPY_HW_SPI0_MISO, 
        
                   DEFAULT_SPI_BAUDRATE, false 
        
               }, 
        
               { 
        
                   {&machine_spi_type}, spi1, 1, 
        
                   DEFAULT_SPI_POLARITY, DEFAULT_SPI_PHASE, DEFAULT_SPI_BITS, DEFAULT_SPI_FIRSTBIT, 
        
                   MICROPY_HW_SPI1_SCK, MICROPY_HW_SPI1_MOSI, MICROPY_HW_SPI1_MISO, 
        
                   DEFAULT_SPI_BAUDRATE, false 
        
               }, 
        
           };

These variables shadow the SPI instance's current state and are used to make it possible to reconfigure one setting without also changing the others.

I think I should switch to the machine_i2s approach and hang the SPI instances off a root pointer. Very similar to what I was planning to do, but actually an established pattern (consistently consistent!) rather than some new nonsense.

Edit: For posterity I have left my individual commits in place as I worked toward this... solution... if this is at all worth considering then I will squash those into something more sensible.

Make consistent with machine_i2s by using a root pointer to manage SPI instance, and allocate them only when they are needed. Signed-off-by: Phil Howard <github@gadgetoid.com>

robert-hh · 2025-03-12T19:27:55Z

As far as I understand memory allocation, It is not needed to register the SPI objects with MP_STATE_PORT(). This is only needed if the allocated object is not directly returned to the calling Python code and must be protected from clearing though gc by registering it as root pointer. Not that it hurts, but it seems not to be needed.

Besides that: why did you move away from the static SPI objects? RAM shortage is not a big issue with the RPI Pico.

robert-hh · 2025-03-12T19:53:06Z

ports/rp2/machine_spi.c

@@ -172,7 +180,12 @@ mp_obj_t machine_spi_make_new(const mp_obj_type_t *type, size_t n_args, size_t n
        }
        self->mosi = mosi;
    }
-    if (args[ARG_miso].u_obj != mp_const_none) {
+
+    if (args[ARG_miso].u_obj == MP_ROM_INT(-1)) {


At this state of the commit series, this if clause is wrong. If the argument is not present, the content of the static SPI object must not be changed.

robert-hh · 2025-03-12T19:58:45Z

Looking at your PR your seem to change things which were not broken. The only required change would have been to skip the default miso pin initialization in case of miso=None. That's in the second commit.

Anything else like changing the behavior of the RP2 port's behavior on re-initialization should be deferred until there is a common policy about it. And then it affects not only SPI.

Gadgetoid · 2025-03-12T20:05:14Z

This is only needed if the allocated object is not directly returned to the calling Python code and must be protected from clearing though gc by registering it as root pointer.

AIUI if it’s allocated on MicroPython’s heap then you must store a pointer somewhere since unreachable allocs will be garbage collected eventually. This could be via a root pointed, tracked alloc, or if we expect the user to hang onto the SPI instance it would happen implicitly. Since the intent is a persistent singleton this is about the best of a bad bunch of ways to do it- in all fairness at least i2s has some real cleanup to do.

It could he allocated in the C heap but insofar as MicroPython is concerned that’s a code smell if not an outright recipe for disaster (a port can be configured with no C heap and all sorts of fun ensues.)

Besides that: why did you move away from the static SPI objects?

Because it becomes easier to reason about. The state persistence between soft resets, was a (debatably) unwanted side-effect of the static objects not being managed by MicroPython itself.

if they are allocated with “make_new” then it becomes clearer that the intent is a sort of singleton pattern rather than persistence.

RAM shortage is not a big issue with the RPI Pico.

it isn’t until it is, arguably any static should have some strong argumentation to support why it deserves to skirt runtime allocation- be that performance or simply because it’s used 100% of the time. SPI has no supporting argumentation it’s just - afaik - the coin toss the implementer made when writing it. (Or an explicit choice to support SPI config surviving soft reset in which case they were - arguably - wrong 🤣)

edit: I appreciate your feedback and critique here, the argumentation is at least as important (if not more) than the code itself because it puts the reasoning behind these choices on permanent record, helps establish those choices as patterns and helps me understand which bits should maybe be elaborated in commit messages/comments. Good for MicroPython on the whole. That said I will no doubt find myself in trouble since I’ve been guessing and not looking up the original authors rationale 😬

Gadgetoid · 2025-03-12T20:23:14Z

“not broken” is extremely debatable given just how many quirks and pitfalls the current code permits, including code that behaves differently after a soft reset and no path back to default values shy of unplugging a Pico.

Anything else like changing the behavior of the RP2 port's behavior on re-initialization should be deferred until there is a common policy about it. And then it affects not only SPI.

Where’s the common policy going to come from if it’s not advocated for in a pull request with code and rationale to back it up?

(yes granted this all got a bit off piste because SPI was staggeringly weird and it seemed remiss not to address the other issues… fix creep? Is that a thing?)

I don't want to comment at the moment on the soft reset behaviour of SPI. But I strongly suggest to split up this PR into one for the original intent (make MISO optional, make id optional) and one for the other changes. The first of those changes can be reviewed and merged much quicker than the latter.

robert-hh · 2025-03-13T08:23:44Z

Since the intent is a persistent singleton this is about the best of a bad bunch of ways to do it

If that's the intent why not keep the static objects.

if we expect the user to hang onto the SPI instance it would happen implicitly.

That is the usual Python behavior. Since with your implementation the SPI object is cleared on soft reset, the persistence would "only" allow to re-create the SPI object with the previous configuration. That's the way I2C onRP2 is implemented, because it lacks the init() method, but unusual among the other ports - and kind of confusing. What is the visible difference in code between spi=SPI() using the default values and spi=SPI(), using a different configuration set in a previous call to SPI()?

The state persistence between soft resets, was a (debatably) unwanted side-effect of the static objects not being managed by MicroPython itself.

My impression is, that this effect was wanted.

Gadgetoid · 2025-03-13T10:28:13Z

I don't want to comment at the moment on the soft reset behaviour of SPI.

A diplomatic choice.

But I strongly suggest to split up this PR into one for the original intent (make MISO optional, make id optional) and one for the other changes. The first of those changes can be reviewed and merged much quicker than the latter.

Agreed. It feels a bit like building on shaky foundations 😆 but at least something will get done.

Since this PR has gone to the wolves with discussion around the former I will (rename and) leave it as-is until we resolve the questions around SPI soft reset, and raise a new PR to fix the original issue(s).

Gadgetoid · 2025-03-13T10:45:03Z

Raised another PR for the original issues - #16922 - and updated the OG post to state my case more succinctly.

Gadgetoid mentioned this pull request Mar 12, 2025

When miso=None is set, the default miso pin changed state. #16912

Open

Gadgetoid force-pushed the patch-rp2-machine-spi-args branch from f41fcb0 to 50968d4 Compare March 12, 2025 12:07

rp2/machine_spi: Bring SPI behaviour closer to docs.

6a31a45

Make `init()` init the SPI. Add pins arguments to `PI(n).init()`. Make `SPI(n, *args, *kwargs)` defer to (and match) `init(*args, *kwargs)`. Signed-off-by: Phil Howard <github@gadgetoid.com>

rp2/machine_spi: Initialise SPI on soft reset.

58f8229

Add a simple init handler that marks the SPI instances as uninitialised so they are reconfigured on soft reset. Signed-off-by: Phil Howard <github@gadgetoid.com>

rp2/machine_spi: Switch to MP_STATE_PORT.

c09a9bd

Make consistent with machine_i2s by using a root pointer to manage SPI instance, and allocate them only when they are needed. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid force-pushed the patch-rp2-machine-spi-args branch from b2fb54f to c09a9bd Compare March 12, 2025 18:11

robert-hh reviewed Mar 12, 2025

View reviewed changes

dpgeorge added the port-rp2 label Mar 12, 2025

Gadgetoid changed the title ~~ports/rp2: Optional spi_id and optional disabling of MISO pin~~ ports/rp2: Prevent machine.SPI from persistent through a soft reset Mar 13, 2025

Gadgetoid changed the title ~~ports/rp2: Prevent machine.SPI from persistent through a soft reset~~ ports/rp2: Prevent machine.SPI defaults from persisting through a soft reset Mar 13, 2025

Gadgetoid changed the title ~~ports/rp2: Prevent machine.SPI defaults from persisting through a soft reset~~ ports/rp2: Prevent machine.SPI settings from persisting through a soft reset Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ports/rp2: Prevent machine.SPI settings from persisting through a soft reset #16917

ports/rp2: Prevent machine.SPI settings from persisting through a soft reset #16917

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ports/rp2: Prevent machine.SPI settings from persisting through a soft reset #16917

Are you sure you want to change the base?

ports/rp2: Prevent machine.SPI settings from persisting through a soft reset #16917

Conversation

Uh oh!

In Detail

SPI Settings Persisting Across Soft Resets

SPI().init() Does Not Initialise Anything

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!