RC2014Z80 / picoterm

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

Build problems ... #13

Closed spock64 closed 1 year ago

spock64 commented 1 year ago

I have been building the latest release and have hit a few problems. I am building on a ubuntu 22.04 host and am using the standard usb library. I have downloaded the latest pico sdk, etc.

The following applies to the 80col-mono build ...

CXXFLAGS="-z muldefs" export CXXFLAGS

When run, I see the normal startup screen, but:

When I flash the pre-built firmware onto the pico, it works fine.

I have followed the instructions you give to the letter (apart from the tinyusb library stuff).

Where do I start debugging this? Is my build environment different to yours?

mchobby commented 1 year ago

Hi Spock64, You should not have multiple definition while compiling the code (I do care about it). This could be the possible cause of your issue. Could you give me the original message ? I'm presently compiling on Linux Mint Mate 19.1 (an older Ubuntu 18.4 LTS derivative).

I guess that Tiny USB version is still 0.12 as used for the pre-build firmware.

Cheers, Dominique

dquadros commented 1 year ago

Regarding TinyUSB, the current version has breaking changes and is not used by the Pico SDK right now. See https://github.com/raspberrypi/pico-sdk/pull/889,

spock64 commented 1 year ago

Hi Dominique, Here's the output from the linking stage ...

/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: CMakeFiles/picoterm.dir/picoterm.c.obj:(.bss.config+0x0): multiple definition of `config'; CMakeFiles/picoterm.dir/main.c.obj:(.bss.config+0x0): first defined here
/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: CMakeFiles/picoterm.dir/home/j/src/pi-pico/picoterm/common/picoterm_config.c.obj:(.bss.config+0x0): multiple definition of `config'; CMakeFiles/picoterm.dir/main.c.obj:(.bss.config+0x0): first defined here
collect2: error: ld returned 1 exit status
make[3]: *** [CMakeFiles/picoterm.dir/build.make:1463: picoterm.elf] Error 1
make[2]: *** [CMakeFiles/Makefile2:1752: CMakeFiles/picoterm.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:1759: CMakeFiles/picoterm.dir/rule] Error 2
make: *** [Makefile:124: picoterm] Error 2

As you can see, ld is complaining !

I can get the binary to build by telling ld that it should allow the multiple definitions ...

CXXFLAGS="-z muldefs" export CXXFLAGS

I am guessing that I am using different versions of tools to you?

spock64 commented 1 year ago

Regarding TinyUSB, the current version has breaking changes and is not used by the Pico SDK right now. See raspberrypi/pico-sdk#889,

Thanks Daniel - I will look at this once I hear back from Dominique.

mchobby commented 1 year ago

@spock64 Picoterm_config is involved into the parameter storage and retrieval from Flash. I will try to reproduce the compiler issue... I will certainly install a new computer to setup Ubuntu 22.04. I have few time this week... so it will be for saturday.

spock64 commented 1 year ago

@mchobby

I found a few moments to look at this - my CXXFLAGS="-z muldefs" export CXXFLAGS fix is a red herring - this causes multiple versions of config to be created at link time. This means that main.c, picture.c and picture_config.c ended up with their own versions of config - so it's no surprise that things didn't work!

So digging into the real problem, I notice that both the definition and declaration of config are in picoterm_config.h - this is a problem because main.c and picoterm.c include this header file. This results in three separate versions of config being presented to the ld. This is reason why we have problems!

Now it could be that the older (gcc9?) in your version of Ubuntu allowed the definitions to be merged - but in the latest Ubuntu (gcc11 I think), the compiler does not allow this merging - this was done to make the behaviour consistent with what C++ expects. I did try using -fcommon as flag to see if this resolved things but I am not certain that the flag was passed through properly by CMake.

I did come across these problems in 1985 when writing a C compiler for my degree course ... things clearly have not improved since then :-)

So fixing the underlying bug means splitting the declaration and definition of configby ...

typedef struct PicotermConfig { char magic_key[6]; uint8_t version; uint8_t colour_preference; // version 2 uint32_t baudrate; uint8_t databits; uint8_t parity; uint8_t stopbits; // 1, 2 } t_config;

t_config config;

extern t_config config;

With these changes, everything works as expected.

I would write a pull request but I am not that competent with git - let me know if you would like me to do this?

mchobby commented 1 year ago

@spock64 Awesome contribution! (I hate C programming). I did test it with success on my own compiler. Updates are now pushed on Github.

As reward GIFT, download the lastest version and press CTRL+SHIFT+N to inspect the NuPetSCII extra characters ;-)

Cheers, Dominique