newlib: enabling floating point printf for cortex-m cpus by aabadie · Pull Request #5094 · RIOT-OS/RIOT

aabadie · 2016-03-16T17:45:45Z

Addressing one of the comment from #5078.

jnohlgard · 2016-03-16T17:48:38Z

could you do a buildsizes-diff summary?
https://github.com/RIOT-OS/RIOT/wiki/Comparing-build-sizes

You could do it for examples/hello-world and for your bmp180 driver test (which needs float printf in order to print anything)

kaspar030 · 2016-03-16T18:03:50Z

I would like to see this optional, bound to a printf_float pseudomodule.

LudwigKnuepfer · 2016-03-16T18:05:35Z

I would like to see this optional, bound to a printf_float pseudomodule.

Isn't it easier then to just document the existence of this flag?

kaspar030 · 2016-03-16T18:48:37Z

We didn't need it until now. If we add it and suddenly all builds gain some kilobytes of code, that's bloat....

LudwigKnuepfer · 2016-03-16T18:58:27Z

@kaspar030 I did not argue against making it optional.

DipSwitch · 2016-03-16T20:46:37Z

I would prefer it in a module or as well. Or a build flag like USE_FLOAT_PRINTF and SCANF ofc

DipSwitch · 2016-03-16T20:48:13Z

Which you can set in your application, and the BMP180 could check this flag to decide to printf float or val *1000 int

aabadie · 2016-03-17T08:20:00Z

Sorry for the (small) answer delay.

Here are the results based on the bmp180 test application with and without _printf_float activated.

text    data    bss dec BOARD/BINDIRBASE

6548    56  0   6604    cc2538dk
20136   132 2708    22976   without-float
26684   188 2708    29580   with-float

6548    56  0   6604    fox
18212   132 2804    21148   without-float
24760   188 2804    27752   with-float

6548    56  0   6604    frdm-k64f
18124   1156    3204    22484   without-float
24672   1212    3204    29088   with-float

6548    56  0   6604    iotlab-m3
18228   132 2804    21164   without-float
24776   188 2804    27768   with-float

6556    56  0   6612    limifrog-v1
19380   132 2796    22308   without-float
25936   188 2796    28920   with-float

6540    56  0   6596    msbiot
20524   132 2852    23508   without-float
27064   188 2852    30104   with-float

6540    56  0   6596    mulle
21008   1192    3272    25472   without-float
27548   1248    3272    32068   with-float

6548    56  0   6604    nucleo-f103
18212   132 2812    21156   without-float
24760   188 2812    27760   with-float

6540    56  0   6596    nucleo-f303
18412   132 2804    21348   without-float
24952   188 2804    27944   with-float

6540    56  0   6596    nucleo-f401
20156   132 2804    23092   without-float
26696   188 2804    29688   with-float

6556    56  0   6612    nucleo-l1
19244   132 2788    22164   without-float
25800   188 2788    28776   with-float

6548    56  0   6604    openmote-cc2538
20744   132 2964    23840   without-float
27292   188 2964    30444   with-float

6548    56  0   6604    pba-d-01-kw2x
18124   1156    3204    22484   without-float
24672   1212    3204    29088   with-float

6548    56  0   6604    remote
20768   132 2908    23808   without-float
27316   188 2908    30412   with-float

6884    56  0   6940    samr21-xpro
24676   132 2796    27604   without-float
31560   188 2796    34544   with-float

6540    56  0   6596    stm32f3discovery
18540   132 2804    21476   without-float
25080   188 2804    28072   with-float

6540    56  0   6596    stm32f4discovery
20452   132 2828    23412   without-float
26992   188 2828    30008   with-float

So there's indeed a significant memory overhead when activating this flag (~ 25%).
As suggest by @kaspar030, I can try in this PR to add a pseudo module which activate this (I'm +1 with this idea).

DipSwitch · 2016-03-17T08:28:29Z

No problem for the delay! Take your time :)

aabadie · 2016-03-17T09:29:07Z

Just pushed a commit adding a pseudo module for printf with float and added a small test application. It works on a SAMR21 board.

DipSwitch · 2016-03-17T11:49:53Z

sys/Makefile.include

 endif

+ifneq (,$(filter printf_float,$(USEMODULE)))
+    export LINKFLAGS += -u _printf_float


DipSwitch · 2016-03-17T11:52:58Z

Please add the two lines I inserted as comment. I don't know what the linker does if we call -u _printf_float and there is no nano.specs :)

jnohlgard · 2016-03-17T13:39:50Z

@DipSwitch You suggested change will execute the link test for nano specs twice, once in sys and once in cortexm_common. It's better to set an environment variable in cortexm_common and check that from sys/Makefile.include.

aabadie · 2016-03-17T14:33:33Z

I pushed a change with @gebart suggestion (not tested, I have no board with me).

kaspar030 · 2016-03-17T14:46:33Z

tests/printf_float/main.c

+ * @}
+ */
+
+#include "stdlib.h"


please use "<>" for system headers.

jnohlgard · 2016-03-17T19:07:58Z

tests/printf_float/main.c

