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

Update toolchains to newer compilers #25327

Draft
wants to merge 2 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

quiret
Copy link
Contributor

@quiret quiret commented Feb 1, 2023

Description

This pull request updates the platform and package versions of build configurations to their latest version. The build configurations have been rigorously tested (mostly under my HAL SPI rework).

Improved generic_create_variant.py to not mess with official board variant configurations (only touch if board_build.variant value is prefixed with MARLIN_).

Benefits

Newer compilers emit better code. Newer compilers provide newer C++ language features thus more powerful software models can be implemented. Newer toolchains may come with support for even more boards.

Related Issues

#24911 (depends on this PR)

@ellensp
Copy link
Contributor

ellensp commented Feb 2, 2023

I really hope you have thoroughly tested these new compilers on all boards effected, and are not just blindly upgrading them... beyond it still compiles...

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

I really hope you have thoroughly tested these new compilers on all boards effected, and are not just blindly upgrading them... beyond it still compiles...

The upgrade process is being performed slowly! You can see that I have left compatibility .ini configurations as fallback. The following boards have been tested IRL:

  • BTT SKR V1.4
  • MKS TinyBee V1.0
  • MKS Robin E3D V1.1

Also please do not seed distrust into PlatformIO official registry packages. They have been released for a good reason and PlatformIO targets embedded devs. I will nevertheless make sure everything works ;) Progress is necessary!

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

Please watch PRs me-no-dev/ESPAsyncWebServer#1142 and luc-github/ESP3DLib#51 for merging, then change back dependencies if the official packages work.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

I really hope you have thoroughly tested these new compilers on all boards effected, and are not just blindly upgrading them... beyond it still compiles...

@ellensp Hey I need some help testing the things that run on CI locally. Do you know how I can do that? I'd greatly appreciate it! Need to fix the issue for the MKS TinyBee which fails in an unknown Linux script...

Never mind! Got it figured out. I simply took another crash course into Marlin FW, specifically the automated build system scripts and stuff. Really interesting and worth a video! That system looks very stable so users should get an insight if they want to contribute to Marlin FW.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

Please monitor PR espressif/arduino-esp32#7744 + check for the release of arduino-espressif32 version 2.0.7. Then remove the custom dependency from esp32.ini !

@ellensp
Copy link
Contributor

ellensp commented Feb 2, 2023

I noticed that you added gnu++1z to avr env

According to this document https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html

‘gnu++17’
‘gnu++1z’
`GNU dialect of -std=c++17. This is the default for C++ code. The name ‘gnu++1z’ is deprecated.`

So perhaps you should be using gnu++17 instead ?

The document also state "This is the default" is this define even needed?

It does build slightly differently

example ramps build.
With gnu++1z
RAM: [====== ] 60.3% (used 4938 bytes from 8192 bytes)
Flash: [===== ] 51.1% (used 129668 bytes from 253952 bytes)

Without gnu++1z
RAM: [====== ] 60.3% (used 4938 bytes from 8192 bytes)
Flash: [===== ] 51.0% (used 129630 bytes from 253952 bytes)

38 bytes less flash...

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

I noticed that you added gnu++1z to avr env
(...)
The document also state "This is the default" is this define even needed?
(...)
38 bytes less flash...

Thanks for pointing this out! I am still trying to figure out the proper C++ standard targets for each platform. The C++17 standard is already pretty good, but there are really amazing features in C++20 that we should not miss out on.

It is sometimes necessary to override -std flags from dependency packages / platform options, so that is also a reason.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

@ellensp Please note that the currently available GCC for AVR version is 7.3 on PlatformIO and you have mentioned the default -std=c++17 option that is only from GCC version 11.

https://registry.platformio.org/tools/platformio/toolchain-atmelavr/versions
https://gcc.gnu.org/projects/cxx-status.html#cxx17

Worth pointing out because GCC versions differ between platforms! Basically GCC maintenance is a huge mess due to the distributed nature of the compiler.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

As part of my work I can fix the following issue: #24717

@EvilGremlin
Copy link
Contributor

Marlin is reluctant to bump toolchain versions because it will break things, potentially for thousands people. Can you test at least dozens configurations for each MCU? And that's just hardware, not software feature conmbinations. Do you at least cople dozens most popular boards, dozen different displays and machines of every kinematic so you can debug and fix stuff you will break with toolchain updates? As long as current toolchain is satisying - we'd better not touch it.

@ellensp
Copy link
Contributor

ellensp commented Feb 2, 2023

"As part of my work I can fix the following issue: #24717"

That is trivial, main issue is pins_debugging needs re written for stm32 platform, it doesn't support many MPU's properly at all.

Most Issues are with analog pins.

And this should be in a separate PR.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

"As part of my work I can fix the following issue: #24717"

