Enable native PROGMEM for format and parameters #4

earlephilhower · 2018-01-11T02:49:32Z

This patch is related to esp8266/Arduino#3740 , increasing heap RAM on the ESP8266.

With this patch, the *printf_P() no longer needs to be used (and no need to new[], strcpy_P, free[] the format) so the and can be remapped into a simple *printf().

This also transparently adds in support for PMEM strings using the standard "%s" format. Due to the way vprintf() works (calls a passed-in function to actually output the characters either to RAM, serial, file, etc.) there's no real benefit in requiring a new %S format (which I think is supposed to be wide-char string per ANSI C anyway).

The only downside is the strings are now parsed char-by-char using pgm_read_byte instead of via direct pointer access. For programs which only do printfs() this may slow things down a bit (but not yet measured, sorry, as it's not noticeable in anything I'm doing).

Serial.printf(test='%s'\n", "testing1");
Serial.printf(PSTR("test='%s'\n"), "testing2");
Serial.printf("test='%s'\n", PSTR("testing3"));
Serial.printf(PSTR("test='%s'\n"), PSTR("testing4"));
....
generates
....
test='testing1'
test='testing2'
test='testing3'
test='testing4'

The format and any %s parameters can now be read directly from PROGMEM.
This will have some performance impact in *printf-heavy applications as
all parameters (RAM and ROM) are now processed through pgm_read_byte().

The format and any %s parameters can now be read directly from PROGMEM. This will have some performance impact in *printf-heavy applications as all paramters (RAM and ROM) are not processed through pgm_read_byte().

igrr · 2018-01-11T04:20:37Z

Perhaps one benefit of supporting "%S" format is better compatibility with other Arduino variants. If we use "%s" for progmem strings, then libraries need to use different format strings, depending on the platform.

earlephilhower · 2018-01-11T04:33:48Z

Didn't think about that, good point. It's literally a 1-line change to make %s and %S work for RAM or PMEM.

igrr · 2018-01-11T04:28:52Z

newlib/libc/stdio/nano-vfprintf.c

@@ -167,6 +167,7 @@ static char *rcsid = "$Id$";
 #include "fvwrite.h"
 #include "vfieeefp.h"
 #include "nano-vfprintf_local.h"
+#include "../machine/xtensa/pgmspace.h"


General note, for all 4 MRs: common (non-machine-specific) code in newlib typically does not include machine-specific files directly.
Instead, what you can do is provide dummy definitions of pgm_read_byte in stdio/string.h, which will work for all platforms, and then add machine-specific definition of pgm_read_byte to sys/string.h under xtensa machine (path in this repository would be newlib/libc/sys/xtensa/sys/string.h).

I was trying to avoid any chance of namespace collisions by putting it into a directory that is not installed to either the SDK or Arduino SDK, and to minimize duplication. Wouldn't placing it there end up causing there to be two definitions in the tree (one in the SDK, one in the libc)? It's not a big deal to rename, but then you've got two identical functions with the same name in a real build tree, no?

Since we are adding these functions into libc, we can remove them from Arduino.

igrr · 2018-01-11T04:29:41Z

newlib/libc/stdio/nano-vfprintf.c

@@ -229,7 +230,7 @@ _DEFUN(__ssputs_r, (ptr, fp, buf, len),
  if (len < w)
    w = len;

-  (void)memmove ((_PTR) fp->_p, (_PTR) buf, (size_t) (w));
+  (void)memcpy_P ((_PTR) fp->_p, (_PTR) buf, (size_t) (w));


Where is memcpy_P defined?

It's not, and is linked from the Arduino SDK libs. Cut-n-pasting that version somewhere in newlib's not difficult, but I imagine it's not good code hygiene.

How about moving the definition into newlib then (adding at here and removing from Arduino)? As mentioned below, I would like libc to stay more or less independent of the application which is built on top of it, if this is reasonably easy to do. In this case, i think it is.

(We do implement some of the syscalls which newlib depends on in Arduino; but i think syscalls are a special case.)

igrr · 2018-01-11T04:30:25Z

newlib/libc/stdio/nano-vfprintf_i.c

@@ -104,6 +105,20 @@ _printf_common (struct _reent *data,
 error:
  return -1;
 }
+
+// Not defined in the ESP8266 Arduino


I think the reference to Arduino might not be appropriate, as this project is not arduino-specific.

Ah, so newlib is also used by the SDK as well? Just checking the reasoning as I'm only testing w/Arduino SDK now (but can't imagine any meaningful differences).

It's not, but i have a few personal projects which do not use Arduino and use this newlib fork, so I'd like to keep it generic, if reasonable.

igrr · 2018-01-11T04:33:30Z

newlib/libc/stdio/nano-vfprintf_i.c

@@ -104,6 +105,20 @@ _printf_common (struct _reent *data,
 error:
  return -1;
 }
+
+// Not defined in the ESP8266 Arduino
+static const char *memchr_P(const char *src, int c, size_t length)


The dummy definition (or an alias to memchr) can be put in the same file where memchr is defined.
The real definition for xtensa machine can live in xtensa directory. It can also possibly use some knowledge about progmem, i.e. that it is possible to read a 32-bit word once and then check 4 of its bytes, instead of calling pgm_read_byte which will do 4 32-bit reads.

Edit: however, i have not looked at the code generated by GCC for this. Perhaps it optimizes out unneeded loads...

I don't think GCC looks into the assembly itself as the __asm blocks have to manually define what registers they trounce/etc. Optimization's fine and not too complicated, but for starters I wanted something I could reasonably prove was correct. :) This is the actual memchr routine built in the lib (modulo s/*x/pgm_read_byte(&x)/g). The data should come from the I$ anyway given the tight look, so it's not going to cause 4 SPI reads, at least.

Right, it's more of a thing to look into later. I am okay with merging the obviously correct version.

earlephilhower · 2018-01-13T18:07:52Z

Closing in lieu of #5. I've also added some pretty simple but high value changes to the memcpy_P and strncpy_P routines which give ~4x to ~8x speed boost with about a dozen lines of code.

Enable native PROGMEM for format and parameters

472697d

The format and any %s parameters can now be read directly from PROGMEM. This will have some performance impact in *printf-heavy applications as all paramters (RAM and ROM) are not processed through pgm_read_byte().

igrr requested changes Jan 11, 2018

View reviewed changes

igrr mentioned this pull request Jan 11, 2018

Move mprec_*tens arrays to PROGMEM #2

Closed

earlephilhower mentioned this pull request Jan 13, 2018

Support and optimize PROGMEM accesses in libc #5

Merged

earlephilhower closed this Jan 13, 2018

earlephilhower deleted the printf_pmem branch March 9, 2018 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable native PROGMEM for format and parameters #4

Enable native PROGMEM for format and parameters #4

earlephilhower commented Jan 11, 2018 •

edited

Loading

igrr commented Jan 11, 2018

earlephilhower commented Jan 11, 2018

igrr Jan 11, 2018

earlephilhower Jan 11, 2018

igrr Jan 11, 2018

igrr Jan 11, 2018

earlephilhower Jan 11, 2018

igrr Jan 11, 2018 •

edited

Loading

igrr Jan 11, 2018

earlephilhower Jan 11, 2018

igrr Jan 11, 2018

igrr Jan 11, 2018

earlephilhower Jan 11, 2018

igrr Jan 11, 2018

earlephilhower commented Jan 13, 2018

Enable native PROGMEM for format and parameters #4

Enable native PROGMEM for format and parameters #4

Conversation

earlephilhower commented Jan 11, 2018 • edited Loading

igrr commented Jan 11, 2018

earlephilhower commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earlephilhower commented Jan 13, 2018

earlephilhower commented Jan 11, 2018 •

edited

Loading

igrr Jan 11, 2018 •

edited

Loading