fix m_vsnprintf return value #1077

tius2000 · 2017-04-11T22:56:42Z

Changed the logic of m_vsnprintf() to return the correct value if the buffer is not large enough (see #1066 ). The overflow guard is no longer required. Unfortunately this required some changes to the existing logic, I hope I did not break anything. Here is a minimal test application:

/*
	test return value of snprintf
	
	expected result:
	
		>123<      5
		>1234<     6
		>12345<    7
		>123456    8
		>123456    9
		% A 777 0x033A > 42< -1 > 3.14< >  123< >123  <
*/	

#include <SmingCore.h>

static void _test(const char *s) {
	char buf[8];
	size_t r = m_snprintf( buf, sizeof(buf), ">%s<", s );
	debugf("%-10.10s %d", buf, r);
}




void init() {
	Serial.begin(74880); 
	debugf("\n\ntest m_snprintf\n");
	
	//	test return values
	_test("123");
	_test("1234");
	_test("12345");
	_test("123456");
	_test("1234567");

	//	minimal format testing
	m_printf(
		"%% %c %o 0x%04X >%3u< %d >%5.2f< >%5.5s< >%-5.5s<\n", 
		65, 0777, 0x33A, 42, -1, 3.14159, "123", "123"
	);
}

ADiea

Adding a lambda to take care of size every byte is a great idea. But i don' t like use of gotos especially copy label that jumps cases it's hard to follow imho. No chance of loops instead of gotos?

tius2000 · 2017-04-12T08:25:30Z

Well, C is not my primary language, but I will try to reduce the number of gotos ;-)

ADiea · 2017-04-12T12:28:18Z

Sming/system/m_printf.cpp

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


I would rewrite as

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

You are very strict regarding gotos ;-)

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

ADiea · 2017-04-13T12:41:45Z

Have no other remarks. If "unit tests" in basic_string example work ok it's fine with me. Thanks again!

slaff · 2017-04-18T09:39:00Z

