Skip to content

New quirk for sh1107 #3450

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

Merged
merged 12 commits into from
Sep 30, 2020
Merged

Conversation

mdroberts1243
Copy link

This change introduces a new quirk to support the FeatherWing SH1107 128x64 OLED display. A parameter, "column_and_page_addressing" when set to true in the driver will cause the displayio_display_core_set_region_to_update function to use the column lower, column higher and page set commands on the sh1107.

I wrote a new driver to test with: https://github.com/mdroberts1243/mdroberts1243_CircuitPython_DisplayIO_SH1107

Right now python is throwing an error that I'm sending too many keyword arguments to the super().init

Auto-reload is on. Simply save files over USB to run them or enter REPL to disable.
code.py output:
Traceback (most recent call last):
File "code.py", line 32, in
File "mdroberts1243_displayio_sh1107.py", line 84, in init
TypeError: extra keyword arguments given
Press any key to enter the REPL. Use CTRL-D to reload.

@ladyada ladyada requested review from tannewt and jepler September 22, 2020 14:11
Copy link

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

This seems fine if it is working, though from one of your comments it sounds like it's not working yet. I'd also want to find out from @tannewt if the "column_and_page_addressing" flag should go in the "core" object or stay where it's been placed.

@mdroberts1243
Copy link
Author

Thanks Jeff. To clarify, Scott asked me to post a draft PR so he could review and help me get past this problem with the bindings. I don't know what I missed in trying to pass the new parameter from the driver back to the display core. -mark.

@tannewt
Copy link
Member

tannewt commented Sep 22, 2020

@mdroberts1243 I don't see anything wrong with your code here or in your test library. That error can happen if the name of a kwarg doesn't match one in the parse table. Maybe a typo? Does it go away if you comment out the new kwarg? If so, maybe you don't have your code loaded on the device. You can do mp_printf(&mp_plat_print, "hello"); in your C code to print out to serial and confirm it is run.

@mdroberts1243
Copy link
Author

SH1107 DisplayIO Support Update

I thought I would use this draft PR to keep posting progress on trying to get SH1107 supported under DisplayIO.

TL;DR:
I'm stuck because DisplayIO seems to expect the display controller to auto-increment past page (page mode writes) or column (vertical mode writes) boundaries. SH1107 requires you to set the page or column address at the end of each boundary or it will just wrap and start updating the same page or column of memory. I'm not sure this can be worked around and I'm thinking that SH1107 OLED displays should be some kind of FrameBufferIO type display, buffered in memory and then refreshed all at once periodically.

