Marlin: [BUG] Significant STM32F1 flash size growth introduced by c369477cb0

[comps]

Description

I build bugfix-2.0.x for a STM32F103CB board (128K of flash) and, until recently, everything fit, including UBL, even though just barely:

Calculating size .pioenvs/STM32F1/firmware.elf
text       data     bss     dec     hex filename
113704     3696    9936  127336   1f168 .pioenvs/STM32F1/firmware.elf

On recent bugfix-2.0.x, it started failing with

Compiling .pioenvs/STM32F1/src/src/sd/usb_flashdrive/lib/parsetools.cpp.o
Compiling .pioenvs/STM32F1/src/src/sd/usb_flashdrive/usb_host.cpp.o
Linking .pioenvs/STM32F1/firmware.elf
<snip>/arm-none-eabi/bin/ld: .pioenvs/STM32F1/firmware.elf section `.text' will not fit in region `rom'
<snip>/arm-none-eabi/bin/ld: region `rom' overflowed by 31360 bytes

Even though almost 32K seemed too much to be suddenly introduced by a config-less code, I tried reducing it down by disabling UBL and EEPROM_SETTINGS and I managed to get it down some:

region `rom' overflowed by 11888 bytes

However this seemed like an overkill, so I started bisecting recent commits and arrived on c369477cb0 - reverting it on latest (211563e53) bugfix-2.0.x allows my config to compile again, and with UBL and EEPROM enabled too!

Compiling .pioenvs/STM32F1/src/src/module/tool_change.cpp.o
Compiling .pioenvs/STM32F1/src/src/sd/cardreader.cpp.o
Compiling .pioenvs/STM32F1/src/src/sd/usb_flashdrive/Sd2Card_FlashDrive.cpp.o
Linking .pioenvs/STM32F1/firmware.elf
Calculating size .pioenvs/STM32F1/firmware.elf
text       data     bss     dec     hex filename
114256     3696    9936  127888   1f390 .pioenvs/STM32F1/firmware.elf

...
I started looking further, but I don't know the codebase well enough to assess whether static SdFile newDir1, newDir2; could really eat 32K or whether the problem is elsewhere than in the modified code.

Additional Information

Diff online at https://gist.github.com/comps/373ac8f059cf18df03224beaaef24f5a , the modified platformio.ini might be an overkill (using lib_ignore is probably better), but this works as a reproducer.

configs-comps.zip

[Roxy-3D]

How much did the code grow? Looking at the commit, the most expensive thing about it is:

  static SdFile newDir1, newDir2;
  SdFile *sub = &newDir1, *startDir;

I need to know how much the code grew in order to go any further.

[comps]

How much did the code grow? Looking at the commit, the most expensive thing about it is:

How exactly would I determine that? I posted the sizes returned during linking in the original post - I can't get the actual size with the commit as the linking just fails (see the post for how much it overflows).

[robbycandra]

How much did the code grow? Looking at the commit, the most expensive thing about it is:

  static SdFile newDir1, newDir2;
  SdFile *sub = &newDir1, *startDir;

I need to know how much the code grew in order to go any further.

Thats my PR...
I try to compile before and after the commit with SD SUPPORT Enabled.
The result
Before : Data = 3.557, Program = 66.250
After : Data = 3.627, Program = 66.366

So this commit takes 70 byte Data and 116 Program memory.

[robbycandra]

[comps]

I managed to build it for another STM32F1-based MCU that has more than 128K of flash (STM32F103RE), to see what effect the commit has on 32-bit code:

Latest unchanged bugfix-2.0.x (211563e533f4), eg. with the commit:

text       data     bss     dec     hex filename
159892     4200   10136  174228   2a894 .pioenvs/STM32F1/firmware.elf

(firmware.bin has 164092 bytes)

and git-reverting the single commit:

text       data     bss     dec     hex filename
115420     4192   10024  129636   1fa64 .pioenvs/STM32F1/firmware.elf

(firmware.bin has 119612 bytes)

So there seems to be a bigger impact on 32bit code.

[Roxy-3D]

We might need you to generate that firmware again with and without the commit. But generate a detailed link map. Something is causing an extra 44KB of code to be pulled into the image. Probably, some extra library module or something like that is getting pulled in. (Because those new lines of code don't generate any where near that much code space.)

[robbycandra]

I do revert:

Before Revert

After Revert

The PR is Fix SDCard Subdirectory processing, its will not take that much memory.

[tpruvot]

ls -al ./.pioenvs/alfawise_U30/src/src/sd/*.o

26280 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/cardreader.cpp.o
788 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/Sd2Card.cpp.o
19028 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/SdBaseFile.cpp.o
1028 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/SdFatUtil.cpp.o
2320 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/SdFile.cpp.o
4880 mai 13 16:04 ./.pioenvs/alfawise_U30/src/src/sd/SdVolume.cpp.o

remove -g -ggdb in the top of platformio.ini to get small sizes

[comps]

Here are the full pio run logs as I don't know how else to extract a detailed link map:

• bugfix-2.0.x on the commit posted above
• same, with the commit reverted
• generated unified diff between them
• the same diff, with lines reordered, as I couldn't figure out how to force pio run in single job mode

As you can see, there's no real change in terms of what is compiled or linked.

As for object file sizes, they indeed don't change much, upstream:

  7112  .pioenvs/STM32F1/src/src/sd/Sd2Card.cpp.o
 18104  .pioenvs/STM32F1/src/src/sd/SdBaseFile.cpp.o
  1028  .pioenvs/STM32F1/src/src/sd/SdFatUtil.cpp.o
  2320  .pioenvs/STM32F1/src/src/sd/SdFile.cpp.o
  4920  .pioenvs/STM32F1/src/src/sd/SdVolume.cpp.o
 20916  .pioenvs/STM32F1/src/src/sd/cardreader.cpp.o

and reverted:

  7112  .pioenvs/STM32F1/src/src/sd/Sd2Card.cpp.o
 18104  .pioenvs/STM32F1/src/src/sd/SdBaseFile.cpp.o
  1028  .pioenvs/STM32F1/src/src/sd/SdFatUtil.cpp.o
  2320  .pioenvs/STM32F1/src/src/sd/SdFile.cpp.o
  4920  .pioenvs/STM32F1/src/src/sd/SdVolume.cpp.o
 19740  .pioenvs/STM32F1/src/src/sd/cardreader.cpp.o

I also checked all the build dirs and there is no significant change other than the final linked file, so this might be due to a library being pulled in despite platformio saying it isn't. Or it might be due to some compiler weirdness.

--- du-upstream 2019-05-13 19:34:34.053655199 +0200
+++ du-reverted 2019-05-13 19:33:54.268269360 +0200
@@ -250,7 +250,7 @@
 2424   .pioenvs/STM32F1/lib853/EEPROM/flash_stm32.c.o
 25849  .pioenvs/STM32F1/lib853/EEPROM
 39767  .pioenvs/STM32F1/lib853
-164092 .pioenvs/STM32F1/firmware.bin
+119612 .pioenvs/STM32F1/firmware.bin
 15747  .pioenvs/STM32F1/src/src/HAL/HAL_STM32F1/HAL_Servo_STM32F1.cpp.d
 13609  .pioenvs/STM32F1/src/src/HAL/HAL_STM32F1/HAL_timers_STM32F1.cpp.d
 116    .pioenvs/STM32F1/src/src/HAL/HAL_STM32F1/HAL_sdio_STM32F1.cpp.d
@@ -758,8 +758,8 @@
 14855  .pioenvs/STM32F1/src/src/sd/Sd2Card.cpp.d
 14972  .pioenvs/STM32F1/src/src/sd/SdVolume.cpp.d
 2320   .pioenvs/STM32F1/src/src/sd/SdFile.cpp.o
-20916  .pioenvs/STM32F1/src/src/sd/cardreader.cpp.o
-171685 .pioenvs/STM32F1/src/src/sd
+19740  .pioenvs/STM32F1/src/src/sd/cardreader.cpp.o
+170509 .pioenvs/STM32F1/src/src/sd
 1100   .pioenvs/STM32F1/src/src/lcd/ultralcd.cpp.d
 788    .pioenvs/STM32F1/src/src/lcd/extensible_ui/ui_api.cpp.o
 788    .pioenvs/STM32F1/src/src/lcd/extensible_ui/lib/example.cpp.o
@@ -888,9 +888,9 @@
 14910  .pioenvs/STM32F1/src/src/module/servo.cpp.d
 26936  .pioenvs/STM32F1/src/src/module/planner.cpp.o
 337081 .pioenvs/STM32F1/src/src/module
-4387872        .pioenvs/STM32F1/src/src
-4391968        .pioenvs/STM32F1/src
-414884 .pioenvs/STM32F1/firmware.elf
+4386696        .pioenvs/STM32F1/src/src
+4390792        .pioenvs/STM32F1/src
+346492 .pioenvs/STM32F1/firmware.elf
 10976  .pioenvs/STM32F1/lib9a5/30aa480/LiquidTWI2.cpp.o
 12689  .pioenvs/STM32F1/lib9a5/30aa480/LiquidTWI2.cpp.d
 27761  .pioenvs/STM32F1/lib9a5/30aa480
@@ -924,4 +924,4 @@
 11458  .pioenvs/STM32F1/lib756/STM32ADC/utility
 28754  .pioenvs/STM32F1/lib756/STM32ADC
 43862  .pioenvs/STM32F1/lib756
-6803285        .pioenvs/STM32F1/
+6689237        .pioenvs/STM32F1/

[comps]

So I've ruled out my git setup and the function definition change in the commit, will try rewriting the code bit-by-bit tomorrow and if that doesn't narrow it down, I'll dissect the two .elf binaries to see what makes up the actual difference. This indeed seems like a compiler bug/feature. I'm on gcc v8.3.1 (Fedora).

[ghost]

Like @Roxy-3D said, consider in platformio.ini

build_flags = -fmax-errors=5
  -g
  -ggdb
  -Wl,-Map,output.map

I know the -g and -ggdb is a point for a separate discussion, not the point right now.
But then let's have a look at the output.map, you'll find it in the same dir as platformio.ini
Make two, one before, one after and then diff.

[Roxy-3D]

But then let's have a look at the output.map, you'll find it in the same dir as platformio.ini
Make two, one before, one after and then diff.

Most the two maps will be identical in placement and size. But we are going to see something extra being pulled into the image. We probably won't need to do this, but once we know what is being pulled in, at worst case we can look at the compiler generated assembly code and see what specific line is pulling in the extra stuff.

[comps]

The map diff isn't incredibly useful due to the addresses being shifted, but you can see something from the first part at least: https://gist.github.com/comps/61e8570738c384a4077835b34c4bef33

The same thing can be however more cleanly seen when dumping the ELF symbol table using readelf and looking at STT_FILE:

--- binary-reverted/readelf     2019-05-14 12:02:29.296525228 +0200
+++ binary-upstream/readelf     2019-05-14 12:02:33.922566336 +0200
@@ -104,3 +104,6 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS cardreader.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS class_type_info.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS closer.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS configuration_store.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS cp-demangle.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS crtstuff.c
@@ -108,2 +111,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS cxa_atexit.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS del_ops.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS dma.c
@@ -112,2 +116,14 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS ef_sqrt.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_alloc.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_arm.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_call.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_catch.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_exception.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_globals.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_personality.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_term_handler.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_terminate.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_throw.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_type.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS eh_unex_handler.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS endstops.cpp
@@ -116,4 +132,13 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS exti.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fclose.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fflush.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS findfp.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS fini.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS flash.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fputc.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fputs.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fstatr.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fvwrite.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fwalk.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS fwrite.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS gcode.cpp
@@ -123,2 +148,4 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS gpio_f1.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS guard.cc
+00000000      0 FILE    LOCAL  DEFAULT      ABS guard_error.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS hooks.c
@@ -128,2 +155,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS init.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS isattyr.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS iwdg.c
@@ -135,2 +163,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS libgcc2.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS libunwind.o
 00000000      0 FILE    LOCAL  DEFAULT      ABS locale.c
@@ -138,3 +167,5 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS lock.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS lseekr.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS main.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS makebuf.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS malloc.c
@@ -153,2 +184,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS mprec.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS new.cpp
 00000000      0 FILE    LOCAL  DEFAULT      ABS nvic.c
@@ -157,2 +189,4 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS planner.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS pr-support.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS putc.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS pwm.cpp
@@ -161,2 +195,4 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS rcc_f1.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS readr.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS realloc.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS reent.c
@@ -174,2 +210,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS sf_trunc.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS si_class_type_info.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS spi.c
@@ -178,2 +215,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS start_c.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS stdio.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS stepper.cpp
@@ -197,2 +235,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS timer.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS tinfo.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS tolower.c
@@ -203,2 +242,3 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS ubl_motion.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS unwind-arm.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS usart.c
@@ -218,3 +258,5 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS vfprintf.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS vterminate.cc
 00000000      0 FILE    LOCAL  DEFAULT      ABS watchdog_STM32F1.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS wbuf.c
 00000000      0 FILE    LOCAL  DEFAULT      ABS wctomb_r.c
@@ -224,2 +266,4 @@
 00000000      0 FILE    LOCAL  DEFAULT      ABS wirish_time.cpp
+00000000      0 FILE    LOCAL  DEFAULT      ABS writer.c
+00000000      0 FILE    LOCAL  DEFAULT      ABS wsetup.c
 00000000      0 NOTYPE  LOCAL  DEFAULT    UNDEF 

These are getting pulled in for some reason in current bugfix-2.0.x and this simple change makes them go away: https://gist.github.com/comps/c09a6d2601b6d5beed2b9adefdcba2fe .

I'll debug further. Maybe the close() call triggers this, maybe the early return in the other code version.

PS: I already don't use -g / -ggdb, see the config diff my original post (includes platformio.ini). I also tried removing my -Os, but the problem didn't change.

[tpruvot]

i remember there are some tools to inspect the .a content but need to refind what. I know you can do that on windows .lib opening them with 7-zip
Anyway i think you found the cause already.

[robbycandra]

[tpruvot]

else i remember some gcc/g++ linker flag about optimized linkage... "-flto" if i remember right

[comps]

Okay, I don't know how I overlooked it before, but the problem is quite obvious and the fix (at least to compile it small) too:

diff --git a/Marlin/src/sd/cardreader.cpp b/Marlin/src/sd/cardreader.cpp
index 51702fa22..3574e0b4f 100644
--- a/Marlin/src/sd/cardreader.cpp
+++ b/Marlin/src/sd/cardreader.cpp
@@ -643,7 +643,7 @@ uint16_t CardReader::getnrfilenames() {
  */
 const char* CardReader::diveToFile(SdFile*& curDir, const char * const path, const bool echo/*=false*/) {
   // Track both parent and subfolder
-  static SdFile newDir1, newDir2;
+  SdFile newDir1, newDir2;
   SdFile *sub = &newDir1, *startDir;

   const char *dirname_start = path;

Why do we need a static variable there?

[robbycandra]

Because curDir will be used to open file. therefore it need to be static.

[robbycandra]

@comps....
This commit is needed to handel workingDir and workDirDepth.
Before these commit, if we open file from sub Dir.. the workdir is not processed like when we open file from LCD.

Yes, this commit add few bytes, but i dont think its that big. And this commit is needed for feature like Power Loss Recovery. Without this commit, Print will not resume if we open file using M23.

[comps]

I'm not asking anyone to permanently revert this commit, I'm just trying to figure out where the problem is. The few bytes are not a problem, but the extra 44K are as they prevent using Marlin on the only 32-bit board that costs less than $2 that I know of.
I'm trying to figure out if this is a .bss alignment issue caused by declaring the variables as static or what the heck is going on with the build.

[Roxy-3D]

I'm trying to figure out if this is a .bss alignment issue caused by declaring the variables as static or what the heck is going on with the build.

Removing the static should be OK. At least, that is my 90 second analysis of it:

const char* CardReader::diveToFile(SdFile*& curDir, const char * const path, const bool echo/*=false*/) {
  // Track both parent and subfolder
  static SdFile newDir1, newDir2;
  SdFile *sub = &newDir1, *startDir;

If you pull the static defination from newDir1 and newDir2, it will just make them automatic storage.
(I could be wrong... Please make the change and test it for correct operation!!!)
If you pull that declaration... all the global data space needed for newDir1 and newDir2 disappears. You still need enough room on the stack for them to be instantiated within the scope of diveToFile(), but that space is reclaimed at the end of the function.

This is worth while to do if it works... It will save a lot of RAM space. But the thing is, this would not explain the growth in code size. newDir1 and newDir2 are variables. They reside in RAM. So... it is most likely this is not the root cause of why the code image grew 44KB.

[robbycandra]

At first there are only 1 SDFile non static variable.

I make it 2 variable because subDir and curDir cannot point to same varible.
// Open curDir
if (!sub->open(curDir, dosSubdirname, O_READ)) {
before this PR, open always return error because of it.

After exit, curDir pointer still point to this SDFile variable inside this Dive function. And its needed to open the file. Therefore if we use non static variable, the openfile will fail.

[robbycandra]

@comps , what about trying to compile with default configuration + enable SDSupport only? how big is it?

[Roxy-3D]

Therefore if we use non static variable, the openfile will fail.

Just as an FYI: so you can understand how I'm thinking and how Marlin's data structures are optimized.

There is no difference between a static variable and a global variable. The instructions GCC generate are the same. The only difference is declaring some variable static means it can only be seen and referenced from within that .cpp file. But for all practical purposes... The generated code will treat it like a global variable.

What I'm saying is... You should drop the static and move the declaration outside the procedure. Doing that will make them 'global' and I'm 99% sure it is going to function exactly like it used to function.

[robbycandra]

@Roxy-3D , I understand.. move it outside function and make it global.

[tpruvot]

Could someone explain me in which case there are subfolders on the SD ?

Couldnt that be put in an optional function define ?

[comps]

I feel like this is just digging deeper into the global variable hell that CardReader already is (with root, workDir, etc.), but I can indeed confirm that this "fixes" the problem for me (as in: compiles small, I can't test it yet):

diff --git a/Marlin/src/sd/cardreader.cpp b/Marlin/src/sd/cardreader.cpp
index 51702fa22..329b28da1 100644
--- a/Marlin/src/sd/cardreader.cpp
+++ b/Marlin/src/sd/cardreader.cpp
@@ -59,6 +59,7 @@ int8_t CardReader::autostart_index;

 SdFile CardReader::root, CardReader::workDir, CardReader::workDirParents[MAX_DIR_DEPTH];
 uint8_t CardReader::workDirDepth;
+SdFile CardReader::newDir1, CardReader::newDir2;

 #if ENABLED(SDCARD_SORT_ALPHA)
   uint16_t CardReader::sort_count;
@@ -643,7 +644,6 @@ uint16_t CardReader::getnrfilenames() {
  */
 const char* CardReader::diveToFile(SdFile*& curDir, const char * const path, const bool echo/*=false*/) {
   // Track both parent and subfolder
-  static SdFile newDir1, newDir2;
   SdFile *sub = &newDir1, *startDir;

   const char *dirname_start = path;
diff --git a/Marlin/src/sd/cardreader.h b/Marlin/src/sd/cardreader.h
index 2d5bc5eca..b91d54135 100644
--- a/Marlin/src/sd/cardreader.h
+++ b/Marlin/src/sd/cardreader.h
@@ -156,6 +156,7 @@ public:
 private:
   static SdFile root, workDir, workDirParents[MAX_DIR_DEPTH];
   static uint8_t workDirDepth;
+  static SdFile newDir1, newDir2;

   // Sort files and folders alphabetically.
   #if ENABLED(SDCARD_SORT_ALPHA)

Some optimization would be useful in the future.

[robbycandra]

@tpruvot ,

Couldnt that be put in an optional function define?

no, cannot. Its a fix not feature.

Could someone explain me in which case there are subfolders on the SD ?

if you save your gcode file in subfolder and print it from there.

[thinkyhead]

Compilers vary in their output, and there is more at play than just these variables. Here's a sample result from compiling the old code with static local variables versus the updated code with static member variables:

Compiling with Arduino 1.8.9:

Before "fix":  99344 (39%) / 3900 (47%)
 After "fix":  99356 (39%) / 3916 (47%)

Moving the variables to class variables increased the SRAM usage by 16 bytes and the code by 12 bytes.

So it's not a fix for anything. It just makes the compiler generate "different" code.

[Roxy-3D]

This thread is closed.... But that doesn't mean we can't benefit from more knowledge on this topic. If somebody figures this out, please post what is going on. Mean while...

@comps
In Post #https://github.com/MarlinFirmware/Marlin/issues/14007#issuecomment-491899153

I said:

We might need you to generate that firmware again with and without the commit. But generate a detailed link map. Something is causing an extra 44KB of code to be pulled into the image. Probably, some extra library module or something like that is getting pulled in. (Because those new lines of code don't generate any where near that much code space.)

If you post those two link maps... We will figure this out pretty quick...

[comps]

@Roxy-3D I thought I already did above: https://github.com/MarlinFirmware/Marlin/issues/14007#issuecomment-492182192 (see the gist link, not the diff below it). At least that's the difference between the two link maps.

@thinkyhead There's probably a lot more to be saved just by not having a full blown SdFile for every path element up to MAX_DIR_DEPTH statically allocated, though the change above indeed isn't an actual "fix".

[robbycandra]

@comps in the end, what feature you enable in 128K board ?

[comps]

@robbycandra See the gist in Additional Information in the first post for diffs against default Marlin configs, or the zip file with the config files directly.
Some cleanup needed, I'm still working on the thing, but the goal is to have a modular stack of "prototyping" PCBs with the "blue pill" board on top, giving me (and my kids) a platform to easily experiment on - easily add more drivers, etc. Sort of like Tinkerforge, but all DIY and at a fraction of the cost.

[Roxy-3D]

I thought I already did above: #14007 (comment) (see the gist link, not the diff below it). At least that's the difference between the two link maps.

No... That is not what I'm looking for. The detailed link map will have the address of every public symbol. Both code and data. It will be _VERY_ large. Something is being pulled in that is bloating the firmware image by 44 Kb.

[comps]

@Roxy-3D Then please tell me exactly what you're looking for and how to get it. The gist link mentioned above, https://gist.github.com/comps/61e8570738c384a4077835b34c4bef33 , is a diff of the -Map outputs suggested by @FanDjango above. Here are both of the source files that made the diff, in case you want to take a look directly:

• upstream-output-map.txt
• reverted-output-map.txt

[robbycandra]

LOAD /home/user/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/7.2.1/../../../../arm-none-eabi/lib/thumb/v7-m/libnosys.a

[robbycandra]

/home/user/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/7.2.1

[comps]

@thinkyhead Please reply whether closing this without the fix/workaround from #14026 was intentional or a process mistake. The current state is still broken, so it would make sense to either

• reopen this bug
• confirm that you don't want #14026 merged and don't want the leave issue otherwise open

Thanks.

[comps]

If anybody is still following this dead issue, the solution in the end was -fno-threadsafe-statics. It turns out gcc was trying to generate thread-safe code for local statics and this pulled in the extra ~40 KB. Nobody in gcc upstream probably noticed this as local static variables were abolished in the 90's in most societies.

[robbycandra]

@comps, i am glad you finally have a solution for this problem.

[tpruvot]

Its maybe "unrelated" to this issue but... i finally found how to override the ststm32 sdk c++11 standard

i had to add this to platformio.ini :

build_unflags = -std=gnu++11
build_flags = !python Marlin/src/HAL/HAL_STM32F1/STM32F1_flag_script.py
${common.build_flags} -std=gnu++14

that fixes the FLOOR static assert => floorf() is not constant error (required for zprobe config)

fno-threadsafe-statics is already set in this .py script but only for the linker... and unfortunately, its only compatible with g++ (gcc will do a warning with this flag on libs .c files)

[comps]

fno-threadsafe-statics is already set in this .py script but only for the linker... and unfortunately, its only compatible with g++ (gcc will do a warning with this flag on libs .c files)

It's in the .py alright, but AFAICT it's never actually used when called like it's shown in platformio.ini, you would need to import it as a python module which I don't think platformio does. The file hints at "SCons" which seems to be some other build environment.

[pscrespo]

If anybody is still following this dead issue, the solution in the end was -fno-threadsafe-statics.

I was about to post an issue found this through search. There was a huge difference in size and this solved it:

Head at e5438a9a034f2a9ae12c0e2f4b04c1d4aee1fe33 (last commit before c369477cb0267af9c15ef6045448d7fb3340ecbc):

Linking .pioenvs/malyanm200/firmware.elf
Checking size .pioenvs/malyanm200/firmware.elf
Memory Usage -> http://bit.ly/pio-memory-usage
DATA:    [====      ]  41.6% (used 8520 bytes from 20480 bytes)
PROGRAM: [=======   ]  71.0% (used 93048 bytes from 131072 bytes)
Building .pioenvs/malyanm200/firmware.bin
========================================== [SUCCESS] Took 49.66 seconds ==========================================

Head at c369477cb0267af9c15ef6045448d7fb3340ecbc:

Linking .pioenvs/malyanm200/firmware.elf
/home/osboxes/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/7.2.1/../../../../arm-none-eabi/bin/ld: .pioenvs/malyanm200/firmware.elf section `.rodata' will not fit in region `rom'
/home/osboxes/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/7.2.1/../../../../arm-none-eabi/bin/ld: region `rom' overflowed by 6480 bytes
collect2: error: ld returned 1 exit status
*** [.pioenvs/malyanm200/firmware.elf] Error 1
=========================================== [ERROR] Took 54.77 seconds ===========================================

Head at c369477cb0267af9c15ef6045448d7fb3340ecbc with -fno-threadsafe-statics:

Linking .pioenvs/malyanm200/firmware.elf
Checking size .pioenvs/malyanm200/firmware.elf
Building .pioenvs/malyanm200/firmware.bin
Memory Usage -> http://bit.ly/pio-memory-usage
DATA:    [====      ]  42.0% (used 8600 bytes from 20480 bytes)
PROGRAM: [=======   ]  71.1% (used 93200 bytes from 131072 bytes)
========================================== [SUCCESS] Took 51.28 seconds ==========================================

Now I can make sdcard work again in the "bluepill" (F103CB) board. Thank you!

[robbycandra]

@pscrespo , you have to read this whole thread, then you will find the answer.

[pscrespo]

@pscrespo , you have to read this whole thread, then you will find the answer.

Exactly what I did, that's the reason I wrote the last sentence in the comment. :blush:

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.