-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Ensure serial output code match the intention of the developer #20985
Ensure serial output code match the intention of the developer #20985
Conversation
Can easily be tested by adding "SERIAL_PRINT((uint8_t)3, 0)" somewhere in a CPP file, it'll fail to build
Ok, continuing the work here. I've removed base == 0 case, and make it so, if found in the code, the compiler will break instead of compiling buggy code. I've also removed many (but not all) SERIAL macro and replaced them by template instead, leading to:
Before:
After:
Need to sort out the combination problem of SERIAL_ECHOPAIR... (and fix the building issues) |
It would be good to at least test the compilation with all examples from the Configurations repository. |
It's going to take a huge time to build all of them. I'm already running |
I've failed to get a smaller binary size with variadic template. Whatever the mean, the resulting binary is always bigger than a basic macro unrolling/inlining. So, I'm reverting the previous changes since it does not improve anything.
I'm not currently able to build all configuration from Marlin because I don't know what platformio environment to use. I've written a script to automate this, but it's missing additional information to work. If any of you has access to the Marlin's Configuration repository, feel free to add the missing |
Concerning this PR, I think I'm done with it for now. I was not able to reduce binary footprint for SERIAL_ECHOPAIR to more than what it is actually with macro. I've tried with type punning (using a union) so that only one version of SERIAL_ECHOPAIR exists but it produced larger code than macros. I've tried with type erasing too but again, it failed reducing the binary footprint. I think if we intended to have a lower code size than now, we should instead rework the SERIAL_ECHOPAIR idea so something more or less like printf. It might be less efficient, but it'll take a lot less size. Yet, it's a too huge work for me to perform in a PR. |
Compiled it on Before:
After:
|
@qwewer0 Can you post your configuration, please ? |
@X-Ryl669 Sorry, should have known that it is best to include it. |
5c40161
to
750fa2d
Compare
You can just use |
bf940a8
to
6898823
Compare
6898823
to
0e988b8
Compare
It seems it also work for me:
40 more bytes to shave off 😄 |
|
There's something wrong with your build (or you haven't send me your exact configuration file). Please check you have updated packages (in particular toolchain). I've built your with your configuration above and I'm always getting smaller binaries than you. Maybe you need to clean your build for leftover ? If you think you've done all of these, please post the result of:
|
See, there's something different in the RAM usage between you and me, and this can only result from different configuration. There is no change in RAM usage in this PR. |
Don't know how to do that. I will try it if you can guide me. Maybe discord would be a better place for the chit-chat? But here is the compilation (if this even helps at all):
|
I always had that amount of ram used. I will clean install vscode and the other stuff later, hope it will help. |
Seems like, except for
If you are using VSCode, you can open a PIO terminal and paste the command line above, you can then post the result of the command line here. Else, if you are using linux, you can do the same in a terminal.
|
You don't need to clean VScode. You can clean the project via |
So, I'm not sure about this:
After the cleanup, using my #20985 (comment) config.
It seems that I used a bit different config, sorry for the inconvenience. |
Ok, that was crazy! For what it's worth, I've found why there's still a
I'm not sure if there are many platform using At least the mystery is solved! |
Remove a leftover code from a bad merge in another PR
The +16 bytes of RAM was because I had |
Could this be related with the serial refactoring? |
Let's see what shakes out as a result of this general improvement. |
* bugfix-2.0.x: (177 commits) [cron] Bump distribution date (2021-02-11) chmod and paths [cron] Bump distribution date (2021-02-10) Reheat bed first Ender 3 V2 DWIN cleanup (MarlinFirmware#21026) Update M808 comment MAX Thermocouples rework (MarlinFirmware#20447) [cron] Bump distribution date (2021-02-09) Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985) Fix STM32F1 emergency parser (MarlinFirmware#21011) Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007) Fix animated boot screen Fix: Unsupported use of %f in printf (MarlinFirmware#21001) Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021) [cron] Bump distribution date (2021-02-08) Fix LVGL "more" menu user items (MarlinFirmware#21004) Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016) Fix ESP32 I2S init placement (MarlinFirmware#21019) Improve RPi host kernel panic mitigation Melzi, comments cleanup ...
…rmpel/Marlin into rmpel/bugfix-2.0.x/ender-3-skr-14-turbo * 'rmpel/bugfix-2.0.x/ender-3-skr-14-turbo' of github.com:rmpel/Marlin: (177 commits) [cron] Bump distribution date (2021-02-11) chmod and paths [cron] Bump distribution date (2021-02-10) Reheat bed first Ender 3 V2 DWIN cleanup (MarlinFirmware#21026) Update M808 comment MAX Thermocouples rework (MarlinFirmware#20447) [cron] Bump distribution date (2021-02-09) Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985) Fix STM32F1 emergency parser (MarlinFirmware#21011) Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007) Fix animated boot screen Fix: Unsupported use of %f in printf (MarlinFirmware#21001) Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021) [cron] Bump distribution date (2021-02-08) Fix LVGL "more" menu user items (MarlinFirmware#21004) Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016) Fix ESP32 I2S init placement (MarlinFirmware#21019) Improve RPi host kernel panic mitigation Melzi, comments cleanup ...
* bugfix-2.0.x: (177 commits) [cron] Bump distribution date (2021-02-11) chmod and paths [cron] Bump distribution date (2021-02-10) Reheat bed first Ender 3 V2 DWIN cleanup (MarlinFirmware#21026) Update M808 comment MAX Thermocouples rework (MarlinFirmware#20447) [cron] Bump distribution date (2021-02-09) Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985) Fix STM32F1 emergency parser (MarlinFirmware#21011) Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007) Fix animated boot screen Fix: Unsupported use of %f in printf (MarlinFirmware#21001) Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021) [cron] Bump distribution date (2021-02-08) Fix LVGL "more" menu user items (MarlinFirmware#21004) Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016) Fix ESP32 I2S init placement (MarlinFirmware#21019) Improve RPi host kernel panic mitigation Melzi, comments cleanup ...
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
…20985) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Unfortunately this PR invented its own concept for Anyway, this doesn't work… #define SERIAL_ECHOLIST_N(N, V...) SERIAL_ECHOLIST(PSTR(""), LIST_N(N, V)) As long as we're here, I have asked participants in #21010 to report on the current situation, and if there are still problems we will have to do a deeper dive to understand how else this PR may have unintentionally altered behavior, timing, buffering, etc. |
Description
To follow the current serial code rewrite, here's a PR to fix some of the serial macro's calling error (like using
.
.
SERIAL_ECHO
whereSERIAL_CHAR
was intended, ...).I've documented the SERIAL_XXX macros so it's clear which one to use now.
Now, the expectation is that
SERIAL_ECHO
actually echo the argument passed in by converting to a human readable string. Souint8_t a = 32; SERIAL_ECHO(a);
will output32
and notSERIAL_CHAR
must be used to output a character as-is, thusSERIAL_CHAR(a);
will outputWith this change, it's no more required to cast a parameter for
SERIAL_ECHO
if it's auint8
or aint8
, so it's one less trouble/pain to remember for developers.I've changed the default base for serial's print method to use decimal everywhere. Ideally, the
base == 0
case will disappear, since it was an hack. I'll probably do that later on.I'll also remove the variadic SERIAL macro to replace them by variadic templates. The main advantage I see is actually reducing the build size since a single implementation of the template will be in the firmware, instead of a multiple time the bunch of instructions the macros are generating. It'll also helps the code analyzers (like Intellisense / clangserver) since they fails to develop the macros correctly.
It also includes a fix that I've missed in my previous PR for those using a MMU serial on AVR.
Requirements
None
Benefits
See above
Configurations
All
Related Issues
#20783 (last comment at time of writing)