TreeFallSound / pi-stomp

pi-stomp is a DIY high definition, multi-effects stompbox platform for guitar, bass and keyboards
https://treefallsound.com
GNU General Public License v3.0
106 stars 19 forks source link

Oled Display #5

Closed micahvdm closed 3 years ago

micahvdm commented 3 years ago

I have ported lcdgfx.py to lcd128x64.py and tested it on a small SSD1306 display. All seems to be working. Can you please test with your hardware if the display works, if you have one? (I know the writing is tiny, but once the 2,42" Oled comes in, it will be much better).

rreichenbach commented 3 years ago

I don't believe copy/paste of nearly all the lcdgfx.py contents to support each new display is a scalable solution. It will become unmaintainable very quickly. Perhaps one implementation per aspect ratio makes sense, but I'm near certain an OLED with the exact same aspect ratio as GFX could share 95%+ of the code. The remaining code is simply that which is unique to the display driver (GFX, ssd1306, ili9341, etc.). It's even possible that any display with a width of 128 could share nearly all the code. A shorter display could just choose to not display specific zones.

I'm open to ideas here, but very reluctant to have a unique implementation for each new display.

micahvdm commented 3 years ago

I wasn’t going for different, just a direct port! The calls are almost identical. I haven’t yet tested with spi, as I don’t have an spi oled. I agree that all common code could be in something like the lcd.py file. Would simplify adding more graphic type displays. Working on the ili9341 now, but that’s gonna take quite a bit more work due to the nature of the display.

rreichenbach commented 3 years ago

A direct port is still two copies of the same thing. Every time a change is made to one, it has to be made to both. And then what about the next display, now we have 3. That's not sustainable or easily maintainable.

I'm just asking that before we have multiple copies of rather intricate code, we take a step back and figure out how to do things right, to make maintenance and addition of new displays easiest. If you don't want to go that way and just wanna just crank out something to allow you to support whatever display you have, maybe it's ok for now. But I won't let this become a copy/paste mess.

If you don't want to do a little refactor here, let me know, and I'll produce something in the next 24 hours.

micahvdm commented 3 years ago

To be honest, my python knowledge is not the greatest...I’m still figuring it all out, but I’ve been studying your code extensively. I’m starting to figure it all out. I know that’s not much help, but it’s the best I can offer right now. I will get better at this though as I think it’s a fantastic project! Im not sure where to start with the refactor, so maybe it’d be best if you look into that. I’m also looking into getting the file manager working in Modep as I’ve found a cab sim IR loader that will be magical! Spreading myself out, but it’s how my ADD brain works best...

rreichenbach commented 3 years ago

I totally appreciate your enthusiasm! And yes, an IR loader would be sooooo awesome!
I realize I started the bad practice of throwing in all of those stubs for each display. So how 'bout this? I'll spend some time today refactoring common display code (likely into lcd.py) and making it so that we'll just need one subclass per aspect ratio. Then, I'm fairly certain, the unique code per new aspect ratio or display driver would be just a small handful of methods. You could go from there and add whatever displays you want. I have 4 different displays currently to test with, so assuming they all still work, I'll hopefully have a PR for you to review tomorrow. Sound ok?

micahvdm commented 3 years ago

Sounds fantastic! I currently only have oled and ili9341, so that’s what I’ll focus on. The IR loader will be so good! I have also built all of the stock mod plugins with their modgui’s if you’re keen for that?

rreichenbach commented 3 years ago

Since you got this working for that OLED and I can't test it, I'm gonna merge it and then apply my refactor after.