Skip to content

Conversation

@agatti
Copy link
Contributor

@agatti agatti commented Oct 1, 2024

Summary

This commit adds an extra bit of parameters validation to the SPI bus constructor on ESP32. Passing 0 as the number of bits would trigger a division by zero error when performing read/write operations on an SPI bus created in such a fashion.

This fixes #5910.

Testing

This was tested locally on an ESP32-D0WD:

MicroPython v1.24.0-preview.363.g17d823445.dirty on 2024-10-01; Generic ESP32 module with ESP32
Type "help()" for more information.
>>> import machine
>>> machine.SPI(2, 10000000, sck=10, mosi=20, phase=0, bits=0, firstbit=0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Invalid number of bits (0)
>>> machine.SPI(2, 10000000, sck=10, mosi=20, phase=0, bits=-2, firstbit=0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Invalid number of bits (-2)
>>> machine.SPI(2, 10000000, sck=10, mosi=20, phase=0, bits=-1, firstbit=0)
SPI(id=2, baudrate=10000000, polarity=0, phase=0, bits=8, firstbit=0, sck=10, mosi=20, miso=19)
>>> 

Trade-offs and Alternatives

This PR fixes the issue on ESP32, but maybe this is still a problems on other ports. I do not have many non-ESP32 boards that are currently supported around here so I can only claim this fixes the ESP32 version of the bug. Given that the bus instances are constructed in platform-specific code then I'd have to fix all of them if they have this specific issue. I've got no problem in doing the fix on other platforms but I may not be able to perform code validation.

@dpgeorge
Copy link
Member

dpgeorge commented Oct 9, 2024

I think this should be checked in machine_hw_spi_init_internal(), otherwise you can still make it crash via spi.init(bits=0).

}

if (args[ARG_bits].u_int != -1 && args[ARG_bits].u_int <= 0) {
mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("Invalid number of bits (%d)"), args[ARG_bits].u_int);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a message like "invalid bits %d", or just "invalid bits". The latter matches adc.c and machine_i2s.c error messages.

@agatti agatti force-pushed the esp32-spi-zero-bits branch from c32f355 to 635ccf0 Compare October 15, 2024 11:12
@agatti
Copy link
Contributor Author

agatti commented Oct 15, 2024

This should be sorted now, I also changed the error message to be in line with the other two cases you mentioned, thanks!

This commit adds an extra bit of parameters validation to the SPI bus
constructor on ESP32.  Passing 0 as the number of bits would trigger a
division by zero error when performing read/write operations on an SPI
bus created in such a fashion.

Fixes issue micropython#5910.

Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
@dpgeorge dpgeorge force-pushed the esp32-spi-zero-bits branch from 635ccf0 to 86c71a0 Compare October 30, 2024 04:14
@dpgeorge
Copy link
Member

Thanks for updating, looks good now.

@dpgeorge dpgeorge merged commit 86c71a0 into micropython:master Oct 30, 2024
8 checks passed
@agatti agatti deleted the esp32-spi-zero-bits branch November 1, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ESP32 SPI bits=0 results in IntegerDivideByZero

2 participants