extmod/machine_spi.c: Support firstbit=machine.LSB for machine.SoftSPI. #15436
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15436 +/- ##
=======================================
Coverage 98.42% 98.42%
=======================================
Files 161 161
Lines 21253 21253
=======================================
Hits 20919 20919
Misses 334 334 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
extmod/machine_spi.c
Outdated
| mp_raise_ValueError(MP_ERROR_TEXT("firstbit must be MSB")); | ||
| } | ||
| self->spi.firstbit = args[ARG_firstbit].u_int == MICROPY_PY_MACHINE_SPI_MSB ? | ||
| MICROPY_PY_MACHINE_SPI_MSB : MICROPY_PY_MACHINE_SPI_LSB; |
There was a problem hiding this comment.
I don't think there's any need to check for valid inputs like this. Instead doing self->spi.firstbit = args[ARG_firstbit].u_int; is fine (that will anyway give equivalent behaviour because mp_soft_spi_transfer is validating the value as well).
Alternatively, could replace the firstbit variable here with bool msb and do:
self->spi.msb = args[ARG_firstbit].u_int == MICROPY_PY_MACHINE_SPI_MSB;Then in mp_soft_spi_transfer it's simply self->spi.msb ? src[i] : swap_bits(src[i]). That may save a few bytes of code size.
There was a problem hiding this comment.
Changed. The first variant is kept, such that the name of the symbol matches the name of the option. Since the value of MICROPY_PY_MACHINE_SPI_MSB is 0 and self->spi.msb ? ....., is actually self->spi.msb != 0 ? .....` most likely the compiler will generate the same code for both variants in mp_soft_spi_transfer.
There was a problem hiding this comment.
The first variant is kept, such that the name of the symbol matches the name of the option.
Yes, good idea.
And I checked code size and indeed it doesn't change when using bool msb. So the compiler is optimising it as we would expect it to.
|
The review comments are integrated and the value of firstbit is shown by the object print method. |
Being able to send data out in LSB format can be useful, and having support in the low-level driver is much better than requiring Python code to reorder the bits before sending them / after receiving them. In particular if the hardware does not support the LSB format (eg RP2040) then one needs to use the SoftSPI in LSB mode. For this change a default definition of `MICROPY_PY_MACHINE_SPI_MSB/_LSB` was added to `py/mpconfig.h`, making them available to all ports. The identical defines in `esp32/mpconfigport.h` were deleted. Resolves issues micropython#5340, micropython#11404. Signed-off-by: robert-hh <robert@hammelrath.com>
Easy to add, sometimes needed and sought for.
For the change the definition of MICROPY_PY_MACHINE_SPI_MSB/ -LSB was moved to py/mpconfig.h, making them available to all ports. The identical defines in esp32/mpconfigport.h were deleted.
Tested with the RP2 and ESP32 port.