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

Minor changes to the ST7789 1.47 Example that were required to work with raspberry pi pico w #34

Merged
merged 5 commits into from Jan 21, 2023

Conversation

markmcgookin
Copy link
Contributor

Small changes to work with Circuit Python 8 Beta 4 on Raspberry Pi Pico. board.SPI didn't exist and needed to be changed to busio.SPI.

The Dx reference for pins doesn't work and needs updated to be GPx (for the raspberry pi pico at least)

I am unaware if this is across the board, or just for the pico w or just CP8. But I thought I would share as it's taken me quite a while to figure this out.

Also... again this might be an eccentricity of the waveshare 1.47 board, but I had to power it from external 3v3 BEFORE booting my board for this code to work. When trying to drive it from any of the Picos outputs, the screen was either incredibly dim or simply didn't work, it would be great to mention that in documentation on the site somewhere if that's common.

…co. board.SPI didn't exist and needed to be changed to busio.SPI. The Dx reference for pins doesn't work and needs updated to be GPx (for the raspberry pi pico at least)
@markmcgookin
Copy link
Contributor Author

Not sure if I've broken something with the code change or the CI tests are on the fritz... looks like someone was working on them recently. If it's something up with the code please let me know.

@tekktrik
Copy link
Member

You can merge main into your code to grab any new CI changes, but the specific issue the CI has is that it wants to reformat the files. You can make the changes it wants to make by running pre-commit locally and then pushing those changes.

@markmcgookin
Copy link
Contributor Author

markmcgookin commented Nov 13, 2022 via email

@FoamyGuy
Copy link
Contributor

I can try to merge this and resolve CI issues.

I think we will not want to change the example to use only the pico-w pins though. Including them is fine, but we should keep the originals, as they are used by guides and other resources on other devices. In cases like this where there are alternatives it's typical to include a commented out section with the alternative pins or configuration details. I think we'd want something like that here.

Copy link
Contributor

@FoamyGuy FoamyGuy 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 passing the CI checks now, it did need just some minor code formatting changes.

I think we will want to keep the existing pins and SPI setup as an option for anyone using non-pico microcontrollers.

I would recomend adding the new option like this so that people can uncomment the one they need easily:

# built-in, silkscreen labelled SPI bus  
spi = board.SPI()
tft_cs = board.D5
tft_dc = board.D6
tft_rst = board.D9

# pico and pico-w pins for SPI bus
# spi = busio.SPI(board.GP2, board.GP3, board.GP4)
# tft_cs = board.GP5
# tft_dc = board.GP6
# tft_rst = board.GP7

@markmcgookin
Copy link
Contributor Author

markmcgookin commented Nov 22, 2022

Just making this change now. Would we not still need to update the board.SPI() call to buisio.SPI(board.D2, board.D3, board.D4) though.

Looking here it seems that busyio is now a requirement for I2C or SPI.

Board does have values for MISO, MOSI and SCK which could be used, but that might not work for the pico as it has multiple pins that can be used for them... how about if I change it to this:

# built-in, silkscreen labelled SPI bus  
spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
tft_cs = board.D5
tft_dc = board.D6
tft_rst = board.D9

# If using a Raspberry Pi Pico or Pico-w 
# Uncomment the below code to use GP (General Purpose) pins 
# instead of D (Digital)

# spi = busio.SPI(board.GP2, board.GP3, board.GP4)
# tft_cs = board.GP5
# tft_dc = board.GP6
# tft_rst = board.GP7

@markmcgookin
Copy link
Contributor Author

ah sorry man, I've broken the CI again... not sure what I am doing wrong, just using VSCode for these changes.

Error: black....................................................................Failed
- hook id: black
- files were modified by this hook

Error: reformatted examples/st7789_172x320_1.47_simpletest.py

@makermelissa
Copy link
Contributor

ah sorry man, I've broken the CI again... not sure what I am doing wrong, just using VSCode for these changes.

You should just be able to run pre-commit and then push any changes. See https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I reverted the change so we use board.SPI instead of busio in that case. Pylint needed a little help after that due to unused import so I moved it down to the commented section.

This looks good to me now.

Thanks for updating the example to make it easier for folks with Pico devices @markmcgookin!

@FoamyGuy FoamyGuy merged commit e23c487 into adafruit:main Jan 21, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 22, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7789 to 1.5.15 from 1.5.14:
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7789#34 from markmcgookin/main
  > Add upload url to release action
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7789#35 from xgpt/main
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

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.

None yet

5 participants