cpu/esp8266: fix stdio problems by gschorcht · Pull Request #12133 · RIOT-OS/RIOT

gschorcht · 2019-08-31T12:23:00Z

Contribution description

This PR fixes a problem with the standard output that arose in conjunction with the i2c_scan problem described in issue #12125.

The problem occurs when stdio functions puts, putchar and printf are used together with fputs or fprintf and stdout as file parameter. While outputs with puts are printed out immediately,
outputs with fputs are not printed out on linefeed but when the internal buffer of 128 bytes is full. As a result, mixing these functions scrambles the output completely.

The problem is fixed by removing the overridden stdio functions puts, putchar and printf. Instead, the corresponding newlib functions are used always. For this purpose, modules newlib, newlib_syscalls_default and stdio_uart are now enabled by default which also reduces the number of module dependency rules in Makefile.dep.

Testing procedure

Flash and test examples/default with enabled module i2c_scan with command

USEMODULE='i2c_scan' make BOARD=esp8266-esp-12x -C examples/default flash term

Without this PR the output should look like:

2019-08-31 14:17:35,993 - INFO #  i2c_scan 0
2019-08-31 14:17:35,996 - INFO # Scanning I2C device 0...
2019-08-31 14:17:36,002 - INFO # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-08-31 14:17:36,006 - INFO #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-08-31 14:17:36,006 - INFO # 
2019-08-31 14:17:36,007 - INFO # 
2019-08-31 14:17:36,007 - INFO # 
2019-08-31 14:17:36,019 - INFO # 0x00 R R R R R R R R R R R R R R E E0x10 E E E E E E E E E E E E E E E E0x20 E E E E E E E E E E E E E E E E0x30 E E E E E E E E
2019-08-31 14:17:36,020 - INFO # 
2019-08-31 14:17:36,020 - INFO # 
2019-08-31 14:17:36,021 - INFO # 
2019-08-31 14:17:36,030 - INFO #  E E E E E E E E0x40 E E E E E X E E E E E E E E E E0x50 E E E E E E E E E E E E E E E E0x60 E E E E E E E E E E E E E E E E0x70

With this PR the output should look like:

> i2c_scan 0
2019-08-31 14:21:33,249 - INFO #  i2c_scan 0
2019-08-31 14:21:33,252 - INFO # Scanning I2C device 0...
2019-08-31 14:21:33,257 - INFO # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-08-31 14:21:33,260 - INFO #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-08-31 14:21:33,263 - INFO # 0x00 R R R R R R R R R R R R R R E E
2019-08-31 14:21:33,266 - INFO # 0x10 E E E E E E E E E E E E E E E E
2019-08-31 14:21:33,271 - INFO # 0x20 E E E E E E E E E E E E E E E E
2019-08-31 14:21:33,274 - INFO # 0x30 E E E E E E E E E E E E E E E E
2019-08-31 14:21:33,277 - INFO # 0x40 E E E E E X E E E E E E E E E E
2019-08-31 14:21:33,280 - INFO # 0x50 E E E E E E E E E E E E E E E E
2019-08-31 14:21:33,283 - INFO # 0x60 E E E E E E E E E E E E E E E E
2019-08-31 14:21:33,287 - INFO # 0x70 E E E E E E E E R R R R R R R R

Issues/PRs references

Related to issue #12125.

benpicco

Works nicely on esp8266-esp-12x.The UART output looks more in line with other RIOT boards too (no double newlines)

This also saves a good chunk of ROM.

before:

   text	   data	    bss	    dec	    hex
  83275	   4324	   7304	  94903	  172b

with this patch:

   text    data     bss     dec     hex         
  73323    4324    7096   84743   14b07

benpicco · 2019-08-31T13:18:43Z

Uh, but Murdock is unhappy

Test: comparing addresses of 'printf' and 'iprintf' symbols
[FAILED] '40101cf8' != '40101cf8' is False
Makefile:34: recipe for target 'test-newlib' failed

I guess tests/libc_newlib needs an update.

gschorcht · 2019-08-31T13:20:59Z

@benpicco Thanks for testing.

Unfortunatly, Murdock is not happy yet. There are two apps that aren't compilable anymore:

tests/libc_newlib: Compilation fails since printf and iprintf have same address. I don't know at the moment, what the problem is if iprintf is simply mapped to printf.
tests/pkg_libb2: Undefined references since these functions get an suffix when they are compiled, for example blake2bSUFFIX. I have no idea why.

gschorcht · 2019-08-31T13:27:53Z

Test: comparing addresses of 'printf' and 'iprintf' symbols
[FAILED] '40101cf8' != '40101cf8' is False
Makefile:34: recipe for target 'test-newlib' failed

It is not clear for me why the test complains that addresses are equal. I'm a bit unsure if it would be the right way to blacklist ESP8266 for this test.

Maybe @cladmi can say more about this test and whether it is really a problem if it fails.

benpicco · 2019-08-31T14:03:39Z

tests/pkg_libb2 also fails on master with the same error, so this is not related to this PR.

cladmi · 2019-09-02T10:06:45Z

Test: comparing addresses of 'printf' and 'iprintf' symbols
[FAILED] '40101cf8' != '40101cf8' is False
Makefile:34: recipe for target 'test-newlib' failed
It is not clear for me why the test complains that addresses are equal. I'm a bit unsure if it would be the right way to blacklist ESP8266 for this test.

Maybe @cladmi can say more about this test and whether it is really a problem if it fails.

The goal of this test was to detect if newlib-nano is indeed used when requested.

I had no real idea how to do the detection so used the fact described in the
newlib-nano that iprintf is mapped to printf

RIOT/tests/libc_newlib/Makefile

Lines 23 to 31 in d991883

    
           # Newlib-nano removed the integer only 'iprintf' functions which are now mapped 
        
           # to printf. 
        
           # 
        
           # `main.c` uses both functions to be sure they are included. 
        
           # It also duplicates the same test in C to verify it at runtime. 
        
           # 
        
           # Source: 
        
           # 
        
           # https://github.com/32bitmicro/newlib-nano-1.0/blob/f157c994b9a2c4bd8d0cfe9fe8fdd9cd54f8c63b/newlib/README.nano#L32

However, it appears that it is not a reliable condition :'(

Could it be that esp is using newlib-nano and not newlib ?

I will try debugging a bit.

cladmi · 2019-09-02T10:24:32Z

TL;DR: Test is right, you just need to add USEMODULE += newlib_nano after the USEMODULE += newlib to declare it in the build system, and verify everything else keeps working :D

Explanation

According to this test the esp8266 newlib is using the newlib-nano specs.
So the test is correct and showing an error in the configuration.

For some toolchain it must be explicitly enabled and handled in the build system but it looks like it is by default for esp8266. However, we rely on having the newlib_nano to be in USEMODULE.

To verify this, I faked enabling newlib_nano in my test, and it detects newlib_nano:

Configure the test to expect `newlib_nano`

diff --git a/tests/libc_newlib/Makefile b/tests/libc_newlib/Makefile
index f21f0761d..f3a4e781c 100644
--- a/tests/libc_newlib/Makefile
+++ b/tests/libc_newlib/Makefile
@@ -2,6 +2,8 @@ include ../Makefile.tests_common
 
 USEMODULE += embunit
 
+CFLAGS += -DMODULE_NEWLIB_NANO=1
+
 include $(RIOTBASE)/Makefile.include
 
 # Compile time tests
@@ -20,6 +22,9 @@ ifneq (,$(filter newlib,$(USEMODULE)))
   endif
 endif
 
+# Force trying 'NEWLIB_NANO'
+CMP_OP = =
+
 # Newlib-nano removed the integer only 'iprintf' functions which are now mapped
 # to printf.
 #

BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=esp8266-esp-12x make flash term

...
Test: comparing addresses of 'printf' and 'iprintf' symbols
[SUCCESS] '40101cf8' = '40101cf8' is True
...
2019-09-02 12:15:11,712 - INFO # sysmem: 5116 (used 612, free 4504)
2019-09-02 12:15:11,712 - INFO # 
2019-09-02 12:15:11,713 - INFO # Board configuration:
2019-09-02 12:15:11,715 - INFO # 	PWM_DEV(0): channelmain(): This is RIOT! (Version: 2019.10-devel-559-g39984c-pr/riot/12133/fix_stdio_problems)
2019-09-02 12:15:11,715 - INFO # Newlib/nano test
2019-09-02 12:15:11,715 - INFO # .
2019-09-02 12:15:11,715 - INFO # OK (1 tests)
2019-09-02 12:15:57,686 - INFO # Exiting Pyterm

So this correctly compiles and detects the _NANO_FORMATTED_IO macro.

RIOT/tests/libc_newlib/main.c

Lines 39 to 43 in 3901740

    
           #ifdef MODULE_NEWLIB_NANO 
        
           #ifndef _NANO_FORMATTED_IO 
        
           #error newlib-nano is enabled but the header is not visible by the build system 
        
           #endif /* _NANO_FORMATTED_IO */ 
        
           #endif /* MODULE_NEWLIB_NANO */

Adding `newlib_nano` to `cpu/esp8266` correctly makes this test pass too without modifications

diff --git a/cpu/esp8266/Makefile.include b/cpu/esp8266/Makefile.include
index 6f2a999c6..3104bd587 100644
--- a/cpu/esp8266/Makefile.include
+++ b/cpu/esp8266/Makefile.include
@@ -62,6 +62,7 @@ PSEUDOMODULES += esp_spiffs
 USEMODULE += esp
 USEMODULE += mtd
 USEMODULE += newlib
+USEMODULE += newlib_nano
 USEMODULE += newlib_syscalls_default
 USEMODULE += periph
 USEMODULE += ps

I love when tests can do reviews automatically :)

gschorcht · 2019-09-04T12:38:29Z

@cladmi Thanks for clarifying that. Indeed, the newlib-xtensa was compiled with newlib-nano-formatted-io enabled. Using module newlib_nano solves the problem. Thanks again.

The overridden stdio functions `puts`, `putchar` and `printf` were removed. Instead, the corresponding newlib functions are always used. Using the newlib functions fixes output conflicts when using `f *` functions like `fprintf`,` fputs`, ... with `stdout` as the file parameter.

The modules `newlib, `newlib_syscalls_default` and `stdio_uart` are now used by default for output to the UART interface. This also reduces the dependency rules.

To avoid unresolved symbols for unused functions during linking, compiler option `-ffunction-sections` is used now. Linker option `--warn-unresolved-symbols` is removed to get errors if required symbols cannot be resolved.

UART FIFO must contain only 1 byte when newlib's `printf` function is used. Otherwise, outputs that are still not sent over UART are lost when `printf` is called asynchronousely.

gschorcht · 2019-09-04T13:16:43Z

@benpicco Murdock is happy now.

benpicco · 2019-09-04T14:07:42Z

And my esp8266 is happy now too!

gschorcht · 2019-09-04T14:12:21Z

@benpicco Thanks again for reviewing and testing.

gschorcht mentioned this pull request Aug 31, 2019

cpu/esp*: fix of the error code for I2C address ACK errors #12134

Merged

benpicco approved these changes Aug 31, 2019

View reviewed changes

benpicco mentioned this pull request Aug 31, 2019

pkg/libb2: rename config.h to avoid conflicts in the global namespace, set HAVE_ALIGNED_ACCESS_REQUIRED based on CPU (fixes build on esp8266) #12135

Merged

gschorcht added 4 commits September 4, 2019 14:41

cpu/esp8266: use always newlib stdio functions

69a5cc7

The modules `newlib, `newlib_syscalls_default` and `stdio_uart` are now used by default for output to the UART interface. This also reduces the dependency rules.

cpu/esp8266: fix unresolved symbols

646a173

To avoid unresolved symbols for unused functions during linking, compiler option `-ffunction-sections` is used now. Linker option `--warn-unresolved-symbols` is removed to get errors if required symbols cannot be resolved.

cpu/esp8266: enable newlib with nano-formatted-io

7fe1056

gschorcht force-pushed the cpu/esp8266/fix_stdio_problems branch from 39984ce to 0de34b6 Compare September 4, 2019 12:52

cpu/esp8266: periph/uart FIFO resized to 1 byte

37ecb4f

UART FIFO must contain only 1 byte when newlib's `printf` function is used. Otherwise, outputs that are still not sent over UART are lost when `printf` is called asynchronousely.

gschorcht force-pushed the cpu/esp8266/fix_stdio_problems branch from 0de34b6 to 37ecb4f Compare September 4, 2019 12:59

benpicco merged commit a55230e into RIOT-OS:master Sep 4, 2019

gschorcht deleted the cpu/esp8266/fix_stdio_problems branch September 4, 2019 14:12

cladmi mentioned this pull request Sep 10, 2019

cpu/esp: treat undefined reference as errors #11246

Closed

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

gschorcht mentioned this pull request Oct 18, 2019

cpu/msp430: update to modern gcc/ELF/newlib toolchain #12457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/esp8266: fix stdio problems#12133

cpu/esp8266: fix stdio problems#12133
benpicco merged 5 commits intoRIOT-OS:masterfrom
gschorcht:cpu/esp8266/fix_stdio_problems

gschorcht commented Aug 31, 2019

Uh oh!

benpicco left a comment

Uh oh!

benpicco commented Aug 31, 2019

Uh oh!

gschorcht commented Aug 31, 2019

Uh oh!

gschorcht commented Aug 31, 2019

Uh oh!

benpicco commented Aug 31, 2019

Uh oh!

cladmi commented Sep 2, 2019

Uh oh!

cladmi commented Sep 2, 2019

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

benpicco commented Sep 4, 2019 •

edited

Loading

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gschorcht commented Aug 31, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Aug 31, 2019

Uh oh!

gschorcht commented Aug 31, 2019

Uh oh!

gschorcht commented Aug 31, 2019

Uh oh!

benpicco commented Aug 31, 2019

Uh oh!

cladmi commented Sep 2, 2019

Uh oh!

cladmi commented Sep 2, 2019

Explanation

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

benpicco commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benpicco commented Sep 4, 2019 •

edited

Loading