+int main(void)
+{
+    double floating_point_value = 2016.0317;
+    char result[8] = {0};


This does not really mean what it seems like it does. See http://stackoverflow.com/questions/201101/how-to-initialize-an-array-in-c
It won't make any difference here, just a heads up.

DipSwitch · 2016-03-17T21:46:13Z

Looks good! Please squash and rename the commit to something like: arm/newlib: Add pseudomodule to enable floating point printf support

aabadie · 2016-03-17T22:14:05Z

@DipSwitch commits amended and squashed. Thanks !

DipSwitch · 2016-03-17T22:22:43Z

Good buzy!

jnohlgard · 2016-03-18T08:45:01Z

sys/Makefile.include

 endif

+ifneq (,$(filter printf_float,$(USEMODULE)))
+    ifdef USE_NANO_SPECS


IMO ifeq (1,$(USE_NANO_SPECS)) is more flexible since we can set USE_NANO_SPECS=0 if we don't want it as well.

----- Mail original -----

IMO ifeq (1,$(USE_NANO_SPECS)) is more flexible since we can set
USE_NANO_SPECS=0 if we don't want it as well.
Yes it sounds reasonable. I changed it.

jnohlgard · 2016-03-18T13:10:29Z

ACK

DipSwitch · 2016-03-18T22:19:54Z

Now all we have to do is wait for http://olsr.org/ to come back up...

aabadie · 2016-03-21T13:03:45Z

All checks have passed, seems ready for merging :)

kaspar030 · 2016-03-21T13:05:29Z

&go!

newlib: enabling floating point printf for cortex-m cpus

aabadie mentioned this pull request Mar 16, 2016

[DRIVER] bmp180 sensor driver implementation #5078

Merged

DipSwitch added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system labels Mar 16, 2016

DipSwitch added this to the Release 2016.04 milestone Mar 16, 2016

DipSwitch self-assigned this Mar 16, 2016

DipSwitch reviewed Mar 17, 2016
View reviewed changes

aabadie force-pushed the printf_float_cortex-m branch from 80663f7 to 521a3ee Compare March 17, 2016 14:31

kaspar030 reviewed Mar 17, 2016
View reviewed changes

tests/printf_float/main.c Outdated

* @}

*/

#include "stdlib.h"

Copy link
Copy Markdown

Contributor

kaspar030 Mar 17, 2016

Choose a reason for hiding this comment

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

please use "<>" for system headers.

aabadie force-pushed the printf_float_cortex-m branch from 701ea30 to 4ff3110 Compare March 17, 2016 15:20

jnohlgard reviewed Mar 17, 2016
View reviewed changes

aabadie force-pushed the printf_float_cortex-m branch 2 times, most recently from 9787fe2 to 19bcefa Compare March 17, 2016 21:36

aabadie force-pushed the printf_float_cortex-m branch from 19bcefa to 4c6b073 Compare March 17, 2016 22:11

DipSwitch added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 17, 2016

jnohlgard reviewed Mar 18, 2016
View reviewed changes

aabadie force-pushed the printf_float_cortex-m branch from 4c6b073 to cb69d5c Compare March 18, 2016 13:01

DipSwitch added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 20, 2016

aabadie force-pushed the printf_float_cortex-m branch from cb69d5c to 82e43c3 Compare March 21, 2016 10:46

arm/newlib: Add pseudomodule to enable floating point printf support

e1fcee6

aabadie force-pushed the printf_float_cortex-m branch from 82e43c3 to e1fcee6 Compare March 21, 2016 10:48

kaspar030 changed the title ~~Enabling floating point printf for cortex-m cpus~~ newlib: enabling floating point printf for cortex-m cpus Mar 21, 2016

kaspar030 added a commit that referenced this pull request Mar 21, 2016

Merge pull request #5094 from aabadie/printf_float_cortex-m

f5e1912

newlib: enabling floating point printf for cortex-m cpus

kaspar030 merged commit f5e1912 into RIOT-OS:master Mar 21, 2016

aabadie deleted the printf_float_cortex-m branch March 21, 2016 13:29

kYc0o mentioned this pull request Apr 20, 2016

floating point numbers are not displayed correctly when using printf/sprintf #17

Closed

kYc0o mentioned this pull request Jul 25, 2016

newlib-nano: Printf formatting does not work properly for some numeric types #1891

Open

This was referenced Jul 22, 2019

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS #11883

Merged

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS [backport 2019.07] #11890

Closed

Conversation

aabadie commented Mar 16, 2016

Uh oh!

jnohlgard commented Mar 16, 2016

Uh oh!

kaspar030 commented Mar 16, 2016

Uh oh!

LudwigKnuepfer commented Mar 16, 2016

Uh oh!

kaspar030 commented Mar 16, 2016

Uh oh!

LudwigKnuepfer commented Mar 16, 2016

Uh oh!

DipSwitch commented Mar 16, 2016

Uh oh!

DipSwitch commented Mar 16, 2016

Uh oh!

aabadie commented Mar 17, 2016

Uh oh!

DipSwitch commented Mar 17, 2016

Uh oh!

aabadie commented Mar 17, 2016

Uh oh!

DipSwitch Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

DipSwitch commented Mar 17, 2016

Uh oh!

jnohlgard commented Mar 17, 2016

Uh oh!

aabadie commented Mar 17, 2016

Uh oh!

kaspar030 Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

jnohlgard Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

DipSwitch commented Mar 17, 2016

Uh oh!

aabadie commented Mar 17, 2016

Uh oh!

DipSwitch commented Mar 17, 2016

Uh oh!

jnohlgard Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 18, 2016

Uh oh!

DipSwitch commented Mar 18, 2016

Uh oh!

aabadie commented Mar 21, 2016

Uh oh!

kaspar030 commented Mar 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants