Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to add new values into code? #508

Closed
tp1de opened this issue May 18, 2022 · 45 comments
Closed

How to add new values into code? #508

tp1de opened this issue May 18, 2022 · 45 comments
Labels
question Question about something

Comments

@tp1de
Copy link
Contributor

tp1de commented May 18, 2022

I would like to add new values / entities into the code. From Discord I got from @MichaelDvP the following introduction:

There is a guide "contributing.md" how to make the PR, but for the code structure you have to read the code.
It's mostly selfexplaining.

Start with reading a simple device like switch.cpp to see the registertelegram() with it's callback process(telegramname).
In the callback the value is read by has_update(..) and publishing the value is handled by the register _device_value().
That's neary all to do.

In thermostat for hc dependend telegrams you have to add a check in heatingcircuit(telegram).

@tp1de tp1de added the question Question about something label May 18, 2022
@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

I would like to start with a first test field for the boiler:

wwsupplyDelta - delta temp for ww load from boiler to dhw :
type 0xEA, offset 0x09, type number (uint8_t), range 5 to 40

Questions: Please correct

  1. What are the steps needed to introduce a new entity? (data definition, range, text to display ...)
  2. Telegram type 0xEA is already registered so I haven't to do anything?
  3. I understand that I need to add then has_update in boiler.cpp (line 614)
  4. publishing the value is handled by the register _device_value() - what do I need to do?
  5. Anything else?

@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

I am a bit confused looking in more detail to the code. In lines 619-627 in boiler.cpp wwComfort is defined with offset 9.
(I think the offset is decimal isn't it?).

On my RC310 wwComfort is at offset 0x0D with valid values {"0":"high","216":"eco"}.
On offset 0x09 the wwsuplyDelta is available.

@MichaelDvP
Copy link
Contributor

Steps to do:

  • add uint8_t wwSupplyDelta_; to boiler.h (line 90)
  • add has_update(telegram, wwSupplyDelta_, 9); to boiler.cpp, line 776 (in telegram 0xEA)
  • add register_device_value(DeviceValueTAG::TAG_DEVICE_DATA_WW, &wwSupplyDelta_, DeviceValueType::UINT, nullptr, FL_(wwSupplyDelta), DeviceValueUOM::DEGREES); to boiler.cpp line 471
  • add MAKE_PSTR_LIST(wwSupplyDelta, F("wwsupplydelta"), F("supply deltatemperature")) to locale_EN.h, line 570

Min/max values are only for commands, then you have to add a set-callback and min/max to the register_device_value and declare the set-callback in boiler.h and code it in boiler.cpp.

Line 619ff is telegram 0x33, your boiler uses 0xEA, look at line 770 ff.

@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

Line 619ff is telegram 0x33, your boiler uses 0xEA, look at line 770 ff.

too much code for my old eyes :)

Thanks for the clarification and I will start testing ...

@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

@MichaelDvP the first steps work. The value is shown correctly with the dashboard and on web-api. Super !

May I get a bit help on the command to change the value?

@MichaelDvP
Copy link
Contributor

Take a look at other writable values from the 0xEA telegram.

  • add to the register_device_value after the uom the setting callback , MAKE_CF_CB(set_wwSupplyDelta));
  • after the callback you can also add min,max values.
  • create a function (around line 1352, near the other ww settings) and add function header to boiler.h.
// set ww supply Delta
bool Boiler::set_wwSupplyDelta(const char * value, const int8_t id) {
    int v = 0;
    if (!Helpers::value2temperature(value, v)) {
        return false;
    }
    write_command(0xEA, 9, v, 0xEA);
    return true;
}

Instead of 0xEA you can also use EMS_TYPE_UBAParameterWWPlus

BTW: the name delta suggests that it is a temperature difference, is it really unsigned? Also for relative values you have to use value2temperature(value, v, true)

@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

Value changes work now. Thanks, I was missing the MAKE_CF_CB(set_wwSupplyDelta) addition. 👍

I just ask myself how (and where) to add min/max values?
The min/max info should be included in web-api details for the entity as well.

BTW: the name delta suggests that it is a temperature difference, is it really unsigned? Also for relative values you have to use value2temperature(value, v, true)

Yes I just took the name I had from testing. I will check names before continuing.
(in German: Vorlauftemperatur-Erhöhung) - only positive values between 0-40 °C allowed.
Maybe use dhw_flowtemp_increase ????

@MichaelDvP
Copy link
Contributor

(in German: Vorlauftemperatur-Erhöhung)

this is implemented as wwFlowTempOffset_ for the ems-boilers, you only have to add it to 0xEA and modify the set_ww_flowTempOffset to

    if (is_fetch(EMS_TYPE_UBAParameterWWPlus)) {
        write_command(EMS_TYPE_UBAParameterWWPlus, 9, v, EMS_TYPE_UBAParameterWWPlus);
    } else {
        write_command(EMS_TYPE_UBAParameterWW, 5, v, EMS_TYPE_UBAParameterWW);
    }

to add the limits:

    register_device_value(DeviceValueTAG::TAG_DEVICE_DATA_WW,
                          &wwFlowTempOffset_,
                          DeviceValueType::UINT,
                          nullptr,
                          FL_(wwFlowTempOffset),
                          DeviceValueUOM::DEGREES_R,
                          MAKE_CF_CB(set_ww_flowTempOffset),
                          0, 
                          40);

@tp1de
Copy link
Contributor Author

tp1de commented May 18, 2022

this is implemented as wwFlowTempOffset_ for the ems-boilers, you only have to add it to 0xEA and modify the set_ww_flowTempOffset to

So I should cancel my changes I made until now?

@MichaelDvP
Copy link
Contributor

So I should cancel my changes I made until now?

Adapt them to use same names as other boilers. Yes, sometimes we have to revoke changes if we find a better way.

@tp1de
Copy link
Contributor Author

tp1de commented May 19, 2022

@MichaelDvP
I try to understand how to handle/define enums and link enum entries to hex-values for read an write.

I see that the enum texts are in locale_EN.h but I do not really understand the steps needed to introduce a new enum list and allocate the enum entries to hex-values for read and write.

Could you please advice how to do this?

@tp1de
Copy link
Contributor Author

tp1de commented May 19, 2022

For a first try I would like to introduce boiler.wwcomfort - type 0xEA offset 0x0D with values {"0":"high","216":"eco"}

@MichaelDvP
Copy link
Contributor

Enums are normaly numbered 0, 1, 2.., having the DeviceValueType::ENUM and a pointer to a list of entries in the option field of register_device_value(). The list is created in locale_EN.h with MAKE_PSTR_LIST() makro.
Reading is with has_update() or, if the index starts not at zero, with has_enumupdate() where the startvalue is set.
For writing there is the Helpers::value2enum() giving back the index of the text from the enum list..

boiler.wwcomfort is a bit special, because it's not really enum, but contains some fixed signed values for reducing the ww temperature. "eco" is -40°C (0xD8) and "intelligent" -20°C (0xEC). I'm not sure if other values are accepted in this field, my boiler does not have it (fixed to 0x00 (hot), not writable). You can copy the logic from telegram 0x33 to 0xEA offset 13 and modfiy the set_ww_mode() to check EA telegram and write to it.

@tp1de
Copy link
Contributor Author

tp1de commented May 19, 2022

boiler.wwcomfort is a bit special, because it's not really enum, but contains some fixed signed values for reducing the ww temperature. "eco" is -40°C (0xD8) and "intelligent" -20°C (0xEC). I'm not sure if other values are accepted in this field, my boiler does not have it (fixed to 0x00 (hot), not writable). You can copy the logic from telegram 0x33 to 0xEA offset 13 and modfiy the set_ww_mode() to check EA telegram and write to it.

The wwcomfort logic on my RC310 is different. If comfort is set to high, then the defined hysteris is used and the ww storage is heated whenever temp is below defined hysteresis - so with 6°C hysteresis approx. 5-6 times a day. With the use of eco the hysteresis is approx. 2-2.5 times as large and 2 heating cycles per day are enough. But in between the ww temp will be lower then with comfort=high.

@tp1de
Copy link
Contributor Author

tp1de commented May 19, 2022

The existing wwcomfort has 3 enum entries: Hot, Eco, Intelligent.
The RC310 only has 2 possible values: High, Eco.

What would you recommend? Use the existing one or make a new one wwcomfort1 ?

@MichaelDvP
Copy link
Contributor

Using the existing is difficult in boiler-class. Thermostats, and modules have model flag and we can use different enums for different models. But most boilers are not flaged.
If it's only two states, i think it's best to add a new value as boolean and call it wwEcoMode_. This makes it easy to handle with reading has_value(telegram, wwEcoMode_, 13) and writing write_command(EMS_TYPE_UBAParameterWWPlus, 13, b ? 0xD8 : 0, EMS_TYPE_UBAParameterWWPlus);

@tp1de
Copy link
Contributor Author

tp1de commented May 19, 2022

Understood .... on the other side it wood be good to see the enum texts as close as there are shown in the dialog of the thermostat.
If there are enum entries like for thermostat controlmode which do not make any sense (for my RC310) is does not help either.

Would you mind me creating wwcomfort1 as enum? The wwcomfort1 will then be used by type 0xEA.

@MichaelDvP
Copy link
Contributor

Would you mind me creating wwcomfort1 as enum? The wwcomfort1 will then be used by type 0xEA.

If this fit's better to the boiller control, do it.

@tp1de
Copy link
Contributor Author

tp1de commented May 21, 2022

@MichaelDvP Thanks for your support so far. I now made PR's for my boiler entities.
Now I want to start with thermostat values and I am a bit confused and need your help:

  1. There is an existing entity nofrosttemp. In the code it is linked to type 29B / 29C offset 6. Changing values works.
    But changing on RC310 it is type 2B9/2BA (hc1/hc2) offset 12.

  2. I want to introduce a new entity offthreshold .
    (if outdoor temp is below offthreshold boiler runs on ecotemp reference, if outddoortemp is above offthreshold hc is switched of)
    Changing on RC310 it is same type as nofrosttemp in (1) 2B9/ 2BA with offset 9.

  3. Values are signed integer.

So my questions are:

  • Shall I ignore the different telegram types in (1)? Or shall there be changes to 2B9/2BA?
  • How do I implement the new entities offthreshold for each hc?

Please advise.

@MichaelDvP
Copy link
Contributor

The values mostly are taken from Norberts1, or users reported it. I don't have a RC3xx compatible thermostat and can not reproduce.
2B9, offset 6 is listed from NorbertS1 as "HC outdoor frost protection threshold" and mostly reported value of 5°C fits.
29B, offset 9 is listed as "Outdoor Temperature Threshold", dont know what this is, but value 0 can not be a nofrost temp.
29B offset 12 is listed as "Outdoor Temperature Threshold for prevention of ECO", for RC35 we have this already as hc->noreducetemp, but (in RC35) below this temperature the thermostat keeps normal heating and do not go to eco, normally a very low value (-20°C).

What are the settings called on the thermostat display?

Implementing values to thermostat is a bit more complicated than boiler. In thermostat.h there is a sub-class heatingcircuit for the hc dependend values and others are in themostat class. At the end of thermostat.cpp the register value is one for main values register_device_values() and one routine for the hc values register_device_values_hc(std::shared_ptr<Thermostat::HeatingCircuit> hc), both have sections for models, search the RC300 section.
All temperature settings are done in set_temperature(const float temperature, const uint8_t mode, const uint8_t hc_num) { depending on mode values defined in thermostat.h. Try to check and trace a existing value through the code to see how it works.

@tp1de
Copy link
Contributor Author

tp1de commented May 22, 2022

@MichaelDvP
Hi Michael, I have to admit that I feel like being within an IQ-Test since hours to understand the thermostat coding :)
Not too easy to understand the coding practice with the set_temperature mode-depending logic.
I hope that I have managed now ....

My findings / questions:

  1. The set_temp works with using a key parameter mode and then uses an if-structure per model:
    eg. else if ((model == EMS_DEVICE_FLAG_RC300) || (model == EMS_DEVICE_FLAG_RC100))

  2. Then switch (mode) {

case HeatingCircuit::Mode::NOFROST:
          set_typeid      = curve_typeids[hc->hc()];
          validate_typeid = set_typeid;
          offset          = 6;
          factor          = 1;
          break;
  1. For my RC310 the NOFROST Mode does not make sense with the curve_typeids and offset. I do not know if RC100 might be ok. It should be rather set_typeids and offset 12. Shall I split RC300/RC310 and RC100 or shall I correct typeids and offset?

  2. On my RC310 the text (in German) is:
    4.1 Absenkart: Außen --> only with outdoor temp control the next parameters are valid --> ems-esp entity is missing
    4.2 Reduzierter Betrieb unter: 4°C -->ecotemp used for hc if temp is <= 4°C and hc is switched off if above in ecomode
    4.3 Durchheizen unter: -5°C --> below this temp target temp is based on comforttemp

  3. entity names.
    for 4.1 not available yet - setbacktype?
    for 4.2 I used offthreshold for testing - any other proposal? - maybe using reducedmode?
    for 4.3 nofrost or is it used for something else? - maybe using permanentmode?

@MichaelDvP
Copy link
Contributor

Manual RC300:
grafik
We have these values already for RC35, we should use the same names and variables.
Absensenkart -> reducemode
Reduzierter Betrieb unter -> reducetemp
Durchheizen unter -> noreducetemp
Frostschutz -> nofrostmode
Frostschutz Grenztemp. -> nofrosttemp

Please read the explanation of "Durchheizen" in the manual, it is NOT a nofrost function, it is used if max. power of boiler is not large enough to heat up to comfort again after beeing in eco, so keep comfort (which needs less power). Nofrost only prevents freezing of pipes if the house is not in use.

@tp1de
Copy link
Contributor Author

tp1de commented May 23, 2022

@MichaelDvP @proddy Thanks for explaining the existing variables.

You might recognize that I still have problems in understanding & finding the right variables. I do not find them in the online documentation or maybe I am not looking at the right place. Is there any overview of variables and there content available?
I would appreciate to get it. Since I am retired I do have some spare time to support to prepare / correct the documentation about variables and their usage. Just tell me how to do it.

For the RC310 I will add the mentioned variables since they are not available yet..

@proddy
Copy link
Contributor

proddy commented May 23, 2022

We've always wanted to explain the variables, and made a start a while ago at https://emsesp.github.io/docs/#/Boiler?id=ems-telegrams-offsets-for-the-device-type-boiler but it's hard and time-consuming to keep it up to date.

It may be easier to extract the names directly from the source code using a script. ideas welcome!?

@tp1de
Copy link
Contributor Author

tp1de commented May 23, 2022

A good start would be a long-text explanation within locale_EN.h maybe with a reference to the manual (page no). But I understand that the reference might be a bit difficult for all the different thermostat devices.

@tp1de
Copy link
Contributor Author

tp1de commented May 24, 2022

@proddy @MichaelDvP

Let me ask your opinion about names to be used. As @MichaelDvP indicated RC35 uses variable names:
reducemode , reducetemp, noreducetemp, nofrostmode, nofrosttemp

Since I haven't found any English manual for the RC310 i changed the language setting on my RC310 from German to Englisch.
The "reduced mode" is called "control type for setback mode": 1:outside, 2: reduced mode, 3:room temperature threshold

Since enums for RC310 are different, shall I use "reducemode1" or better "setbackmode" for the RC310 to be consistent with the display? What about reducetemp / noreducetemp?

The "nofrostmode" is called frost protection - so frostprotection would better fit.
"nofrosttemp" is named frost protection limit temperature. - frostprotectionlimit would be better fit.

What is your point of view?

@proddy
Copy link
Contributor

proddy commented May 24, 2022

I really don't know. I only have an RC20. Any change to the entity names will be a breaking change so be cognizant of that. What would be better is to extend the fantastic Customization service so each entity can be renamed as we do with the Dallas & Analog Sensors.

@tp1de
Copy link
Contributor Author

tp1de commented May 24, 2022

Since the entities reducemode , reducetemp, noreducetemp, are not available yet for the RC310 this would not be a breaking
change. So shall I use the existing ones and make a new entity reducemode1 - or shall a use setbackmode instead of reducemode1?

@MichaelDvP
Copy link
Contributor

MichaelDvP commented May 24, 2022

The naming is always difficult. If a function is requested by a user, we first check if we have this already in for other thermostats and try to use the same (save ram, especially for 8266 this was very important). If it's new, we pick the name the user requests. It's very confusing to have different names the same function in thermostat-models, it's more confusing to have a name with different meaning for thermostat models.
Sadly Bosch also changes names with each thermostat model. It's hard to keep an overview.

Edit: Another reference is Norberts list, If a value is listed there it's best to choose the same name for later checks.

@tp1de
Copy link
Contributor Author

tp1de commented May 24, 2022

I do understand both remarks, but I am still a bit lost what to do:

Telegram type 0x01B9 offset 05 on Norberts list ist named Eco Mode.
In ems-esp it is reducemode for RC35 but not available for RC310
On RC310 it is setback mode

I understand @MichaelDvP that I should use the existing fields and make a new enum entry reducemode1:

OK?

@MichaelDvP
Copy link
Contributor

Yes, we already have different names for same function, like nighttemp, ecotemp, T1. But i don't like the same name for different functions: You can rename nofrosttemp to frostprotecttemp in RC300, but don't use nofrosttemp for something other.

@tp1de
Copy link
Contributor Author

tp1de commented May 25, 2022

@MichaelDvP @proddy

Hi guys, I have already added quite some new values for the RC310 (PR pending). Up to yet I was only touching existing telegram types. Now I have the first new one which is not defined yet. I will try to introduce the new telegram with register_telegram_type() . Anything else to consider?

@MichaelDvP
Copy link
Contributor

In thermostat.h add a vector (~line 160), and a process_ prototype,
in thermostat.cpp:

  • fill the vector (~line140)
  • register the telegrams in the loop (with fetch-flag false)
  • add a hc-check in heatingcircuit (~line 305)
  • set fetch flag for active hc (~line 383)
  • add ' process_` function somewhere in the range of other RC300 hc-dependend process_xx (~1017)
  • (in actual dev with master thermostat) add telegram to master check: emsesp.cpp, line 175ff.

In the documentation you'll find upper left a improve this article, click it and you can edit and preview direct in browers. If done, you can make a branch and PR with a single button click.

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

Thanks Michael. I am a bit irritated in the moment. The typeids defined yet for the RC300/RC310 are consecutive hex-numbers.

No I want to introduce a new field wwpriority for each hc. I just have hc1 (radiators) and hc2 (underfloor).
The hex addresses are hc1: 0x02CC and hc2: 0x02CE.
Shall I assume hc3 is 0x02D0 and so on and create a new typeids?
Fetch flag has to be true, otherwise the telegram is only send when changes occur.

And telegram length and content differ as well:
For hc1 length is 1 byte and offset 3 with 0/1 (off/on) - 10 0B FF 03 01 CC 01 F6
For hc2 length is 6 bytes and offset 3 with 00/FF (off/on) - 10 0B FF 00 01 CE FF 13 0A FF 1E 00 20

Any advice how to handle this?

@MichaelDvP
Copy link
Contributor

wwprio is already defined for RC35, long name hcx dhw priority, The hc is set by TAG, so we need the dhw in text filed.

It seems a bit strange that RC300 telegrams are not numbered in row, all others are. Have you checked in terminal with read 10 2CC, read 10 2CD, etc. to check the complete telegrams?
BTW: for the process_ functions we have often a a complete telegram as example in function comment, This helps to check for new values. If you can add these examples to RC300 functions it would be nice.

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

Yes I checked. Telegrams are as above mentioned with set flag set. A read command in terminal gives the same reply.
0x02CD in between returns 6 empty bytes with 0x00.

wwprio is already defined for RC35, long name hcx dhw priority, The hc is set by TAG, so we need the dhw in text filed.

I will use the existing name - thanks for info.

BTW: for the process_ functions we have often a a complete telegram as example in function comment, This helps to check for new values. If you can add these examples to RC300 functions it would be nice.

Yes I will do.

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

  • (in actual dev with master thermostat) add telegram to master check: emsesp.cpp, line 175ff.

I do not really understand what I have to do there? After line 204 because thermostat id = 0x10 for RC310?

@MichaelDvP
Copy link
Contributor

I do not really understand what I have to do there? After line 204 because thermostat id = 0x10 for RC310?

RC310 is a master-thermostat, there is no adaptation needed. RC200 is compatible, but a single hc thermostat (0x18 for hc 1, 0x19 for hc2, etc, Combining multiple RC200 is matched there to a "master" at 0x18). The 0x10 section is only for junkers, where 0x10 controlls hc1 and hc2 and for hc3 there is an extra thermostat.
Please add the telegrams to the 0x18 section, then RC200 can use the values too.

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

@MichaelDvP @proddy

I do have some difficulties reading the new variables / telegrams. Is there any way to create console or system log messages for debugging ?

@MichaelDvP
Copy link
Contributor

Sure, insert wherever you want a LOG_DEBUG(F("some text with value %d"),value); or to another loglevel like LOG_INFO(...);

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

Seems to work now. Thanks so far.

@tp1de
Copy link
Contributor Author

tp1de commented May 26, 2022

Just a question: For RC300/310 the typeids have 8 entries in thermostat.cpp.
But the RC300/RC310 supports only 4 heating circuits. So why 8 entries?

@MichaelDvP
Copy link
Contributor

Long time we had only 4 entries, and than came this guy with more than four heatingcircuits. see #294, #297

@tp1de
Copy link
Contributor Author

tp1de commented May 27, 2022

Ok understood. I made the PR for wwprio (RC310) with only 4 entries since the typeids numbers are not in a row.

@MichaelDvP
Copy link
Contributor

With only 4 entries you have to take care not access or to loop through higher indices. Some loops use monitor_typeids as loopcount. I'll comment in the PR.

@emsesp emsesp locked and limited conversation to collaborators May 27, 2022
@proddy proddy converted this issue into discussion #529 May 27, 2022
@proddy proddy reopened this Feb 19, 2023
@proddy proddy closed this as completed Feb 19, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
question Question about something
Projects
None yet
Development

No branches or pull requests

3 participants