micropython / micropython-esp32

Old port of MicroPython to the ESP32 -- new port is at https://github.com/micropython/micropython
MIT License
669 stars 216 forks source link

Hardware SPI imposes a fixed-length buffer #225

Closed mattytrentini closed 5 years ago

mattytrentini commented 6 years ago

It appears that there's a fixed-length buffer when using HW SPI (note: SW SPI works fine):

# Start with SW SPI
spi=SPI(baudrate=1000000, mosi=Pin(23, Pin.OUT), sck=Pin(18, Pin.OUT), miso=Pin(19, Pin.IN))
spi.write(b'\x00'*4092) # OK
spi.write(b'\x00'*4093) # OK

# Now for HW SPI
spi=SPI(1, baudrate=1000000, mosi=Pin(23, Pin.OUT), sck=Pin(18, Pin.OUT), miso=Pin(19, Pin.IN))
spi.write(b'\x00'*4092) # OK
spi.write(b'\x00'*4093) # Error!
E (262116879) spi_master: spi_device_queue_trans(620): txdata transfer > host maximum

The error is raised in the ESP-IDF in spi_master.c:636. The variable max_transfer_sz, which specifies the maximum length of the transmission, appears to be specified in spi_master.c:154 but seems dependent on DMA configuration.

I'm not sure how configurable the buffer lengths need to be (it may be helpful to specify different length buffers - shorter for low memory usage or longer for perhaps better throughput and lower power?) but, regardless, it seems that write should just work, dealing with stepping through multiple buffers internally as need be.

dpgeorge commented 6 years ago

it seems that write should just work, dealing with stepping through multiple buffers internally as need be

Yes I agree, HW SPI should just work for an arbitrary length buffer.

I don't know if the ESP-IDF would consider doing it on their side (ie splitting up the write if it's too big), but if they don't then we would need an IDF function that can query the maximum length so the buffer can be split correctly. Does such a function already exist?

MrSurly commented 6 years ago

Beginning work on this.

MrSurly commented 6 years ago

@mattytrentini Can you be my tester? I don't have any devices that need that sort of SPI xfer size.

mattytrentini commented 6 years ago

Sure @MrSurly, no worries, happy to help.

BTW note that you don't need any device to be connected for the error to occur - you can just send a >4092 byte buffer to an instance of a HW SPI. But I'll verify that data that was expected to be sent was actually sent...

And I've been meaning to investigate after @dpgeorge's comment. The buffer size is available in max_transfer_sz (for each of spihosts) though access to it is only via the spi_host_t structure. And the idea to address the issue inside the ESP-IDF has a lot of merit...

nickzoic commented 6 years ago

If it's something which makes sense to fix upstream (because it's relevant beyond MicroPython), and it's in the visible bits of ESP-IDF, then I think it makes sense to make it a PR there to at least get Espressif's feedback.

MrSurly commented 6 years ago

If it's something which makes sense to fix upstream (because it's relevant beyond MicroPython), and it's in the visible bits of ESP-IDF, then I think it makes sense to make it a PR there to at least get Espressif's feedback.

This is a good point. I think I'll raise an issue first; sometimes their attitude is "not a bug!" -- wouldn't want to waste the effort.

MrSurly commented 6 years ago

But I'll verify that data that was expected to be sent was actually sent...

Right, I can "dry-test" of course, but I want to make sure it works for your specific use-case.

And I've been meaning to investigate after @dpgeorge's comment. The buffer size is available in max_transfer_sz (for each of spihosts) though access to it is only via the spi_host_t structure.

Unfortunately: static spi_host_t *spihost[3];

And the idea to address the issue inside the ESP-IDF has a lot of merit...

It does.

MrSurly commented 6 years ago

@mattytrentini https://github.com/MrSurly/micropython-esp32/tree/issue_225

I also fixed up the actual number of bits are to be sent, based on the SPI "bits" (per word) parameter, combined with the actual number of bytes in the data. Testing this is as simple as just setting the bits parameter to something like 4 or 12 (with some zero-byte buffering at the end), and seeing if it still works.

MrSurly commented 6 years ago

Submitted an issue to ESP-IDF https://github.com/espressif/esp-idf/issues/1329

MrSurly commented 6 years ago

Okay, so Espressif's answer was extremely terse, but what it meant is that you're supposed to set the max transfer bytes when you create the SPI object. Since there's about 0% chance of changing the µPy API to include this, and an equal chance of code changes by Espressif, I think we need to do chunking under the hood.

Edit: Now that I look more closely, I had assumed that @negativekelvin worked for Espressif, but I think I'm mistaken. In the end, my conclusion is the same.

dpgeorge commented 6 years ago

what it meant is that you're supposed to set the max transfer bytes when you create the SPI object

Can't you set the max to "infinity", something like 0xffffffff?

MrSurly commented 6 years ago

Can't you set the max to "infinity", something like 0xffffffff?

It allocates a 12-byte structure[1] for every DMA descriptor it thinks it needs. Basically transfer_size_in_bytes / 4094 * 12. For 0xffffffff that would be about 12MiB. This leads to "what's a 'reasonable' size?" Hard to answer that one.

[1] Assuming it's doing 4-byte boundaries, and rounding the 9-byte structure up to 12.

dpgeorge commented 6 years ago

For 0xffffffff that would be about 12MiB.

In that case I agree with you that the chunking should be done transparently via uPy.

dpgeorge commented 5 years ago

This was fixed upstream in commit https://github.com/micropython/micropython/commit/9123c8d9460017b8c8dc4a38d9a26968eacaa4cb