mjhjmtu

Sorry for novice question in Arduino, but I'm trying to figure out why res is keeping its value when up_cmd0 is being called again.

for example - if num=8 in first run and the result is :up_cmd0_res: 8, and second run num=12, the result is: up_cmd0_res: 812

void up_cmd0(){
 char num[8];
 char *res = "up_cmd0_res:";
 itoa(counter2, num, 10);
 strcat(res ,num);
 client.publish(inTopic,res);

char *res = "up_cmd0_res:";

In principle, res should be a const char *. Const because it is
pointing to a literal string, and you cannot (ar at least, you are not
supposed to) change a literal string. The compiler should warn you on
that... if you enable compiler warnings.

strcat(res ,num);

This is very wrong. strcat() expects as its first argument an pointer
to a modifyable array large enough for the resulting concatenation
(and the terminating ). In other words, you have to allocate enough
room for the whole string, then fill the beginning of the allocated
array with the first substring, then only call strcat():

char res[24];
strcpy(res, "up_cmd0_res:");
strcat(res, num);

What happened is that you initially had, in memory, something like this:

<----- memory reserved for the string literal ---->
'u' 'p' '_' 'c' 'm' 'd' '0' '_' 'r' 'e' 's' ':' NUL
 ^-- res

On the first call to stracat(), you overwrite the terminating NUL and
end up with

<----- memory reserved for the string literal ---->
'u' 'p' '_' 'c' 'm' 'd' '0' '_' 'r' 'e' 's' ':' '8' NUL
 ^-- res

Note that the new terminating NUL is now occupying a byte in memory that
hasn't been allocated for this purpose. You have likely overwritten some
global variable. Next time you call strcat(), you again replace the
terminating NUL and continue to overwrite whatever happens to be stored
in this part of the RAM. Expect a crash or program misbehavior.

Only the pointer, *res, is a local variable. The string "up_cmd0_res:" itself is elsewhere in RAM, stored as a literal and not meant to be modified.

Your strcat() call overwrites (extends) the literal each time you call it - which, by the way means that the growing string will be overwriting something else - whatever was following the string literal in global memory!!

You need to provide a character buffer within your function, large enough to hold "up_cmd0_res:" plus the longest thing you might ever append to it, plus one more byte for the zero-terminator. That buffer should be re-initialized on every function entry, before you append anything to it. Then make the strcat call to append to it. (These two calls will each manage the terminator for you).

The fact that this program didn't crash (or did it?) is only just by happenstance. If you call this function as it is initially written, enough times, your string literal will grow until it overwrites something important and either produces incorrect results (by over-writing some other data, or crashes (by overwriting the stack, especially the current function's return-address).

It is incorrect to strcat into a field pointing to a literal. The code below is shorter, easier to understand, and does not corrupt memory:

 char res [20];
 sprintf (res, "up_cmd0_res:%i", counter2);
 client.publish(inTopic,res);

The field res needs to be long enough to hold the literal string (which is not modified doing it this way) plus the number you are adding to the end, and a terminating 0x00 byte.

can you please explain what "literal" is?

OK. A literal is when you literally put a string into a variable. For example:

char *res = "up_cmd0_res:";

So res isn't pointing to some variable, it is pointing to a literal string in the code. Now check this out:

void setup() 
 Serial.begin (115200);
 char * a = "foo";
 Serial.println (a);
 strcpy (a, "bar");
 Serial.println (a);
 char * b = "foo";
 Serial.println (b);


void loop()

Are you expecting this code to print:

foo
bar
foo

It should, right?, because the variable b contains "foo".

However it actually prints:

foo
bar
bar

This is because the strcpy has modified a literal string ("foo") to now contain "bar". The compiler is smart enough to think that every time it sees "foo" in the code it can place a single piece of memory with "foo" in it, because it is a literal (and literals don't change). But now you have modified that.

The compiler does give a warning:

/tmp/arduino_modified_sketch_905830/sketch_aug19a.ino:7:12: warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
 char * b = "foo";
 ^

Imagine if you were able to change the literal 5 so that every time you added 5 to something it actually added 42? That wouldn't be so great, right?

Your code was worse than that, because you didn't just replace a string with something the same length, you used strcat which replaces it with something longer.

Because you're changing a string literal. These literals are in static memory, so they persist between function calls.

On top of that, changing them results in undefined behaviour.

You're also writing out of the bounds of the string.

My recommendation:

void up_cmd0()
 const size_t int_max_digits = floor(log10(pow(2, 8 * sizeof(int) - 1))) + 1;
 char num[int_max_digits + 2]; // max digits + minus sign + null terminator
 const char *str = "up_cmd0_res:";
 char buffer[strlen(str) + sizeof(num)] = ;
 strcat(buffer, str);
 itoa(counter2, num, 10);
 strcat(buffer, num);
 // ...

You shouldn't be using globals to get values from one function to the other (counter2), try to pass it as a parameter to the function.