That is trivial, main issue is pins_debugging needs re written for stm32 platform, it doesn't support many MPU's properly at all.

Most Issues are with analog pins.

And this should be in a separate PR.

@ellensp I think that a proper implementation of pin debugging should be in a seperate PR. But I will adjust some things for compatibility sake.

@quiret
Copy link
Contributor Author

quiret commented Feb 2, 2023

Marlin is reluctant to bump toolchain versions because it will break things, potentially for thousands people. Can you test at least dozens configurations for each MCU? And that's just hardware, not software feature conmbinations. Do you at least cople dozens most popular boards, dozen different displays and machines of every kinematic so you can debug and fix stuff you will break with toolchain updates? As long as current toolchain is satisying - we'd better not touch it.

@EvilGremlin As I have said, PlatformIO packages are there for a reason. They are tried & tested by the embedded community. Thus it is safe to assume that they are ready to be deployed on real hardware. Nevertheless I will perform big tests on real hardware.

@quiret quiret marked this pull request as ready for review February 2, 2023 21:06
ini/features.ini Outdated Show resolved Hide resolved
ini/stm32-common.ini Outdated Show resolved Hide resolved
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x-tcupdate branch from a045464 to dd0d58c Compare February 3, 2023 02:26
@EvilGremlin
Copy link
Contributor

@quiret

PlatformIO packages are there for a reason. They are tried & tested by the embedded community. Thus it is safe to assume that they are ready to be deployed on real hardware

Not in a slightest. Marlin is obviously included in that testers community, and proven to be a benchmark for toolchains and platformio itself. There were multiople occasions where Marlin was first to hit a bug or deficiency there. Sometimes latest is better, sometimes it's much worse. You're obviously free to push forward but be ready to break stuff and introduce elusive bugs (i.e. stumbling upon compiler bug). I recommend making separate PR fot each chip family toolchain update if at all possible (yes, 6 PRs for STM)

@quiret
Copy link
Contributor Author

quiret commented Feb 3, 2023

(...) I recommend making separate PR fot each chip family toolchain update if at all possible (yes, 6 PRs for STM)

@EvilGremlin I agree with you on that, but on the condition that you mean separate PR for each STM chip family. This PR targets exclusively STM32F1 which is a very stable and known MCU series by ST. They are well documented. The changes I bring to Marlin related to ST are made by following their official documentation. I argue that we are on the safe side.

I have already planned to do another PR that targets STM32F4, even before you raising that concern.

@quiret quiret marked this pull request as draft February 3, 2023 14:48
@quiret
Copy link
Contributor Author

quiret commented Feb 3, 2023

I am putting this on hold until I have fixed some technical difficulties that arrised during testing. Please wait until I undraft this PR! In the mean time I have less technically challenging PRs that I want to put, which also drive Marlin forwards.

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 2 times, most recently from e90c213 to 4b9bb85 Compare March 7, 2023 05:17
@quiret
Copy link
Contributor Author

quiret commented Mar 23, 2023

I am sorry but I have decided to halt my activities on this PR for an indetermined amount of time. You are free to decide whether to continue it yourself. Thank you to the Marlin firmware community for providing this interesting playing ground for testing my ideas and vision. I hope that Marlin will continue to go towards the best 3D printing solution! Hopefully my ideas have inspired you on how to continue this software!

@sjasonsmith
Copy link
Contributor

@thinkyhead I don't think this change is practical to consume as it is. It is too much change all bundled together. I support the notion of updating toolchains, but they should probably be done one architecture at a time in separate PRs. That way if one architecture has issues a simple git revert can undo the change. It also prevents uncertainly on one architecture from blocking progress on all others by coupling changes unnecessarily.

In general, I agree with @quiret that it is best to move forward to newer toolchains on a regular basis. Yes, it will break things sometimes, but the alternative is to accumulate large amounts of technical debt across many years. That makes it hard to adopt bugfixes or other improvements in the toolchains we depend upon.

@EvilGremlin
Copy link
Contributor

Agreed, probably even more granular, like separate PRs for boards with FSMC screen and with USB OTG.

@thinkyhead
Copy link
Member

@sjasonsmith — I agree that this should not be merged all at once, so I've been slowly integrating small parts of this as time allows. Today I'm just bringing this PR up to date with a new merge, and then following up to see if I can make the CI tests pass. I see ESP32 is up to version 6.4.0 now, so this PR might also be updated to point to that version.

Over time we can continue to pick up small parts of this and integrate them as we have time to test, perhaps starting with ESP32 since it will have the smallest impact on the smallest number of boards. I would definitely like to get updated to the more modern STM32 platform too since it apparently has some nice improvements.

thinkyhead and others added 2 commits November 30, 2024 21:09
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x-tcupdate branch from 52729d3 to 504fad7 Compare December 1, 2024 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants