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

Add small 5x3 font #3337

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ItRainsSmiles
Copy link

In order to support more shown digits on small matrix sizes, the font arsenal has been expanded to a tiny 5x3 font.

This font's main strengths are the digits, which have been edited for a tiny but clear appearance, which is very useful for a clock.
There are no free spaces between the digits.

@softhack007 softhack007 requested a review from blazoncek August 23, 2023 00:25
wled00/FX.cpp Outdated Show resolved Hide resolved
wled00/FX.cpp Outdated Show resolved Hide resolved
wled00/FX.cpp Outdated Show resolved Hide resolved
wled00/FX.cpp Outdated
@@ -7332,7 +7333,7 @@ uint16_t mode_2Ddistortionwaves() {
uint16_t cy1 = beatsin8(15-speed,0,rows-1)*scale;
uint16_t cx2 = beatsin8(17-speed,0,cols-1)*scale;
uint16_t cy2 = beatsin8(14-speed,0,rows-1)*scale;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :) @softhack007

wled00/FX.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @softhack007 to remove all changes not related to the added font in Scrolling text mode.
Also reduce reducing the size of font array would be wise as characters above 126 are not supported (only ASCII are) by the display function..

wled00/src/font/clear_3x5.h Outdated Show resolved Hide resolved
@blazoncek
Copy link
Collaborator

Please do not force push any more commits.

@ItRainsSmiles
Copy link
Author

I agree with @softhack007 to remove all changes not related to the added font in Scrolling text mode. Also reduce reducing the size of font array would be wise as characters above 126 are not supported (only ASCII are) by the display function..

Done :) @blazoncek

@ItRainsSmiles
Copy link
Author

Please do not force push any more commits.

okay :) @blazoncek

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Font still includes characters beyond 0x7F.

0x80, 0x80, 0x00, 0x80, 0x80, /* 0x7C bar */
0xC0, 0x40, 0x20, 0x40, 0xC0, /* 0x7D braceright */
0x60, 0xC0, 0x00, 0x00, 0x00, /* 0x7E asciitilde */
0x80, 0x00, 0x80, 0x80, 0x80, /* 0xA1 exclamdown */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fonts from here on are unnecessary and only consume flash space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blazoncek unnecessary characters got removed :)

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Copy link
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now, thanks.
As the author says he edited many characters himself, I think we can accept the licensing situation as-is.
Approved from my side 👍

@softhack007
Copy link
Collaborator

@blazoncek I guess this one is low-risk so it could go into -b5, what do you think?

@blazoncek
Copy link
Collaborator

Yes, but we also need to see the increase of binary size (you may have issues with plenty of usermods, etc).

Copy link
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small change to make sure the char array has enough entries.

0xA0, 0xE0, 0xE0, 0xE0, 0x00, /* 0x77 w */
0xA0, 0x40, 0x40, 0xA0, 0x00, /* 0x78 x */
0xA0, 0xA0, 0x60, 0x20, 0x40, /* 0x79 y */
0xE0, 0x60, 0xC0, 0xE0, 0x00, /* 0x7A z */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you deleted a few too much now...
WLED expects the array to have all chars from 0x20 thru 0x7E

if (chr < 32 || chr > 126) return; // only ASCII 32-126 supported

So your table needs to have everything up to 126 = 0x7E.
Please re-add the missing 4 chars. We want to avoid crashing due to array-out-of-bounds read.

@softhack007
Copy link
Collaborator

Just checked Flash size increase from this PR:

esp8266: Flash used +490 bytes
esp32: Flash used +488 bytes

@blazoncek
Copy link
Collaborator

@ItRainsSmiles can you please rebase this PR for 0_15 branch?

@blazoncek blazoncek linked an issue Feb 9, 2024 that may be closed by this pull request
@blazoncek blazoncek added keep This issue will never become stale/closed automatically waiting for feedback addition information needed to better understand the issue labels Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effect enhancement keep This issue will never become stale/closed automatically waiting for feedback addition information needed to better understand the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DIGITS CLOCK / New Font?
3 participants