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 support for large font. #226

Closed

Conversation

francoisgeorgy
Copy link

Here is my proposal for a firmware upgrade to support large fonts.

I put the code in the experimental folder because I'm still very new in this project and my code probably needs some work before being merged in the firmware.

I included four fonts that, imho, display nicely in he EuroPi OLED. Of course, other truetype fonts can be imported with the script mentioned in the largefont_writer.py module.

To use a large font, simply add the font= argument to either Display.text() or Display.centre_text() methods. Example :

import freesans17
oled.centre_text('Hi folks', font=freesans17)

Regarding the centre_text() method, I paid attention not to introduce any regression. I tested the updated method and the current one with the 8x8 monospaced font to be sure there was no differences. There is only one change: the current code allows a text of four lines to be displayed, the first line will be clipped. The new version only allows three lines, like the specifications says. This change comes from using an integer division // instead of round() when computing the number of lines.

I included the contrib/largefont_demo.py script that allows to quickly check if the large font support is working or not. It displays a text using the regular 8x8 font and the four new FreeSans large fonts. The display loops between these five fonts. The screen border is also alternatively displayed in order to get a better overview of how the fonts are aligned.

francoisgeorgy and others added 14 commits February 15, 2023 13:45
- introduce ConfigPoints and ConfigPointsBuilder classes
- make configurable the value that the TM write switch writes
- add config point for pico board type
fall back to defaults properly
add int config point
add tests for different config point failures
make the TM pulses bits configurable
move europi_config.py to the firmware
diagnostic temp units are configurable
also black format diagnostic
- removed builder
- simplified EuroPiScript.config_point()
- introduced ConfigPoint class
- rename config_points.py -> configuration.py
- move configPoint helpers out of ConfigSpec class
- move europi config to its own var, this removes the need care about conflicts and namespacing
- only load config points that exist in the spec
- take a range object for IntegerConfigPoint instead of recreating its api
- europi_script validates config when loading it
- move generic file utils out of europi_script
- moved config file methods out of EuroPiScript
add config point for cpu freq
add make target to copy config files
* Initial commit of a sequential switch implementation

* Put the screensaver timeout back in; it got accidentally deleted

* Add the logic script

* Copypasta

* Remove the sequential switch script

* Replace comment blocks with docstrings, small code refactoring

* Change the order of Ain and Din to match the orientation on the panel instead of being alphabetical

---------

Co-authored-by: Chris I-B <c.iverachbrereton@gmail.com>
self.writers[n] = Writer(self, font)
return self.writers[n]

def text(self, s, x, y, c=None, font=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is here a c parameter again? It's not used later in the function?
-edit:
super().text() does accept a c (color) parameter: FrameBuffer.text(s, x, y[, c])

Copy link
Author

Choose a reason for hiding this comment

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

The c parameter is there to keep the method signature similar to the one in the FrameBuffer class. I left this c parameter in case someone uses code that expect the FrameBuffer.text(...) method signature.

Copy link
Contributor

@redoxcode redoxcode left a comment

Choose a reason for hiding this comment

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

I think this should be implemented in a way that still enables the color parameter to work.
Please see my suggestions.

software/firmware/europi.py Outdated Show resolved Hide resolved
software/firmware/experimental/largefont_writer.py Outdated Show resolved Hide resolved
francoisgeorgy and others added 9 commits February 21, 2023 21:53
Co-authored-by: redoxcode <117983047+redoxcode@users.noreply.github.com>
Co-authored-by: redoxcode <117983047+redoxcode@users.noreply.github.com>
…seful when using a custom font which may not be monospaced.

The import for largefont_writer is changed too, the largefont_writer module must be placed in lib/.
Co-authored-by: redoxcode <117983047+redoxcode@users.noreply.github.com>
Co-authored-by: redoxcode <117983047+redoxcode@users.noreply.github.com>
…seful when using a custom font which may not be monospaced.

The import for largefont_writer is changed too, the largefont_writer module must be placed in lib/.
…ont_support

# Conflicts:
#	software/firmware/europi.py
freq(OVERCLOCKED_CPU_FREQ)

# Reset the module state upon import.
reset_state()
Copy link
Contributor

@redoxcode redoxcode Feb 23, 2023

Choose a reason for hiding this comment

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

Some how github shows the whole file as changed in the div?
Maybe because of changed line ending as described here?
Maybe we should fix this as it might mess up things for other users merging this PR into their forks and people doing the next PR?

Copy link
Author

Choose a reason for hiding this comment

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

I rebased my PR to get the last updates from the main branch.
The line ending problem is unfortunately a common one. Maybe it's on my side because I'm using both a Mac and Linux box and, since I just replaced my Mac with a new Mac Mini, I forgot to check my git config on this new machine.

We could delete this PR and I could re-create a new one. Maybe it's the simplest solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. Maybe I can try to make a suggestion that fixes this...

Co-authored-by: redoxcode <117983047+redoxcode@users.noreply.github.com>
@francoisgeorgy
Copy link
Author

Looks good now. Thank you for your help!

@redoxcode
Copy link
Contributor

Glad that worked. LGTM now

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

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

Hello @francoisgeorgy thanks for the PR. The addition of some new fonts has been asked for before and would go a long way towards making the EuroPi UI more flexible.

One problem I'm having, however, is that the PR contains a bunch of unrelated commits that are making it difficult to identify your actual changes. Most of these seem related to the config changes, but there are others as well.

It would be helpful to have these extra commits removed before doing an in depth review of your changes. If you need help with the git actions to make this happen, please feel free to reach out.

@francoisgeorgy
Copy link
Author

Hi @mjaskula . You re right, this PR has become a mess. I created it just before the big changes related to the new configuration feature. And, on top of that, we had a linebreak issue.
I'll try to rebase it. Or maybe it's simpler to trash it a simply create a new PR? Do you have a preference?

@mjaskula
Copy link
Collaborator

Hi @mjaskula . You re right, this PR has become a mess. I created it just before the big changes related to the new configuration feature. And, on top of that, we had a linebreak issue. I'll try to rebase it. Or maybe it's simpler to trash it a simply create a new PR? Do you have a preference?

Whichever is easier for you is fine.

@francoisgeorgy
Copy link
Author

Hi,
I tried to rebase my PR and cherry-pick only the commits related to the large_font changes, but this created event more commits. I probably made a mistake somewhere.

I think creating a new PR is the simplest solution, but I fear we my lose all the history of the current PR.

If you know how to clean the current PR I'd would be more than happy to follow your instructions. Otherwise I propose that we close the current PR and I open a new one.

@mjaskula
Copy link
Collaborator

mjaskula commented Mar 1, 2023

Hi, I tried to rebase my PR and cherry-pick only the commits related to the large_font changes, but this created event more commits. I probably made a mistake somewhere.

I think creating a new PR is the simplest solution, but I fear we my lose all the history of the current PR.

If you know how to clean the current PR I'd would be more than happy to follow your instructions. Otherwise I propose that we close the current PR and I open a new one.

I tried looking into it and repairing it on my local checkout, but I do not understand when GH is showing those commits. I think that we might as well cut our losses and open a new PR. You can put links between the new and old PRs such that the history is not lost. It will be more inconvenient, but not lost.

@francoisgeorgy
Copy link
Author

This PR is replaced by #234.

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

4 participants