Proposal to optionally run HomieNode::loop() also in disconnected state

[euphi]

See #354, #487

[jamesmyatt]

Why is this moved? It may break functionality that assumes that loopFunction is executed before the individual node loop methods (for example). Or, in fact, all of the connection logic that is immediately below this.

[euphi]

After the loop is before the loop.

Prior the change, the loop-function was called as last. Now it is called first, so the order has not changed.

[jamesmyatt]

OK. Maybe it's not so bad, but the end of the loop and the start are only the same if there's nothing else in the Arduino loop function.

[jamesmyatt]

I'm strongly opposed to this implementation. It could break existing HomieNodes that assume that the loop methods are only executed when MQTT is connected. Furthermore, if a particular HomieNode expects the runLoopDisconnected to be set to function correctly, then there is no way to enforce that and users may forget to set the flag, creating unexpected behaviour.

I suggest to have two "loop" methods per HomieNode: one that only runs when connected (as currently) and one that runs even when disconnected. Then it is up to the implementation of each HomieNode to determine when the loop functionality should run.

I'm also worried about unintended consequences of moving where the node loop methods are executed within the homie loop method.

[euphi]

runLoopDisconnected is initialized to false, so the default behaviour has not changed.

Furthermore, runLoopDisconnected is a field of HomieNode, so each HomieNode can set the flag itself. It even couldoverride the setter, so no one else can change this.
(I think it is sufficient that the setter is non-virtual, because no user would access a derived class as HomieNode unless he knows what he's doing - however, if you think that this is not enough, the setter can be made virtual).

[jamesmyatt]

OK. I take back my comments about runLoopDisconnected. I was a bit hasty; I thought it was a property of Homie rather than of the Node. I don't think you need a setter since the property is protected and it should only be set during the Node constructor.

I still think it would be better with two loop methods for the Node, one which runs always and one which runs when connected.

[timpur]

Compermise ? Pass is connected into the loop ?

[euphi]

@nzbuu you're rigth with the setter. There is no need for it, if you can set the flag in constructor. I will add an optional parameter to HomieNode::HomieNode(..).

[euphi]

Oh, and I don't see the benefit of passing a connected parameter to loop(), because it is easy to "ask" homie for it's current state.

[timpur]

More for convenience

But if this loop is running at max speed then asking for the status every look could slow down the loop speed ??? Idk

[stritti]

Would be cool to have loops which are running without connection to build more robust controllers which are working without connections too.

Any progress on this issue?

[euphi]

As this is for the development-version (both v2 and v3), I think the minor risk of side effecs due to the different running order in the loop is acceptable.
This closes #571, #536, #487, #354

[kleini]

For this breaks all my existing HomieNodes. Previously I could assume that MQTT is ready when the HomieNode::loop() method is called. This is not the case any more and results in MQTT problems. My HomieNodes read sensor values in the loop method() and send them via MQTT. MQTT sending does not work any more as MQTT ready is not reached and SendPromise::send() refuses to send data if MQTT ready is not reached.
For me this merge has a big impact and breaks all my HomieNodes although runLoopDisconnected is still false in my cases.
So my question is, is this intended that HomieNode::loop() should be executed although MQTT ready is not reached and SendPromise::send() refuses to send messages? Do I need to check manually for MQTT ready to make send() not fail?