search

dwhinham / mt32-pi

🎹🎶 A baremetal kernel that turns your Raspberry Pi 3 or later into a Roland MT-32 emulator and SoundFont synthesizer based on Circle, Munt, and FluidSynth.
https://twitter.com/d0pefish
GNU General Public License v3.0
1.26k stars 79 forks source link

Update ssd1306.cpp #285

Closed ahmadexp closed 2 years ago

ahmadexp commented 2 years ago

add support for SSD1305

ahmadexp commented 2 years ago

@dwhinham, would you please consider this merge. This merge allows us to support the SSD1305 OLEDs

dwhinham commented 2 years ago

There is no need to ping me, I can see the PR just fine.

I have been sick with COVID for the last week and am starting to catch up.

There is zero documentation with this PR. It would have been nice to see some explanation of what it does and why you made the changes. At bare minimum it should contain an explanation of how you have tested it.

I will not be able to test this screen as I do not own the hardware. I will merge it if we are 100% sure it won't break other screens.

ahmadexp commented 2 years ago

I have the explanation on the wiki side. I have tested it and I can assure it does not break anything.

dwhinham commented 2 years ago

I really don't like the way we are abusing the width parameter as a magic number to detect this display controller, as the width/height parameters in the config file are meant to represent how many user-facing pixels there are, not the size of the OLED's internal framebuffer. Hence, this commit breaks the pattern.

I will merge a similar commit to get the screen working for now, with additional notes to revisit this and try to clean it up in future.

I see the wiki content you have added, but the information about hacking the HATs etc. should probably should be moved to a separate page. It is useful information, but the page is meant to document displays only, so stuff about hacking the button wiring etc. is making it cluttered. I'll amend this shortly.

ahmadexp commented 2 years ago

For sure. I definitely agree with you. My thought was that making an entirely new file for a single value change was not reasonable.