homieiot / homie-esp8266

💡 ESP8266 framework for Homie, a lightweight MQTT convention for the IoT
http://homieiot.github.io/homie-esp8266
MIT License
1.36k stars 307 forks source link

Virtual Methods for HomieNode - auto-register Node #94

Closed euphi closed 8 years ago

euphi commented 8 years ago

As proposed in #87 and implemented in #91, virtual methods were added to HomieNode, so most code for each node can be encapsulated in a class derived from HomieNode.

In the current state, the only necessary function to call from outside the class is registerNode(). So, it would be nice to have this step performed automatically.

For now, I see two possible solutions with some major drawbacks:

  1. Call Homie.registerNode(*this) in constructor of HomieNode:

    Drawback: It is necessary that Homie is constructed first. There is no easy way to ensure this, because the initialization order is not defined for objects in different compilation units. Alternatively, Homie could be constructed dynamically when first accessed ("Singleton") with the drawback off dynamic memory allocation (and maybe some other pitfalls).

  2. Create Node registry file that is included from Homie once, so it is initialized after Homie.

    Drawback: More complicated than just calling registerNode(..) in the main setup().

@unaiur : Your proposed soluton to call registerNode() from HomieNode::setup() does not work, because nobody calls virtual HomieNode::setup() when the node has not yet registered?!

unaiur commented 8 years ago

We can also use a gcc extension called init_priority.

https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/C---Attributes.html

unaiur commented 8 years ago

Sadly, esp8266/Arduino doesn't execute constructors in the right order. I've contributed a fix upstream:

https://github.com/esp8266/Arduino/pull/2074

unaiur commented 8 years ago

How do you see this proposal?

https://github.com/unaiur/homie-esp8266/tree/autoregister.

It removes the array of nodes and converts it to a linked list. First & last pointers are stored as static members of HomieNode. Also, it adds 3 static methods to HomieNode: for_each() that executes a lambda function on every registered node, find() which looks up a node, and getNodeCount() which returns the number of registered nodes.

euphi commented 8 years ago

Looks great :+1:

Memory overhead is small, no dynamic memory is used and there is no more need for a fixed Node limit. Apropos "dynamic": Personally I don't have a use case [1], but it may make sense to create and delete nodes dynamically. Therefore a static method to remove a node from the list is necessary, that can be called from HomieNode::~HomieNode() (or use a double-linked list).

[1] maybe something like creating Nodes for simple inputs/output (pushbutton/switches) based on a configuration file.

Unfortunately, I don't have the time to test (and won't in the next weeks).

marvinroger commented 8 years ago

Nice! Well done. Will definitely merge it. I like the way it simplifies the code while adding benefits (no need to register nodes, no hard limits).

@euphi I am not for dynamic nodes based on a configuration file, I want Homie for ESP8266 to be code-based rather than configuration-based for operation. SmingNode is configuration-based, if you want to take a look at it. :)

marvinroger commented 8 years ago

Thanks!