Better Info on flash & RAM requirements

[maribu]

Contribution description

This PR adds the make targets print-sizebars and info-sizebars. Both display a horizontal stacked bar diagram of the RAM/ROM usage, in case the variables RAM_LEN and ROM_LEN are set. If BOOTLOADER_SIZE is set, it will also be taken into account. While make print-sizebars will rebuild the app and show the diagram, make info-sizebars will do so without a rebuild.

Testing procedure

Go into your favorite RIOT application and run for a couple of boards

make BOARD=foo print-sizebars

and

make
make BOARD=foo info-sizebars

The diagrams should appear for all ATmega based and STM32 based boards.

Issues/PRs references

None

---

Update: Rewrote the description to match the current contribution of this PR, after #9781 implemented a check for overflow of RAM/ROM during link time.

[Josar]

@maribu for your size question see #9028, but only for AVR MCU.

[cladmi]

I like the idea but have some issues with the implementation.

Some remarks before testing:

It would be better to add new targets for the output. The output of print-size is I think parsed at least by murdock, and maybe used by others.
It would be safer to separate it first. It is easier to add new things that change existing ones. And making it the default, if it should, could be a dedicated PR.

For the script, python3 only, don't bother. It is already enforced by many of our scripts.

Regarding the new variables, there are already similar variables in cpu/cortexm_common/Makefile.include and re-using the same name would be better for future work on OTA.

They would be ROM_LEN, RAM_LEN ROM_OFFSET.

I will give feedback on the python inline.

[maribu]

@Josar: Thanks for the hint. Care to provide documentation/example on how to generate the output you mentioned in the PR?

It is very unfortunate that in my PR the variable SIZE_FLAGS is used when already SIZEFLAGS is arround. Am I correct that you would override both SIZE and SIZE_FLAGS variables to get the output? So my PR would break this feature and should be updated to still allow running a custom size command - if this PR does still make any sense at all.

[maribu]

@cladmi: Thanks for the feedback.

So is this correct?

• ROM_LEN = available total flash size in Bytes
• RAM_LEN = available total RAM size in Bytes
• ROM_OFFSET = Number of bytes on the flash not available to RIOT (e.g. because a bootloader is stored there)

Using a new target would also solve the problem of breaking with the PR of @Josar.

Maybe make flash could still depend on the new target? The motivation of this PR is that I had trouble debugging a strange issue that turned out to be using more RAM/ROM than available. I really would like "make flash" to fail, if the application cannot work correctly because of more RAM/ROM used than available. (And warn if the estimated usage is close to the available RAM/ROM, as exact RAM/ROM usage is more complex than just summing up the sections - even without having dynamic memory management in mind).

Btw: I'm really bad at giving names. Any suggestion for a name of the new target?

[Josar]

@maribu i added a description how to use the SIZEFLAGS to the PR.

[maribu]

@Josar: The -C flag for avr-size seems to be unoffical hack according to this German forum, that was applied to WinAVR. So there is value in having a way to get size information on vanilla binutils as well. I will wait for cladmi's feedback and than update the PR to still allow avr-size -C were available.

Additionally, maybe other targets than AVR can profit, in case no overflow detection is available there, too.

[Josar]

I also think all platform would benefit from such a solution.

I know the -C flag is a specific avr-size feature.

[cladmi]

@maribu that is right for the variables

For the output, KiB is not better than Bytes, as for some PRs number of bytes difference must be checked to know increased code size and also detect impact of build system change.
I personally like enough the text data bss dec hex format and I use dec and hex too.
But if it is a separate command it is just personal taste and would not remove the other output.

I do not have the avr compiler here to test the size features and bar features yet.

To be sure, do I understand correctly if I say that for arm this already handled by the linker checks size and would then not produced an elf and so the new-print could not be run ?

make -C tests/unittests BOARD=samr21-xpro
/usr/lib/gcc/arm-none-eabi/8.1.0/../../../../arm-none-eabi/bin/ld: /home/cladmi/git/work/RIOT/tests/unittests/bin/samr21-xpro/tests_unittests.elf section `.text' will not fit in region `rom'                 
/usr/lib/gcc/arm-none-eabi/8.1.0/../../../../arm-none-eabi/bin/ld: /home/cladmi/git/work/RIOT/tests/unittests/bin/samr21-xpro/tests_unittests.elf section `.bss' will not fit in region `ram'                  
/usr/lib/gcc/arm-none-eabi/8.1.0/../../../../arm-none-eabi/bin/ld: region `rom' overflowed by 192060 bytes
/usr/lib/gcc/arm-none-eabi/8.1.0/../../../../arm-none-eabi/bin/ld: region `ram' overflowed by 18324 bytes

This would mean it could be only enabled for avr, which is completely fine.

If the goal is to fix the lack on rom/ram size check during link, it could be done as a post linker check and delete the elf file on failure, so something like

$(ELFFILE): $(BASELIBS)
	$(Q)$(_LINK) -o $@
	$(Q)$(POST_LINK_CHECK) $(POST_LINK_FLAGS) $@

I am also bad at names.

[maribu]

@cladmi: I added now the new targets "print-sizebars" and "info-sizebars" instead of changing existing targets. (Just like print-buildsize and info-buildsize the print version rebuilds the elf file, the info version doesn't.) The script no longer creates the table, as print-buildsize and info-buildsize already do that. It just creates the horizontal stacked bars.

Also, by using the ROM_LEN, RAM_LEN and ROM_OFFSET variables the tool seems to just work for all ARM targets I tested so far.

Finally, I added print-sizebars for all boards using cpu/atmega_common as dependency for the flash target. Thus, in case ROM_LEN and RAM_LEN are set, an check for overflown RAM/ROM is performed for AVR boards.

One thing of concern: The ifeq (,$(ROM_LEN)) lines in Makefile.include look out of place there. Maybe you can suggest a place where I can add flags for the python script. The variables ROM_LEN, RAM_LEN and ROM_OFFSET have to be available at that point.

[cladmi]

I will look more into it after the release.

I reviewed the tests results for the release, and I think many arduino errors are actually issues with an overflow that is not detected during link. So exactly what your script is trying to solve.

So I would still be in favor of also having also a $(Q)$(POST_LINK_CHECK) $(POST_LINK_FLAGS) $@ step using your script with an option to only print and fail if it detects an overflow, and maybe print in case of warnings. If you could support a mode like this it would be great :)
I could take care of the POST_LINK_CHECK PR or help integrating it.

It makes more sense for me to say that the compilation failed and not flashing or another target as the resulting firmware is invalid and should not be produced. It would also make it detected directly in murdock.

[maribu]

@cladmi: I reverted the commit adding print-sizeinfo to the flash target on AVR platform and added the "--silent" flag to the script. With that flag, only RAM/ROM overflow checks are performed. I also added the "check-buildsize" that calls the script with "--silent".

It now only needs to be added as post link check for platforms missing RAM/ROM overflow checks. Feel free to just push to this PR if you have any suggestion.

[cladmi]

Now that I had time to look, I think there is a solution for the linker check.

I am currently on an arch linux computer with avr-binutils-2.31.1 and I have this linker script used avr-ld --verbose

GNU ld (GNU Binutils) 2.31.1
  Supported emulations:
   avr2
   avr1
   avr25
   avr3
   avr31
   avr35
   avr4
   avr5
   avr51
   avr6
   avrxmega1
   avrxmega2
   avrxmega3
   avrxmega4
   avrxmega5
   avrxmega6
   avrxmega7
   avrtiny
using internal linker script:
==================================================
/* Script for -n: mix text and data on same page */
/* Copyright (C) 2014-2018 Free Software Foundation, Inc.
   Copying and distribution of this script, with or without modification,
   are permitted in any medium without royalty provided the copyright
   notice and this notice are preserved.  */
OUTPUT_FORMAT("elf32-avr","elf32-avr","elf32-avr")
OUTPUT_ARCH(avr:2)
__TEXT_REGION_LENGTH__ = DEFINED(__TEXT_REGION_LENGTH__) ? __TEXT_REGION_LENGTH__ : 8K;
__DATA_REGION_LENGTH__ = DEFINED(__DATA_REGION_LENGTH__) ? __DATA_REGION_LENGTH__ : 0xffa0;
__EEPROM_REGION_LENGTH__ = DEFINED(__EEPROM_REGION_LENGTH__) ? __EEPROM_REGION_LENGTH__ : 64K;
__FUSE_REGION_LENGTH__ = DEFINED(__FUSE_REGION_LENGTH__) ? __FUSE_REGION_LENGTH__ : 1K;
__LOCK_REGION_LENGTH__ = DEFINED(__LOCK_REGION_LENGTH__) ? __LOCK_REGION_LENGTH__ : 1K;
__SIGNATURE_REGION_LENGTH__ = DEFINED(__SIGNATURE_REGION_LENGTH__) ? __SIGNATURE_REGION_LENGTH__ : 1K;
__USER_SIGNATURE_REGION_LENGTH__ = DEFINED(__USER_SIGNATURE_REGION_LENGTH__) ? __USER_SIGNATURE_REGION_LENGTH__ : 1K;
MEMORY
{
  text   (rx)   : ORIGIN = 0, LENGTH = __TEXT_REGION_LENGTH__
  data   (rw!x) : ORIGIN = 0x800060, LENGTH = __DATA_REGION_LENGTH__
  eeprom (rw!x) : ORIGIN = 0x810000, LENGTH = __EEPROM_REGION_LENGTH__
  fuse      (rw!x) : ORIGIN = 0x820000, LENGTH = __FUSE_REGION_LENGTH__
  lock      (rw!x) : ORIGIN = 0x830000, LENGTH = __LOCK_REGION_LENGTH__
  signature (rw!x) : ORIGIN = 0x840000, LENGTH = __SIGNATURE_REGION_LENGTH__
  user_signatures (rw!x) : ORIGIN = 0x850000, LENGTH = __USER_SIGNATURE_REGION_LENGTH__
}

So should now be possible to configure these with the newer versions.

It looks included since binutils 2.26 https://github.com/pathscale/binutils/blame/5c2c133020e41fc4aadd80a99156d2cea4754b96/ld/scripttempl/avr.sc#L17-L23 and should be available in ubuntu stable (bionic): https://packages.ubuntu.com/bionic/binutils-avr

I am building a bionic docker image to check.

Even, with an old avr we may be able to just ship the new linker script to maintain compatibility for some time. This way, no need for any "post-link check" as it would be done at link time.

This would remove the need for any error case handling in the print script as no elf should be generated if there is an overflow. I will try something with this. It would change the requirements for this PR if it works.

[maribu]

@cladmi: #9781 clearly is the better approach. Should I revert the last commit adding the "check-buildsize" feature so that only the eye-candy remains? Or should I close the PR?

[cladmi]

@maribu reverting the check-buildsize I would say yes. And for the PR it is as you want.

[maribu]

@cladmi: I reverted the last commit, rebased against current master and squashed. Now the PR only provides eye-candy :-)

[maribu]

Almost forgot about this. I now rebased this PR on top of current master to solve the merge conflicts.

This PR now uses RAM_LEN, ROM_LEN and BOOTLOADER_SIZE already defined for many boards.

[maribu]

Rebased to solve the merge conflict

[maribu]

Changed python script to force using Python3, as Python2 now is officially legacy. This allowed cleaning up the script a bit.

[maribu]

OK. I would like to cross this PR of the list. Either by getting it merged, or by closing it. To me, either is fine.

[kYc0o]

@cladmi what's missing here?

[emmanuelsearch]

@cladmi ping. Seems like we converged here, no?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.