DaveGut / HubitatActive

Hubitat Environment Developments
64 stars 87 forks source link

LED device status fix #13

Closed Jon8RFC closed 3 years ago

Jon8RFC commented 3 years ago
  1. Move sendEvent to button-press action so that it is consistent with multiple button presses (multiple presses cycled the status incorrectly)
  2. Remove superfluous debug logging (it's already in ledOn and ledOff)
  3. Add "error" level status to led device status (test proof-of-concept by setting response.system.set_led_off.err_code == 0 and then turning LED on/off)
  4. Remove seemingly unnecessary sendEvent from line 809
  5. Minor typo fixes
DaveGut commented 3 years ago

Jon, I appreciate your input. However, I am will not be making these changes. I decided early on to run the wifi TP-Link devices as state machines and to have Hubitat to reflect the state reported by the device. Therefore, all states are set in the command parse section of the return from the on/off/level commands. If the parse returns a simple error level, a refresh is sent to obtain the actual state.

You are welcome to modify the app and drivers to meet your individual needs and desires.

Again, thanks for the input.

Dave

Jon8RFC commented 3 years ago

Thanks again for making these drivers!

The current code results in this unexpected behavior displayed in Hubitat, which is an incorrect state of the device: https://youtu.be/pKVpund6x1A

The changes I made still make the same assumption that the 6.3.2 code does of "assume that whatever sent command was accepted and applied appropriately, unless the device returns an error state", but the displayed state in Hubitat is reflected as expected. I'm not a software developer, by any means, but this fixes the issue I'm experiencing and maybe others experience it, too but haven't mentioned it.

It seems like a bug in the 6.3.2 driver, but maybe I'm misunderstanding something. I could post in the driver thread and ask if others experience the same thing, because maybe it's just my devices that display the LED state incorrectly.

DaveGut commented 3 years ago

However, the attribute switch (and ledstate) should reflect the actual state of the devices switch (not the last button pressed). Your evidence does not preclude nor exclude that fact. Also, pressing the buttons rapidly may cause communications problems with wifi devices (they could be busy handling one request while another is being received - they then ignore the second request).

Jon8RFC commented 3 years ago

I believe the video, at 0:14, showcases that the actual state of the device is not reflected accurately using the 6.3.2 code. It seems like this code:

else if (response.system.set_led_off) {
            if (response.system.set_led_off.err_code == 0) {
                def onOff = "on"
                if (device.currentValue("led") == "on") { onOff = "off" }
                sendEvent(name: "led", value: onOff)
                logDebug("distResp: Led On/Off = ${onOff}")
            } 

doesn't, and can't, read the state of the LED. It just makes an assumption that whatever command was sent didn't cause an error, and thus the Hubitat displayed status is changed to the opposite of whatever it was previously set to, which is exactly how it works in practice as shown in the video. Am I misunderstanding how the Hubitat displayed LED state is supposed to function?

The rate of clicking the button also plays no role, because I can click it and wait many seconds and click again and the displayed state becomes incorrect. The 6.3.2 driver will only change the displayed state IF it receives that response.system.set_led_off.err_code == 0 as well, so it seems like it's functioning fine and receiving commands correctly.

My updated code does not change the switch functionality or the switch's displayed state in Hubitat, at all.

DaveGut commented 3 years ago

I will look into it and correct as I see fit.

DaveGut commented 3 years ago

Just ran a test on a switch (HS200) using the code. What I did was turn on debug logging, then turned off, on, off, then on the LED. The log shows three lines for each showing (1) the command being sent, (2) the JSON of the response from the switch, and (3) the final value of ON/OFF. My observations is that the attribute changed shortly after the command as expected.

image

Jon8RFC commented 3 years ago

Yes, it changes in the logs almost as you'd expect it to (except for the typo), because you're not clicking twice in succession on the same button. The bug is that you can click twice, as shown in the video, on the same button, and the display is changed incorrectly.

Because you aren't pressing the same button twice in succession, you won't see the bug in the logs and won't see it with the device conflicting with the display in Hubitat. If you're trying to NOT reproduce the bug by not clicking twice on the same button, you won't reproduce it.

Press the "Led On" button twice, or even ten times, in succession without ever pressing the Off button, and observe the display status along with whether the LED is actually on or off. In 6.3.2, Hubitat and the logs are not displaying an accurate and matching status of the device's LED actual on or off state; 6.3.2 is displaying what it was coded to display, which is to set the displayed status to the opposite of what it was prior.

You also have a typo in the debug code line (probably just a copy & paste mistake) which can make it confusing to read as to which button you're actually pressing, where it says "ledOn" when you're actually clicking the ledOff button, and it's in the log you have there as well: ledon_bug

That's a correction I made in the code I submitted: https://github.com/DaveGut/HubitatActive/pull/13/commits/80759b9ebfac87305d312ef1344e5aaa7df66b58#r677018269