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

Auto generate font #1097

Merged
merged 31 commits into from May 10, 2022
Merged

Auto generate font #1097

merged 31 commits into from May 10, 2022

Conversation

yehoshuapw
Copy link
Contributor

closes #964 (supersedes)

this adds a script src/displayapp/fonts/generate.py to generate the font files (using lv_font_conv) from a singular config file (src/displayapp/fonts/fonts.json).

the json has each font, and it's relevant options (size,bpp,compress, and "patches") and sources.
in addition there's also a "feature" feature, allowing extra sources with a extra flag (and the config has an example usage of adding hebrew, which won't do anything unless -e hebrew is given)

currently, this manages to generate exactly the same as what is already in place.

TODO:
integrate this into the CMake (and add lv_font_conv to the build images).
(my CMake-fu is not that strong, so I'll get to that tomorrow - but opening this to get comments about the rest of it from anyone interested)

@yehoshuapw
Copy link
Contributor Author

@Riksu9000 i'd like to get your thoughts about whats already here. thanks!

@Riksu9000 Riksu9000 self-requested a review April 18, 2022 17:58
@Riksu9000 Riksu9000 changed the title Audo generate font [WIP] Auto generate font [WIP] Apr 19, 2022
Copy link
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

I like this solution. The README should be updated as a part of this PR by removing the old instructions and writing something about this new method.

src/displayapp/fonts/generate.py Outdated Show resolved Hide resolved
src/displayapp/fonts/generate.py Outdated Show resolved Hide resolved
src/displayapp/fonts/fix_jetbrains_mono_bold_20_zero.py Outdated Show resolved Hide resolved
src/displayapp/fonts/fonts.json Outdated Show resolved Hide resolved
@Riksu9000 Riksu9000 added this to the 1.10.0 milestone Apr 19, 2022
@yehoshuapw yehoshuapw force-pushed the audo-generate-font branch 2 times, most recently from 797c74d to ebe42a4 Compare April 20, 2022 17:59
@yehoshuapw
Copy link
Contributor Author

and now the font .c files are generated at compile time with CMake.

@yehoshuapw yehoshuapw changed the title Auto generate font [WIP] Auto generate font Apr 20, 2022
@Riksu9000
Copy link
Contributor

I've been thinking that the font "features" probably isn't going to be very useful to us, since we only ship a single firmware, and that's likely not going to be changing. The other use case would be if fork maintainers pushed their changes in this repo, which I don't think should be our responsibility to handle either.

Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, that's definitely something I want to add tot he project!

The generation of the fonts in CMake and then in Python and in Github Actions look good to me!

If you don't mind, I think we should make 2 addition:

  • Add lv_font_conv (and how to install it via npm) as a needed dependency to build the project in doc/buildAndProgram.md as the project won't build without it.
  • Add the installation of npm and lv_font_conv in the Dockerfile so we can build the fonts inside the docker container. Note than I plan on switching the CI to docker so that we won't have to duplicate those changes in the future ([WIP] Github Action with docker to build the firmware #1045).
  • Oh and also maybe specify which version of lv_font_conv we support, and enforce this version in Docker/Action workflows to avoid broken builds because of a change in the tool ?

I can help with those points if you want to ;)

src/displayapp/fonts/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

I noticed that changing the font configuration will regenerate all the fonts, even if only one was changed. Not a big issue, but maybe worth considering if something can be done about that.

src/displayapp/fonts/README.md Outdated Show resolved Hide resolved
@yehoshuapw
Copy link
Contributor Author

For InfiniSim

Added a bit of changes here (use CMAKE_CURRENT_LIST_DIR to reach things, instead of assuming paths),
and included this CMakeLists.txt there (and changed the GLOB there to use the list inside the CMake here, since globbing on filenames won't work anymore)

@yehoshuapw
Copy link
Contributor Author

For InfiniSim

Also, I did (except when I forgot once) compile before pushing, and here watching the checks status, so it's weird that the simulator check succeeded.

so I had a look, and in the simulator workflow here I manually generated the scripts - I just removed it, so it will fail here too (until InfiniTimeOrg/InfiniSim#28 is merged).
(so both these PRs should be merged about the same time)

Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

This looks totally fine to me!
There's just 1 last issue : the build in the docker container does not work properly. See this log message:

[ 35%] Generating displayapp/fonts/jetbrains_mono_42.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_76.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_76.c
[ 35%] Generating displayapp/fonts/lv_font_navi_80.c
[ 35%] Generating displayapp/fonts/open_sans_light.c
[ 35%] Generating displayapp/fonts/open_sans_light.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_extrabold_compressed.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_extrabold_compressed.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_bold_20.c
[ 35%] Generating displayapp/fonts/jetbrains_mono_42.c
[ 35%] Generating displayapp/fonts/lv_font_sys_48.c
[ 36%] Generating displayapp/fonts/jetbrains_mono_bold_20.c
[ 36%] Generating displayapp/fonts/lv_font_navi_80.c
  File "  File "/sources/src/displayapp/fonts/generate.py/sources/src/displayapp/fonts/generate.py", line ", line 21
21
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
  File "/sources/src/displayapp/fonts/generate.py  File "/sources/src/displayapp/fonts/generate.py", line 21
  File "        def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                              ", line 21
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
/sources/src/displayapp/fonts/generate.py      def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
         File "  File "/sources/src/displayapp/fonts/generate.py", line  /sources/src/displayapp/fonts/generate.py21", line 
     21 def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):

           File "/sources/src/displayapp/fonts/generate.py", line ", line 21
 21    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):

                                             def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                                ^
  SyntaxError :   invalid syntax 
     ^
SyntaxError                       ^
    SyntaxError        ^
     File "SyntaxError :  invalid syntax/sources/src/displayapp/fonts/generate.py
 ", line  21 
     def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
      :   def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
invalid syntax  
:  invalid syntax
          ^
    ^
SyntaxError^
: SyntaxErrorinvalid syntaxSyntaxError: 
:   File "invalid syntaxinvalid syntax
/sources/src/displayapp/fonts/generate.py
", line 21
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                      File "   /sources/src/displayapp/fonts/generate.py ", line   21 
     ^
def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                            ^
SyntaxError: invalid syntax
  File "/sources/src/displayapp/fonts/generate.py", line 21
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                            ^
SyntaxError: invalid syntax
                   ^
SyntaxError: invalid syntax
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:65: recipe for target 'src/displayapp/fonts/jetbrains_mono_extrabold_compressed.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_extrabold_compressed.c] Error 1
make[2]: *** Waiting for unfinished jobs....
    src/CMakeFiles/pinetime-app.dir/build.make:73: recipe for target 'src/displayapp/fonts/jetbrains_mono_76.c' failed
 SyntaxErrormake[2]: *** [src/displayapp/fonts/jetbrains_mono_76.c] Error 1
    :   File " [ 36%] Generating displayapp/fonts/lv_font_sys_48.c
invalid syntaxmake[2]: *** Waiting for unfinished jobs....
/sources/src/displayapp/fonts/generate.py 
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:73: recipe for target 'src/displayapp/fonts/jetbrains_mono_76.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_76.c] Error 1
", line 21 
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
    src/CMakeFiles/pinetime-app.dir/build.make:77: recipe for target 'src/displayapp/fonts/jetbrains_mono_42.c' failed
          make[2]: *** [src/displayapp/fonts/jetbrains_mono_42.c] Error 1
               ^
    SyntaxError :   invalid syntax 
       ^
SyntaxError: invalid syntax
src/CMakeFiles/pinetime-app.dir/build.make:65: recipe for target 'src/displayapp/fonts/jetbrains_mono_extrabold_compressed.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_extrabold_compressed.c] Error 1
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:77: recipe for target 'src/displayapp/fonts/jetbrains_mono_42.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_42.c] Error 1
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:61: recipe for target 'src/displayapp/fonts/lv_font_navi_80.c' failed
make[2]: *** [src/displayapp/fonts/lv_font_navi_80.c] Error 1
src/CMakeFiles/pinetime-app.dir/build.make:69: recipe for target 'src/displayapp/fonts/jetbrains_mono_bold_20.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_bold_20.c] Error 1
src/CMakeFiles/pinetime-app.dir/build.make:85: recipe for target 'src/displayapp/fonts/open_sans_light.c' failed
make[2]: *** [src/displayapp/fonts/open_sans_light.c] Error 1
src/CMakeFiles/pinetime-app.dir/build.make:81: recipe for target 'src/displayapp/fonts/lv_font_sys_48.c' failed
make[2]: *** [src/displayapp/fonts/lv_font_sys_48.c] Error 1
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:85: recipe for target 'src/displayapp/fonts/open_sans_light.c' failed
make[2]: *** [src/displayapp/fonts/open_sans_light.c] Error 1
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:69: recipe for target 'src/displayapp/fonts/jetbrains_mono_bold_20.c' failed
make[2]: *** [src/displayapp/fonts/jetbrains_mono_bold_20.c] Error 1
src/CMakeFiles/pinetime-app.dir/build.make:61: recipe for target 'src/displayapp/fonts/lv_font_navi_80.c' failed
make[2]: *** [src/displayapp/fonts/lv_font_navi_80.c] Error 1
CMakeFiles/Makefile2:409: recipe for target 'src/CMakeFiles/pinetime-app.dir/all' failed
make[1]: *** [src/CMakeFiles/pinetime-app.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
  File "/sources/src/displayapp/fonts/generate.py", line 21
    def gen_lvconv_line(dest: str, size: int, bpp: int, sources: typing.List[Source], compress:bool=False):
                            ^
SyntaxError: invalid syntax
src/CMakeFiles/pinetime-mcuboot-app.dir/build.make:81: recipe for target 'src/displayapp/fonts/lv_font_sys_48.c' failed
make[2]: *** [src/displayapp/fonts/lv_font_sys_48.c] Error 1
CMakeFiles/Makefile2:214: recipe for target 'src/CMakeFiles/pinetime-mcuboot-app.dir/all' failed
make[1]: *** [src/CMakeFiles/pinetime-mcuboot-app.dir/all] Error 2

I think this is caused by the version of Python : the docker image uses Python 2.7.17, while my computer is running Python 3.10.

Do you think it would cause any issue if we called python3 instead of python in the cmake command that generates the font?

src/displayapp/fonts/CMakeLists.txt Show resolved Hide resolved
@JF002
Copy link
Collaborator

JF002 commented May 10, 2022

@yehoshuapw I've just tried the new docker image based on ubunutu 20.04, and I still got the same issues with the Python script.
It looks like the container based on Ubuntu 20.04 is still based on Python 2.7:

infinitime@cacd01d36e16:/$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 20.04.4 LTS
Release:        20.04
Codename:       focal
infinitime@cacd01d36e16:/$ python --version
Python 2.7.18
infinitime@cacd01d36e16:/$ python3 --version
Python 3.8.10

I'm not sure why it works for you and not for me... Did I miss anything?

PS : Please excuse me for the time it takes to finish this review... Those little details about Docker and the CI are taking more time than I expected...

@Riksu9000
Copy link
Contributor

Ubuntu 22.04 is released and it has a package called python-is-python3 which symlinks python to python3. Would this be acceptable?

@JF002
Copy link
Collaborator

JF002 commented May 10, 2022

Ubuntu 22.04 is released and it has a package called python-is-python3 which symlinks python to python3. Would this be acceptable?

Yes, probably! As long as it still builds the firmware and generates the DFU images, that fine for me :)

EDIT : this seems to work:

diff --git a/docker/Dockerfile b/docker/Dockerfile
index 1943177a..e2867c51 100644
--- a/docker/Dockerfile
+++ b/docker/Dockerfile
@@ -1,4 +1,4 @@
-FROM ubuntu:20.04
+FROM ubuntu:22.04
 
 ARG DEBIAN_FRONTEND=noninteractive
 RUN apt-get update -qq \
@@ -15,11 +15,11 @@ RUN apt-get update -qq \
       unzip \
       wget \
       curl \
-# aarch64 packages
+      python-is-python3 \
+      # aarch64 packages
       libffi-dev \
       libssl-dev \
       python3-dev \
-      python \
       git \
       apt-utils \
     && curl -sL https://deb.nodesource.com/setup_18.x | bash - \
@@ -30,6 +30,7 @@ RUN apt-get update -qq \
 
 RUN pip3 install adafruit-nrfutil
 RUN pip3 install -Iv cryptography==3.3
+RUN pip3 install cbor
 RUN npm i lv_font_conv@1.5.2 -g
 
 # build.sh knows how to compile

@yehoshuapw
Copy link
Contributor Author

I'm not sure why it works for you and not for me... Did I miss anything?

I must have made some mistake (left the python2 version? left .c files in the build dir?), because indeed the image I built here does have the "python" version as 2.7.

updated as you suggested, and seems to work. (building the image and then infinitime, from the start)

(I'm also wondering if any dist except for arch (which I don't even use) doesn't have a "python3" anymore without installing some extra things. nonetheless having a newer version which works tends to be worth it)

@JF002
Copy link
Collaborator

JF002 commented May 10, 2022

(I'm also wondering if any dist except for arch (which I don't even use) doesn't have a "python3" anymore without installing some extra things. nonetheless having a newer version which works tends to be worth it)

On Manjaro, I still have python and python3, but since python 3 is support to replace python 2, it's probably better to assume that python is Python 3

updated as you suggested, and seems to work. (building the image and then infinitime, from the start)

Thanks, that looks very good to me :)

@JF002
Copy link
Collaborator

JF002 commented May 10, 2022

Now... How do we handle that in InfiniSim ?

@yehoshuapw
Copy link
Contributor Author

yehoshuapw commented May 10, 2022

Now... How do we handle that in InfiniSim ?

I opened a PR there too, InfiniTimeOrg/InfiniSim#28 which seems to work.
however, each of the PR's will break the other if not merged there, so both should be merged at about the same time.

@NeroBurner
Copy link
Contributor

I think nearly identical to here. Need to install npm and the lv-font generator in the CI. And need to add those generated fonts to InfiniSim. I'll need to have a thorough look at this PR and see how and where to fonts.c files are generated.
For my personal guideline, every generated file should be in the ${CMAKE_BINARY_DIR} and not in the source directory.

Maybe I can make the font-generating CMakeLists.txt file generic enough so I just need to do add_subdirectory in InfiniSim and link to a infinisim-fonts target or something like that 🤔

@yehoshuapw
Copy link
Contributor Author

yehoshuapw commented May 10, 2022

where to fonts.c files are generated.

in build/displayapp/fonts (or wherever the build dir is)

Maybe I can make the font-generating CMakeLists.txt file generic enough so I just need to do add_subdirectory in InfiniSim and link to a infinisim-fonts target or something like that thinking

I'm new at CMake, so go for it - and if you want to make any edits here, you are welcome to.

@JF002
Copy link
Collaborator

JF002 commented May 10, 2022

I'll merge this PR as it's working as expected in InfiniTime. We'll follow up with the integration in InfiniSim in anothe PR (probably in the InfiniSim project).

Maybe I can make the font-generating CMakeLists.txt file generic enough so I just need to do add_subdirectory in InfiniSim and link to a infinisim-fonts target or something like that thinking

It should be possible to create a 'font' target or a GenerateFont() function that we'll be able to include in InfiniSim, so we don't have to duplicate the whole code needed to generate the fonts.

@yehoshuapw I'm happy to see you've already started work in InfiniSim too! Thanks again for this nice PR! 🥇

@JF002 JF002 merged commit 4cb07ba into InfiniTimeOrg:develop May 10, 2022
@yehoshuapw yehoshuapw deleted the audo-generate-font branch May 10, 2022 20:29
NeroBurner added a commit to NeroBurner/InfiniTime that referenced this pull request May 10, 2022
In InfiniTimeOrg#1097 new font
generation capabilites were added. Generalize the font creation to
make it possible to reuse the `displayapp/fonts/CMakeLists.txt` file
for `InfiniSim` and just add the new cmake file to the project and
link against the new `infinitime_fonts` target.

In the following a list of changes.

Allow non-global installed `lv_font_conv` executable installed with

```sh
npm install lv_font_conv@1.5.2
```

In CMake we search for `lv_font_conv` executable. Add the found
executable to the python script `generate.py`, to remove the need for
`lv_font_conv` to be in the path.

Search for `python3` executable, if CMake version 3.12 is available.
Otherwise use `python` as hard coded executable.

Instead of adding the generated fonts to `SOURCE_FILES` variable, create
a static library `infinitime_fonts` instead. Link this library to the
executables instead.
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request May 10, 2022
Since InfiniTimeOrg/InfiniTime#1097 the fonts
are generated. Support this new way of using fonts.
NeroBurner added a commit to NeroBurner/InfiniTime that referenced this pull request May 14, 2022
In InfiniTimeOrg#1097 new font
generation capabilites were added. Generalize the font creation to
make it possible to reuse the `displayapp/fonts/CMakeLists.txt` file
for `InfiniSim` and just add the new cmake file to the project and
link against the new `infinitime_fonts` target.

In the following a list of changes.

Allow non-global installed `lv_font_conv` executable installed with

```sh
npm install lv_font_conv@1.5.2
```

In CMake we search for `lv_font_conv` executable. Add the found
executable to the python script `generate.py`, to remove the need for
`lv_font_conv` to be in the path.

Search for `python3` executable, if CMake version 3.12 is available.
Otherwise use `python` as hard coded executable.

Instead of adding the generated fonts to `SOURCE_FILES` variable, create
a static library `infinitime_fonts`. Link this library to the
executables instead.

Use `add_custom_target()` together with `add_custom_command()` to
generate the font.c files once (like the original PR does).
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request May 14, 2022
Since InfiniTimeOrg/InfiniTime#1097 the fonts
are generated. Support this new way of using fonts.

When InfiniTime isn't a subdirectory of InfiniSim we need to provide
the binary dir when using `add_subdirectory()`. Use `fonts`.

Update README with instructions to install `lv_font_conv`.
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request May 14, 2022
Since InfiniTimeOrg/InfiniTime#1097 the fonts
are generated. Support this new way of using fonts.

When InfiniTime isn't a subdirectory of InfiniSim we need to provide
the binary dir when using `add_subdirectory()`. Use `fonts`.

Update README with instructions to install `lv_font_conv`.
Riksu9000 pushed a commit that referenced this pull request May 16, 2022
In #1097 new font
generation capabilites were added. Generalize the font creation to
make it possible to reuse the `displayapp/fonts/CMakeLists.txt` file
for `InfiniSim` and just add the new cmake file to the project and
link against the new `infinitime_fonts` target.

In the following a list of changes.

Allow non-global installed `lv_font_conv` executable installed with

```sh
npm install lv_font_conv@1.5.2
```

In CMake we search for `lv_font_conv` executable. Add the found
executable to the python script `generate.py`, to remove the need for
`lv_font_conv` to be in the path.

Search for `python3` executable, if CMake version 3.12 is available.
Otherwise use `python` as hard coded executable.

Instead of adding the generated fonts to `SOURCE_FILES` variable, create
a static library `infinitime_fonts`. Link this library to the
executables instead.

Use `add_custom_target()` together with `add_custom_command()` to
generate the font.c files once (like the original PR does).
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