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

Add I2C bus support #19

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Add I2C bus support #19

merged 2 commits into from
Jul 24, 2024

Conversation

EAGrahamJr
Copy link
Contributor

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

Fixes #16 by adding the same constructs as found in SSD1306.
jepler
jepler previously requested changes Jul 12, 2024
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 for this contribution!

Please see my comment about how to structure the try/except blocks and let me know if you need more clarification.

If possible please add some information about how you tested this and on what hardware.

adafruit_displayio_sh1106.py Outdated Show resolved Hide resolved
@jepler jepler requested a review from a team July 14, 2024 20:22
@jepler
Copy link
Member

jepler commented Jul 14, 2024

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
Copy link
Contributor Author

future planned changes

🤣

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

@dhalbert
Copy link
Contributor

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

@EAGrahamJr
Copy link
Contributor Author

@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. 😀 )

@dhalbert
Copy link
Contributor

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?

@tannewt
Copy link
Member

tannewt commented Jul 24, 2024

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.)

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

This is fine too. We'll clean up these try/excepts once we drop 8.x support.

@tannewt tannewt merged commit e674d6e into adafruit:main Jul 24, 2024
1 check passed
@EAGrahamJr EAGrahamJr deleted the issue-16 branch July 24, 2024 17:28
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 30, 2024
Updating https://github.com/adafruit/Adafruit_CircuitPython_BME280 to 2.6.25 from 2.6.24:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME280#69 from rwmanos/patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_BME680 to 3.7.8 from 3.7.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#73 from FoamyGuy/precommit_copyright
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#70 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SH1106 to 1.4.0 from 1.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SH1106#19 from EAGrahamJr/issue-16

Updating https://github.com/adafruit/Adafruit_CircuitPython_EMC2101 to 1.2.6 from 1.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_EMC2101#34 from rpavlik/fix-name

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731 to 3.4.1 from 3.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#55 from grandinquisitor/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyPortal to 6.3.4 from 6.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyPortal#134 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_NTP to 3.3.0 from 3.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_NTP#39 from mMerlin/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_PortalBase to 2.1.1 from 2.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PortalBase#102 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 4.1.6 from 4.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#202 from FoamyGuy/precommit_copyright
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#173 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Where is the I2C support?
4 participants