Fix SSD1306 128x32: wrong column-start address - #707
Open
jonwaterschoot wants to merge 2 commits into
Open
Conversation
Fixes electro-smith#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. Confirmed independently by three reports on electro-smith#634 (including a maintainer) against SSD130x4WireSpi128x32Driver; the same class also backs the I2C 128x32 alias. Also fixes the same line in SSD1307Driver::TransferPageDma() for consistency, though no public alias currently instantiates that class at height==32.
SSD130xI2CTransport::SendData() issued a full I2C transaction (START+address+ACK+STOP) per byte -- 512 transactions for one full 128x32 frame (4 pages x 128 bytes), plus ~12 more for per-page setup commands. The SSD1306 auto-increments its column pointer for every data byte that follows a single 0x40 prefix within a transaction, so there's no need to restart the bus per byte. Batches a page's data into one transaction (page bytes + the 0x40 prefix); measured on a 128x32 panel, this cut a full-screen Update() from ~30-40ms to a few ms. Separate from the column-start fix in the previous commit -- this is a transport-layer performance change, not a correctness fix, so it's easy to drop if unwanted.
Author
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #634.
SSD130xDriver::Update()(and the equivalent switches inSSD1307Driver) set the page column-start command to0x12(column 32) forheight==32panels instead of the standard0x10(column 0) used by thedefaultcase 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 (
SSD130x4WireSpi128x32Driveris an alias for this sameSSD130xDriverclass, just over SPI instead of I2C) and that @stephenhensley confirmed hitting independently. Also fixes the same line inSSD1307Driver::TransferPageDma()for consistency, though no public alias currently instantiates that class atheight==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.