Skip to content

SAUL: Reduce RAM requirements#11392

Closed
maribu wants to merge 2 commits intoRIOT-OS:masterfrom
maribu:saul
Closed

SAUL: Reduce RAM requirements#11392
maribu wants to merge 2 commits intoRIOT-OS:masterfrom
maribu:saul

Conversation

@maribu
Copy link
Member

@maribu maribu commented Apr 15, 2019

Contribution description

  • Use fmt instead of printf/stdio for output in phydat and SAUL
    • This reduces RAM requirements a bit at the cost of slightly increased ROM

Testing procedure

examples/saul should still work as expected on you favorite board.

Issues/PRs references

None


Update: Changed description, as the third commit is split off into a separate PR

maribu added 2 commits April 15, 2019 11:41
- Using fmt instead of printf reduces RAM requirements
- fmt was already a dependency of phydat_str via fmt_s16_dfp, using fmt for
  all formatted output seems to be more consistent
- Using fmt reduces RAM requirements a bit compared to stdio/printf
- Added fmt as explicit dependency of saul. (It was already an indirect
  dependency via phydat.)
@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: SAUL Area: Sensor/Actuator Uber Layer labels Apr 15, 2019
@maribu maribu requested review from leandrolanzieri and smlng April 15, 2019 09:57
@smlng
Copy link
Member

smlng commented Apr 15, 2019

I agree with the usage (and changes) of fmt the other changes I find a bit controversial. Mainly that adding a new sensor now requires to change 2 enums instead of one, however I like the general idea of having a class and start numbering from there.

TL;DR: please split this in 2 PRs one for fmt (which I would merge directly) and one for the other change which (to me) need some discussion, for instance @leandrolanzieri might have an opinion on this too.

smlng
smlng previously requested changes Apr 15, 2019
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above, I suggest to split this PR

@maribu
Copy link
Member Author

maribu commented Apr 15, 2019

I suggest to split this PR

Done, see #11394 for the stringification

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 15, 2019
@smlng
Copy link
Member

smlng commented Apr 15, 2019

I cannot see the RAM improvement, though I see an increase in ROM size. I compared this PR against master with board samr21-xpro using examples/saul and tests/saul in all cases I only saw increased text size compared to master

@maribu
Copy link
Member Author

maribu commented Apr 15, 2019

With gcc-avr in version 8.3.0 I get

   text	   data	    bss	    dec	    hex	filename
  10332	   2400	    971	  13703	   3587	with_pr.elf
  10066	   2474	    971	  13511	   34c7	wo_pr.elf

for the Arduino-Mega2560.

Basically, it depends on the application and toolchain if and how much memory is saved, as it will can only save RAM if printf does not get linked in.

Keeping in mind that e.g. GCC does not use printf() in a lot of cases (e.g. printf("%s\n", foo) will be replaced by puts(foo)), this can happen more often than one would guess when just looking at the source code. This is also why it does save RAM on the AVR. But that obviously is not the case on ARM :-/

@stale
Copy link

stale bot commented Oct 17, 2019

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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Oct 17, 2019
@smlng smlng dismissed their stale review October 21, 2019 07:40

comments addressed

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Oct 21, 2019
@smlng smlng added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines labels Oct 21, 2019
@smlng
Copy link
Member

smlng commented Oct 21, 2019

I tested and can confirm that the output stays the same, the problem is I also see a size increase of > 200B for board pba-d-01-kw2x.

Also the code is not that readable anymore, with all these separate calls to fmt print* functions.

So I'd like to get comments from others on this: @kaspar030, @haukepetersen and @leandrolanzieri ?

@maribu
Copy link
Member Author

maribu commented Oct 21, 2019

I think the issue is that with other RIOT internal modules using stdio, this will basically result in to implementations for output being used. It would make most sense to me to decide on which one we would use throughout RIOTs internals, and then potentially update each module as a separate PR.

We could put the merging on a hold until everything has been converted to see how big the improvements will end up when it is completed.

Some arguments for moving away from stdio:

  • Reduced RAM requirements
  • Link time garbage collection becomes possible with fmt_*() and print_*()
    • E.g. with stdio float support has to be explicitly turned on via a module, but when it is not added but used no errors appear during compilation. So hard to debug run time issues occur, that will confuse users unaware of the pseudo module to enable float support
  • The use ofstdio in production code is not MISRA compliant
    • Some companies care a lot about MISRA compliance, so this by itself has some merit
    • The arguments presented by MISRA to disallow use of stdio in production code do still apply, even if one generally does not care about MISRA
  • stdio has been a constant source of bugs
    • Due to its complexity in the implementation of stdio itself
    • Due to its poor API in code using it. But that has been mostly mitigated by compilers parsing the format strings and checking if the arguments match in type and number

@benpicco
Copy link
Contributor

#12305 might offer a more general solution.

@maribu
Copy link
Member Author

maribu commented Oct 29, 2019

#12305 might offer a more general solution.

For those architectures using newlib this is definitely a nice thing. But not every architecture is using newlib. Also, the memory requirements of stdio (and the missing possibility of link time garbage collection for implementations of format specifiers) are not the only reason to get rid of it.

@maribu
Copy link
Member Author

maribu commented Nov 26, 2019

I close this for now. A proper discussion on whether the move away from printf() will not be possible based on this PR. When #12305 is in and I have a demo branch to compare how not using stdio at all would compare to this (in terms of lines of code, ROM size, RAM size, ...), an inform decision on this suggestion can be made.

@maribu maribu closed this Nov 26, 2019
@maribu maribu deleted the saul branch August 23, 2020 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: SAUL Area: Sensor/Actuator Uber Layer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants