homieiot / homie-esp8266

💡 ESP8266 framework for Homie, a lightweight MQTT convention for the IoT
http://homieiot.github.io/homie-esp8266
MIT License
1.36k stars 308 forks source link

Dynamically created property name, format, unit #710

Open sovcik opened 3 years ago

sovcik commented 3 years ago

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:

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.

RunningPenguin commented 3 years ago

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?

sovcik commented 3 years ago

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

RunningPenguin commented 3 years ago

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();
    }
sovcik commented 3 years ago

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.