letscontrolit / ESPEasy

Easy MultiSensor device based on ESP8266/ESP32
http://www.espeasy.com
Other
3.29k stars 2.22k forks source link

Combination of P001 and P029 (DomoticzMQTTHelper) causes incorrect behaviour. #2834

Closed wdonker closed 1 year ago

wdonker commented 4 years ago

In my setup I use Domoticz as controller of several nodes running EspEasy (NodeMCU and Sonoff). Since I recently updated software to mega-20191208 the combination of P001 and P029 causes incorrect behaviour. This is due to one or maybe even 2 bugs in handling Domoticz MQTT messages; I will try to explain.

My setup contains 2 nodes using P029. The first is a NodeMcu that has only 1 device (P029) used to dim some GPIO. The second one is a Sonoff basic with 2 device: a P029 device to switch the relay and a P001 device that handles toggling the button (using rules). The P029 device (actually C002.ino) subscribes to the MQTT-topic domoticz/out and listens to messages that contain the idx configured in the P029-task. The payload of such a message is like below (this is an example of a message meant for the NodeMCU node controlling the Hal/light):

""Battery"" : 255, ""Level"" : 45, ""RSSI"" : 12, ""description"" : """", ""dtype"" : ""Light/Switch"", ""id"" : ""000140B4"", ""idx"" : 100, ""name"" : ""Hal"", ""nvalue"" : 2, ""stype"" : ""Switch"", ""svalue1"" :"

Now, since my latest upgrade, after switching the Hal/light ESP Easy publishes the result to the same MQTT topic (domoticz/out) with payload:

""log"": ""GPIO 12 Set PWM to 450"", ""plugin"": 1, ""pin"": 12, ""mode"": ""PWM"", ""state"": -2

The Sonoff module receives this message and does NOT recognize that it is not the correct structure for a message from Domoticz. It (C002.ino) looks for the idx and since it is not there the idx is set to 0. The second device (the button) is configured with 'send to controller' unchecked and 'idx' = 0. Now ESP Easy handles the message as being meant for this device and toggles the button, which in turn switches of the relay. That was not intended ;-)

IMO 2 bugs are involved here:

  1. the most important is that C002.ino does not check for a valid MQTT-payload (that at least contains an idx) and also does not check that the idx found belongs to a device of type P029 (since that is the only type that could be relevant). I think this bug is already present for a long time (at least since 20180319) but comes to light now ESP Easy publishes the status message. The code concerned starts at line 85 of C002.ino:
          for (taskIndex_t x = 0; x < TASKS_MAX; x++) {
            // We need the index of the controller we are: 0...CONTROLLER_MAX
            if (Settings.TaskDeviceEnabled[x] && (Settings.TaskDeviceID[ControllerID][x] == idx)) // get idx for our controller index
            {
    The check on idx <> 0 is missing here.
    BTW: comments mention index of controller where it should be index of task
  1. ESP Easy should maybe not publish a status message to MQTT-topic domoticz/out. If the goal is to inform the sender (Domoticz) of the result then it should be on topic domoticz/in and structured like other messages to that topic (but I doubt this is useful in this case). Controller.ino tries to change the topic for the output message by pubname.replace(F("/#"), F("/status")); but that does not work for pubname domoticz/out.

P.S. As a workaround I changed the idx of the Sonoff button from 0 to some value not in use by Domoticz.

TD-er commented 4 years ago

An IDX of 0 should not be allowed at all in Domoticz. So I think having a check for the IDX value does sound like a proper improvement to me.

I have to re-read this later this week when I am less tired (have been moving to another place for the past few days)

wdonker commented 4 years ago

Indeed, Domoticz starts numbering devices from 1.

Take your time, moving to a new home is stressful and the workaround is ok for now.

wdonker commented 1 year ago

This issue can be closed since it was fixed together with issue #3846.