Use c-string methods in xplg_wemohue.ino to save memory

[emontnemery]

Use c-string methods in xplg_wemohue.ino (-1k flash)
Move sensor and extension jump tableto flash (-70 byte RAM)

Note: I have verified the changes xplg_wemohue.ino only by accessing the API through browser (valid JSON, seems fully aligned with before change). Let me know if this functionality can be tested with Domoticz.

[arendst]

I'm trying to get the grasp of your function pointer changes... At least they save a lot of (future) space.

The rest will take time to implement as I want to investigate the consequences too.

Thnx for you many input.

[emontnemery]

cool. let me know if I can help with the testing!

[arendst]

@emontnemery What do you think (or is common knowledge) is the preferred way of concatenating a string?

I see you use a construction like:

int tmpiter = 0;
tmpiter += snprintf_P(mqtt_data+tmpiter, sizeof(mqtt_data)-tmpiter, "Start text");
tmpiter += snprintf_P(mqtt_data+tmpiter, sizeof(mqtt_data)-tmpiter, " Additional text");

as I use:

snprintf_P(mqtt_data, sizeof(mqtt_data), "Start text");
snprintf_P(mqtt_data, sizeof(mqtt_data), "%s Additional text", mqtt_data);

I see the advantage of mine in that it doesn't use an extra variable (useful as used in my string concatenation between sensors).

Just asking, not judging.

[emontnemery]

I didn't give it much thought..
Your version is slightly slower, because the end of the string must be found for each operation, but at the same time much less error prone.
My version is faster, but also error prone. In this case I forget to bounds check the return value from snprintf so my version is quite broken.
Your version is much better in this case, let me know if I should update the PR.

[arendst]

No need to update PR. I worked it already.

[arendst]

This won't work as the char string will become too long (>893 chars) for more than one device. We'll have to stick to Strings here.

[emontnemery]

Oops! But instead of using mqtt_data, a buffer can be allocated on the stack, that's what String has to do anyway.

char tmp[1024];
snprintf_P(tmp, sizeof(tmp), "Start text");
snprintf_P(tmp, sizeof(tmp), "%s Additional text", tmp);

By the way, snprintf behaviour is undefined when output is same as output buffer, but maybe it works when you use it for concatenation like this?

[emontnemery]

@arendst I'll rework the changes to use local variable instead of global variable. Should I do within this PR or close this and open a new one?

[arendst]

You may do it here.

[emontnemery]

OK, now fixed:

• Works with worst case response length (4 devices, each friendly name 32 characters + NUL)
• Return value from snprintf handled, and response string is correctly truncated

Let me know if I should rebase the branch since there has been changes to sonoff/xplg_wemohue.ino

I still do:

char* wp = cresponse;
char* end = &cresponse[RESPONSE_SIZE];
if (wp < end) wp += snprintf_P(wp, end-wp, "Start text");
if (wp < end) wp += snprintf_P(wp, end-wp, " Additional text");

Instead of:

snprintf_P(cresponse, sizeof(cresponse), "Start text");
snprintf_P(cresponse, sizeof(cresponse), "%s Additional text", cresponse);

Because the second method has undefined behavior (http://pubs.opengroup.org/onlinepubs/9699919799/functions/sprintf.html):

If copying takes place between objects that overlap as a result of a call to sprintf() or snprintf(), the results are undefined.

[emontnemery]

I just now realized this style of code is used throughout the project:

snprintf_P(cresponse, sizeof(cresponse), "Start text");
snprintf_P(cresponse, sizeof(cresponse), "%s Additional text", cresponse);

Maybe it's fine, as long as one is very careful to only use it for concatenation?
Should I rewrite the PR to use that style instead if the explicit concatenation with a write pointer?
A suggestion from stack overflow is to write a wrapper for snprintf to do concatenation in safe way without copies of code to keep track of a write pointer and out-of-bounds checks everywhere, that could also be an alternative.

[arendst]

I prefer the current use concatenation as used in the project.

What's the overhead of the Stack Overflow wrapper?

[emontnemery]

OK, I'll update the PR to use the sprintf-style concatenation instead!

This is the proposal from SO, I'll test and see if it makes any impact on code size.
(It needs to be updated to make sure bufSize - len is positive though as size_t is unsigned)

size_t 
snprintfcat(
    char* buf,
    size_t bufSize,
    char const* fmt,
    ...)
{
    size_t result;
    va_list args;
    size_t len = strnlen( buf, bufSize);

    va_start( args, fmt);
    result = vsnprintf( buf + len, bufSize - len, fmt, args);
    va_end( args);

    return result + len;
}

[arendst]

This may be a problem with snprintf_P using Progmem data. I suggest to stay using the projects snprintfs

[emontnemery]

There is a progmem version of vsnprint: vsnprintf_P.
But anyway, I'll revert to just using snprintf for concatenation since it seems to work fine.

[emontnemery]

I just rebased this patch, and it turns out code size is now lower when using C++ string methods!
@arendst has there been some changes in the Arduino core to explain this?

[arendst]

Don't know.