Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FourWire and I2CDisplay argument validation #5450

Merged
merged 4 commits into from Oct 10, 2021

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Oct 9, 2021

resolves #5449

Check spi argument for None and raise a ValueError instead of crashing.

Tested successfully with code from the issue. on:

Adafruit CircuitPython 7.0.0-176-gf13db0d0e-dirty on 2021-10-09; Raspberry Pi Pico with rp2040
Board ID:raspberry_pi_pico

With the build from the PR it does successfully raise the ValueError instead of crashing into the broken state.

ladyada
ladyada previously approved these changes Oct 9, 2021
@ladyada
Copy link
Member

ladyada commented Oct 9, 2021

didnt test but lgtm

Copy link
Collaborator

@microdev1 microdev1 left a comment

Choose a reason for hiding this comment

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

@FoamyGuy Thanks for catching & fixing this issue. One suggestion, looks good otherwise.

shared-bindings/displayio/FourWire.c Outdated Show resolved Hide resolved
locale/circuitpython.pot Outdated Show resolved Hide resolved
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks, good find!

I wonder if you can't use mp_arg_validate_type with &busio_spi_type instead. as it stands, won't passing in something else, like the number 7 or an empty list (or even, more plausibly, a bitbangio SPI object) still cause problems?

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

(see above comment)

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Oct 9, 2021

Latest commit changes to use mp_arg_validate_type instead of the if None. This has the benefit of already having it's own error message so we also don't end up needing the string / translations with this method. Thanks for the tip @jepler

microdev1
microdev1 previously approved these changes Oct 9, 2021
@jepler
Copy link
Member

jepler commented Oct 9, 2021

I'm not sure why more boards didn't fail, but it seems you need to include the header that gives the declaration of busio_spi_type:

--- a/shared-bindings/displayio/FourWire.c
+++ b/shared-bindings/displayio/FourWire.c
@@ -32,6 +32,7 @@
 #include "py/binary.h"
 #include "py/objproperty.h"
 #include "py/runtime.h"
+#include "shared-bindings/busio/SPI.h"
 #include "shared-bindings/displayio/Group.h"
 #include "shared-bindings/microcontroller/Pin.h"
 #include "shared-bindings/util.h"

@jepler
Copy link
Member

jepler commented Oct 9, 2021

Do you mind checking out the I2CDisplay code while you're working on it?

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Oct 9, 2021

Do you mind checking out the I2CDisplay code while you're working on it?

Indeed I2CDisplay does exhibit a similar hard crash with this reproducer:

import board
import displayio
import adafruit_displayio_ssd1306

displayio.release_displays()
oled_reset = board.D9

# Use for I2C
i2c = board.I2C()

display_bus = displayio.I2CDisplay(None, device_address=0x3C, reset=oled_reset)

WIDTH = 128
HEIGHT = 32  # Change to 64 if needed

display = adafruit_displayio_ssd1306.SSD1306(display_bus, width=WIDTH, height=HEIGHT)

I'll put a fix in for it as well.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Oct 9, 2021

Latest commit fixes the SPI include and implements similar validation in I2CDisplay

@microdev1 microdev1 changed the title check for None spi when initializing FourWire Fix FourWire and I2CDisplay argument validation Oct 10, 2021
@microdev1 microdev1 requested a review from jepler October 10, 2021 03:05
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thank you, so clean & to the point now. Nice to have those hardfaults fixed.

@jepler jepler merged commit 797f0a1 into adafruit:main Oct 10, 2021
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.

Passing None for SPI to FourWire puts pico in broken state
4 participants