-
-
Notifications
You must be signed in to change notification settings - Fork 97
(v2) normalize command infrastructure for devices (mqtt & console) #445
Comments
@MichaelDvP anything to add? |
If all home automation systems can do that, it's ok.
is than in telnet? As a context or a third parameter?
The mqtt commands should have exactly the same name as the mqtt pubish. i.e. if published as How do you think about nested commands like |
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 |
worked a bit of this today. It changes a lot of files so needs careful testing. |
I have thought about that an tested in small range only inside thermostat and would prefer if the command is (this was my test-command finally: thermostat_set_command.txt) |
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. |
@MichaelDvP I'm working on the thermostat bit now. I see you implemented parsing of nested commands like Is this important to have? It adds a lot of complexity to the new code I'm refactoring. Wouldn't just |
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. |
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 The payload must have a
this does impose some restrictions imposed by the json library that I'm not happy about, like
will this work? |
also @MichaelDvP I'm not to happy using the where ? |
on the other hand, |
It really needs to be a verb, like set is. So options are I'll stick with |
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 |
I made a real word test first, but limited to my system.
some observations:
|
thanks for testing and the feedback. |
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? |
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 |
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. |
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? |
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. |
changed it anyway :-) |
@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. |
yes, this sucks so I'm working on change that allows the data and id to be a string or a number. |
Q&D this works:
|
that would work too. I used |
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. |
Something else I noticed: Also the reported So if you set So maybe this needs some alignment? |
First we need to get the right commands for your Junkers thermostat. Could you look at the code in 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? |
According to the thermostat the supported modes are There also seems to be a Vacation mode in the thermostat but I never used that before. The description for that mode is:
Indeed, I wanted to point out that reported mode is incorrect, or I would rather say inconsistent compared to the sent command. |
If i understand correctly the actual situation is:
You like to have the temperature publish and commands to be |
Yes, FW200 to be pricise, so I cannot confirm that this report is valid for other Junkers thermostats.
Yes, that is correct.
Also correct
Yes
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:
|
I mean the publish, not the subscription.
|
Yes, indeed, I don't see those. So never missed them until now that I know 😄
And this is the output of the thermostat_data publish: |
Junkers values as mentioned in #445, save some memory for remote ther…
I removed the Rx queue on the latest v2_cmd. It never got more than one record in so no point using a buffer and expensive list. Makes the code tidier and I would expect slightly quicker. Apart from that no other side effects as far as I can tell. I'm still getting the odd Rx corrupted telegrams. In my tests
conclusion? no idea. I think perhaps we've built a quantum computer where the observer is changing the outcome. I'm now running another test at 80Hz to see if that helps. -- edit --
they look like valid telegrams, just cut short because the BREAK is falsely detected. |
I tried the latest code. It works like a charm! |
@proddy i've pushed a uart branch based on latest v2_cmd to make sure that the timer is not triggert by something else in the framework and sends a unwanted break. |
@MichaelDvP just tried that now. Got the first Rx fail after 3 minutes (which is usual btw) so that change didn't seem to make a difference. I'll need to go back to a v2_cmd build where everything was working fine, and then do a diff. |
My last rx errors were 15.8. as reported in #398, before there were also a few errors (not logged). I made a |
This is the status of ems:
I guess I need to look at incomplete telegrams for failed reads? So those are 0 at 19h uptime. The failed tx are coming from Norberts hardware on RPi which is also still connected to the bus. |
Yes, that look good.
I remember, the rpi uses modem device-id, but do not reply to version request from ems-esp. So you get a tx-fail every minute. |
still its nice to see that you have no incomplete Rx's. |
Eventually I'll disconnect the RPi from the bus after I finish moving some remaining automations from OpenHAB to Home Assistant. So implementing a fix is not really necessary from my side. |
I tested 8 different builds going back as early as b9 on the old v2 branch from July 28 and each test gave Rx incompletes every 1 hr or so. Then I tried rolling back the espressif core to 2.6.1 and 2.6.0 but that didn't help either. I can't figure out why we're seeing this now. I'll open a new Issue for this. In any case it has nothing to do with the v2_cmd changes so I'll merge this back into v2. |
merged into the v2 branch. |
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
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:cmd
payload={"target":"boiler", "cmd":"flowtemp", "data":55}
cmd
payload={"target":"thermostat", "cmd":"mode", "data":"auto"}
cmd
payload={"target":"thermostat", "cmd":"mode", "id":2, "data":"auto"}
cmd
payload={"target":"system", "cmd":"fetch"}
cmd
payload={"target":"system", "cmd":"gpio", "id":1, "data":"on"}
homeassistant/climate/ems-esp/hc<num>/<cmd_temp|cmd_mode>
. At some point, we should have our own component installed via HACS.set <cmd> <data> [hc]
in the associated context. 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".set
commands for debugging mainly such as setting the wifi credentials or changing the UART tx modeThe text was updated successfully, but these errors were encountered: