Skip to content

Commit

Permalink
Fix MQTT message trigger
Browse files Browse the repository at this point in the history 
Fixes esphome/esphome-core#335
  • Loading branch information
@OttoWinter
OttoWinter committed Dec 27, 2018
1 parent 6aac419 commit b5cdb9f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion esphomeyaml/components/mqtt.py
View file
Expand Up @@ -173,7 +173,7 @@ def to_code(config):
add(mqtt.set_reboot_timeout(config[CONF_REBOOT_TIMEOUT]))

for conf in config.get(CONF_ON_MESSAGE, []):
rhs = mqtt.make_message_trigger(conf[CONF_TOPIC])
rhs = App.register_component(mqtt.make_message_trigger(conf[CONF_TOPIC]))
trigger = Pvariable(conf[CONF_TRIGGER_ID], rhs)
if CONF_QOS in conf:
add(trigger.set_qos(conf[CONF_QOS]))
Expand Down

5 comments on commit b5cdb9f

@voicevon
Copy link

Choose a reason for hiding this comment

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

I am very confused now. This bug should be at somewhere inside Esphomelib, NOT Esphomeyaml. Am I right ? If it's too complex to explain, Please give me a simple answer yes/no. Thanks.

@brandond
Copy link
Collaborator

Choose a reason for hiding this comment

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

esphomeyaml generates the c++ 'main.cpp' file that gets compiled and linked against esphomelib. While most of the complicated stuff is in esphomelib, the code generated here is responsible for some critical bits, like ensuring that components get created and registered properly.

In this case, a trigger was being created but not registered, so nothing was wired up to it.

@voicevon
Copy link

@voicevon voicevon commented on b5cdb9f Dec 28, 2018 •
edited

Choose a reason for hiding this comment

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

First , I did some studying on esphomelib's code, but have never touched esphomeyaml yet. I fully agree with you the first paragraph.

Based on all of my known,

  1. In this case, the registering of trigger should be done once it created.
  2. As a rule, every components(at least sensor, switch, trigger. ) will be registered once it's created.
  3. In this case,the registering should be inside of MQTTMessageTrigger *make_message_trigger(const std::string &topic); or deeper in depth.
  4. esphomeyaml does manage the source code of main.cpp in esphomelib only, It should create component , should not register any component. Saying the registering is "automatically" here.

Conclusion:
I am still confused. The above item at least one is wrong, I can't find out it.

@voicevon
Copy link

Choose a reason for hiding this comment

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

My fault is item 3, trigger is registered out of MQTTMessageTrigger *make_message_trigger(const std::string &topic). it is in setup() of main.cpp.

@OttoWinter
Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically make_message_trigger should do the registering, but that would require including application.h - and that file includes all other header files, so doing so would massively slow down the build.

As the whole automation engine is esphomeyaml-only anyway (nobody would write automation code in pure -lib) I don't care if the fix is in esphomeyaml.

Please sign in to comment.