Skip to content

Conversation

@smurf0969
Copy link

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

Added a global to allow changing of the default font
Changed setFont to use global OLEDDISPLAY_DEFAULTFONT
Changes due to travis ci warnings
helmut64 added a commit to helmut64/OLED_SSD1306 that referenced this pull request Jun 2, 2019
…default font.

I believe this is a very important feature because it eliminates
the memory requirement for a font never being used.
@helmut64
Copy link
Contributor

helmut64 commented Jun 2, 2019

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
Copy link
Member

@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
Copy link
Contributor

helmut64 commented Jun 3, 2019

@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:

  • The flash memory of the ArialMT_Plain_10 is referenced and therefore is gets linked into the app, even if you set this later via a setDefaultFont() function after the constructor.
    Keeping the fontData with a NULL pointer, it will crash if users don’t call setDefaultFont().
    Therefore my idea was to pass it as a default parameter in init().

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
Copy link
Member

marcelstoer commented Jun 3, 2019

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
Copy link
Contributor

helmut64 commented Jun 4, 2019

Sounds good and should work as well for smurf0969.

@marcelstoer marcelstoer mentioned this pull request Jun 4, 2019
@marcelstoer marcelstoer closed this Jun 4, 2019
@marcelstoer
Copy link
Member

marcelstoer commented Jun 4, 2019

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants