eslavnov / pylips

Control Philips TVs (2015+) and Ambilight (+ Hue) through their reverse-engineered API (+ MQTT support!)
MIT License
342 stars 60 forks source link

[BUG] MQTT never goes to "update" loop #35

Closed MartinTerp closed 5 years ago

MartinTerp commented 5 years ago

Describe the bug When running in mqtt mode, the program never goes to the update loop, eventhough mqtt_update is set to true

To Reproduce Steps to reproduce the behavior:

  1. set mqtt_update = true
  2. python ./pylips.py
  3. "Started MQTT status updater" message never appears

Expected behavior To update the mqtt broker and display verbose info regarding this

Screenshots

Additional context

It seems like https://github.com/eslavnov/pylips/blob/04ba4bc38bd0e6179a43b76fe78adf72bb1177b5/pylips.py#L361 will do a blocking loop, meaning that i will never start the updater, instead i propose to use: self.mqtt.loop_start(), since it will run in its own thread, and allow for the updater to run, this function also handles reconnects.

eslavnov commented 5 years ago

Hi, thanks for reporting this! Seems like I've (again) messed up the mqtt part... I've been using loop_start() before but it lead to some issues, that's why I've switched to loop_forever(). I thought I've tested it properly but I guess not...

I'll take a look at it next week and will fix it. Quick question: is mqtt_listen set to True or False in your config?

MartinTerp commented 5 years ago

its set to True, i dont know that much about mqtt so did a bit of debugging to see where the code died :) if i locally set it to loop_start it seems to work fine, for me, dont know what issues you had the last time :)

eslavnov commented 5 years ago

Thanks, I think there is a flaw in the logic that handles the mqtt connection that affects some combinations of mqtt_update and mqtt_listen. The change to loop_forever() fixed one of those combinations, but seems like it broke the other one... Should be easy enough to fix and I'll make sure to properly test all combinations.

eslavnov commented 5 years ago

Hey @MartinTerp,

Thank you again for the bug report, I think I've fixed it. Could you please pull the latest version and let me know if it works for you now?

MartinTerp commented 5 years ago

Yes that seems to work :)