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

fix m_vsnprintf return value #1077

Merged
merged 12 commits into from
Apr 20, 2017
Merged

fix m_vsnprintf return value #1077

Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
477fb93
fix m_vsnprintf return value
tius2000 Apr 11, 2017
c330658
number of gotos reduced
tius2000 Apr 12, 2017
8966d0c
whitespace fixed
tius2000 Apr 12, 2017
7b09153
one more goto removed ;-)
tius2000 Apr 12, 2017
b9b3628
minor fix
tius2000 Apr 12, 2017
8877ea9
whitespace restored
tius2000 Apr 18, 2017
6b8deeb
PROGMEM support added and stack size reduced
tius2000 Apr 19, 2017
974c9a3
PROGMEM support added and stack size reduced
tius2000 Apr 19, 2017
e52da77
minor changes
tius2000 Apr 19, 2017
480ea2f
Revert "minor changes"
tius2000 Apr 19, 2017
b2f0e62
Revert "PROGMEM support added and stack size reduced"
tius2000 Apr 19, 2017
b1f09e4
Revert "PROGMEM support added and stack size reduced"
tius2000 Apr 19, 2017
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
308 changes: 140 additions & 168 deletions Sming/system/m_printf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ Descr: embedded very simple version of printf with float support
#include <stdarg.h>
#include "osapi.h"

#define MPRINTF_BUF_SIZE 256

#define OVERFLOW_GUARD 24
#define INITIAL_BUFFSIZE 128

