ThingPulse / esp8266-oled-ssd1306

Driver for the SSD1306 and SH1106 based 128x64, 128x32, 64x48 pixel OLED display running on ESP8266/ESP32
https://thingpulse.com
Other
2k stars 638 forks source link

Allow overriding default font #238

Closed smurf0969 closed 5 years ago

smurf0969 commented 5 years ago

Some small changes that allow changing the default font by setting a define.

helmut64 commented 5 years ago

I created a branch of the OLED library and added a lot of enhancements including mbed-os support in addition to Arduino support. All enhancements should be backward compatible. As ThingPulse does not work on merges/enhancements for some time, I added your enhancement to my branch: https://github.com/helmut64/OLED_SSD1306/tree/mbed_os_support

I merged your pull request into my branch (mbed_os_support ) and hope that my branch gets adapted by ThingPulse, otherwise I continue with the OLED library on github account.

Merged pull request #238 to overwrite the default font. I believe this is a very important feature because it is the memory requirement for a font never being used. I do it a little bit different, the init() functions allows an optional parameter with the default font,

marcelstoer commented 5 years ago

@helmut64 same comment from my side as with #247.

I do it a little bit different, the init() functions allows an optional parameter with the default font

Why XOR instead of both? IMO it's useful to be able to set the default font on init and through dedicated function. We could merge this one and then layer your addition on top of it in a follow-up PR, no?

helmut64 commented 5 years ago

@marcelstoer at present the default font ArialMT_Plain_10 (2731 bytes in flash) is referenced and therfore eats this flash memory, even you you never use it. A dedicated function will not work because:

PS: My friend from Arduino Hannover has use cases for Flipdot displays which works with the AVR therefore flash memory is limited. The Flipdot also uses optimized custom fonts (e.g. 8x7)

https://www.youtube.com/watch?time_continue=2&v=Qho6l2jwI9w

marcelstoer commented 5 years ago

I see your point. I didn't know that you're primarily concerned about memory consumption.

A dedicated function will not work

Well, it will work but it comes at the cost of potentially wasting some memory as you correctly pointed out. In some setups this may be acceptable, in others it won't.

So, it'd be totally ok for me to drop this PR and accept a new one from you that implements the optional init() param.

helmut64 commented 5 years ago

Sounds good and should work as well for smurf0969.

marcelstoer commented 5 years ago

a new one from you that implements the optional init() param.

Hadn't realized that this feature is already in your Mbed OS branch (#243).