The Good

  • I've had great support so far in attempting this project! Kudos to both Scott and Jeff. Especially appreciated Scott's live-stream deep dive into the way DisplayIO is implemented which helped to kick this off.
  • I have learned a lot about CircuitPython, the building process, using GItHub, DisplayIO and SH1107 (this started with me placing a Covid-support order on Adafruit to get some stuff to play CircuitPython with ;-) )
  • What I (we) set out to do works perfectly. Setting the quirk (with suitable downgrade in name) to True allowed me to implement conditional code in displayio_display_core_set_region_to_update() that would set upper and lower column addresses and the page address appropriately to the beginning of the subrectangle that was to be painted. I've been able to see bitmaps and text painted onto the OLED using DisplayIO, but only in the swath of a single page (8 pixels deep across the 'width' of 64 columns on the FeatherWing OLED).
  • We found a bug in the MP_ARGS Parsing!
    And finally:
  • I had an excuse to buy an oscilloscope that decodes I2C (didn't need it or use it, but "No job is worth starting unless you can buy a new tool.")

The Bad

  • DisplayIO seems to expect a display controller that would be able to write any subrectangle of the display's size and just spits out the data continuously once that has been set up. There doesn't seem to be a way to impose boundaries (like only write one column's worth or one page worth)
  • SH1107 only supports two write modes... page and vertical (column). It will auto-increment across a page (8 pixels deep) for the width of the display (64 columns in the case of the FeatherWing) OR along a column of pages (16 pages, 8 pixels deep for 128 pixels total). When you reach the boundary the page or column will stay the same and the writes will wrap to the beginning of the page or column and overwrite what you've just done. You are supposed to set the page or column, depending on mode, each time you reach the boundary.

The Ugly

  • You could always use the adafruit_framebuf driver I put in place (when the fix to the text() bug propagates through the system). How do I contribute that, BTW?
  • It seems to me this kind of display might be best served by a DisplayIO FrameBufferIO driver. I saw some docs, but it wasn't clear to me whether there are displays using this and how to get started creating one. I would imagine (dangerous to ASSume) that you could do all the DisplayIO functions on a memory buffer and then automagically paint the screen periodically to update the display.

Where to go from here?

  • Looking for ideas.
  • Am I missing something about DisplayIO? Is it possible for it to honour page/column boundaries and only send one page or one column of data each time?
  • Are there FrameBufferIO drivers like I'm imagining? Can I have a worked example?

Thanks for making it this far!

@tannewt
Copy link
Member

tannewt commented Sep 23, 2020

I think you can add a second quirk (or just call this the sh quirk and have it do two things.) The second thing you could do is change the subrectangle that is updated here: https://github.com/adafruit/circuitpython/blob/main/shared-module/displayio/Display.c#L277 Basically change each subrectangle to be the smallest area you can update.

@mdroberts1243
Copy link
Author

I think you can add a second quirk (or just call this the sh quirk and have it do two things.) The second thing you could do is change the subrectangle that is updated here: https://github.com/adafruit/circuitpython/blob/main/shared-module/displayio/Display.c#L277 Basically change each subrectangle to be the smallest area you can update.

Thanks. I might try switching to 'vertical' mode as well, then the display will have a horizontal orientation naturally without the user having to set it up. Console output will work better too in that case.

Bandwidth is somewhat limited next few days, so progress might be slow...

@mdroberts1243
Copy link
Author

mdroberts1243 commented Sep 25, 2020

I have done a clean up and committed changes "Mostly-working-version with comments".

I have also updated the driver and the example code: https://github.com/mdroberts1243/mdroberts1243_CircuitPython_DisplayIO_SH1107 (don't know how to contribute that!)

Currently everything looks like it is working except label text (bitmap_label will work fine). An easy way to see the problem with text is to run the example code and then enter the REPL. You will see that the terminalio<?> output works fine but when you enter code into the REPL the text you enter will wrap back to the beginning of the line at about the 1/2 way point on the screen.

See the circuitpython discord for a picture.

PS Forgot to mention I changed the name of the quirk to "SH1107_addressing" and changed to the vertical mode of the display driver, so now each row is one pixel deep and 128 pixels wide in 16 bytes of data. This makes the display naturally horizontal.

PPS One thing that worries me is that you need to be on byte boundary in x with your subrectangle since each byte will paint 8 pixels horizontally. I wasn't sure that was handled correctly but I tried different odd starting points and things seem to work o.k.

@mdroberts1243
Copy link
Author

I fixed the masking error I had in setting the page on the controller and now all the basic operations work. You can use it with the REPL even.

How do I contribute the driver and example code linked above? Not sure how to create a "Pull Request" for something new.

Let me know if you need anything else.

-mark.

@mdroberts1243 mdroberts1243 marked this pull request as ready for review September 25, 2020 13:31
@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

I fixed the masking error I had in setting the page on the controller and now all the basic operations work. You can use it with the REPL even.

How do I contribute the driver and example code linked above? Not sure how to create a "Pull Request" for something new.

Let me know if you need anything else.

-mark.

For the driver we can either fork your copy into the adafruit org or you can transfer it. We'll maintain it since we sell a product with the display. Would you like us to fork your repo?

@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

Also, congrats! You found all of the boards with displays initialized in board_init. You'll need to add extra , false to all of those calls to make the compiler happy.

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.

Really nice work @mdroberts1243, thank you! Just a couple suggestions to make the code clearer. You will also need to fix up the display construct calls for boards with built in displays. Thanks!

@mdroberts1243
Copy link
Author

mdroberts1243 commented Sep 25, 2020

I fixed the masking error I had in setting the page on the controller and now all the basic operations work. You can use it with the REPL even.
How do I contribute the driver and example code linked above? Not sure how to create a "Pull Request" for something new.
Let me know if you need anything else.
-mark.

For the driver we can either fork your copy into the adafruit org or you can transfer it. We'll maintain it since we sell a product with the display. Would you like us to fork your repo?

Yes, please fork and maintain! I'm willing to transfer it as well, but don't know how that is accomplished.

@mdroberts1243
Copy link
Author

Also, congrats! You found all of the boards with displays initialized in board_init. You'll need to add extra , false to all of those calls to make the compiler happy.

O.k. That was a bit tedious!

Also, there is an issue in build-arm (electronut_labs_blip) where it complains about too few arguments on the set region to update... I suppose they've made their own copy of that for some reason? I probably need to change that too...

@mdroberts1243
Copy link
Author

mdroberts1243 commented Sep 26, 2020

I have made all the requested changes and updates. But a few boards are still failing the checks but due to messages related to the number of arguments passed to displayio_display_core_set_region_to_update. The errors seem to be the same. Boards include: electronut_labs_blip, pewpew_m4, pybadge, pybadge_airlift, pygamer, pygamer_advance, ugame10. Many of these didn't show this problem until after the original common_hal function changes were made.

@tannewt I need help understanding why these errors are being thrown and what is causing them... as far as I know the files they are related to are common to all the boards! Every board should see the same number of arguments into displayio_display_core_set_region_to_update

Also, in testing the new logic I decided to move a label around the screen by a pixel or two in x and y and see if there were any issues with strange clipping etc. This heavily loaded the I2C bus and I found that after a few seconds the program/system would lock up, freeze the display and only be recovered by a hard reset. My conditions:

  • STM32F405 Feather Express
  • SH1107 128x64 OLED FeatherWing (removed other devices from the bus)
  • mu-editor on Ubuntu 18.04 LTS with Python 3.8
  • CircuitPython 6.0.0-alpha.3-329-g19dbff67f-dirty

Is there a known issue with I2C? Or this combination of stuff? Alpha CP? STM too new?

I also have seen the software not release the I2C properly (requires reset), which might be related to the above problem too.

-mark.

@mdroberts1243
Copy link
Author

A small status update:

  • I decided to build the quirk for the GrandCentral M4 Express board I bought.
  • The SH1107 quirk built and ran fine with my driver
  • I wasn't able to recreate the lockup/freeze that I saw on the STM32F405 Feather, even after hammering the I2C bus with continuous updates (moving "SH1107" around the screen with random increments)
  • In separate testing I found I would see OS ErrNo 5 (I/O Error) on the STM32F405 when running the simpletest for the AS7341 as well.

So I'm concluding that there's an issue with the STM32F405 port and/or CircuitPython 6.0 alpha on the STM Feather. I confirmed the STM I2C bus is at 400kHz and that is supported by the chip and the display controller. I don't know what speed the GrandCentral M4 Express is running at, but it updates quite quickly.

@tannewt
Copy link
Member

tannewt commented Sep 28, 2020

@mdroberts1243 I think those are the boards with the _stage module enabled. It calls set region to update itself here: https://github.com/adafruit/circuitpython/blob/main/shared-module/_stage/__init__.c#L46

I think you'll want to pull the parameter out of the display object like the other params.

@mdroberts1243
Copy link
Author

@tannewt I made the change to _stage and rebuilt the Grand Central M4 Express just to make sure nothing was broken. I had to resolve one conflict and merged it in. But now the checks are queued and seem to be stuck. It is the checks that I want to see!

-mark.

@mdroberts1243
Copy link
Author

@tannewt The checks failed. Failing in a different way then before and the various board builds didn't run at all. The only thing different was I was asked to resolve a conflict, which I did, accepting a bunch of changes to seeduino wio terminal apparently in order to add my quirk to the end of the display construct. In ports/atmel-samd/boards/seeeduino_wio_terminal/board.c

Anyway, things seem to have regressed and I'm at a loss to interpret the failure.

@tannewt
Copy link
Member

tannewt commented Sep 29, 2020

Note that the one build failure is a clone failure and can be ignored.

@tannewt tannewt force-pushed the New_quirk_for_SH1107 branch from b34e2bf to ef245ef Compare September 30, 2020 18:07
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.

Thanks @mdroberts1243! This looks good.

@tannewt tannewt merged commit 5ac3c36 into adafruit:main Sep 30, 2020
@tannewt
Copy link
Member

tannewt commented Sep 30, 2020

@mdroberts1243 mdroberts1243 deleted the New_quirk_for_SH1107 branch September 30, 2020 19:30
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.

3 participants