homie-esp8266 icon indicating copy to clipboard operation
homie-esp8266 copied to clipboard

Dynamically created property name, format, unit

Open sovcik opened this issue 3 years ago • 4 comments

My app creates nodes dynamically based on provided configuration file and I was struggling for some time with strange node/property names. Then I realized that while node/property id is copied using strdup, node/property name, unit, datatype, format are only referenced

Example of problematic code:

for(int j=0;j<NUMBER_OF_VALVES;j++){
    String vid = valves[j]->getIdStr() + String("rt");
    String vin = valves[i]->getIdStr() + String(" Runtime");
    prg_node[i]->advertise(vid.c_str()).setName(vin.c_str()).setDatatype("integer").setFormat("0:120").settable();
}

In above code:

  • setting property id works without problem
  • setting property name is an issue as string buffer is released after finishing the cycle

I assume the reason is saving memory by avoiding string duplication.

Question Would it be OK to copy all strings to achieve more consistent behavior? And for memory limiting situations implement e.g. setName with progmem signature, so developer can store strings to progmem and setName will copy them from there?

I can create PR for that.

sovcik avatar May 28 '21 06:05 sovcik

You used: vid = valves[j] and vin = valves[i] are you sure the index "i" and "j" are correctly used and you didn't mix them up?

RunningPenguin avatar Jun 06 '21 09:06 RunningPenguin

Sorry, typo in provided example. Otherwise it wouldn't compile, right? :-) The issue is clearly visible in reference source code.

sovcik avatar Jun 06 '21 09:06 sovcik

perhaps you could use a temporary buffer and strcpy/strcat functions to build your "vin" string. I have done this here for the id but I think it should work for the name too. Of course you have to write some more lines of code but perhaps it's easier than changing the behavior of the advertise function.

char tempIdBuffer[254] = {0};
char tempAdvertiseIndexStr[10] = {0};
uint8_t readableInvId = 0;

for (uint8_t j = 0; j < number_of_inverters_to_advertise; j++)
   {
       readableInvId = j + 1;                                               //because humans count most of the time from 1 onwards not from 0
       memset(&tempAdvertiseIndexStr[0], 0, sizeof(tempAdvertiseIndexStr)); //zero out all buffers we could work with "messageToDecode"
       delayMicroseconds(250);                                              //give memset a little bit of time to empty all the buffers
       itoa(readableInvId, tempAdvertiseIndexStr, 10);
       delayMicroseconds(250); //give memset a little bit of time to empty all the buffers

       //Frequency
       memset(&tempIdBuffer[0], 0, sizeof(tempIdBuffer)); //zero out all buffers we could work with "messageToDecode"
       delayMicroseconds(250);                            //give memset a little bit of time to empty all the buffers
       strncpy(tempIdBuffer, cPropFrequency, strlen(cPropFrequency));
       delayMicroseconds(250); //give memset a little bit of time to empty all the buffers
       strncat(tempIdBuffer, tempAdvertiseIndexStr, sizeof(tempAdvertiseIndexStr));
       delayMicroseconds(250); //give memset a little bit of time to empty all the buffers
       Homie.getLogger() << "_advertise tempIdBuffer 1: " << tempIdBuffer << endl;
       advertise(tempIdBuffer).setName("Frequency").setDatatype("float").setFormat("%.2f").setUnit("Hz").settable();
   }

RunningPenguin avatar Jun 10 '21 15:06 RunningPenguin

Well, actually not possible that way. You created tempbuffer for property id, which is being "copied" and so temporary buffering is possible. The problem with e.g. property name is that you have to pass buffer/variable which will be available during the property life-time.

sovcik avatar Jun 11 '21 05:06 sovcik