adafruit / Adafruit_CircuitPython_DisplayIO_SH1106

CircuitPython library for SH1106 OLED displays
MIT License
4 stars 4 forks source link

Add I2C bus support #19

Closed EAGrahamJr closed 2 months ago

EAGrahamJr commented 3 months ago

Fixes #16 by adding the same constructs as found in SSD1306.

jepler commented 3 months ago

I intended to say that "from typing import Union" will raise an ImportError exception on any CircuitPython version, causing the protection against future planned changes to CircuitPython (in which displayio.FourWire etc are removed) to no loger work as intended.

I regret that what I wrote had the alternate interpretation "this code can't even work under any circumstances", because that got us off on the wrong foot.

At the risk of over-explaining: The first ImportError exception means that the subsequent line "from busdisplay import BusDisplay" will never be used, and the interpreter will jump down to "from displayio import FourWire", always using the old compatibility names to be used. This will run on any current CircuitPython version, because both the old and new names are available right now, but will fail once the old displayio.FourWire name is removed in CircuitPython 10.

You can see what I mean by running this block of code:

try:
    from os import ThisDoesNotExist
    print("this will not print, as this code is not used")
except ImportError:
    print("the first ImportError exception jumped to this except block")

I've asked other CircuitPython librarians to review this PR so that someone else has the chance to vet and clarify what I'm saying and correct me if I'm mistaken.

EAGrahamJr commented 3 months ago

future planned changes

:rofl:

No problemo! You just happened to hit me in a very pedantic mood and I was "no, it _works! I can see it!" :grinning:

dhalbert commented 2 months ago

@EAGrahamJr Q: just want to confirm that you tested these updates with I2C.

EAGrahamJr commented 2 months ago

@EAGrahamJr Q: just want to confirm that you tested these updates with I2C.

Yes, so I can confirm "Works for me".

However, I am concerned that this is introducing something that isn't consistent across the drivers, as it's not quite the same as the ever-popular SSD1306, which is different from all the other displayio drivers. If the intent is to completely move to displayio then I would drop this PR and follow the pattern of the other drivers.

(Some of the lack of consistency across all the drivers is understandable but a little maddening for someone dropping in and diving a bit deeper into code. :grinning: )

dhalbert commented 2 months ago

However, I am concerned that this is introducing something that isn't consistent across the drivers, as it's not quite the same as the ever-popular SSD1306, which is different from all the other displayio drivers. If the intent is to completely move to displayio then I would drop this PR and follow the pattern of the other drivers.

(Some of the lack of consistency across all the drivers is understandable but a little maddening for someone dropping in and diving a bit deeper into code. 😀 )

@tannewt @makermelissa I'm not familiar enough with the drivers to have an opinion on this. Do you?

EAGrahamJr commented 2 months ago

@tannewt @makermelissa I'm not familiar enough with the drivers to have an opinion on this. Do you?

For reference, I see

But this indicates otherwise: https://github.com/adafruit/Adafruit_CircuitPython_ST7789/blob/fd4c53e6b0ed4805362362837aa7dd71ae7fe1da/adafruit_st7789.py#L45

(I'm not trying to be a PITA, but I do have a bit of a "thing" about consistent APIs :smiley: )

tannewt commented 2 months ago

For now, use displayio. The split changes were done too early because we added a warning about it too early. displayio will work with 8.x and 9.x. Once we're on 9.x and 10.x pre-release, then we can update everything over to the split modules (and we'll re-enable the warning in 9.x.)