TheAgentK / tuya-mqtt

Nodejs-Script to combine tuyaapi and openhab via mqtt
MIT License
173 stars 80 forks source link

convertMessage function breaks on/off for switch #14

Closed tsightler closed 5 years ago

tsightler commented 5 years ago

Perhaps this is not a bug but I'm really struggling to understand what the latest patch was intended to do, and, for my simple environment (just switches and plugs) it completely broke a setup that worked perfectly before. Was there intentionally a change in behavior?

Specifically I don't understand this code:

function boolToString(istate) {
    return istate == 1 ? 'on' : "off";
}

function convertMessage(message) {
    var status = message.toString();
    status = boolToString(status);
    status = status.toLowerCase();
    return status;
}

Typically the message is either "ON" or "OFF", at least, it was, and the examples are still written this way, however, the code above will always return off in this case. It will work if you pass it 1/0 or maybe somehow true/false, although true/false as a string will not work. Even more confusing, earlier in the code there was already a function call boolToString which behaved differently:

function boolToString(istate) {
    return istate ? 'true' : "false";
}

Just having two functions with the same name that do different things seems incorrect. I'd submit a patch to try to fix it, but I'm not sure what the intended behavior of this patch was supposed to be.

tsightler commented 5 years ago

BTW, my "fix" is just to do this for now:

function convertMessage(message) {
    var status = message.toString();
 //   status = boolToString(status);
    status = status.toLowerCase();
    return status;
}

Simply remarking out that single line and the code returns to working just fine for me.

TheAgentK commented 5 years ago

The latest changes were intended to support the new MQTT 2.4 binding.

The new binding sends different messages over MQTT than before. Unfortunately I destroyed the support for the version 1.x.

Do you still use the verions 1.x?

tsightler commented 5 years ago

No, I'm the one that submitted the initial patch for 2.4 support because I wanted to use the 2.4 binding. I was able to use both the 1.x and 2.4 binding with my code interchagebly.

However, in the last two weeks you submitted a new patch set, one titled remove custom set function. It's this patch that breaks my 2.4 setup, and doesn't work with 1.x either, unless somebody was actually sending 0/1 instead of ON/OFF with their binding.

I cannot figure what the intention of this code was because it's logic does not make sense to me. You receive the message, which should be either "ON" or "OFF", but the converMessage function calls "boolToString" which only returns "on" of the message is "1". So no matter what, the switch is always turned off. It's not clear to me what you intended that code to do.

If people are using the 1.x binding, they don't even use the message as there's a separate topic for "on" vs "off" and the message should be null. Technically we could update the instructions to simply tell people how to make the 1.x binding send the same message as the 2.4 bindings and remove all of this code.

I'd love to get the resolved because I have some other enhancements which I've coded up which I think users would find useful. I've added the ability to define swithes in a config file, and elminitated the requirment to provide an IP address (uses the TuyaAPI resolveID function). This works better when IP might change due to DHCP. I've also added auto discovery using the MQTT Home Assistant discovery protocol, which also works with OpenHAB, and also detects when devices are offline and even, optionally, sets unreachable state so that users can be aware the device is offline.

I can always just fork the project and call it something else, but prefer to send the patches upstream if you are interested in them.

TheAgentK commented 5 years ago

@tsightler

This ticket has not been processed for a relatively long time. I think this problem has been fixed with the latest changes, especially with the DEV branch.

I made some changes and the "convertMessage" function is no longer used because it was really a bit buggy.

If I don't hear from you anymore, I will close this ticket in the next days.

Commit 86e602d6cf93da54cb00efd838cc622e7fd0921c fixes #14

tsightler commented 5 years ago

I was running a version with my own patches around this anyway and, in a sad turn, a number of my devices were upgraded to the new Tuya 2.0 firmware, which breaks the Tuyapi code completely. Because of this I've had to revert to cloud control for my devices anyway, thus not using tuya-mqtt anymore, as much as I prefer it. If the guys working on the tuyapi stuff eventually crack the 2.0 firmware, I may be back, otherwise I'll live with cloud control or convert all my Tuya switches to open firmware.

If you feel confident it's resolved, that's good enough for me! Thanks!

TheAgentK commented 5 years ago

I already read the firmware 2.0 in the TuyAPI project. I hope that my devices will be spared. But I also hope for a quick solution of the TuyAPI guys.