Fix SSD1306 128x32: wrong column-start address

[jonwaterschoot]

Fixes #634.

SSD130xDriver::Update() (and the equivalent switches in SSD1307Driver) set the page column-start command to 0x12 (column 32) for height==32 panels instead of the standard 0x10 (column 0) used by the default case and every reference SSD1306 driver. Every page write started 32 columns in: the intended content shifted 32 columns right, and the last 32 columns wrapped back around to the left edge, garbling whatever was drawn there.

This is the same root cause @Len42 tracked down in #634 (SSD130x4WireSpi128x32Driver is an alias for this same SSD130xDriver class, just over SPI instead of I2C) and that @stephenhensley confirmed hitting independently. Also fixes the same line in SSD1307Driver::TransferPageDma() for consistency, though no public alias currently instantiates that class at height==32.

Second commit is a separate, unrelated perf change (batches SSD1306 I2C page writes into one transaction instead of one per byte) — happy to drop it if you'd rather keep this PR scoped to just the bug fix.

[jonwaterschoot]

I was made aware that creating a PR using LLM made fixes is not the best route. I'll try and add things like this in less direct ways like raising an issue / ticket in the future. Please do close this PR if it's considered bad practice. I'm not looking for credits, just wanted to share / give back what i thought was a small fix.

[stephenhensley]

My initial hesitance to patch this when #634 was created was because a device (bluemchen, I believe -- a reprogrammable 4hp eurorack module with small OLED was the initial project that added the explicitly different column-start.
Having never had that size display I wasn't sure that fixing it for one wouldn't break it for the other.

Given that this PR ends up making the high_column_addr the same regardless of display height, and there is at least one case, where that value needs to be different, it might be better to add a new member to the Config struct that defaults to 0x10, but could be set externally without having to edit the library for cases where it does need to be different.

The burst write looks like a nice improvement. I don't have an I2C OLED to test with at the moment, but it looks okay to me.

Re. LLM usage: I don't have any particular issue with contributors using LLMs to help them fix problems, write code, etc.
I think there is a distinction between that, and sending agents off to make untested contributions places, etc.
You're more than welcome to make PRs like these.

[jonwaterschoot]

I've spent some more time with Claude going through this. Built a test to verify. Here's the summary:

Looks like you were right to hesitate. I traced that line back to the commit that added it: it came in with #326, and that PR's description gives the reason:

"My primary motivation ... is my kxmx_bluemchen Eurorack module, which makes use of a 64x32 SSD1306 OLED via I2C."

So 0x12 is correct for bluemchen, and my first patch would have indeed broken it.
In LLM's words:

64-column modules are bonded to the middle 64 columns of the controller's 128-column RAM, so they need a 32-column offset (0x10 | 32 >> 4 = 0x12). The bug is only that the switch keys on height alone, so 128x32 panels get the offset too. There's already precedent for the distinction a few lines up — Init() does if(width == 64) inside case 32: for the COM-pins command. The column-start line never got the same treatment.

The general rule, for what it's worth: offset = (controller RAM columns − panel width) / 2, with 128 columns on the SSD1306/SSD1309 and 132 on the SH1106. That gives 0 for 128x32, 32 for 64x32, and 2 for an SH1106 128x64 — which is exactly the 0x02 lower-column start oled_sh1106.h already sends.

So the new commit does both things you asked for: the default is derived from geometry (0x12 only for 64x32, 0x10 otherwise) and it lives in Config, overridable per instance without editing the library.

uint8_t high_column_addr = (width == 64 && height == 32) ? 0x12 : 0x10;

Config stays an aggregate under gnu++14, and every in-tree call site default-constructs then assigns, so nothing existing changes. Same treatment for SSD1307Driver's Update() and TransferPageDma() so both drivers are configured the same way, though no SSD1307 alias is 64x32 today.

Tested on a 128x32 I2C panel, using the stock SSD130xI2c128x32Driver:

Left is the patched default. Right is the same build with high_column_addr forced to 0x12 — byte-for-byte what master currently sends for any height==32 panel — and it reproduces #634 exactly: content 32 columns right, the last 32 columns wrapped back to the left edge. So that one image is both the bug reproduced and confirmation that the Config override actually reaches the panel, which is the escape hatch bluemchen needs.

I don't have a 64x32 , or any other OLEDs for that matter, to verify e.g. bluemchen directly, which is precisely why the default is derived from geometry rather than flattened to 0x10.

The burst write is exercised on the same hardware by the firmware this came out of.

Two things Claude found but I deliberately left this alone, your call on both:

oled_sh1106.h:24 has a fourth copy of the same case 32: 0x12. Nothing is broken today since both SH1106 aliases are 128x64, so it's latent.
By the rule above, the 64x48 aliases would want 0x12 and currently get 0x10.

No one seems to have reported it, so I'd rather not change untested behaviour on a guess rather just mention it.

Re. LLM usage, appreciated, thanks for saying so. Still, I'm out of my comfort zone, and I hope I was thorough enough with the tests to substantiate the PR.