static void defaultPrintChar(uart_t *uart, char c) {
return uart_tx_one_char(c);
Expand All @@ -20,18 +18,17 @@ static void defaultPrintChar(uart_t *uart, char c) {
void (*cbc_printchar)(uart_t *, char) = defaultPrintChar;
uart_t *cbc_printchar_uart = NULL;

#define SIGN (1<<1) /* Unsigned/signed long */

#define is_digit(c) ((c) >= '0' && (c) <= '9')

#define MIN(a, b) ( (a) < (b) ? (a) : (b) )

static int skip_atoi(const char **s)
{
int i = 0;
while (is_digit(**s))
i = i * 10 + *((*s)++) - '0';
return i;
int num = 0;
while (1) {
char c = pgm_read_byte(*s);
if ( !is_digit(c) ) return num;
num = num * 10 + (c - '0');
++*s;
}
}

void setMPrintfPrinterCbc(void (*callback)(uart_t *, char), uart_t *uart)
Expand Down Expand Up @@ -68,27 +65,23 @@ int m_snprintf(char* buf, int length, const char *fmt, ...)
return n;
}

int m_vprintf ( const char * format, va_list arg )
int m_vprintf(const char *fmt, va_list va)
{
if(!cbc_printchar)
{
return 0;
}

char buf[MPRINTF_BUF_SIZE], *p;
size_t size = INITIAL_BUFFSIZE - 1;

// need to retry if size is not big enough
while (1) {
char buffer[size + 1];
auto sz = m_vsnprintf(buffer, sizeof(buffer), fmt, va);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, change auto to size_t.

if (sz > size) {
size = sz;
continue;
}

int n = 0;
m_vsnprintf(buf, sizeof(buf), format, arg);

p = buf;
while (p && n < sizeof(buf) && *p)
{
cbc_printchar(cbc_printchar_uart, *p);
n++;
p++;
const char *p = buffer;
while (char c = *p++) cbc_printchar(cbc_printchar_uart, c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Format the while with opening and closing curly braces.

while (char c = *p++)  {
     cbc_printchar(cbc_printchar_uart, c);
}

return sz;
}

return n;
}

/**
Expand Down Expand Up @@ -117,143 +110,122 @@ int m_printf(const char* fmt, ...)

int m_vsnprintf(char *buf, size_t maxLen, const char *fmt, va_list args)
{
int i, base, flags;
char *str;
const char *s;
int8_t precision, width;
char pad;

char tempNum[40];

for (str = buf; *fmt; fmt++)
{
if(maxLen - (uint32_t)(str - buf) < OVERFLOW_GUARD)
{
*str++ = '(';
*str++ = '.';
*str++ = '.';
*str++ = '.';
*str++ = ')';

//mark end of string
*str = '\0';

//return maximum buffer len, so caller can detect not_enough_space
return maxLen;
}

if (*fmt != '%')
{
*str++ = *fmt;
continue;
}

flags = 0;
fmt++; // This skips first '%'

//reset attributes to defaults
precision = -1;
width = 0;
pad = ' ';
base = 10;
bool minus = 0;

do
{
if ('-' == *fmt) minus = 1, fmt++;

//skip width and flags data - not supported
while ('+' == *fmt || '#' == *fmt || '*' == *fmt || 'l' == *fmt)
fmt++;

if (is_digit(*fmt)) {
if (*fmt == '0') {
pad = '0';
fmt++;
}
width = skip_atoi(&fmt);
}

if('.' == *fmt)
{
fmt++;
if (is_digit(*fmt))
precision = skip_atoi(&fmt);
}
else
break;
}while(1);

switch (*fmt)
{
case 'c':
*str++ = (unsigned char) va_arg(args, int);
continue;

case 's': {
s = va_arg(args, char *);

if (!s) s = "(null)";
size_t len = strlen(s);
len = MIN( len, precision );
len = MIN( len, maxLen - size_t(str - buf) - OVERFLOW_GUARD);
width = MIN( width, maxLen - size_t(str - buf) - OVERFLOW_GUARD);

int padding = width - len;
while (!minus && padding-- > 0) *str++ = ' ';
while (len--) *str++ = *s++;
while (minus && padding-- > 0) *str++ = ' ';

continue;
}

case 'p':
s = ultoa((unsigned long) va_arg(args, void *), tempNum, 16);
while (*s && (maxLen - (uint32_t)(str - buf) > OVERFLOW_GUARD))
*str++ = *s++;
continue;

case 'o':
base = 8;
break;

case 'x':
case 'X':
base = 16;
break;

case 'd':
case 'i':
flags |= SIGN;
case 'u':
break;

case 'f':

s = dtostrf_p(va_arg(args, double), width, precision, tempNum, pad);
while (*s && (maxLen - (uint32_t)(str - buf) > OVERFLOW_GUARD))
*str++ = *s++;
continue;

default:
if (*fmt != '%')
*str++ = '%';
if (*fmt)
*str++ = *fmt;
else
--fmt;
continue;
}

if (flags & SIGN)
s = ltoa_wp(va_arg(args, int), tempNum, base, width, pad);
else
s = ultoa_wp(va_arg(args, unsigned int), tempNum, base, width, pad);

while (*s && (maxLen - (uint32_t)(str - buf) > OVERFLOW_GUARD))
*str++ = *s++;
}

*str = '\0';
return str - buf;
size_t size = 0;
auto add = [&](char c) {
if (++size < maxLen) *buf++ = c;
};

while ( char f = pgm_read_byte(fmt) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you read every byte using pgm_read_byte that will cause overhead. Is that really needed in a very core component? What are the advantages?

Copy link
Contributor Author

@tius2000 tius2000 Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You a right, this will make every call to *printf a tiny bit slower. The code size is almost identical. I see the following advantages:

  • it will reduce total flash size and complexity as a many additional methods are no longer needed
  • all classes and methods using m_printf will benefit immediately
  • life will be much easier for users as they do not need to care which method to call and which format specifier to use
  • format strings should go to flashmem anyway to reduce ram size
  • it is faster and uses less memory than the existing implementation for format strings in flashmem
  • printf is mostly used for serial output where execution speed does not matter that much
  • for time critical operations direct c string manipulation should be used instead of the printf functions

IMHO the main advantage is that it eliminated a major pitfall for users working with progmem.

// copy verbatim text
if (f != '%') {
add(f);
fmt++;
continue;
}
fmt++;

const char* s; // source for string copy
char tempNum[40]; // buffer for number conversion

// reset attributes to defaults
bool minus = 0;
uint8_t ubase = 0;
int8_t precision = -1;
int8_t width = 0;
char pad = ' ';

while (char f = pgm_read_byte(fmt)) {
if (f == '-') minus = 1;
else if (f == '+') ; // ignored
else if (f == ' ') ; // ignored
else if (f == '#') ; // ignored
else break;
fmt++;
}

// process padding
if (pgm_read_byte(fmt) == '0') {
pad = '0';
fmt++;
}

// process width ('*' is not supported yet)
if ( is_digit(pgm_read_byte(fmt)) ) {
width = skip_atoi(&fmt);
}

// process precision
if( pgm_read_byte(fmt) == '.' ) {
fmt++;
if ( is_digit(pgm_read_byte(fmt)) ) precision = skip_atoi(&fmt);
}

// ignore length specifier
for ( char f = pgm_read_byte(fmt); f=='l' || f=='h' || f=='L'; fmt++) ;

// process type
switch (char f = pgm_read_byte(fmt++)) {
case '%':
add('%');
continue;

case 'c':
add( (unsigned char) va_arg(args, int) );
continue;

case 's':
case 'S': {
s = va_arg(args, char *);

if (!s) s = PSTR("(null)");
size_t len = strlen_P(s);
if (len > precision) len = precision;

int padding = width - len;
while (!minus && padding-- > 0) add(' ');
while (len--) add(pgm_read_byte(s++));
while (minus && padding-- > 0) add(' ');
continue;
}

case 'p':
s = ultoa((unsigned long) va_arg(args, void *), tempNum, 16);
break;

case 'd':
case 'i':
s = ltoa_wp(va_arg(args, int), tempNum, 10, width, pad);
break;

case 'f':
s = dtostrf_p(va_arg(args, double), width, precision, tempNum, pad);
break;

case 'o':
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rewrite as

case 'o':
case 'x':
case 'X':
if('o' == f)
base=8;
else
base=16;
case 'u':

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are very strict regarding gotos ;-)

I changed it a bit to get rid of the fall-through, too.

ubase = 8;
break;

case 'x':
case 'X':
ubase = 16;
break;

case 'u':
ubase = 10;
break;

default:
add('%');
add(f);
continue;
}

// format unsigned numbers
if (ubase) s = ultoa_wp(va_arg(args, unsigned int), tempNum, ubase, width, pad);

// copy string to target
while (*s) add(*s++);
}
*buf = 0;
return size;
}