Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable native PROGMEM for format and parameters #4

Closed
wants to merge 1 commit into from
Closed

Enable native PROGMEM for format and parameters #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions newlib/libc/machine/xtensa/pgmspace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#ifndef ARDUINO

#ifndef __PGMSPACE__
#define __PGMSPACE__

#include <stdint.h>

#define PROGMEM __attribute__((section(".irom.text")))

// flash memory must be read using 32 bit aligned addresses else a processor
// exception will be triggered
// order within the 32 bit values are
// --------------
// b3, b2, b1, b0
// w1, w0

#define pgm_read_with_offset(addr, res) \
asm("extui %0, %1, 0, 2\n" /* Extract offset within word (in bytes) */ \
"sub %1, %1, %0\n" /* Subtract offset from addr, yielding an aligned address */ \
"l32i.n %1, %1, 0x0\n" /* Load word from aligned address */ \
"slli %0, %0, 3\n" /* Mulitiply offset by 8, yielding an offset in bits */ \
"ssr %0\n" /* Prepare to shift by offset (in bits) */ \
"srl %0, %1\n" /* Shift right; now the requested byte is the first one */ \
:"=r"(res), "=r"(addr) \
:"1"(addr) \
:);

static inline uint8_t pgm_read_byte_inlined(const void* addr) {
register uint32_t res;
pgm_read_with_offset(addr, res);
return (uint8_t) res; /* This masks the lower byte from the returned word */
}

/* Although this says "word", it's actually 16 bit, i.e. half word on Xtensa */
static inline uint16_t pgm_read_word_inlined(const void* addr) {
register uint32_t res;
pgm_read_with_offset(addr, res);
return (uint16_t) res; /* This masks the lower half-word from the returned word */
}

// Make sure, that libraries checking existence of this macro are not failing
#define pgm_read_byte(addr) pgm_read_byte_inlined(addr)
#define pgm_read_word(addr) pgm_read_word_inlined(addr)

#endif //__PGMSPACE__

#endif // ARDUINO

29 changes: 15 additions & 14 deletions newlib/libc/stdio/nano-vfprintf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ static char *rcsid = "$Id$";
#include "fvwrite.h"
#include "vfieeefp.h"
#include "nano-vfprintf_local.h"
#include "../machine/xtensa/pgmspace.h"
Copy link
Owner

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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


/* The __ssputs_r function is shared between all versions of vfprintf
and vfwprintf. */
Expand Down Expand Up @@ -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));
Copy link
Owner

Choose a reason for hiding this comment

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

Where is memcpy_P defined?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@igrr igrr Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

fp->_w -= w;
fp->_p += w;
return 0;
Expand Down Expand Up @@ -321,7 +322,7 @@ _DEFUN(__ssprint_r, (ptr, fp, uio),
if (len < w)
w = len;

(void)memmove ((_PTR) fp->_p, (_PTR) p, (size_t) (w));
(void)memcpy_P ((_PTR) fp->_p, (_PTR) p, (size_t) (w));
fp->_w -= w;
fp->_p += w;
/* Pretend we copied all. */
Expand Down Expand Up @@ -523,15 +524,15 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap),
for (;;)
{
cp = fmt;
while (*fmt != '\0' && *fmt != '%')
while (pgm_read_byte(fmt) != '\0' && pgm_read_byte(fmt) != '%')
fmt += 1;

if ((m = fmt - cp) != 0)
{
PRINT (cp, m);
prt_data.ret += m;
}
if (*fmt == '\0')
if (pgm_read_byte(fmt) == '\0')
goto done;

fmt++; /* Skip over '%'. */
Expand All @@ -551,7 +552,7 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap),
* -- ANSI X3J11
*/
flag_chars = "#-0+ ";
for (; cp = memchr (flag_chars, *fmt, 5); fmt++)
for (; cp = memchr (flag_chars, pgm_read_byte(fmt), 5); fmt++)
prt_data.flags |= (1 << (cp - flag_chars));

if (prt_data.flags & SPACESGN)
Expand All @@ -566,7 +567,7 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap),
prt_data.l_buf[0] = '+';

/* The width. */
if (*fmt == '*')
if (pgm_read_byte(fmt) == '*')
{
/*
* ``A negative field width argument is taken as a
Expand All @@ -584,15 +585,15 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap),
}
else
{
for (; is_digit (*fmt); fmt++)
prt_data.width = 10 * prt_data.width + to_digit (*fmt);
for (; is_digit (pgm_read_byte(fmt)); fmt++)
prt_data.width = 10 * prt_data.width + to_digit (pgm_read_byte(fmt));
}

/* The precision. */
if (*fmt == '.')
if (pgm_read_byte(fmt) == '.')
{
fmt++;
if (*fmt == '*')
if (pgm_read_byte(fmt) == '*')
{
fmt++;
prt_data.prec = GET_ARG (n, ap, int);
Expand All @@ -602,21 +603,21 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap),
else
{
prt_data.prec = 0;
for (; is_digit (*fmt); fmt++)
prt_data.prec = 10 * prt_data.prec + to_digit (*fmt);
for (; is_digit (pgm_read_byte(fmt)); fmt++)
prt_data.prec = 10 * prt_data.prec + to_digit (pgm_read_byte(fmt));
}
}

/* The length modifiers. */
flag_chars = "hlL";
if ((cp = memchr (flag_chars, *fmt, 3)) != NULL)
if ((cp = memchr (flag_chars, pgm_read_byte(fmt), 3)) != NULL)
{
prt_data.flags |= (SHORTINT << (cp - flag_chars));
fmt++;
}

/* The conversion specifiers. */
prt_data.code = *fmt++;
prt_data.code = pgm_read_byte(fmt); fmt++;
cp = memchr ("efgEFG", prt_data.code, 6);
#ifdef FLOATING_POINT
/* If cp is not NULL, we are facing FLOATING POINT NUMBER. */
Expand Down
17 changes: 16 additions & 1 deletion newlib/libc/stdio/nano-vfprintf_i.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "fvwrite.h"
#include "vfieeefp.h"
#include "nano-vfprintf_local.h"
#include "../machine/xtensa/pgmspace.h"

/* Decode and print non-floating point data. */
int
Expand Down Expand Up @@ -104,6 +105,20 @@ _printf_common (struct _reent *data,
error:
return -1;
}

// Not defined in the ESP8266 Arduino
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

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.

static const char *memchr_P(const char *src, int c, size_t length)
Copy link
Owner

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

{
while (length--)
{
if (pgm_read_byte(src) == c)
return (void *) src;
src++;
}

return NULL;
}

int
_printf_i (struct _reent *data, struct _prt_data_t *pdata, FILE *fp,
int (*pfunc)(struct _reent *, FILE *, _CONST char *, size_t len),
Expand Down Expand Up @@ -216,7 +231,7 @@ _printf_i (struct _reent *data, struct _prt_data_t *pdata, FILE *fp,
string, and take prec == -1 into consideration.
Use normal Newlib approach here to support case where cp is not
nul-terminated. */
char *p = memchr (cp, 0, pdata->prec);
char *p = memchr_P (cp, 0, pdata->prec);

if (p != NULL)
pdata->prec = p - cp;
Expand Down