Skip to content

Conversation

@n1LS
Copy link
Contributor

@n1LS n1LS commented Nov 3, 2025

fixes #885

  • adds a 3rd font
  • updates the font data selection in chargfx.c
  • unifies the references to the font names and count in Configuration.cpp/ThemeView.cpp

@maks maks changed the title Feature/wide font wide font Nov 4, 2025
Copy link
Collaborator

@democloid democloid left a comment

Choose a reason for hiding this comment

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

I don't see a reason to split the fonts into different files and some copyright notices get lost on the way, can we revert that change and keep everything on one file?

Can you also document on the description of this PR how it affects the flash usage?

@maks
Copy link
Collaborator

maks commented Nov 4, 2025

@n1LS could I also ask if you could attach here a image of what the entire font looks like now in the current state of the PR code as you previously did on comments in the associated issue or is the image in #885 (comment) the latest version that matches what is in this PR?

@n1LS
Copy link
Contributor Author

n1LS commented Nov 5, 2025

@democloid: Sure thing, will do.
My thought process: Considering you are enforcing an 80-col limit in 2025 it seemed reasonable to split a 350-line header file, that'll only get longer with the added special characters ;-) Also it's generated resources - not code and should imo be separated to be one resource per file (easier tracking of changes, easier to regenerate, shorter files, ...)

Flash usage:

Building master:

Memory region         Used Size  Region Size  %age Used
           FLASH:      675004 B         2 MB     32.19%

Building feature/wide-font:

Memory region         Used Size  Region Size  %age Used
           FLASH:      676412 B         2 MB     32.25%

PR increases flash usage by 1408 B (1536 B would be the raw font data)

@maks Here's a render of the font as it's currently included:
font-ascii

@maks
Copy link
Collaborator

maks commented Nov 5, 2025

Brill, thanks @n1LS !
In regards to the header file, we're not likely to ever change the existing fonts and quite unlikely to add more fonts (not including the upcoming extended chars) so I think its not a big concern to have it all in one file.

My one last bit of feedback is a personal aesthetic one: to me the lower case f looks rather out of place with the rest of the font. Otherwise its a ✅ for me.

@n1LS
Copy link
Contributor Author

n1LS commented Nov 5, 2025

f

@maks Pick your poison :-)

@n1LS n1LS requested a review from democloid November 6, 2025 08:05
@maks
Copy link
Collaborator

maks commented Nov 6, 2025

f

@maks Pick your poison :-)

For me personally I prefer number 3, but really would be ok with any of them.

@n1LS
Copy link
Contributor Author

n1LS commented Nov 6, 2025

font-ascii

Updated to reflect the current state of the code.

@n1LS n1LS force-pushed the feature/wide-font branch from 6e6effd to 9b8a8ec Compare November 6, 2025 08:34
@maks maks self-requested a review November 6, 2025 09:49
Copy link
Collaborator

@maks maks left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Collaborator

@democloid democloid left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@maks maks merged commit cc194ee into xiphonics:master Nov 7, 2025
1 check passed
@n1LS n1LS deleted the feature/wide-font branch November 30, 2025 19:57
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.

Add new (wider) font

3 participants