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

Ensure serial output code match the intention of the developer #20985

Merged
merged 26 commits into from
Feb 8, 2021

Conversation

X-Ryl669
Copy link
Contributor

@X-Ryl669 X-Ryl669 commented Feb 3, 2021

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 where SERIAL_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. So uint8_t a = 32; SERIAL_ECHO(a); will output 32 and not .
SERIAL_CHAR must be used to output a character as-is, thus SERIAL_CHAR(a); will output .

With this change, it's no more required to cast a parameter for SERIAL_ECHO if it's a uint8 or a int8, 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)

@X-Ryl669 X-Ryl669 marked this pull request as draft February 3, 2021 13:23
Can easily be tested by adding "SERIAL_PRINT((uint8_t)3, 0)" somewhere in a CPP file, it'll fail to build
@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 3, 2021

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:

  1. Huge code size saving
  2. No more limitation in the number of arguments for the macros
  3. Compile-time checking of parameters (well sort-of, right now, only ECHOPAIR would benefit from this, but I've not enabled for ECHOPAIR since it makes code larger... grrr...)

Before:

RAM:   [=======   ]  74.6% (used 48880 bytes from 65536 bytes)
Flash: [====      ]  44.7% (used 234540 bytes from 524288 bytes)

After:

RAM:   [=======   ]  74.6% (used 48880 bytes from 65536 bytes)
Flash: [====      ]  44.5% (used 233380 bytes from 524288 bytes)

Need to sort out the combination problem of SERIAL_ECHOPAIR... (and fix the building issues)

@rhapsodyv
Copy link
Sponsor Member

It would be good to at least test the compilation with all examples from the Configurations repository.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 4, 2021

It's going to take a huge time to build all of them. I'm already running make tests-all-local and it takes more than an hour to finish on my computer (it's faster to let Github run them although it's not exactly the same).
So, until I consider the PR ready, I'll avoid this. When it's ready, I'll try to run as many as I can.

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.
@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 4, 2021

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 platfomio.env files in each subfolder containing the expected environment to build. Once this is done, we'll be able to run $ buildroot/ci-check/build_all_configs.sh bugfix-2.0.x to build all configurations known to Marlin and check it does not break.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 4, 2021

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.

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 4, 2021

Compiled it on STM32F103RC_btt_512K

Before:

RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [====      ]  45.0% (used 235804 bytes from 524288 bytes)

After:

RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [=====     ]  45.0% (used 235972 bytes from 524288 bytes)

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 4, 2021

@qwewer0 Can you post your configuration, please ?

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 4, 2021

@X-Ryl669 Sorry, should have known that it is best to include it.

Configuration.zip

@thinkyhead
Copy link
Member

I'm not currently able to build all configuration from Marlin because I don't know what platformio environment to use.

You can just use mftest -a and it will pick the correct environment.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

It seems it also work for me:

RAM:   [=======   ]  74.6% (used 48880 bytes from 65536 bytes)
Flash: [====      ]  44.5% (used 233340 bytes from 524288 bytes)

40 more bytes to shave off 😄

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 5, 2021

diff --git a/Marlin/src/core/serial_base.h b/Marlin/src/core/serial_base.h
index 795cf772ac..81403b55f2 100644
--- a/Marlin/src/core/serial_base.h
+++ b/Marlin/src/core/serial_base.h
@@ -132,7 +132,7 @@ struct SerialBase {
   FORCE_INLINE void println(double c)              { println(c, 2); }

   // Print a number with the given base
-  void printNumber(unsigned long n, const uint8_t base) {
+  NO_INLINE void printNumber(unsigned long n, const uint8_t base) {
     if (!base) return; // Hopefully, this should raise visible bug immediately

     if (n) {
@@ -158,7 +158,7 @@ struct SerialBase {
   }

   // Print a decimal number
-  void printFloat(double number, uint8_t digits) {
+  NO_INLINE void printFloat(double number, uint8_t digits) {
     // Handle negative numbers
     if (number < 0.0) {
       write('-');
bugfix:
RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [====      ]  45.0% (used 235804 bytes from 524288 bytes)

Patch:
RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [=====     ]  45.1% (used 236668 bytes from 524288 bytes)

With that two line changed:
RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [=====     ]  45.0% (used 236060 bytes from 524288 bytes)

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

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:

~/.platformio/packages/toolchain-gccarmnoneeabi/bin/arm-none-eabi-objdump -tT -C .pio/build/STM32F103RC_btt_512K/firmware.elf | grep "::print" | grep Serial

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

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.

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 5, 2021

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:

~/.platformio/packages/toolchain-gccarmnoneeabi/bin/arm-none-eabi-objdump -tT -C .pio/build/STM32F103RC_btt_512K/firmware.elf | grep "::print" | grep Serial

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):

Processing STM32F103RC_btt_512K (platform: ststm32@~10.0; board: genericSTM32F103RC; framework: arduino)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Tool Manager: toolchain-gccarmnoneeabi @ 1.70201.0 is already installed
Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/ststm32/genericSTM32F103RC.html
PLATFORM: ST STM32 (10.0.1) > STM32F103RC (48k RAM. 256k Flash)
HARDWARE: STM32F103RCT6 72MHz, 48KB RAM, 512KB Flash
DEBUG: Current (blackmagic) External (blackmagic, cmsis-dap, jlink, stlink)
PACKAGES:
 - framework-arduinoststm32-maple 3.10000.201129 (1.0.0)
 - tool-stm32duino 1.0.2
 - toolchain-gccarmnoneeabi 1.70201.0 (7.2.1)
Converting Marlin.ino
LDF: Library Dependency Finder -> http://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 29 compatible libraries
Scanning dependencies...
Dependency Graph
|-- <SoftwareSerialM> 1.0.0
|-- <TMCStepper> 0.7.1
|   |-- <SoftwareSerialM> 1.0.0
|-- <U8glib-HAL> 0.4.3
|   |-- <Wire> 1.0
|-- <STM32ADC> 1.0
|-- <EEPROM>
|-- <USBComposite for STM32F1> 0.99
|-- <Wire> 1.0
|-- <Servo(STM32F1)> 1.1.2
Building in release mode
...
Linking .pio\build\STM32F103RC_btt_512K\firmware.elf
Building .pio\build\STM32F103RC_btt_512K\firmware.bin
Checking size .pio\build\STM32F103RC_btt_512K\firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [===       ]  29.4% (used 14464 bytes from 49152 bytes)
Flash: [=====     ]  45.0% (used 236060 bytes from 524288 bytes)
================================================================================================= [SUCCESS] Took 302.92 seconds =================================================================================================
Environment           Status    Duration
--------------------  --------  ------------
STM32F103RC_btt_512K  SUCCESS   00:05:02.925

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 5, 2021

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.

I always had that amount of ram used. I will clean install vscode and the other stuff later, hope it will help.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

Seems like, except for tool-stm32duino, we've the same versions. I can't explain the differences.
I'm using the configuration file from this post.

Don't know how to do that. I will try it if you can guide me

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.
If you are using Windows, I guess you'll need to trigger a bash terminal somehow, since Microsoft Powershell will not understand the command above. In that case, I'm sorry I can't really help.

Processing STM32F103RC_btt_512K (platform: ststm32@~10.0; board: genericSTM32F103RC; framework: arduino)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/ststm32/genericSTM32F103RC.html
PLATFORM: ST STM32 (10.0.1) > STM32F103RC (48k RAM. 256k Flash)
HARDWARE: STM32F103RCT6 72MHz, 48KB RAM, 512KB Flash
DEBUG: Current (blackmagic) External (blackmagic, cmsis-dap, jlink, stlink)
PACKAGES:
 - framework-arduinoststm32-maple 3.10000.201129 (1.0.0)
 - tool-stm32duino 1.0.1
 - toolchain-gccarmnoneeabi 1.70201.0 (7.2.1)
Converting Marlin.ino
LDF: Library Dependency Finder -> http://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 29 compatible libraries
Scanning dependencies...
Dependency Graph
|-- <SoftwareSerialM> 1.0.0
|-- <TMCStepper> 0.7.1
|   |-- <SoftwareSerialM> 1.0.0
|-- <U8glib-HAL> 0.4.3
|   |-- <Wire> 1.0
|-- <STM32ADC> 1.0
|-- <EEPROM>
|-- <USBComposite for STM32F1> 0.99
|-- <Wire> 1.0
|-- <Servo(STM32F1)> 1.1.2
Building in release mode
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/HAL.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/HAL_SPI.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/MarlinSerial.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/SPI.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/Servo.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/dogm/u8g_com_stm32duino_swspi.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/eeprom_bl24cxx.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/eeprom_flash.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/eeprom_if_iic.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/eeprom_sdcard.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/eeprom_wired.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/fast_pwm.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/msc_sd.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/onboard_sd.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/sdio.cpp.o
Compiling .pio/build/STM32F103RC_btt_512K/src/src/HAL/STM32F1/tft/tft_fsmc.cpp.o
Linking .pio/build/STM32F103RC_btt_512K/firmware.elf
Checking size .pio/build/STM32F103RC_btt_512K/firmware.elf
Building .pio/build/STM32F103RC_btt_512K/firmware.bin
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [===       ]  29.4% (used 14448 bytes from 49152 bytes)
Flash: [====      ]  44.2% (used 231844 bytes from 524288 bytes)
================================================================================================================= [SUCCESS] Took 5.64 seconds =================================================================================================================

Environment           Status    Duration
--------------------  --------  ------------
STM32F103RC_btt_512K  SUCCESS   00:00:05.635

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

You don't need to clean VScode. You can clean the project via platformio run -e STM32F103RC_btt_512K -t clean. I don't remember exactly, but if you have Platformio extension, I think there's a button in the status bar for that.

@qwewer0
Copy link
Contributor

qwewer0 commented Feb 5, 2021

So, I'm not sure about this:

~/.platformio/packages/toolchain-gccarmnoneeabi/bin/arm-none-eabi-objdump -tT -C .pio/build/STM32F103RC_btt_512K/firmware.elf | grep "::print" | grep Serial
grep : The term 'grep' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
At line:1 char:129
+ ... mp -tT -C .pio/build/STM32F103RC_btt_512K/firmware.elf | grep "::prin ...
+                                                              ~~~~
    + CategoryInfo          : ObjectNotFound: (grep:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

After the cleanup, using my #20985 (comment) config.

Bugfix:
RAM:   [===       ]  29.4% (used 14448 bytes from 49152 bytes)
Flash: [====      ]  44.3% (used 232204 bytes from 524288 bytes)

(already committed) Patch:
RAM:   [===       ]  29.4% (used 14448 bytes from 49152 bytes)
Flash: [====      ]  44.3% (used 232460 bytes from 524288 bytes)

The two line changed:
RAM:   [===       ]  29.4% (used 14448 bytes from 49152 bytes)
Flash: [====      ]  44.2% (used 231844 bytes from 524288 bytes)

It seems that I used a bit different config, sorry for the inconvenience.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Feb 5, 2021

Ok, that was crazy!

For what it's worth, I've found why there's still a Print::print in your binary. It comes from Marlin/src/lcd/dogm/lcdprint_u8g.cpp, more specifically the u8g.print() lines in there (as stated by:

~/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/9.3.1/../../../../arm-none-eabi/bin/ld: .pio/build/STM32F103RC_btt_512K/FrameworkArduino/IPAddress.cpp.o: reference to _ZN5Print5printEc
~/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/9.3.1/../../../../arm-none-eabi/bin/ld: .pio/build/STM32F103RC_btt_512K/FrameworkArduino/Print.cpp.o: definition of _ZN5Print5printEc
~/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/9.3.1/../../../../arm-none-eabi/bin/ld: .pio/build/STM32F103RC_btt_512K/src/src/lcd/dogm/lcdprint_u8g.cpp.o: reference to _ZN5Print5printEc

u8g happens to be U8GLIB_ST7920_128X64_RRD in your configuration and this is a SPI instance. SPI instance still use the Arduino's print stuff, so we end up with both print method here.

I'm not sure if there are many platform using print with SPI's instance (it's a strange practice I think to output ASCII text on SPI), but for those, the saving in this PR will not cover the dual print code overhead. I can't fix for them.

At least the mystery is solved!

Remove a leftover code from a bad merge in another PR
@qwewer0
Copy link
Contributor

qwewer0 commented Feb 5, 2021

The +16 bytes of RAM was because I had CANCEL_OBJECTS enabled on my end.

@rhapsodyv
Copy link
Sponsor Member

Could this be related with the serial refactoring?

#21010

@thinkyhead thinkyhead merged commit e7c7119 into MarlinFirmware:bugfix-2.0.x Feb 8, 2021
@thinkyhead
Copy link
Member

Let's see what shakes out as a result of this general improvement.

rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 11, 2021
* 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 added a commit to rmpel/Marlin that referenced this pull request Feb 13, 2021
…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
  ...
rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 13, 2021
* 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
  ...
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 26, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Mar 11, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
…20985)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
@thinkyhead
Copy link
Member

thinkyhead commented Aug 16, 2021

Unfortunately this PR invented its own concept for SERIAL_ECHOLIST and in the process eliminated SERIAL_ECHOLIST_N, which takes a count and a list of values. Note that SERIAL_ECHOLIST_N was only ever used in one file throughout all of Marlin. So now in order to do debugging in that file I am forced to spend extra time creating a new SERIAL_ECHOLIST_N … and extra time composing this message admonishing contributors to be please strive to be less clever and make smaller incremental changes so that we can more easily evaluate them.

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.

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.

None yet

6 participants