Skip to content
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

Allow space character (ascii 32) in all fonts #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bxparks
Copy link

@bxparks bxparks commented Feb 5, 2023

If a font does not have an encoding for the space character, it will normally be skipped and nothing is printed. Calling alwaysAllowSpace(true) causes the space character to be printed for all fonts. The width of the space character will be the FONT_WIDTH of the font.

Additional PR Notes

I wonder if you would consider merging this feature. I've been carrying this patch in my fork for a few years. With the recent change to MIT License, I'd like to upstream it to eliminate the git rebase conflicts that I sometimes have to deal with.

This will be most useful for fixed-sized fonts when: (1) Printing numbers which are right-justified with leading spaces instead of leading zeros; (2) Blinking letters by overwriting them with the space character.

If a font does not have an encoding for the space character, it will normally
be skipped and nothing is printed. Calling alwaysAllowSpace(true) causes the
space character to be printed for all fonts. The width of the space character
will be the FONT_WIDTH of the font.
@machmar
Copy link

machmar commented Feb 6, 2023

Hi, from a recent pull request:

I don't use pull requests since development is not in this repository.

(link to the message)
It is possible he'll add it to the next release.

@greiman
Copy link
Owner

greiman commented Feb 6, 2023

I will put it on my list. May be a long time if I decide to do it.

@bxparks
Copy link
Author

bxparks commented Feb 6, 2023

I don't use pull requests since development is not in this repository.

Yes, that was my pull request, and I don't understand the purpose of that comment. Does it mean that:

  1. This project does not accept any PRs? Or
  2. The maintainer wants the PR in a different form, like a diff, or a git diff format? Or
  3. The maintainer will manually copy the diff over to their private source control tool?, Or
  4. The maintainer will implement the feature on their own, Or
  5. Something else?

I am using the GitHub Pull Request as a tool for communicating the proposed change. If the maintainer does not want to receive PRs, it would be useful to write that explicitly in the README.md.

@machmar
Copy link

machmar commented Feb 6, 2023

It means that pull request will not be merged. Check the link I attached bellow the quote.
I'm sorry for making mess in this, I just saw this reply from the author on another pull request and wanted to be helpful.

@greiman
Copy link
Owner

greiman commented Feb 7, 2023

I decided to add non-font space. Here is a zip with my mod. Please test it.
SDD1306Ascii.zip

Here is the diff:

C:\Users\Bill>diff  old\SSD1306Ascii.cpp new\SSD1306Ascii.cpp
340,341c340,347
<   // Error if not in font.
<   if (ch < first || (first + count) <= ch) {
---
> #define ENABLE_NONFONT_SPACE 1  // This will be moved to SSD1306Ascii.h
>   bool nfSpace = false;
>   if (first <= ch && ch < (first + count)) {
>     ch -= first;
>   } else if (ENABLE_NONFONT_SPACE && ch == ' ') {
>       nfSpace = true;
>   } else {
>     // Error if not in font.
344d349
<   ch -= first;
347c352,354
<   if (fontSize() < 2) {
---
>   if (nfSpace) {
>     // non-font space.
>   } else if (fontSize() < 2) {
371c378
<         uint8_t b = readFontByte(base + c + r*w);
---
>         uint8_t b = nfSpace ? 0 : readFontByte(base + c + r*w);

I decided to have it enabled by default since it only adds a few bytes.

Here is my test program:


// Test for non-font space

#include <Wire.h>
#include "SSD1306Ascii.h"
#include "SSD1306AsciiWire.h"

// 0X3C+SA0 - 0x3C or 0x3D
#define I2C_ADDRESS 0x3C

// Define proper RST_PIN if required.
#define RST_PIN -1

SSD1306AsciiWire oled;

void setup() {
  Wire.begin();
  Wire.setClock(400000L);

#if RST_PIN >= 0
  oled.begin(&Adafruit128x64, I2C_ADDRESS, RST_PIN);
#else // RST_PIN >= 0
  oled.begin(&Adafruit128x64, I2C_ADDRESS);
#endif // RST_PIN >= 0
  oled.setFont(lcdnums14x24);
  oled.clear();
  oled.print("12 34");
}

void loop() {}

Edit: I compared program size with #define ENABLE_NONFONT_SPACE 0 And the AVR compiler is smart enough to give the same size as the unmodified version.

@bxparks
Copy link
Author

bxparks commented Feb 11, 2023

It works fine. You have a bunch of trailing whitespaces that you probably want to remove.

Are you going to require ENABLE_NONFONT_SPACE to be defined to enable this, or hardcode that to 1? It's more cumbersome to require that feature flag, since I have <SSD136AsciiXXX.h> included in multiple *.cpp and *.h files, so I have to add the #define ENABLE_NONFONT_SPACE 1 in multiple places. It wouldn't be a problem if the Arduino IDE provided the ability to define a global -D MACRO=value flag, but it doesn't.

@greiman
Copy link
Owner

greiman commented Feb 12, 2023

I plan to just define ENABLE_NONFONT_SPACE in SSD1306Ascii.h with:

#ifndef ENABLE_NONFONT_SPACE 
#define ENABLE_NONFONT_SPACE 1
#endif  // ENABLE_NONFONT_SPACE 

I don't think many people will need to disable it since the feature take about 32 bytes of flash in AVR and is only is invoked for fonts without a space and only the user writes a space.

The trailing whitespaces will vanish and the format may change when I use clang-format. Also I will make any changes found by cppcheck or Google cpplint.

I may put all option #defines between #ifndef #endif. This has been very popular in my other libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants