Change property from enum to unordered map & add verbose logging

[croessi]

I did a lot of experimenting, but in the end I was able to move the definition of registers, names and their types into one single unordered map. One of the biggest adavantages is, that the definition is now only happening in one single file:

    const std::unordered_map<std::string, Property> propertyMap {
        {"kINDEX_NOT_FOUND",{0x0000,et_default}},
        {"kFEHLERMELDUNG",{0x0001,et_default}},
        {"kKESSELSOLLTEMP",{0x0002,et_dec_val}},
        {"kSPEICHERSOLLTEMP",{0x0003,et_dec_val}},
        {"kVORLAUFSOLLTEMP",{0x0004,et_dec_val}},
        {"kRAUMSOLLTEMP_I",{0x0005,et_dec_val}},
        {"kRAUMSOLLTEMP_II",{0x0006,et_dec_val}},
        {"kRAUMSOLLTEMP_III",{0x0007,et_dec_val}},
        {"kRAUMSOLLTEMP_NACHT",{0x0008,et_dec_val}},
        {"kUHRZEIT",{0x0009,et_zeit}},
        {"kDATUM",{0x000a,et_datum}},
        {"kGERAETE_ID",{0x000b,et_dev_id}},

However, If i put all ~3000 registers in the map, OTA flashing behaves weird for me: Although flash is only about 60%, it seems the ESP is not taking the new program, but switches back to his previous flash content. With the ~200 entries in this pull request, everything is fine.

Additionally this pull request add a verbose output, making it easier to do reverse engineering.

[kr0ner]

Hi @croessi. I really appreciate your effort on this :) Can you create a sperate PR for the changes to simplevariant? https://github.com/kr0ner/OneESP32ToRuleThemAll/pull/13/files#diff-301dbb15d5160b0dbbf2ebee578392c0740580231dc3233b95ff3cef1923d623R19-R27

The other approach with the map I must say is not the way I want to go. We already know about the limitations and the undefined behavior when we have many entries. Also referencing properties by their name as strings is error prone and you will find out about typos only during runtime. From memory perspective we don't gain anything, because every instruction referring to a property will require a string to be stored in memory and also looks less clean then just Property::kSomeValue. So I'd propose to go with my enum solution, since it does not require any changes to the business logic, but saves us the third place where we need to add mappings. going this way there is a small overhead when adding new properties, but referencing existing ones is much nicer.

[croessi]

I agree with the drawback of an unordered map. However, I want to make it as easy to add new mappings and to minimize the probability of errors (especially if we allow multiple contributers to add and correct mappings). So I gave it some thought and found another way with a literal type for the property:

class PropertyType {
public:
    const char* name;
    uint16_t address;
    Type type;
    ....

and a class holding the properties

class Property {
public:
static constexpr PropertyType kINDEX_NOT_FOUND = PropertyType("kINDEX_NOT_FOUND",0x0000,et_default);
static constexpr PropertyType kFEHLERMELDUNG = PropertyType("kFEHLERMELDUNG",0x0001,et_default);
...

The literal type supports direct initialization as constexpr, allowing to definie new mappings within a single line of code! This should reduce the probability of mapping errors and easen maintainability a lot. I did test it within my setup and works very well. See the other pull request for the full changes.

[kr0ner]

@croessi I agree, that managing all this in a central place would greatly increase maintainability. Currently I'm not sure how this shall look like in the future. Could be an extension to the macro or sth. in the direction of your approach. I need to try the different solutions on my own and see what fits best. Please give me some time to do that.

Edit: ok, I decided to give it a try ... your solution looks promising. I did some small changes, so that it ... in theory .. should not require any changes to the existing code at all https://godbolt.org/z/r6x36qs6r

[kr0ner]

I managed to avoid some of the remaining duplication and also killed the register during callback registration. Some properties are handled in the switch case and not via callback, they would be left out. There is 1 Byte overhead so far ... but that is a small price to pay.

Adding a Property is as easy as

   PROPERTY(FEHLERMELDUNG, 0x0001);
   PROPERTY(SPEICHERSOLLTEMP, 0x0003, Type::et_dec_val);

[kr0ner]

I've pushed the changes https://github.com/kr0ner/OneESP32ToRuleThemAll/commit/fa5ff90b2d241ab060f31a7ee5993e4d178af9df

your input was very helpful 🙇

[croessi]

Thanks @kr0ner for integrating this! Sorry Iwas very bussy over the summer since our baby boy arrived. Woud you be open for the variant handling concept, as well? Or do you propose a different approach?

[kr0ner]

Thanks @kr0ner for integrating this! Sorry Iwas very bussy over the summer since our baby boy arrived. Woud you be open for the variant handling concept, as well? Or do you propose a different approach?

congratulations :tada: Sure thing ... I tried to clean up as much as possible before introducing the variants. Should be good to go now. I'd propose to have a base heatpump.yaml with the common settings and one yaml for each variant to do the specific stuff there. This e.g. Thz504.yaml will inherit everything from the base.

The property.h should be more or less the only thing that needs to be changed for the variants in code. The macros we agreed in the first place should do the job. Since we know upfront which variant we want to support and there can be only one at a time. Those macros can then also be used in communication.h to change the settings there.