-
Notifications
You must be signed in to change notification settings - Fork 216
ssd1306: avoid unnecessary heap allocations #767
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
base: dev
Are you sure you want to change the base?
Conversation
It is seems to me Buser should have a command() method to avoid the big buffer juggling since commands always write a single byte. type Buser interface {
// ... other methods
command(cmd byte) error
}
func (b *I2CBus) command(cmd byte) error {
b.cmdbuf[0] = 0x00 // cmdbuf is only length 2 on I2CBus since SPI does some DC pin stuff. in place of 0x40/0x00 byte
b.cmdbuf[1]= cmd
return b.wire.Tx(b.Address,b.cmdbuf[:],nil)
} This will likely make it much more readable and easier to understand |
1fac33f
to
fd49267
Compare
Tested using my Thumby using SPI display. Before:
After:
|
Also I switched the target branch to |
Thank you, @deadprogram for testing! |
Tested using my RP2040-Zero using I2C display (128x64)
|
0cbe343
to
923f36d
Compare
The lost in refactoring exported methods |
923f36d
to
8441db8
Compare
@@ -0,0 +1,52 @@ | |||
package common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things:
common
is not a good package name (yes, I may be guilty of doing the same in CYW43439, but I purposefully made it unimportable!)- You've made a brand new common package just ot abstract away a very vital part of the example code, which was serving the purpose of an example, by demonstrating how the API can be used. I feel like 50 lines of code does not justify a new package, especially if it is not reusable code, this is just the example code copy-pasted over...
I do understand it is very uncomfy to write multiple examples with the same logic- but I'm pretty sure we can leverage build tags to have the shared logic in the same file and various surrounding files with the specific MCU definitions and probably a instantiateSSD1306
function that returns a initialized SSD1306 device ready to use.
Please revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no build tags to use, these displays can be attached to any MCU.
First, I can have a global var and a README file with description how to use -ldflags
.
Second, alternatively, invent some build tags like "ssd1306_i2c_128x32" and describe use in README.
Both alternatives are hardly more friendly to newcomers though...
Third alternative is to duplicate the code <- suboptimal for maintenance, but doable.
Forth alternative is to revert changes in examples altogether loosing HeapInUse printout <- sort of defies purpose, as it will be harder to verify the PR does eliminate heap allocations.
Choose your poison, @soypat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so we single out "thumby" and leave the screen size editable by users in other cases.
Probably, good to have an example for non-thumby SPI, but that's details.
Thanks for taking time and making demo, let me adjust my code now.
This re-uses internal buffer to avoid unnecessary heap allocations in
ssd1306
driverSwitches away from the legacy
I2C
interface.See similar: #766
Draft since not tested in all configurations (I2C and SPI)Tested with both I2C and SPI now.No unnecessary allocations and better FPS, w/o drops on GC, see results below.
Tested on XIAO BLE (nRF52840 chip) and 128x32 display connected via I2C at 400KHz.
Before change:
After change: