Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tasmota.py): add tasmota as a controller #575

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

cmiguelcabral
Copy link
Contributor

Using SO73 (buttons) or SO114 (switches), is possible to use Tasmota as a controller. This commits adds compatibility to ControllerX to use it.

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 19, 2022

Example usage:

---
luz_do_escritorio:
  module: controllerx
  class: TasmotaLightController
  integration:
    name: tasmota
    device: Switch1
  controller: stat/botoneira_do_escritorio/RESULT
  light: light.luz_do_escritorio

luz_da_secretaria:
  module: controllerx
  class: TasmotaLightController
  integration:
    name: tasmota
    device: Button2
  controller: stat/botoneira_do_escritorio/RESULT
  light: light.luz_da_secretaria

@xaviml
Copy link
Owner

xaviml commented Sep 25, 2022

Hi @cmiguelcabral,

What does the Tasmota integration bring to the table that MQTT is not already supporting? In general, I only accept new integration (note integration is different than devices) when it brings something new that is not part of DYI solutions. For this, I recommend to configure it through YAML with no need of changing code. For example, you could achieve the same with the following config:

luz_do_escritorio:
  module: controllerx
  class: LightController
  integration:
    name: mqtt
    payload_key: Switch1
  controller: stat/botoneira_do_escritorio/RESULT
  light: light.luz_do_escritorio
  mapping:
    "SINGLE": "toggle"
    "DOUBLE": "on_min_brightness"
    "TRIPLE": "set_half_brightness"
    "QUAD": "on_full_brightness"
    "HOLD": "on_min_max_brightness"

luz_da_secretaria:
  module: controllerx
  class: LightController
  integration:
    name: mqtt
    payload_key: Button2
  controller: stat/botoneira_do_escritorio/RESULT
  light: light.luz_do_escritorio
  mapping:
    "TOGGLE": "toggle"
    "ON": "on"
    "OFF": "off"_min_brightness
    "HOLD": "on_min_max_brightness"

Are these controllers/devices always sending same actions that you configured, or they are customized to your setup?

Regards,
Xavi M.

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 25, 2022

The example you posted doesn't work because Tasmota sends the message in a nested property.
Example:
{ "Button1": { "Action": "SINGLE" } }

If you check your documentation (https://xaviml.github.io/controllerx/examples/tasmota-switchmode11/), you see that you are using rules inside Tasmota to make it work with ControllerX.
My commit makes it work out-of-the-box without the need for rules on the Tasmota side or adjustments on the ControllerX side.

I don't see why this feature couldn't be part of ControllerX as it doesn't influence anything about other integrations/controllers and Tasmota is being used more and more by the community.

I was also thinking about adding the opposite later, making ControllerX control Tasmota directly over MQTT without going over HA. (Something like the Z2MLight).

@xaviml
Copy link
Owner

xaviml commented Sep 25, 2022

Hi @cmiguelcabral ,

So, Tasmota allows to integrate devices in a "standard" way? I though it was only for DYI purposes where one can build their own actions. If so, I have these questions to consider it as a new integration or not:

  • Do the actions always come in the same type of payload?
  • Are the actions (SINGLE, TOGGLE, ON, OFF, ...) always the same no matter the device?

Sorry for all these questions, but I've never used Tasmota and I need to unserstand how to make it fit with ControllerX.

Thanks!
Xavi M.

@htvekov
Copy link
Contributor

htvekov commented Sep 25, 2022

Hi @xaviml (long time, no see 😉) and @cmiguelcabral

Just a few comments from the guy who initiated that these changes actually was done to Tasmota switchmode11 and did that ControllerX documentation as well 🙂 It was NOT meant as a 'standard' part of ControllerX though, as the Tasmota config for the average user might be a bit overwhelming.

So if an actual Tasmota configuration in ControllerX could make the nested messages somewhat more user friendly and all communication is done via MQTT, then I guess it would be an improvement for all using Tasmota and ControllerX together.
But please remember that my notes regarding especially HA state machine & also Appdaemon are still valid. A HA automation with a MQTT trigger is IMHO still preferred to eg. toggle a light where you want as little lag as possible.

Regards
Henning

@cmiguelcabral
Copy link
Contributor Author

I would be happy to answer all the questions you have.

You can set peripherals (Digital Inputs) as Switches or Buttons. All of them will always publish the message in the same type of payload, and that would be:
{ "PERIPHERAL": { "Action": "action_type" } }

  • If the peripheral is a Button, the possible action_types are: SINGLE|DOUBLE|TRIPLE|QUAD|HOLD. If you disable multi-press, then the actions are TOGGLE|HOLD. (and I just opened a PR to add the HOLD_RELEASE, specifically to use it on ControllerX)

  • If the peripheral is a Switch, the possible actions are: TOGGLE|HOLD. If the switch is set to follow mode then it would be ON|OFF depending on the position of the switch.

But the payload will always be the same, independently of the type of peripheral you use.

This is why I thought it would make sense to add it to ControlleX as this opens a huge amount of possibilities. Right now, at my place, I have some dozens of Zigbee bulbs, all connected through a device running Tasmota in the wall. I use Tasmota to send commands to the bulbs, and in the case MQTT is down, it would then control its relay directly. ControllerX makes it way easier to do instead of using automations in HA or using the rules on Tasmota.

Please consider this before discarding it...

@cmiguelcabral
Copy link
Contributor Author

Hi @xaviml (long time, no see 😉) and @cmiguelcabral

Just a few comments from the guy who initiated that these changes actually was done to Tasmota switchmode11 and did that ControllerX documentation as well 🙂 It was NOT meant as a 'standard' part of ControllerX though, as the Tasmota config for the average user might be a bit overwhelming.

So if an actual Tasmota configuration in ControllerX could make the nested messages somewhat more user friendly and all communication is done via MQTT, then I guess it would be an improvement for all using Tasmota and ControllerX together. But please remember that my notes regarding especially HA state machine & also Appdaemon are still valid. A HA automation with a MQTT trigger is IMHO still preferred to eg. toggle a light where you want as little lag as possible.

Regards Henning

Cheers Henning!!

I think that a guy that has HACS, Appdaemon and ControllerX installed, can easilly use Tasmota.
Also, I believe there are more people using Tasmota on Shelly devices than the ones using the original Firmware, even so, ControllerX supports Shelly...

Regards,
Miguel Cabral

@htvekov
Copy link
Contributor

htvekov commented Sep 25, 2022

Well, I was mostly thinking of the needed Tasmota rule configuration that could be somewhat tricky to understand.

Anyway, seems like we share somewhat of identical setup with 'Tasmotized' devices in behind the wall switches and Zigbee lights everywhere 😉 The main reason I took this route some years ago was the WAF 👩‍🦰 and the fallback to the Tasmota switch relays in the event that HA/MQTT server was down for whatever reason.

Cant you just use CLEAR as HOLD_RELEASE ? I haven't really looked at those Tasmota logs for some years. But AFAIR it only fires once (and instantly) upon button release.

@xaviml
Copy link
Owner

xaviml commented Sep 25, 2022

Hi Miguel,

Thanks for the explanation. First of all, sorry if I gave you the impression that I would be discarding it, I just needed a bit more of context, and you gave it to me :)

Second, it totally makes sense to have a Tasmota integration given how Tasmota works, but I still have a question:

I see in your PR you only added a TasmotaLightController. Whereas other integrations have multiple device that are supported. Is this because no matter the device, they will always trigger the same action events? Is the idea to use same controller class for all devices that are integrated with Tasmota?

Regarding the PR:

  • I would keep the get_default_actions_mapping simple like other integrations, and have a single method, not 2 for tasmota. They can be defined in the same action.
  • I see that the device is Button1 and Switch2. Any reason is not just Button or Switch?

Regards,
Xavi M.

@cmiguelcabral
Copy link
Contributor Author

Hi Miguel,

Thanks for the explanation. First of all, sorry if I gave you the impression that I would be discarding it, I just needed a bit more of context, and you gave it to me :)

Second, it totally makes sense to have a Tasmota integration given how Tasmota works, but I still have a question:

I see in your PR you only added a TasmotaLightController. Whereas other integrations have multiple device that are supported. Is this because no matter the device, they will always trigger the same action events? Is the idea to use same controller class for all devices that are integrated with Tasmota?

Regarding the PR:

  • I would keep the get_default_actions_mapping simple like other integrations, and have a single method, not 2 for tasmota. They can be defined in the same action.
  • I see that the device is Button1 and Switch2. Any reason is not just Button or Switch?

Regards, Xavi M.

Because in Tasmota you can have many buttons or switches, so it's possible for you to have Button1, Button2, Button3...(and so on) and at the same time Switch1, Switch2, Switch3 (and so on). AFAIK you can go up to 8 buttons and 8 switches together in the same device. I just made this as an example.

As Buttons and switches have different actions I split them, but they actually can be joined together as a unique type with the actions:
TOGGLE
OFF
ON
SINGLE
DOUBLE
TRIPLE
QUAD
PENTA
CLEAR

And as I told you before in the email, this is just a draft for you to see and tell me what you want me to do or change before opening the actual PR.

@xaviml
Copy link
Owner

xaviml commented Sep 25, 2022

Hi @cmiguelcabral ,

So these Button1, Button2, ... names are automatically set by tasmota or is set by you?

Also, could you send a link or explain what actions are performed to trigger those events?

And as I told you before in the email, this is just a draft for you to see and tell me what you want me to do or change before opening the actual PR.

Thanks! I do appreciate your PR and it is something it makes sense to have in ControllerX, so we can work together to integrate. The question is: do you want to work on the changes, or you want me (or I can) work in your branch?

Regards,
Xavi M.

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 25, 2022

So these Button1, Button2, ... names are automatically set by tasmota or is set by you?

Tasmota sets those names automatically, but you can change those names to others that you want. That's why I made it possible to choose the name using the "device" property.

If you change the name for, let's say "best_switch_ever", then you get:
image

Also, could you send a link or explain what actions are performed to trigger those events?

Here you can find documentation about the way buttons and switches work:
https://tasmota.github.io/docs/Buttons-and-Switches/

Thanks! I do appreciate your PR and it is something it makes sense to have in ControllerX, so we can work together to integrate. The question is: do you want to work on the changes, or you want me (or I can) work in your branch?

We can do it either way.
I'm available to follow your suggestions and update the PR myself, or, if you find it faster to do it yourself it's also ok. I would just like to be the PR owner and become a contributor... :-)

This is how you set them on Tasmota:
image

@cmiguelcabral
Copy link
Contributor Author

By the way,
As you can see in the PR, there is a placeholder for Switchmode 11 and 12, because it has some bugs now. The bugs make it impossible to use this mode right now.
Another thing, right now Tasmota doesn't have a CLEAR or RELEASE action after Holding a button. But I already opened a PR on their side to fix this, let's see what I can get there...
arendst/Tasmota#16657

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 25, 2022

Looking into it now, we will either need to merge the buttons and the switches into only "get_default_actions_mapping", or add a separate property to define if it's a button or a switch.
The way I did won't work if someone changes the name for something that doesn't have "Button" or "Switch" in the name.

So we can do it in two ways:
1:
image

2:
image

Here's an example of it working;
image

@htvekov
Copy link
Contributor

htvekov commented Sep 26, 2022

By the way, As you can see in the PR, there is a placeholder for Switchmode 11 and 12, because it has some bugs now. The bugs make it impossible to use this mode right now. Another thing, right now Tasmota doesn't have a CLEAR or RELEASE action after Holding a button. But I already opened a PR on their side to fix this, let's see what I can get there... arendst/Tasmota#16657

You're absolutely right, @cmiguelcabral. I'm currently running old, old Tasmota v9.2.0.1 on my production switches (if it ain't broke - don't fix it) and neither HOLD nor CLEAR is there as standard button action. But again, I haven't configured any buttons at all, as I'm solely relying on the switch states and my rules. In one of my other devices I can see that the HOLD action is there, but not the CLEAR (HOLD_RELEASE).

07:19:15.589 RUL: SWITCH1#STATE=4 performs "backlog publish tasmota/office {"action": "inc-dec"};rule3 0"
07:19:15.600 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:15.607 MQT: tasmota/office = {"action": "inc-dec"}
07:19:15.677 MQT: kontor_loftlys/stat/RESULT = {"Rule3":"OFF","Once":"ON","StopOnError":"OFF","Length":88,"Free":423,"Rules":"on switch1#state=4 do backlog publish tasmota/office {\"action\": \"inc-dec\"};rule3 0 endon"}
07:19:15.943 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.330 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.646 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.966 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.290 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.609 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.633 RUL: SWITCH1#STATE=7 performs "backlog publish tasmota/office {"action": "clear"};rule3 1"
07:19:17.641 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.648 MQT: tasmota/office = {"action": "clear"}
07:19:17.712 MQT: kontor_loftlys/stat/RESULT = {"Rule3":"ON","Once":"ON","StopOnError":"OFF","Length":88,"Free":423,"Rules":"on switch1#state=4 do backlog publish tasmota/office {\"action\": \"inc-dec\"};rule3 0 endon"}
07:19:18.546 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}

@cmiguelcabral
Copy link
Contributor Author

By the way, As you can see in the PR, there is a placeholder for Switchmode 11 and 12, because it has some bugs now. The bugs make it impossible to use this mode right now. Another thing, right now Tasmota doesn't have a CLEAR or RELEASE action after Holding a button. But I already opened a PR on their side to fix this, let's see what I can get there... arendst/Tasmota#16657

You're absolutely right, @cmiguelcabral. I'm currently running old, old Tasmota v9.2.0.1 on my production switches (if it ain't broke - don't fix it) and neither HOLD nor CLEAR is there as standard button action. But again, I haven't configured any buttons at all, as I'm solely relying on the switch states and my rules. In one of my other devices I can see that the HOLD action is there, but not the CLEAR (HOLD_RELEASE).

07:19:15.589 RUL: SWITCH1#STATE=4 performs "backlog publish tasmota/office {"action": "inc-dec"};rule3 0"
07:19:15.600 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:15.607 MQT: tasmota/office = {"action": "inc-dec"}
07:19:15.677 MQT: kontor_loftlys/stat/RESULT = {"Rule3":"OFF","Once":"ON","StopOnError":"OFF","Length":88,"Free":423,"Rules":"on switch1#state=4 do backlog publish tasmota/office {\"action\": \"inc-dec\"};rule3 0 endon"}
07:19:15.943 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.330 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.646 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:16.966 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.290 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.609 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.633 RUL: SWITCH1#STATE=7 performs "backlog publish tasmota/office {"action": "clear"};rule3 1"
07:19:17.641 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}
07:19:17.648 MQT: tasmota/office = {"action": "clear"}
07:19:17.712 MQT: kontor_loftlys/stat/RESULT = {"Rule3":"ON","Once":"ON","StopOnError":"OFF","Length":88,"Free":423,"Rules":"on switch1#state=4 do backlog publish tasmota/office {\"action\": \"inc-dec\"};rule3 0 endon"}
07:19:18.546 MQT: kontor_loftlys/stat/SWITCH1T = {"TRIG":"ON"}

That's it!!
It has a CLEAR that is fired internally, but nothing is published.
But that's actually solved, my PR was just merged:
arendst/Tasmota#16657

So we can add the CLEAR to the list of Button actions.
This makes the Hold from buttons behave a bit differently than the hold from switches (the later ones don't have the release option). Given this, I would go for the second approach of the two I showed above, What do you thing @xaviml?

@htvekov
Copy link
Contributor

htvekov commented Sep 26, 2022

Hi Miguel,
Thanks for the explanation. First of all, sorry if I gave you the impression that I would be discarding it, I just needed a bit more of context, and you gave it to me :)
Second, it totally makes sense to have a Tasmota integration given how Tasmota works, but I still have a question:
I see in your PR you only added a TasmotaLightController. Whereas other integrations have multiple device that are supported. Is this because no matter the device, they will always trigger the same action events? Is the idea to use same controller class for all devices that are integrated with Tasmota?
Regarding the PR:

  • I would keep the get_default_actions_mapping simple like other integrations, and have a single method, not 2 for tasmota. They can be defined in the same action.
  • I see that the device is Button1 and Switch2. Any reason is not just Button or Switch?

Regards, Xavi M.

Because in Tasmota you can have many buttons or switches, so it's possible for you to have Button1, Button2, Button3...(and so on) and at the same time Switch1, Switch2, Switch3 (and so on). AFAIK you can go up to 8 buttons and 8 switches together in the same device. I just made this as an example.

As Buttons and switches have different actions I split them, but they actually can be joined together as a unique type with the actions: TOGGLE OFF ON SINGLE DOUBLE TRIPLE QUAD PENTA CLEAR

And as I told you before in the email, this is just a draft for you to see and tell me what you want me to do or change before opening the actual PR.

Will SINGLE message still fire once before DOUBLE is fired ?
Or in other words are you waiting for the defined hold delay set via SETOPTION32 command and only issuing one event message ?

@cmiguelcabral
Copy link
Contributor Author

Hi Miguel,
Thanks for the explanation. First of all, sorry if I gave you the impression that I would be discarding it, I just needed a bit more of context, and you gave it to me :)
Second, it totally makes sense to have a Tasmota integration given how Tasmota works, but I still have a question:
I see in your PR you only added a TasmotaLightController. Whereas other integrations have multiple device that are supported. Is this because no matter the device, they will always trigger the same action events? Is the idea to use same controller class for all devices that are integrated with Tasmota?
Regarding the PR:

  • I would keep the get_default_actions_mapping simple like other integrations, and have a single method, not 2 for tasmota. They can be defined in the same action.
  • I see that the device is Button1 and Switch2. Any reason is not just Button or Switch?

Regards, Xavi M.

Because in Tasmota you can have many buttons or switches, so it's possible for you to have Button1, Button2, Button3...(and so on) and at the same time Switch1, Switch2, Switch3 (and so on). AFAIK you can go up to 8 buttons and 8 switches together in the same device. I just made this as an example.
As Buttons and switches have different actions I split them, but they actually can be joined together as a unique type with the actions: TOGGLE OFF ON SINGLE DOUBLE TRIPLE QUAD PENTA CLEAR
And as I told you before in the email, this is just a draft for you to see and tell me what you want me to do or change before opening the actual PR.

Will SINGLE message still fire once before DOUBLE is fired ? Or in other words are you waiting for the defined hold delay set via SETOPTION32 command and only issuing one event message ?

No, only one is fired.
You will get only, SINGLE or DOUBLE... or HOLD

@cmiguelcabral
Copy link
Contributor Author

That's actually the problem with SwitchMode 11/12. As the TOGGLE is sent always even together with DOUBLE, or HOLD. And the POWER_RELEASE is also fired everytime.

@htvekov
Copy link
Contributor

htvekov commented Sep 26, 2022

Well, it's actually by design that the internal Tasmota switchstates are fired that way.
I do not wish to wait for 0,8 seconds (my current setoption32 delay value) before Tasmota fires a TOGGLE or SINGLE event.
When I flick a light on/off, I want instant reaction.

Above issue has in many cases been debated with one button devices. There are use cases where you want the initial first TOGGLE fired instantly without any lag at all and are willing to sacrifice the ease of implementation. Other use cases that are not extremely time important, would on the other hand benefit from the much easier event handling with only one event fired.

@htvekov
Copy link
Contributor

htvekov commented Sep 26, 2022

This is also the reason why I've restricted my Tasmota events to either:

  1. TOGGLE and DOUBLE events for 'regular' lightbulbs /LEDs (groups) without dimming capabilities.
  2. TOGGLE and HOLD events for zigbee lightbulbs (groups) with dimming capabilities.

@cmiguelcabral
Copy link
Contributor Author

Yes, I understand and agree with you.
But what I mean is that the Switchmode 11/12 doesn't work as expected, as it doesn't meet what's documented.
It is stated you have a TOGGLE fired immediately on button press, and that the TOGGLE is also fired together with DOUBLE, but if you need to use both there's a POWER_DELAY fired only after TOGGLE if there is no DOUBLE. Actually that POWER_DELAY never happens...
This makes the usage of this switchmode impossible as you aren't able to make use of the single click. I think this is a bug and not by design.
Also, the POWER_CLEAR that is fired after the HOLD (POWER_INCREASE) is the same that's fired on every other type of click.
So, Switchmode11/12 is great to be used with Rules, but a mess to be used outside of Tasmota. Even the events it triggers on HA are a bit messy.

@htvekov
Copy link
Contributor

htvekov commented Sep 26, 2022

Yes, I understand and agree with you. But what I mean is that the Switchmode 11/12 doesn't work as expected, as it doesn't meet what's documented. It is stated you have a TOGGLE fired immediately on button press, and that the TOGGLE is also fired together with DOUBLE, but if you need to use both there's a POWER_DELAY fired only after TOGGLE if there is no DOUBLE. Actually that POWER_DELAY never happens... This makes the usage of this switchmode impossible as you aren't able to make use of the single click. I think this is a bug and not by design. Also, the POWER_CLEAR that is fired after the HOLD (POWER_INCREASE) is the same that's fired on every other type of click. So, Switchmode11/12 is great to be used with Rules, but a mess to be used outside of Tasmota. Even the events it triggers on HA are a bit messy.

I agree. That seems to be a bug indeed. Has always thought that the general Tasmota events were somewhat messy.
That's why I chose to stick with the internal switchstate events only for all my switches and via rules decide exactly what events are to be handled with a specific mqtt topic and message.

A Tasmota event 'clean up' seems to be needed 😉

@cmiguelcabral
Copy link
Contributor Author

Yes, I understand and agree with you. But what I mean is that the Switchmode 11/12 doesn't work as expected, as it doesn't meet what's documented. It is stated you have a TOGGLE fired immediately on button press, and that the TOGGLE is also fired together with DOUBLE, but if you need to use both there's a POWER_DELAY fired only after TOGGLE if there is no DOUBLE. Actually that POWER_DELAY never happens... This makes the usage of this switchmode impossible as you aren't able to make use of the single click. I think this is a bug and not by design. Also, the POWER_CLEAR that is fired after the HOLD (POWER_INCREASE) is the same that's fired on every other type of click. So, Switchmode11/12 is great to be used with Rules, but a mess to be used outside of Tasmota. Even the events it triggers on HA are a bit messy.

I agree. That seems to be a bug indeed. Has always thought that the general Tasmota events were somewhat messy. That's why I chose to stick with the internal switchstate events only for all my switches and via rules decide exactly what events are to be handled with a specific mqtt topic and message.

A Tasmota event 'clean up' seems to be needed 😉

True, but with SetOption73 and/or SetOption114 enabled, if you don't use Switchmode 11/12, it works decently and makes up a nice controller.
And now, with the CLEAR message on hold release, the switchmode 11 becomes a bit useless.

@xaviml
Copy link
Owner

xaviml commented Sep 26, 2022

Hi @cmiguelcabral ,

I'm available to follow your suggestions and update the PR myself, or, if you find it faster to do it yourself it's also ok. I would just like to be the PR owner and become a contributor... :-)

Sounds good to me. I will be leaving some comments in the PR.

The way I did won't work if someone changes the name for something that doesn't have "Button" or "Switch" in the name.
So we can do it in two ways:

Let's go with 2nd option, although I am not convinced about the naming since we will have a TasmotaButtonSwitchController and a TasmotaSwitchSwitchController. If you have another naming suggestion, it will be welcome, otherwise, we can just go with those.

So we can add the CLEAR to the list of Button actions.

That is great!!

As you can see in the PR, there is a placeholder for Switchmode 11 and 12, because it has some bugs now. The bugs make it impossible to use this mode right now.

I suggest removing those from the PR for now if they still have some bugs. They can be added once they serve a purpose and no bugs are presented.

Regards,
Xavi M.

@xaviml xaviml marked this pull request as ready for review September 26, 2022 13:18
Copy link
Owner

@xaviml xaviml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good! But some changes will be required apart from the ones already mentioned:

  • Add documentation about new integration in docs/docs/start/integrations.md. Probably mention that this integration requires enabling MQTT plugin for AppDaemon. You can reference this page, like done for MQTT and Zigbee2MQTT integrations.
  • Add get_tasmota_actions_mapping() function in apps/controllerx/cx_core/controller.py, like done with other integrations.
  • It is better if you use this image for the devices. You will need to copy and paste the same iamge and change the name according to their device names (which is the class names without the LightController or SwitchController part).
  • Add an entry to RELEASE_NOTES.md. Uncomment the ## :pencil2: Features title and add an entry explaining the new integration with 1 short sentence.

Thanks!

apps/controllerx/cx_core/integration/tasmota.py Outdated Show resolved Hide resolved

class TasmotaIntegration(Integration):
name = "tasmota"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since device attribute is mandatory, I suggest overwriting the __init__() to check if kwargs contains device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this, I will need some help, as this is the first time I code python... :-\

apps/controllerx/cx_devices/tasmota.py Outdated Show resolved Hide resolved
apps/controllerx/cx_devices/tasmota.py Outdated Show resolved Hide resolved
apps/controllerx/cx_devices/tasmota.py Outdated Show resolved Hide resolved
apps/controllerx/cx_devices/tasmota.py Outdated Show resolved Hide resolved
@xaviml
Copy link
Owner

xaviml commented Sep 26, 2022

By the way, I recommend you pulling the latest from main and rebasing it to your branch git rebase main, then you will need to push force your branch: git push -f origin tasmota. This way, it will fix the pipeline and the test will run correctly.

Using SO73 (buttons) or SO114 (switches), is possible to use Tasmota as a controller. This commits adds compatibility to ControllerX to use it.
@xaviml
Copy link
Owner

xaviml commented Sep 27, 2022

Hi @cmiguelcabral ,

Fantastic job!!

I don't know if the Actions I chose as default are ok.

The default action you chose looks good to me. In the end, it will vary depending on the user, so they can configure that with custom mappings.

I'm just testing the integration, it is freakin fast...

That is great!! MQTT integration with ControllerX surprised me the first time I added it.. There is a lot of difference wrt listening to HA entities...

R egarding this, I will need some help, as this is the first time I code python... :-\

Since device attribute is mandatory, I suggest overwriting the __init__ to check if kwargs contains device.

If you want and allow me, I will be adding an extra commit on top of yours to:

  • Add an early check for the component attribute since it is mandatory and it makes sense to error out when device is initializing.
  • Add some tests for the integration.
  • Fix the tests that are currently failing in the pipeline.
  • Maybe adding some words or links to the documentation you added (in any case, it is good documentation).

Thank you :)

@cmiguelcabral
Copy link
Contributor Author

The default action you chose looks good to me. In the end, it will vary depending on the user, so they can configure that with custom mappings.

I'm thinking about some examples of how to use this. But that can come later.

If you want and allow me, I will be adding an extra commit on top of yours to:

  • Add an early check for the component attribute since it is mandatory and it makes sense to error out when device is initializing.
  • Add some tests for the integration.
  • Fix the tests that are currently failing in the pipeline.
  • Maybe adding some words or links to the documentation you added (in any case, it is good documentation).

Please do it.
I was about to ask you exactly that! :-)

@xaviml
Copy link
Owner

xaviml commented Sep 27, 2022

@cmiguelcabral go ahead and check my last commit as well as testing the following if possible:

  • Check the integration it is still working for you.
  • Check if you get an error when initializing when no component is added to the configuration.
  • Check my commit for any typos or inconsistencies you might find.

Thanks!

@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Base: 96% // Head: 96% // Increases project coverage by +0% 🎉

Coverage data is based on head (edfed50) compared to base (7cb7bbf).
Patch coverage: 98% of modified lines in pull request are covered.

❗ Current head edfed50 differs from pull request most recent head c50f152. Consider uploading reports for the commit c50f152 to get more accurate results

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #575   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        61     63    +2     
  Lines      2530   2590   +60     
===================================
+ Hits       2427   2486   +59     
- Misses      103    104    +1     
Impacted Files Coverage Δ
apps/controllerx/cx_core/integration/tasmota.py 97% <97%> (ø)
apps/controllerx/controllerx.py 100% <100%> (ø)
apps/controllerx/cx_core/controller.py 95% <100%> (+<1%) ⬆️
apps/controllerx/cx_devices/tasmota.py 100% <100%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@htvekov
Copy link
Contributor

htvekov commented Sep 27, 2022

Wow - you guys are... fast!🚀🚀😆
I'll test this as well. Curious about the response time 😉

And Xavi, I already told you many months ago that HA state machine is..... ☠slow (compared to MQTT)🤣

@xaviml
Copy link
Owner

xaviml commented Sep 27, 2022

And Xavi, I already told you many months ago that HA state machine is.....

You did @htvekov ... and I am glad you pushed me to integrate MQTT into ControllerX, it is one of the top features in my opinion!

Once @cmiguelcabral is happy with the checks, I will merge and create a prerelease, so you can download easily from HACS and test it out.

Regards,
Xavi M.

@cmiguelcabral
Copy link
Contributor Author

Go ahead!!

@htvekov, in fact, it is way better when using mqtt directly.
The HA state machine comes handy only for the MIN_MAX and stuff like that.

@cmiguelcabral
Copy link
Contributor Author

Wow - you guys are... fast!🚀🚀😆 I'll test this as well. Curious about the response time 😉

And Xavi, I already told you many months ago that HA state machine is..... ☠slow (compared to MQTT)🤣

Will it be now that you update your Tasmotas?!

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 27, 2022

Wait...

Something is not right:
image

  • Check the integration it is still working for you.

It works, but I get this error every time I push a button.

  • Check if you get an error when initializing when no component is added to the configuration.

image

  • Check my commit for any typos or inconsistencies you might find.

Everything seems ok to me.

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 27, 2022

I get what's going on now!
We cannot check for the component_key because, for a given device, all the buttons and switches will publish in the same topic. So when I press Button1, there is no Switch4 (or any other component in the same device). So we get an error for each component we have in the same device.

This is an example of a device with 4 components. When I press one, the other 3 will throw errors:

image

Comment on lines 29 to 41
if component_key in payload:
try:
action_key = str(json.loads(payload)[component_key][payload_key])
except json.decoder.JSONDecodeError:
raise ValueError(
f"`key` is being used ({payload_key}). "
f"Following payload is not a valid JSON: {payload}"
)
except KeyError:
raise ValueError(
f"Following payload does not contain `{payload_key}`: {payload}"
)
await self.controller.handle_action(action_key)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will need to be reverted.

@xaviml
Copy link
Owner

xaviml commented Sep 28, 2022

Hi @cmiguelcabral ,

I understand, I assumed that component_key was always mandatory. I will revert to previous behaviour and change tests accordingly.

What it matters is that if "component" is not in the payload, it should ignore the message and not error out.

Thanks!

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 28, 2022

Yes, that's it.
The problem only occurs when we have a device with multiple components, but even so, it works. But throws the error, which is a bit annoying... :)

In this case, all the changes you made are ok, just this part needs to be adapted to ignore when the command comes from other component.
You can either do it the same way you're doing the payload check and just return:

if component_key not in payload:
  return

xaviml added a commit to cmiguelcabral/controllerx that referenced this pull request Sep 28, 2022
@xaviml
Copy link
Owner

xaviml commented Sep 28, 2022

Hi @cmiguelcabral,

I added the fix to not error out if the key that fails with is the component_key. Could you please check again that everything is okay. Then, I will merge the PR.

Thanks!

@cmiguelcabral
Copy link
Contributor Author

Hi @cmiguelcabral,

I added the fix to not error out if the key that fails with is the component_key. Could you please check again that everything is okay. Then, I will merge the PR.

Thanks!

I noticed and just checked out your commit.
Give me some minutes to test.

@cmiguelcabral
Copy link
Contributor Author

Now everything seems to be working perfectly.

image

@cmiguelcabral
Copy link
Contributor Author

cmiguelcabral commented Sep 28, 2022

I think now it's done!

Anyway, I'm no python expert, but I would do it the way I suggested, I think it has better performance.
Let's say you have a device with 10 buttons.
Every time you press one of them will need to do this part of the code 10 times. One of them will effectively run, and the other 9 will be discarded. But you'll try to first decode a JSON, then fail, and ignore the failure.

        try:
            action_key = str(json.loads(payload)[component_key][payload_key])
        except json.decoder.JSONDecodeError:
            raise ValueError(
                f"`key` is being used ({payload_key}). "
                f"Following payload is not a valid JSON: {payload}"
            )
        except KeyError as e:
            if e.args[0] == component_key:
                return

If you just discard it at the beginning, you'll avoid all of this...
Just by doing this before the 'try'.

if component_key not in payload:
  return

xaviml added a commit to cmiguelcabral/controllerx that referenced this pull request Sep 28, 2022
@xaviml
Copy link
Owner

xaviml commented Sep 28, 2022

You convinced me.. I didn't want to do it at first because "payload" is a string, and the "component_key not in payload" conditions just checks that the string "component_key" is preset in the string "payload". However, I agree that the solution just works fine and as you said it would be better for performance.

Just force pushed last commit.

Thank you for raising the concern :)

@xaviml
Copy link
Owner

xaviml commented Sep 28, 2022

You can no check new documentation here:

I am launching now new beta release. In a few minutes, it will be out to grab from HACS:

https://github.com/xaviml/controllerx/releases/tag/v4.24.0b0

@cmiguelcabral
Copy link
Contributor Author

Yey!!!

I don't know how many people will actually use this, but for me will be a lifesaver!!

I have so many devices that I need to configure one by one, and now, everything will be done in just one place, on yaml files!!

Thank you for the patience!!

@xaviml
Copy link
Owner

xaviml commented Sep 28, 2022

I will let you know if I need your help if someone comes with an issue with the tasmota integration.

Thank you for the work and the contribution! :)

@cmiguelcabral
Copy link
Contributor Author

I will let you know if I need your help if someone comes with an issue with the tasmota integration.

Be my guest, I'll be happy to help!

Thank you for the work and the contribution! :)

I really enjoyed doing this.
I'll try to make more contributions every time a new idea comes up!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants