emsesp / EMS-ESP

ESP8266 firmware to read and control EMS and Heatronic compatible equipment such as boilers, thermostats, solar modules, and heat pumps
https://emsesp.github.io/docs
GNU Lesser General Public License v3.0
300 stars 96 forks source link

(v2) normalize command infrastructure for devices (mqtt & console) #445

Closed proddy closed 3 years ago

proddy commented 3 years ago

The MQTT commands inherited from v1.9 and the new Console commands are not always in sync. The code needs some refactoring and simplification. This will also help reduce the memory hog when running mqtt, telnet, and web at the same time on an ESP8266.

Specification

1) sending commands to EMS-ESP is done by a single topic called cmd. The payload determines the device/sub-system and the action. An additional id field can be used to set the heating circuit. For example:

This will simplify the code significantly. When using a console command it will just invoke the MQTT function effectively simulating an incoming MQTT request. It will mean each command function will take now a char * and be responsible for deciphering the values from the payload string. For example the setcomfort() function will no longer an int values 1, 2 or 3 but "hot", "eco", "intelligent".

4) The WebUI will be used for configuring EMS-ESP. The console will have a few set commands for debugging mainly such as setting the wifi credentials or changing the UART tx mode

proddy commented 3 years ago

@MichaelDvP anything to add?

MichaelDvP commented 3 years ago

If all home automation systems can do that, it's ok. Before you use "data", now "value", is there a special reason? But i think normal json with : instead of = and how do you want to manage the different hc on thermostat?

{"target":"thermostat", "hc":2, "cmd":"mode", "value":"day"}

is than in telnet? As a context or a third parameter?

# thermostat
# hc 2
# mode day

The mqtt commands should have exactly the same name as the mqtt pubish. i.e. if published as mode the command should be mode not setmode.

How do you think about nested commands like {"thermostat":{"hc2":{"mode":"auto"}}} and #thermostat hc2 mode auto

proddy commented 3 years ago

i was making changes while you were replying it seems - I think most of your comments have been addressed. I was thinking whether we should use 'mode' or 'setmode'. The reason I choose 'set' was 1) because it's an action and 2) maybe one day we'll need to add a get command to query a value via MQTT. But thinking about it more you're right. How about the command is 'mode' and in the console the command would be set mode <hc> <data>

proddy commented 3 years ago

worked a bit of this today. It changes a lot of files so needs careful testing.

MichaelDvP commented 3 years ago

I have thought about that an tested in small range only inside thermostat and would prefer if the command is set <cmd> <data> [hc] with hc optional as last parameter defaults to first active hc if not set. Also there is no confusion with thermostat global- and boiler-commands: set <cmd> <data>

(this was my test-command finally: thermostat_set_command.txt)

proddy commented 3 years ago

that's good since my implementation follows that same standard. I fixed a few bugs along the way too. Now you just need to register the command and it'll will propagate to the mqtt and console functions. Since it's quite a major change I'll create a feature branch and let you know when it's pushed.

proddy commented 3 years ago

@MichaelDvP I'm working on the thermostat bit now. I see you implemented parsing of nested commands like {"hc2":{"temp":21}}.

Is this important to have? It adds a lot of complexity to the new code I'm refactoring. Wouldn't just {"cmd":"temp", "hc":"1", data="21"} suffice? Or do you have a specific use case.

MichaelDvP commented 3 years ago

No, not important. I added this a while ago in 1.5 for me and when switching to 2.0 some of my scripts were using it, but easy to change. For the actual splitting in boiler_cmd/boiler_data and thermostat_cmd/thermostat_data i also find it more intuitive to use the same structure for data and command. But if you go for a single cmd it's clear to use a different structure. (I think you plan to deserialize in the cmd-function to check the target and the thermostat-on-command will receive cmd, data, hc seperated) BTW: i forgot some thermostat commands to add to cmd/data.

proddy commented 3 years ago

Right now I took a slightly different approach. I eliminated the 'target'. Now each device when its recognized will subscribe to its own MQTT topic called "<device>_cmd" like thermostat_cmd and boiler_cmd. There is a special one for the system.

The payload must have a cmd and optional are data, id, or hc. So these are all valid

thermostat_cmd {"cmd":"temp", "hc":1, "data"="21"} // hc must be numeric
thermostat_cmd {"cmd":"temp", "data"="21"} // hc is optional
thermostat_cmd {"cmd":"temp", "data"="21", "id":1} // id here is the same as hc
boiler_cmd {"cmd":"comfort", "data"="eco"}
system_cmd {"cmd":"gpio", "id":1, "data"="on"} // sets D1 to HIGH

this does impose some restrictions imposed by the json library that I'm not happy about, like

will this work?

proddy commented 3 years ago

also @MichaelDvP I'm not to happy using the set command as it conflicts with the internal settings of EMS-ESP. e.g. set master <n> or set tx_mode <n> change the settings which are also available via the web.

where set comfort eco in the boiler context is used to request a change on an EMS device. I think a better word would be call or cmd or change.

?

MichaelDvP commented 3 years ago

cmd looks a bit odd in the help as cmd <cmd> <data>, change <cmd> <data> is much better, or use ems <cmd> <data> since it changes a ems value.

on the other hand, cmd is more in line with the mqtt command.

proddy commented 3 years ago

It really needs to be a verb, like set is. So options are run <cmd> exec <cmd> call <cmd> do <cmd>

I'll stick with call for now. It's only one line that needs to be changed anyway ;-)

proddy commented 3 years ago

I created a branch https://github.com/proddy/EMS-ESP/tree/v2_cmd with the latest changes. There's quite a bit. Still needs some more testing but let me know if you run into issues.

This branch has all the latest updates from v2.

I've been using the standalone build for testing. If you have gcc installed type make run and it'll run the simulation with a few tests being automatically executed.

MichaelDvP commented 3 years ago

I made a real word test first, but limited to my system. some fixes:

some observations:

proddy commented 3 years ago

thanks for testing and the feedback.

proddy commented 3 years ago

only subscriptions for thermostat and boiler, system_cmd is missing

I called it system, Should it be system_cmd ?

Capture

proddy commented 3 years ago

mixing gpio and Wemos-D-numbers leads to confusion.

What do you suggest? Stick to the GPIO numbers or D numbers? It uses D* numbers now and maps to gpio values (using your code). Should the command be renamed to "pin" for example?

proddy commented 3 years ago

mqtt reconnects every 5 minutes (exactly 297 seconds)

I'm not seeing this. Been running for 15hrs will no MQTT errors. Make sure you don't have multiple EMS-ESPs running (e.g. ESP8266 and ESP32) using the same MQTT client ID.

I'll keep testing

MichaelDvP commented 3 years ago

The esp32 has a lot of spare gpio, most without wemos D-numbers, so it's better to use gpio, but readme/wiki should note the difference to D-numbers.

The reconnects are gone when i clear the value in my mqtt broker. Also there no more error messages. I guess the commands are registered after subscription of thermostat_cmd, but the broker connection is publish on subscribe. I'll try with QoS.

I don't have a system subscription, but i see this. // add the system MQTT subscriptions, only if its a fresh start with no previous subscriptions

proddy commented 3 years ago

there was a bug in the MQTT re-subscribe so I fixed that. Also renamed the topic to "system_cmd" so it's consistent with the rest.

I'll change to use the gpio numbers. This also means your MQTT logic will change too as I believe the code you added took pin numbers D0-D3. Ok if I change that?

MichaelDvP commented 3 years ago

The D0-D3 was from #375. Since the command has changed, there is no problem. I think it's clear that `gpio' command also means gpio-numbers.

proddy commented 3 years ago

changed it anyway :-) pin <gpio> [data]. In MQTT the id is the gpio,

proddy commented 3 years ago

The latest build 2.0.0b12-2 in v2_cmd addresses some of the memory issues. What I did is

So compared to 2.0.0b11 from the v2 branch, the available heap on my ESP8266 has gone up from 64% to 75%. The web loads all the time now and shows around 2% fragmentation and 65% free memory from within the UI. With both telnet and Web active its around 55% free memory.

If you guys can test, let me know of any feedback and then I'll merge it back into v2 and remove this branch.

bbqkees commented 3 years ago

Testing now. Indeed the web interface loads nicely on the ESP8266! Lets see how this holds up over the weekend.

What I did notice since b11 is that I now have incomplete RX telegrams, which I have never seen before.

Do you plan on moving around specific settings in the web interface or do you plan to leave it as it is now? I need to start writing new instructions for V2.

MichaelDvP commented 3 years ago

Very smooth, i'm testing now on 8266 with NTP enabled and 80MHz. Now issues so far. Web keeps responsive and reloads fast, also with parallel telnet session. After closing telnet heap fragmentation (as percentage) is a bit higher than before, but the largest block is bigger and always >9k, enough for good web response. If telnet is reopend fragmentation is low again. Also after some telnet open/close/open.. all is stable.

proddy commented 3 years ago

thanks for testing. I have 1 incomplete Rx too (meaning a corrupt telegram). I don't remember seeing that before so will need to check why. I also noticed the console (not just telnet, also serial) doesn't free up all the heap when it closes the session. There's always a 1-2kb left behind. It's always been like that in v2 and I haven't been able to find the root cause yet. I eventually gave up searching as it's not such a huge problem I think as only power-users would use the telnet interface.

MichaelDvP commented 3 years ago

i get two subscriptions for boiler_cmd on the mqtt-broker, but none for system_cmd,

mqtt.0 | 2020-08-13 14:54:35.827 | info | (1502) Client [ems-esp] publishOnSubscribe
mqtt.0 | 2020-08-13 14:54:35.826 | info | (1502) Client [ems-esp] subscribes on "mqtt.0.ems.thermostat_cmd"
mqtt.0 | 2020-08-13 14:54:35.626 | info | (1502) Client [ems-esp] publishOnSubscribe
mqtt.0 | 2020-08-13 14:54:35.625 | info | (1502) Client [ems-esp] subscribes on "mqtt.0.ems.boiler_cmd"
mqtt.0 | 2020-08-13 14:54:34.827 | info | (1502) Client [ems-esp] publishOnSubscribe
mqtt.0 | 2020-08-13 14:54:34.826 | info | (1502) Client [ems-esp] subscribes on "mqtt.0.ems.boiler_cmd"
mqtt.0 | 2020-08-13 14:54:34.815 | info | (1502) Client [ems-esp] reconnected. Old secret 1597320264839_7724. New secret 1597323274814_4982

also show mqtt give:

MQTT subscriptions:
 topic: ems/boiler_cmd, [cmd]: comfort wwactivated wwtapactivated wwonetime wwcirculation flowtemp wwtemp burnmaxpower burnminpower boilhyston boilhystoff burnperiod pumpdelay
 topic: ems/thermostat_cmd, [cmd]: wwmode temp mode remotetemp datetime minexttemp clockoffset calinttemp display building language control pause party holiday nighttemp daytemp nofrosttemp ecotemp heattemp summertemp designtemp offsettemp holidaytemp

MQTT queue is empty

sending commands result in:

000+00:11:49.277 E 1: [mqtt] No MQTT handler found for topic ems/system_cmd and payload {"cmd":"pin","data":"on","id":"5"}
000+00:12:52.777 E 2: [mqtt] No MQTT handler found for topic ems/system_cmd and payload {"cmd":"send","data":"0B 90 37 20 00"}
proddy commented 3 years ago

strange. works for me. I'll do some further testing. Are you on ESP8266 or ESP32?

Capture

MichaelDvP commented 3 years ago

I'm on esp8266, NTP and 80MHz.

Another strange thing. show devices lists for every device all known type-ids:

These EMS devices are currently active:

Boiler: GB125/MC10 (DeviceID:0x08, ProductID:72, Version:03.00)
 This Boiler will respond to telegram type IDs: 0x10 0x11 0x18 0x19 0x34 0x1C 0x2A 0x33 0x14 0x35 0x15 0x16 0x1A 0xD1 0xE3 0xE4 0xE5 0xE9 0xA3 0x06 0x3E 0x3D 0x48 0x47 0x52 0x51 0x5C 0x5B 0xA5 0x37 0xAA 0xAB 0xAC
 Subscribed MQTT topics: boiler_cmd

Thermostat: RC35 (DeviceID:0x10, ProductID:86, Version:01.18) ** master device **
 This Thermostat will respond to telegram type IDs: 0x10 0x11 0x18 0x19 0x34 0x1C 0x2A 0x33 0x14 0x35 0x15 0x16 0x1A 0xD1 0xE3 0xE4 0xE5 0xE9 0xA3 0x06 0x3E 0x3D 0x48 0x47 0x52 0x51 0x5C 0x5B 0xA5 0x37 0xAA 0xAB 0xAC
 Subscribed MQTT topics: thermostat_cmd

Mixing Module: MM10 (DeviceID:0x21, ProductID:69, Version:02.01)
 This Mixing Module will respond to telegram type IDs: 0x10 0x11 0x18 0x19 0x34 0x1C 0x2A 0x33 0x14 0x35 0x15 0x16 0x1A 0xD1 0xE3 0xE4 0xE5 0xE9 0xA3 0x06 0x3E 0x3D 0x48 0x47 0x52 0x51 0x5C 0x5B 0xA5 0x37 0xAA 0xAB 0xAC

Controller: BC10/RFM20 (DeviceID:0x09, ProductID:68, Version:02.03)
 This Controller will respond to telegram type IDs: 0x10 0x11 0x18 0x19 0x34 0x1C 0x2A 0x33 0x14 0x35 0x15 0x16 0x1A 0xD1 0xE3 0xE4 0xE5 0xE9 0xA3 0x06 0x3E 0x3D 0x48 0x47 0x52 0x51 0x5C 0x5B 0xA5 0x37 0xAA 0xAB 0xAC

Maybe there is something wrong with my built (merging your v2_cmd). I'll fetch your v2_cmd to a new branch and compile again.

proddy commented 3 years ago

is your MQTT Qos 1 or 2? I didn't test that

proddy commented 3 years ago

This Thermostat will respond to telegram type IDs: 0x10 0x11 0x18 0x19 0x34 0x1C 0x2A 0x33 0x14 0x35 0x15 0x16 0x1A 0xD1 0xE3 0xE4 0xE5 0xE9 0xA3 0x06 0x3E 0x3D 0x48 0x47 0x52 0x51 0x5C 0x5B 0xA5 0x37 0xAA 0xAB 0xAC

I see this too. it's a bug.

MichaelDvP commented 3 years ago

is your MQTT Qos 1 or 2? I didn't test that

QoS 0 and CleanSession on

(I've changed mqtt.cpp line 86 to subscribe(1, topic, cb); and system_cmd worked again.) Now compiling a fresh branch withou chnages.

proddy commented 3 years ago

i fixed the bug with show devices too, checking that it now

MichaelDvP commented 3 years ago

With clean build system_cmd is subscribed and working.

MichaelDvP commented 3 years ago

Yes, now mqtt subscriptions and type-ids are ok, heap is a little bit less.

I also noticed the console (not just telnet, also serial) doesn't free up all the heap when it closes the session. There's always a 1-2kb left behind.

It's only 0.1-0.2kb for me, and after ~5 times opening/closing the console memory keeps stable and does not drop further. After a while free memory goes up again. I don't think it's a problem.

bbqkees commented 3 years ago

B12 is still running fine on ESP8266. Uptime of one day now, still a responsive and instantly loading web interface.

Its just those incomplete RX telegrams that I never had before.

proddy commented 3 years ago

thats good. same thing here, running fine for 24hrs with 3 corrupted Rx telegrams today, like:

ERROR [telegram] Rx: 17 0B A8 00 01 00 FF F6 01 06 00 01 0D 01 00 FF FF 01 02 02 02 00 00 05 1F 05 1F 02 0E 00 90 (CRC 90 != 10)

because its missing the last byte of the telegram. A correct telegram looks like

OK [telegram] Rx: 17 0B A8 00 01 00 FF F6 01 06 00 01 0D 01 00 FF FF 01 02 02 02 00 00 05 1F 05 1F 02 0E 00 FF DF (notice the extra byte)

Why this has changed I don't know since the UART code is the same as with the previous builds. Still looking into it

-- edit -- I've moved this conversation back to https://github.com/proddy/EMS-ESP/issues/398#issuecomment-674116979

FredericMa commented 3 years ago

I took the latest code for a run but it looks like some thermostat commands are missing:

ems-esp:/# show mqtt
MQTT is connected
MQTT Heartbeat is enabled
MQTT publish fails: 0

MQTT subscriptions:
 topic: ems-esp/boiler_cmd, [cmd]: comfort wwactivated wwtapactivated wwonetime wwcirculation flowtemp wwtemp burnmaxpower burnminpower boilhyston boilhystoff burnperiod pumpdelay
 topic: ems-esp/thermostat_cmd, [cmd]: wwmode temp mode
 topic: ems-esp/system_cmd, [cmd]: pin send

MQTT queue is empty

I have a Junkers FW200.

This command if failing on topic ems-esp/thermostat_cmd : {"cmd":"nofrosttemp" ,"data":17, "id":1} I see this error: MQTT error: no matching cmd: nofrosttemp

With previous firmware the following command was working on the same topic: {"hc1":{"mode":"nofrost","nofrosttemp":"28"}}

Maybe I missed something? Is it also still possible to chain command like before? Like settings the mode and temp at the same time like the example above?

Thanks for your efforts!

proddy commented 3 years ago

cool that you tried it out @FredericMa , thanks. The missing Thermostat commands is a bug, I just forgot to extend the commands to also include Junkers. It's an easy fix.

chaining commands in a single MQTT got removed in the new way we now handle commands. But it's a nice idea. It would mean supporting two different structures:

{"cmd":<cmd> ,"data":<data>, "id":<n>}

and

{<id>:{"<cmd>":"<data>",...}

Do you have a use case you can share so I understand how you're building these MQTT payloads? Is this something that Domoticz does for example?

@MichaelDvP what do you think about adding this back? It'll require some tricky coding to parse each json element and check whether it's a valid command.

MichaelDvP commented 3 years ago

@FredericMa

This command if failing on topic ems-esp/thermostat_cmd : {"cmd":"nofrosttemp" ,"data":17, "id":1}

"data":17does not work anymore, you have to use "data":"17" (with quotation marks)

@proddy i don't think this is realy needed. For manual testing it's less typing, but for normal operation it's no problem to send two or more commands in a row.

proddy commented 3 years ago

"data":17does not work anymore, you have to use "data":"17" (with quotation marks)

yes, this sucks so I'm working on change that allows the data and id to be a string or a number.

MichaelDvP commented 3 years ago

Q&D this works:

                if (doc["data"] == nullptr) {
                    LOG_ERROR(F("MQTT error: invalid payload cmd format. message=%s"), message);
                    return;
                }
                const char * data = doc["data"];
                char data1[30];
                if (data == nullptr) {
                    if (float f = doc["data"]) {
                         Helpers::render_value(data1, f, 10);
                    }
                } else {
                    strcpy(data1, data);
                }

if (!call_command(mf.device_type_, command, data1, n)) {

proddy commented 3 years ago

that would work too. I used JsonVariant id = doc["id"]; if (id.is<char*>()) { etc...

FredericMa commented 3 years ago

Do you have a use case you can share so I understand how you're building these MQTT payloads? Is this something that Domoticz does for example?

Actually I use it for the heat mode. I'm using Home Assistant but implemented some custom code since circuit is controlling the radiators circuit (on the second floor) but on that circuit I have valves that are controlled via KNX. So the KNX valve controller has a bit that indicates heat demand. This is captured by HA and that enables the heating circuit via ems-esp. I want the circuit 1 water temp to be around 65°C so I set the circuit 1 setpoint to current room temp + 5°C. The thermostat actually uses the current room temperature of circuit 2 (floor heating on ground floor) for circuit 1. By using current room temp + 5°C the heater will always target approx 65°C. The it is up to the valves to control the target temperature in each room.

Like Michael says, it is not necessary to have the ability to chain multiple commands. Two separate commands works fine as well.

FredericMa commented 3 years ago

Something else I noticed: I can still set the mode to "heat". I guess because of the mapping over here: https://github.com/proddy/EMS-ESP/blob/67877d07c51b5e456e86bc55ec334151e3c827b1/src/devices/thermostat.cpp#L1497-L1512 But I'm unable to use the heattemp command because only nofrosttemp, nighttemp and daytemp are registered.

Also the reported mode_type is heat, eco or nofrost according to this code I guess: https://github.com/proddy/EMS-ESP/blob/67877d07c51b5e456e86bc55ec334151e3c827b1/src/devices/thermostat.cpp#L627-L636

So if you set nofrosttemp this is for nofrost mode_type that is also reported as mode_type nofrost which is OK. But the following can cause some confusion since if you set nighttemp and night mode_type, you actually set the temp and mode_type for the mode_type that will be reported as eco. The same for daytemp and mode_type day which modify the mode_type reported as heat.

So maybe this needs some alignment?

proddy commented 3 years ago

First we need to get the right commands for your Junkers thermostat. Could you look at the code in Thermostat::add_commands() and let me know which commands should be supported?

Then for the mapping of modes, are you saying when you change the temperature for a mode (like daytemp or nighttemp) it should also automatically switch the mode? Or is it that the reported mode is just in correct?

FredericMa commented 3 years ago

First we need to get the right commands for your Junkers thermostat. Could you look at the code in Thermostat::add_commands() and let me know which commands should be supported?

According to the thermostat the supported modes are Automatic, Comfort, Economy, and Frost So that would map to auto, comfort (or heat), eco and nofrost.

There also seems to be a Vacation mode in the thermostat but I never used that before. The description for that mode is:
You can use this function if you want to set a constant operating mode for several days (e.g. Frost ) without changing the heating program. When the vacation program is active, the heating circuits and domestic hot water systems are operated according to the operating mode set in the vacation program (frost protection is automatically provided).

Then for the mapping of modes, are you saying when you change the temperature for a mode (like daytemp or nighttemp) it should also automatically switch the mode? Or is it that the reported mode is just in correct?

Indeed, I wanted to point out that reported mode is incorrect, or I would rather say inconsistent compared to the sent command.

MichaelDvP commented 3 years ago

If i understand correctly the actual situation is:

You like to have the temperature publish and commands to be heattemp, ecotemp, nofrosttempand add the missing nofrosttemp to the publish, right?

FredericMa commented 3 years ago

Your thermostat is a Junkers FW?

Yes, FW200 to be pricise, so I cannot confirm that this report is valid for other Junkers thermostats.

The thermostat shows modes: Automatic | Comfort | Economy | Frost Ems-esp shows on telnet: mode: Manual | Auto | Holiday modetype: heat | eco | nofrost

Yes, that is correct.

mqtt publish for mode and modetype is the same as terminal

Also correct

mqtt commands for mode is heat | eco | nofrost | auto, also working day | night | nofrost | auto

Yes

Ems-esp shows on terminal and mqtt only daytemp, nighttemp Edit: actually not shown, telegram SetJunkers is not read

Maybe I don't understand the question correctly but the setpoints are shown correctly for all modes. The shown mqtt subscriptions for thermostat are these: topic: ems-esp/thermostat_cmd, [cmd]: wwmode temp mode nofrosttemp nighttemp daytemp

Ems-esp commands on mqtt and call are daytemp, nighttemp, nofrosttemp Yes You like to have the temperature publish and commands to be heattemp, ecotemp, nofrosttemp and add the missing nofrosttemp to the publish, right?

nofrosttemp is actually working since @proddy added the command. It is heattemp and ecotemp that are not working but daytempand ´nighttemp` do work. See line from terminal above.

MichaelDvP commented 3 years ago

Maybe I don't understand the question correctly but the setpoints are shown correctly for all modes. The shown mqtt subscriptions for thermostat are these: topic: ems-esp/thermostat_cmd, [cmd]: wwmode temp mode nofrosttemp nighttemp daytemp

I mean the publish, not the subscription.