Sming/system/m_printf.cpp

 static void defaultPrintChar(uart_t *uart, char c) {
-	return uart_tx_one_char(c);
+    return uart_tx_one_char(c);


The are multiple white-space changes that are not related to this PR. Please, revert them.

Sorry, if this is a FAQ - is there a guide regarding source code formatting for Sming (or even better, a command line tool for windows to do the correct formatting)?

I tried to follow the rule 38 here: Special characters like TAB and page break must be avoided.
These characters are bound to cause problem for editors, printers, terminal emulators or debuggers when used in a multi-programmer, multi-platform environment.

That's why I converted all tabs to space before committing.

That's why I converted all tabs to space before committing.

Changing of formatting and coding style can happen in another PR. This PR is only related to m_vsnprintf changes.

tius2000 · 2017-04-19T10:24:48Z

support for format and data strings in PROGMEM added
format specifier '%S' added for compatibility (behaves like '%s)
m_vprintf now allocates less memory on the stack unless it is actually required

There is no longer a need for separate *_P methods, this should make it easier to solve the conflicts created by the macro printf_P in FakePgmSpace.h.

slaff

Please, move the PROGMEM changes to another PR. This one should be only related to return value of m_vsnprinf. If we have two PRs it will be easier to test and easier to find the cause of the issues, if any.

slaff · 2017-04-19T10:29:39Z

Sming/system/m_printf.cpp

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


Please, change auto to size_t.

slaff · 2017-04-19T10:30:42Z

Sming/system/m_printf.cpp

-		n++;
-		p++;
+		const char *p = buffer;
+		while (char c = *p++) cbc_printchar(cbc_printchar_uart, c);


Format the while with opening and closing curly braces.

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

slaff · 2017-04-19T10:33:19Z

Sming/system/m_printf.cpp

+        if (++size < maxLen) *buf++ = c;
+    };
+
+    while ( char f = pgm_read_byte(fmt) ) {


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?

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.

tius2000 · 2017-04-19T11:35:32Z

I'm quite new to GitHub - how can I create a PR that depends on another PR?

slaff · 2017-04-19T11:42:06Z

I'm quite new to GitHub - how can I create a PR that depends on another PR?

Create a branch for PR 1. Work on it until you are ready. Then create another branch for PR1 that is based on PR1.

git check PR1
# ... work...
# .... ready
git branch PR2
git checkout PR2
# additional work

If there are changes in PR1 you can keep PR2 up to date using the following commands:

git checkout PR1 
# .. make sure that you have the latest code for PR1
git checkout PR2
git merge PR1
# git mergetool # if needed
git rebase PR1
# git mergetool # if needed
git push -f

That should be it.

tius2000 · 2017-04-19T12:49:15Z

I created a new PR #1096 that way, hope I got it right! Also moved the stacksize change to new PR (#1097).

This reverts commit e52da77.

This reverts commit 974c9a3.

This reverts commit 6b8deeb.

fix m_vsnprintf return value

477fb93

tius2000 mentioned this pull request Apr 11, 2017

m_vsnprintf does not return size of required buffer #1066

Closed

ADiea reviewed Apr 12, 2017

View reviewed changes

tius2000 added 2 commits April 12, 2017 10:55

number of gotos reduced

c330658

whitespace fixed

8966d0c

ADiea reviewed Apr 12, 2017

View reviewed changes

tius2000 added 2 commits April 12, 2017 14:41

one more goto removed ;-)

7b09153

minor fix

b9b3628

tius2000 mentioned this pull request Apr 18, 2017

sprintf_P broken #1048

Closed

slaff added this to the 3.2.0 milestone Apr 18, 2017

slaff added the 3 - Review label Apr 18, 2017

slaff reviewed Apr 18, 2017

View reviewed changes

slaff self-assigned this Apr 18, 2017

whitespace restored

8877ea9

ADiea mentioned this pull request Apr 19, 2017

PROGMEM methods for Print.cpp #1094

Closed

tius2000 added 2 commits April 19, 2017 11:44

PROGMEM support added and stack size reduced

6b8deeb

PROGMEM support added and stack size reduced

974c9a3

slaff requested changes Apr 19, 2017

View reviewed changes

minor changes

e52da77

tius2000 mentioned this pull request Apr 19, 2017

m_printf: PROGMEM support added #1096

Closed

tius2000 added 3 commits April 19, 2017 15:00

Revert "minor changes"

480ea2f

This reverts commit e52da77.

Revert "PROGMEM support added and stack size reduced"

b2f0e62

This reverts commit 974c9a3.

Revert "PROGMEM support added and stack size reduced"

b1f09e4

This reverts commit 6b8deeb.

tius2000 mentioned this pull request Apr 19, 2017

m_printf: stacksize reduced #1097

Merged

slaff removed the 3 - Review label Apr 20, 2017

slaff approved these changes Apr 20, 2017

View reviewed changes

slaff merged commit abcf6a2 into SmingHub:develop Apr 20, 2017

tius2000 deleted the m_printf_1704 branch April 20, 2017 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix m_vsnprintf return value #1077

fix m_vsnprintf return value #1077

tius2000 commented Apr 11, 2017 •

edited

Loading

ADiea left a comment

tius2000 commented Apr 12, 2017 •

edited

Loading

ADiea Apr 12, 2017

tius2000 Apr 12, 2017

ADiea commented Apr 13, 2017

slaff Apr 18, 2017

tius2000 Apr 18, 2017 •

edited

Loading

slaff Apr 18, 2017

tius2000 commented Apr 19, 2017

slaff left a comment

slaff Apr 19, 2017

slaff Apr 19, 2017

slaff Apr 19, 2017

tius2000 Apr 19, 2017 •

edited

Loading

tius2000 commented Apr 19, 2017

slaff commented Apr 19, 2017

tius2000 commented Apr 19, 2017 •

edited

Loading

fix m_vsnprintf return value #1077

fix m_vsnprintf return value #1077

Conversation

tius2000 commented Apr 11, 2017 • edited Loading

ADiea left a comment

Choose a reason for hiding this comment

tius2000 commented Apr 12, 2017 • edited Loading

ADiea Apr 12, 2017

Choose a reason for hiding this comment

tius2000 Apr 12, 2017

Choose a reason for hiding this comment

ADiea commented Apr 13, 2017

slaff Apr 18, 2017

Choose a reason for hiding this comment

tius2000 Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

slaff Apr 18, 2017

Choose a reason for hiding this comment

tius2000 commented Apr 19, 2017

slaff left a comment

Choose a reason for hiding this comment

slaff Apr 19, 2017

Choose a reason for hiding this comment

slaff Apr 19, 2017

Choose a reason for hiding this comment

slaff Apr 19, 2017

Choose a reason for hiding this comment

tius2000 Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

tius2000 commented Apr 19, 2017

slaff commented Apr 19, 2017

tius2000 commented Apr 19, 2017 • edited Loading

tius2000 commented Apr 11, 2017 •

edited

Loading

tius2000 commented Apr 12, 2017 •

edited

Loading

tius2000 Apr 18, 2017 •

edited

Loading

tius2000 Apr 19, 2017 •

edited

Loading

tius2000 commented Apr 19, 2017 •

edited

Loading