RC2014Z80 / picoterm

Pi Pico VGA Terminal Emulator For RC2014
BSD 3-Clause "New" or "Revised" License
63 stars 19 forks source link

Errors in CMakeLists.txt ... #36

Closed spock64 closed 1 year ago

spock64 commented 1 year ago

Hi,

I just did a clean checkout of the repo and tried to build the 80 column version.

I found a couple of problems with CMakeLists.txt ... so, using cmake .. from .../80col-mono/Build, the SDK files would not be copied properly due to to this line being wrong:

if(EXISTS "${PICO_SDK_PATH}")

... as${PICO_SDK_PATH}refers to an internal CMake variable (which is not defined). Instead, it should use $ENV{} to access environment variables ... the following line works:

if(EXISTS "$ENV{PICO_SDK_PATH}")

Also, and most importantly, the line:

project(picoterm LANGUAGES CXX C ASM VERSION 2.5.3 )

Needs to be after the call to pico_sdk_init() - otherwise the sdk-related files will not build correctly. Also the SDK files will not copy as CMake shifts the working directory to.../80col-mono from .../80col-mono/Build.

Hope this is helpful! Jason

myount commented 1 year ago

Thanks for this! I was wanting to rebuild it myself because, if I'm honest, I really do dislike Ubuntu Mono that much. I also ran into the issues described, but unfortunately I don't know thing one about CMake.

I didn't have a problem with it copying the SDK files, but I didn't realize from your notes that the existing project(picoterm VERSION ...) line needs to be moved and have LANGUAGES CXX C ASM added to it rather than adding a second one with that after pico_sdk_init(). Before I realized that and fixed it, it was trying to use /usr/bin/cc as the C compiler, rather than the cross-compiler, and as you might imagine, an x86_64 assembler wasn't too keen on compiling the ARM assembly pulled in from the SDK code.


Also, this might be a bit of a derail, but for anyone else who might want to replace the font... do use the LVGL converter mentioned in the comment at the top of font8.c, but don't save the output over top of the existing font8.c; just replace the glyph definitions, and change the .line_height and .base_line values in font8.c's lv_font_t to match the ones from the converter output. Then run compile_font.py to update the NuPETSCII and CP437 fonts. I replaced mine with the aspect-ratio-corrected OlivettiThin 8x16 font from int10h.org (which is period-appropriate, if not necessarily historically accurate), and it works:

Expand for photo ![A photo of a Z3PLUS directory listing displayed by picoterm on a Dell monitor](https://user-images.githubusercontent.com/637064/226145860-506adcfe-4ffa-413c-a79a-62a7deed2369.png)

...although it had a different .line_height and .base_line from Ubuntu Mono, and as you can see, there is some junk appearing at the bottom of the screen now once it scrolls. I haven't used it extensively with the new font yet, just booted it up to see if it displayed at all correctly, so I don't know if this is an indication of incipient memory corruption that's eventually going to crash Picoterm.

mchobby commented 1 year ago

@myount Would you like to open a new issue with your font message. I'm very interrested to dig further more into it. Thanks

mchobby commented 1 year ago

@spock64 I'm just a bit puzzle about usage of $ENV like if(EXISTS "$ENV{PICO_SDK_PATH}") When I do it, it bricks the compilation on my Linux distro. However, the PICO_SDK_PATH is defined (see the code section at bottom).

dom:~/Bureau/RC2014/picoterm/80col-mono/Build$ cmake ..
domeu@dom-HP-ENVY:~/Bureau/RC2014/picoterm/80col-mono/Build$ cmake ..
System is unknown to cmake, create:
Platform/PICO to use this system, please post your config file on discourse.cmake.org so it can be added to cmake
Your CMakeCache.txt file was copied to CopyOfCMakeCache.txt. Please post that file on discourse.cmake.org.
PicoTerm 80Col version = 1.5.3
CMake Error at CMakeLists.txt:9 (message):
  Systen Environment variable PICO_SDK_PATH undefined!

-- Configuring incomplete, errors occurred!
See also "/home/domeu/Bureau/RC2014/picoterm/80col-mono/Build/CMakeFiles/CMakeOutput.log".
See also "/home/domeu/Bureau/RC2014/picoterm/80col-mono/Build/CMakeFiles/CMakeError.log".

However, the PICO_SDK_PATH is defined as stated by the export command here below

dom:~/Bureau/RC2014/picoterm/80col-mono/Build$ export | grep 'SDK'
declare -x PICO_SDK_PATH="../../pico-sdk/"

Do you have more information to share about it? Cheers, Dominique

spock64 commented 1 year ago

Hi Dominique,

So how you declare the environment variable may vary somewhat, but I would expect declare -x PICO_SDK_PATH="../../pico-sdk/" to be in your .bashrc (or similar)? You could also have the environment in a shell script that calls cmake too - everyone has their own preferences.

Certainly when you do export | grep 'SDK' using your example above, your output should be: PICO_SDK_PATH=../../pico-sdk/

Is there something wrong with how you are declaring the environment variable?

Best Jason

spock64 commented 1 year ago

Thanks for this! I was wanting to rebuild it myself because, if I'm honest, I really do dislike Ubuntu Mono that much. I also ran into the issues described, but unfortunately I don't know thing one about CMake.

I didn't have a problem with it copying the SDK files, but I didn't realize from your notes that the existing project(picoterm VERSION ...) line needs to be moved and have LANGUAGES CXX C ASM added to it rather than adding a second one with that after pico_sdk_init(). Before I realized that and fixed it, it was trying to use /usr/bin/cc as the C compiler, rather than the cross-compiler, and as you might imagine, an x86_64 assembler wasn't too keen on compiling the ARM assembly pulled in from the SDK code.

Also, this might be a bit of a derail, but for anyone else who might want to replace the font... do use the LVGL converter mentioned in the comment at the top of font8.c, but don't save the output over top of the existing font8.c; just replace the glyph definitions, and change the .line_height and .base_line values in font8.c's lv_font_t to match the ones from the converter output. Then run compile_font.py to update the NuPETSCII and CP437 fonts. I replaced mine with the aspect-ratio-corrected OlivettiThin 8x16 font from int10h.org (which is period-appropriate, if not necessarily historically accurate), and it works:

Expand for photo ...although it had a different .line_height and .base_line from Ubuntu Mono, and as you can see, there is some junk appearing at the bottom of the screen now once it scrolls. I haven't used it extensively with the new font yet, just booted it up to see if it displayed at all correctly, so I don't know if this is an indication of incipient memory corruption that's eventually going to crash Picoterm.

Cmake is a great build system - but really awkward to get right. The ordering of statements is important!

Glad you got it working - and thanks for the hint on terminal fonts ... I'm not a mono fan either!

mchobby commented 1 year ago

Hi Dominique,

So how you declare the environment variable may vary somewhat, but I would expect declare -x PICO_SDK_PATH="../../pico-sdk/" to be in your .bashrc (or similar)? You could also have the environment in a shell script that calls cmake too - everyone has their own preferences.

Certainly when you do export | grep 'SDK' using your example above, your output should be: PICO_SDK_PATH=../../pico-sdk/

Is there something wrong with how you are declaring the environment variable?

Best Jason

I declare the environment variable as stated in the docs/compiling.md . So I used the

export PICO_SDK_PATH=../../../../../pico/pico-sdk

I define it on need (it is not a permanent defines in bashrc, otherwise I would use an absolute path instead).

The PICO_SDK_PATH is indeed defined into my current terminal. IF I do open a new terminal and key-in export | grep 'SDK' THEN PICO_SDK_PATH is not defined. Now I have a path to follow :-)

mchobby commented 1 year ago

If not defined in.bashrc, it is possible to define an environment variable to execute a given command like the example here below. The following command defines the environment variable PICO_SDK_PATH just before executing cmake ..

env PICO_SDK_PATH="../../../../../pico/pico-sdk" cmake ..

In such situation, the $ENV{PICO_SDK_PATH} can be read from the CMakeList.txt

spock64 commented 1 year ago

Yes, that's the right sort of thing. You don't need the 'env' though ... Just PICO_SDK_PATH="../../../../../pico/pico-sdk" cmake .. will work ... and this is what I use.

This syntax causes the environment variable PICO_SDK_PATH to be defined just for the command that follows. It's very handy and is what I've used since the, er, early 1980's !

mchobby commented 1 year ago

I came very late to the Linux environment. Thanks for the TIP, it is something very useful to know. I will post a newer version of CMakeList that checks the environment variable and use its value when defined. I also recommended to use an absolute path for PICO_SDK_PATH, it is saver for file copy operations.

myount commented 1 year ago

@myount
Would you like to open a new issue with your font message.
I'm very interrested to dig further more into it. Thanks

I was going to try to reproduce it tonight, and get direct VGA captures from the card (or as direct as I can get, going through an OSSC into an Elgato HDMI capture card)... but it turns out that I couldn't get it to produce the garbage again. I guess it was either an artifact of the scaler in the Dell monitor, or the sync settings that the OSSC settled on didn't reveal it.

That said, I did encounter another issue relating to having changed the font that I'll open another issue for.

mchobby commented 1 year ago

This should be resolved now