Add colstart kwarg

Use it instead of special casing colstart based on display width. It shouldn't be assumed because it is up to the display's manufacturer.

@tannewt
if I understand correctly you want changes in the 32 value here, and change it to a parameter in the constructor?

Adafruit_CircuitPython_SSD1306/adafruit_ssd1306.py

Line 72 in 3b350ba

self.page_column_start[0] = self.width % 32

Defaulting to 32
Let me know I will proceed with PR Thanks

I'm not sure. I think the 32 might be packing the value into the registers. colstart may replace self.width instead. It's been a while since I've looked at this.

No problem, So I will need to test then to verify behavior. Ill do it over the weekend. Thanks

@tannewt

Findings

Options for page addressing are disable by default:

Adafruit_CircuitPython_SSD1306/adafruit_ssd1306.py

Line 218 in 3b350ba

page_addressing=False

and introduced in PR #57

I changed this to True and using an Adafruit 0.91" OLED display I got

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "t3.py", line 14, in <module>
  File "/lib/adafruit_ssd1306.py", line 238, in __init__
  File "/lib/adafruit_ssd1306.py", line 80, in __init__
  File "/lib/adafruit_ssd1306.py", line 140, in init_display
  File "/lib/adafruit_ssd1306.py", line 195, in show
  File "/lib/adafruit_ssd1306.py", line 259, in write_framebuf
RuntimeError: Function requires lock

Could not figure out why the 32 is using here, after my initial google research. Be aware that not very familiar with displays. @adamcandy maybe you could put a light on this, before going on a rabbit hole :). Thanks

Oh! The other context might be the displayio driver: https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306/blob/master/adafruit_displayio_ssd1306.py#L65

@jposada202020, the RuntimeError: Function requires lock error looks unrelated to the main thread here. I did not see this error and am running this exact code route successfully (i.e. down to the same i2c version of write_framebuf). It is likely a difference in resilience of supporting libraries in the environment.

A good possibility here is that the lock of the i2c_device cannot be obtained, since it has not been released from a previous use. The new branch below attempts to fix this -- please repeat the above with this branch instead:
adamcandy/Adafruit_CircuitPython_SSD1306/tree/fix-locking-error

Also, instead of modifying the source to set page_addressing=True on line 218, you can just simply provide this option to the function:
adafruit_ssd1306.SSD1306_I2C( ... , page_addressing=True)

e.g. something like:

from board import SCL, SDA
import busio
import adafruit_ssd1306

i2c = busio.I2C(SCL, SDA)
adafruit_ssd1306.SSD1306_I2C(width, height, i2c, page_addressing=True)

I am a bit confused on the main thread, since the piece of code @jposada202020 refers to in reply to the OP of @tannewt is new and did not exist when this issue was opened!

This new code also only applies to the Page Addressing Mode and is not used in the default Horizontal Addressing Mode. This could though be the correct move. This calculation was based on documentation for several of the screens and is general enough to cover all these -- but may need generalising further to work with a larger set of screens.

To me it looks like @tannewt was referring to the logic in the show function lines 175--194, which shifts SET_COL_ADDR depending on display width. (Note here only for Horizontal Addressing Mode.)

It may be best to start with this part first (defaulting to the logic if no argument supplied) -- but has anyone found displays where this is a problem?

While I'm looking at this, in response to @tannewt's comment: #40 (comment)

Oh! The other context might be the displayio driver: https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306/blob/master/adafruit_displayio_ssd1306.py#L65

Looking at the logic that went in with b92bf18 in this library, the logic in the corresponding displayio equivalent adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306 could be improved to include a wider range of displays:

e.g. b92bf18 adds: if (self.height == 32 or self.height == 16) and (self.width != 64)

This follows:

The various screen sizes available with the ssd1306 OLED driver
chip require differing configuration values for the display clock
div and com pin, which are listed below for reference and future
compatibility:
   w,  h: DISP_CLK_DIV  COM_PIN_CFG
 128, 64:         0x80         0x12
 128, 32:         0x80         0x02
  96, 16:         0x60         0x02
  64, 48:         0x80         0x12
  64, 32:         0x80         0x12

(I think the source of this table is mentioned in one of the completed issues here.)

@adamcandy Thank you for all the answers, it is more clear know :)

@jposada202020, the RuntimeError: Function requires lock error looks unrelated to the main thread here. I did not see this error and am running this exact code route successfully (i.e. down to the same i2c version of write_framebuf). It is likely a difference in resilience of supporting libraries in the environment.

A good possibility here is that the lock of the i2c_device cannot be obtained, since it has not been released from a previous use. The new branch below attempts to fix this -- please repeat the above with this branch instead:
adamcandy/Adafruit_CircuitPython_SSD1306/tree/fix-locking-error

Agree I found that very quickly, in order to do my tests I had to completely disconnect the screen from the I2c connector to leave it without any source of power,.

To me it looks like @tannewt was referring to the logic in the show function lines 175--194, which shifts SET_COL_ADDR depending on display width. (Note here only for Horizontal Addressing Mode.)

I try different combinations changing that parameters without any luck either. But As previously mentioned the only display that I have is the Adafruit one, so I could only test in this one, so any further improvements in the library has to be done by someone that has all the display available.

And last thing.... yes you are right...I know that I should have change the parameter in the example code and not in the library, but I work must of my time in the libraries.. so a bad habit... 😉

@adamcandy do not know If I am asking too much but if you could help us out with this issue it would be really appreciate it :).

Thanks again

I'm supportive of supporting more displays. My idea is for the driver to be for the chip rather than a single chip and display combination. I think the combinations are better supported in examples or separate "init everything" libraries.

Glad my comments have been useful @jposada202020 :)

It looks like before we tackle the main thread of this 'enhancement' there's a bug to sort first -- the backtrace you see

I changed this to True and using an Adafruit 0.91" OLED display I got

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "t3.py", line 14, in <module>
  File "/lib/adafruit_ssd1306.py", line 238, in __init__
  File "/lib/adafruit_ssd1306.py", line 80, in __init__
  File "/lib/adafruit_ssd1306.py", line 140, in init_display
  File "/lib/adafruit_ssd1306.py", line 195, in show
  File "/lib/adafruit_ssd1306.py", line 259, in write_framebuf
RuntimeError: Function requires lock

I do not expect this is related to the various parameters and logic needed for specific displays, but that the i2c_device is not being used cleanly -- potentially a bug in the code recently added:

@jposada202020, the RuntimeError: Function requires lock error looks unrelated to the main thread here. I did not see this error and am running this exact code route successfully (i.e. down to the same i2c version of write_framebuf). It is likely a difference in resilience of supporting libraries in the environment.

A good possibility here is that the lock of the i2c_device cannot be obtained, since it has not been released from a previous use. The new branch below attempts to fix this -- please repeat the above with this branch instead:
adamcandy/Adafruit_CircuitPython_SSD1306/tree/fix-locking-error

I have made a simple change that I think has a good chance of fixing this. I do not have the problem here or backtrace -- so as a first step it would be helpful if you could repeat your testing in #40 (comment) but with the new branch. Please let us know if this at least removes the backtrace.

Then we'll be in a good place to experiment with parameters for page addressing with the 0.91" display -- which could give us a general solution for all.

Thank you very much :) will try that tonight :) and let you know the results.

Thanks again

We are improving, no error, however I think we have some problems in the way we are referencing the page address, the screen limits are displaced somehow as you can see in the gif.

@adamcandy

Perfect! This is what we expected and hoped. So the code changes fix the locking error you have experienced. I will create a PR to update the main branch here.

Then we can work on the parameters. Just one thought - if this is a stock Adafruit display, it should be SSD1306-based and support Horizontal Addressing Mode. So if page_addressing is left set to False it should work fine as expected - e.g. the bouncing ball example will use the whole display in the above. Does it work fine in Horizontal Addressing Mode?

Yes, works fine. strange..

Again, perfect! Just as expected. This means the parameters for Page Addressing Mode need some adjusting for 128x32 displays. It could be a number of things -- e.g. the COM_PIN_CFG is different for 128x32 displays (see the table above) but it could be this is only for Horizontal Addressing Mode. Could also be the packing/row number 32 discussed above. We should check, but it could also be that Page Addressing Mode is limited to smaller screen sizes, and even though SSD1306 supports this mode it may not function for larger screens like the 128x32.

So far my development extending this library has been motivated purely pragmatically - so the library has at least one method to produce output on a wider set of displays. Since you have output working on this screen, do we need to modify the implemented Page Addressing Mode further to make it work here?

For me, I would say no, for me is difficult to code without testing, because strange things could happen like the circle getting out of the limits...
And I have already 1 too many oled displays 😄 ...
@tannewt My proposition is.. I could merge the PR from @adamcandy it solves the I2C lock, and if someone with the hardware could implement and test we could add it that way, and close the issue for the moment. guidance is needed here. Thanks :)

@jposada202020 Sounds good. The lock fix PR looks correct to me.

Closing,
Thanks again @adamcandy :)