memorymap: Convert to use memoryview internally.

[jepler]

Importantly, a memoryview can be .cast(), and then accesses to it for 4 byte values don't require allocations or to/from_bytes. (as long as the register values actually fit in CircuitPython "small integers")

This does remove the existing protections enforcing aligned accesses to IO blocks on raspberrypi. However, the behavior in these cases is actually well defined: an 8 or 16 bit write access is replicated across all 32 bits of the register. See RP2040 datasheet 2.1.4. Narrow IO Register Writes and RP2350 datasheet 2.1.5. Narrow IO register writes.

As discussed on Discord, this speeds up the keyboard scanner of my Unicomp Mini M firmware by nearly 10x compared to using DigitalInOut, mostly because 12 pins can be read by a single operation, allowing it to scan the full keyboard in under 1ms when overclocked. (https://adafruit-playground.com/u/jepler/pages/unicomp-mini-m-with-circuitpython currently details the original lower performance keyboard, which took about 6ms to scan the full keyboard when overclocked)

Currently tested only on rp2040.

[jepler]

Here's the code I use in my program to read & update GPIO pins as a group:

SIO_BASE = const(0xd0000000)
SIO_LEN = const(0x1000)
GPIO_IN = const(4//4)
GPIO_OUT_XOR = const(0x1c//4)

sio = memorymap.AddressRange(start=SIO_BASE, length=SIO_LEN).cast('L')

def gpio_in():
    """Fetch the status of all GPIO inputs"""
    return sio[GPIO_IN]

def gpio_out_xor(x):
    """Invert the given pins simultaneously"""
    sio[GPIO_OUT_XOR] = x

[jepler]

Space Savings from this PR, raspberry_pi_pico:

• 840b flash
• 256b RAM
  The RAM savings could be had in main just by making the array of regions be "const", so that it resides in flash and is not copied to RAM.

[tannewt]

Looks good overall. One question about the future API. Could we make cast() check alignment?

[tannewt]

I don't feel like we'll want this long term. I'm happy to have this be in 10.x alongside a new api. Maybe we just want an access name for this? So you do memorymap.access(start, length) instead of memorymap.AddressRange(start, length). (AddressRange looks likes a class still. Thoughts?

[jepler]

Thanks for the review.

Looks good overall. One question about the future API. Could we make cast() check alignment?

As an alternative, what about an optional cast= or type= argument to the constructor (defaulting to 'B'), so that the desired type is returned immediately and alignment is checked then. Code can still get wrong alignment via a cast call but the obvious way to code it would avoid that.

We could change what the core memoryview.cast method does, but that would apply to any kind of object and might break existing code. There's not a way to change how cast works just for memoryviews that trace back to coming from this API, and the underlying memory layout (carefully shared among memoryview/bytes/bytearray/array.array) has no space I can see to store a "requires alignment" bit.

I don't feel like we'll want this long term. I'm happy to have this be in 10.x alongside a new api. Maybe we just want an access name for this?

I could add the name access and mark AddressRange as a depecated alias if you like.

[tannewt]

Thanks for the review.

Looks good overall. One question about the future API. Could we make cast() check alignment?

As an alternative, what about an optional cast= or type= argument to the constructor (defaulting to 'B'), so that the desired type is returned immediately and alignment is checked then. Code can still get wrong alignment via a cast call but the obvious way to code it would avoid that.

Yup, that works for me. I think I'd prefer type but the comment can mention cast.

I don't feel like we'll want this long term. I'm happy to have this be in 10.x alongside a new api. Maybe we just want an access name for this?

I could add the name access and mark AddressRange as a depecated alias if you like.

Sounds good! Thanks for doing this!

[jepler]

In my testing, I've found some weird behavior I need to trace down before this is merged.

CircuitPython has an implementation of memcpy() which uses 4 or 2 byte operations when the source & destination are aligned, and there are at least that many bytes to copy. So, I expected that this would store 1 item in the PIO0 SM0 TX FIFO both before and after my change:

# Old style: Store bytes/bytearray via slice
pio0[PIO_TXF0_OFFSET:PIO_TXF0_OFFSET + 4] = b'1111'

# Print amount in all PIO0 SMx fifos
print(bytes(pio0[PIO_FLEVEL_OFFSET:PIO_FLEVEL_OFFSET+4]))

what I don't understand is: With my code, the store of 4 bytes starting at PIO_TXF0_OFFSET it actually increments the FIFO depth of all 4 PIO0 SMx state machines somehow, as though it's actually storing 16 bytes.

Another potential incompatibility, even if this weird bug wasn't present: The operate-by-4-bytes code in memcpy() requires both source and destination to be aligned. I have assumed that b'1111' would be aligned, but some existing code might use a non-aligned slice of a bytes/bytearray/etc as the other operand. The existing code would give 2/4-bytes based operations in this case, while the new code would not. (e.g., range[0:4] = b'01234'[1:5])

This is making me wonder whether I shouldn't re-think the whole thing: keep the existing AddressRange class, but just make it possible to memoryview(range) or range.cast(). It'd be nice to save a few hundred bytes, but not while it has unexplained bugs or possible regressions.

[tannewt]

what I don't understand is: With my code, the store of 4 bytes starting at PIO_TXF0_OFFSET it actually increments the FIFO depth of all 4 PIO0 SMx state machines somehow, as though it's actually storing 16 bytes.

I'd suggest double checking with C code to do the same thing. Maybe you are memory mirroring a byte to the other three in a word without knowing it.

This is making me wonder whether I shouldn't re-think the whole thing: keep the existing AddressRange class, but just make it possible to memoryview(range) or range.cast(). It'd be nice to save a few hundred bytes, but not while it has unexplained bugs or possible regressions.

I wouldn't worry a ton about it. There aren't many uses of it that I know of. If it works for you, then it is probably ok.

[jepler]

I've got a 2nd take on this at https://github.com/adafruit/circuitpython/compare/main...jepler:circuitpython:memorymap-memoryview-v2?expand=1 -- which I still need to test before PR'ing.

In v2 I still move everything but the range list into shared-module, and add the necessary internal function so that an AddressRange object can be passed to memoryview(); if the region requires alignment aligned, the result of casting it is 'L' type, otherwise 'B' type.

I think there were some missing length checks, including overflow checks; so I have revamped that part too and need to test it properly.