Allow (unpadded) SPI transfers < 8bits

[MrSurly]

From #5225, as described by @mirko

SPI allows word sizes of several bits, not necessarily rounded up to a multiple of 8.
While I don't now about every platform / hardware, at least soft-SPI (GPIO bitbanged SPI) and the ESP IDF driven ESP32 SPI hardware support transferring single bits via SPI.
Looking at the micropython code though, the whole SPI abstraction layer - HW backed or SW - assumes lengths of multiples of bytes, meaning, I can't just adjust the parts related to esp32 / gpio.

To provide a use case - and I indeed see this being an edge case, but keep in mind that it still conforms with the SPI spec: (ab)using SPI to speak SWD.

Suggestion / Feature request: Adjust the SPI code so that we can transmit single bits instead of multiples of bytes.

Happy to do it myself / help / assist / for discussion. However at first glance, the changeset apparently would be larger than anticipated and I'd be glad for opinions / input / help.

[MrSurly]

I have not yet changed the SPI object method signatures, pending a comment from @dpgeorge here: #5225 (comment)

[MrSurly]

TODO:

• Ensure CI is working
• Update SPI method signatures to accept bits, and default to initializer bits if not provided
• Update the ESP32 HW SPI code to work the same as the soft SPI code
• Thoroughly test with logic analyzer on ESP32 platform
• Update documentation

[tve]

What is the performance impact here? E.g. with adding a division into spi_transfer on low-end uC's without HW div? Also, is there no doc update warranted?

[MrSurly]

What is the performance impact here?

Don't know. Is there current performance metrics to compare against?

Also, is there no doc update warranted?

Definitely warranted. Added to the TODO. Not all platforms can support per-transaction bit widths in hardware, and for those platforms that do, there's work to be done to implement it, or at least document that it isn't supported on those platforms. @mirko, can you help with documentation?

[mcauser]

I have some displays that use 9-bit SPI, with that extra bit being the data/command bit.
Would I be able to eventually use this for said displays?

[MrSurly]

@mcauser The idea is to allow transfer sizes granular to the bit level. Right now, we're going to support softspi and ESP32. Other HW platforms may have this in the future.

Short answer: yes.

[mirko]

@MrSurly happy to help with docs, got sick for the moment though. Just stating that I follow and highly appreciate that PR. Will give it a test (and think about the docs) as soon as recovered.

[MrSurly]

@MrSurly happy to help with docs, got sick for the moment though. Just stating that I follow and highly appreciate that PR. Will give it a test (and think about the docs) as soon as recovered.

No worries. Get well. I think I'm done with soft SPI, and working on ESP32 at the moment.

[MrSurly]

I removed the NRF definitions of read, read_into, write, and write_readinto. I don't have HW to test this -- could anybody else here try it out?

[daniel-thompson]

I removed the NRF definitions of read, read_into, write, and
write_readinto. I don't have HW to test this -- could anybody
else here try it out?

I might be able to but, to be honest, software SPI on nrf might not be that important. It has very flexible routing of SPI blocks to pins so software fallback should be pretty rare for this chip (hence enabling it by default in mpportconfig).

[MrSurly]

@daniel-thompson
Hi Daniel,

The softSPI code in extmod will call whatever init or transfer function is defined. For NRF (and ESP32 hw SPI), when you call read (et al), it's calling the softspi read function, which then just executes transfer, which can be either the softSPI bitbang implementation, or the port-specific hardware implementation.

In NRF, it simply replaced all of the softSPI xfer functions with it's own, which was unnecessary since they were identical to the softSPI versions. "removing" them doesn't functionally change anything in NRF, but it does mean that the method signatures in extmod/machine_spi.h won't have to be removed.

The method signatures in extmod/machine_spi.h changed because we added the new optional bits parameter. This unfortunately meant that this changes all the method signatures for all the other port SPI implementations (except NRF, which had it's own). Functionally, the other ports do accept the bits parameter on xfer functions, but they ignore it.

Essentially, this change removed unnecessary code while keeping the exact same functionality (I hope), and simplifying the implementation.

[mirko]

Did some basic testing on ESP32 with hardware as well as software SPI stack with various wordsizes in the same session. Patterns captured by LA as expected.

[codecov-commenter]

Codecov Report

Merging #5542 (bc8597d) into master (7c54b64) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5542      +/-   ##
==========================================
- Coverage   98.27%   98.24%   -0.04%     
==========================================
  Files         154      154              
  Lines       20076    20007      -69     
==========================================
- Hits        19730    19656      -74     
- Misses        346      351       +5     

Impacted Files	Coverage Δ
py/obj.c	97.22% <0.00%> (-0.80%)	⬇️
py/bc.c	89.13% <0.00%> (-0.57%)	⬇️
py/map.c	99.49% <0.00%> (-0.51%)	⬇️
py/runtime.c	99.24% <0.00%> (-0.31%)	⬇️
py/vm.c	98.98% <0.00%> (-0.05%)	⬇️
py/showbc.c	98.51% <0.00%> (-0.02%)	⬇️
py/compile.c	99.93% <0.00%> (-0.01%)	⬇️
py/emitbc.c	100.00% <0.00%> (ø)
py/objtype.c	100.00% <0.00%> (ø)
py/objmodule.c	93.54% <0.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c54b64...bc8597d. Read the comment docs.

[jimmo]

Thanks for the PR @MrSurly

@dpgeorge, @projectgus and I were just talking about this. We're considering merging this if you're still interested in updating it, but need to address the following:

1. Rebase on latest master
2. We think the meaning of the bits argument to spi_transfer should change to instead be the total number of bits (or clocks) to send. (I think this is what @dpgeorge was getting at in Allow (unpadded) SPI transfers < 8bits #5225 (comment)). So the bits argument to the constructor is how many bits to read from each word of the input, and the argument to read/write is the number of total bits to send.
3. Some minor renaming of variables, e.g. bytesPerChunk should be bytes_per_chunk.
4. It's convenient that ESP32 (and obviously SoftSPI) support this but it's never going to be supported on many other ports. So I think each other port should add if (bits % self->bits != 0) { raise } in their implementation spi_transfer. This allows us to support the bits argument everywhere.
5. It's a non-trivial code-size increase even for ports that don't support this in hardware. Both for the soft-spi implementation and the additional argument handling. It would be really good if this could be <100 bytes (e.g. on PYBV11). Unfortunately it's difficult to answer this without doing the above.

To expand on (2):

spi = machine.SPI(..., bits=3)
spi.write(b'\x07\x02\x04', bits=7)

would write the bit pattern 111 010 1 (i.e. consume 3 bits at a time from each byte of the input, and output 7 total bits)

spi = machine.SPI(..., bits=8)
spi.write(b'\xaa\x55\x33\x03', bits=26)

would write the bit pattern 10101010 01010101 00110011 11 (i.e. consume each byte, but only two bits from the last)

I think this also matches conceptually with the sort of uses cases. i.e. you're using SPI to send an N-bit payload, that's packed into M-bits per byte.

cc @mirko

[mirko]

I'm definitely (still) interested and kept maintaining that PR locally, as I'm heavily using this.
However back then I also noticed (and am still noticing) increased delays in normal SPI transfers (posted screenshots from scope and/LA somewhere about this), as this change seems to increase some overhead.
Does it make sense to (still) push my current WIP-branch?

[mirko]

And at least this commit would need to be applied as well, as otherwise this results in corrupted SPI transfers:
mirko@5c2230c

[projectgus]

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.