luebbe / homie-node-collection

Collection of Node implementations for the Homie-ESP8266 library
MIT License
26 stars 12 forks source link

Properties are created even if nodes not instantiated #6

Closed ivanfmartinez closed 4 years ago

ivanfmartinez commented 5 years ago

I'm using Arduino IDE, and the behaviour is to compile all classes and include in the binary .

As the ESP have enough memory this is not a big problem.

But the properties are being defined and show for every device during the configuration :

https://github.com/luebbe/homie-node-collection/blob/master/src/AdcNode.cpp#L18

I propose to use a beforeHomeSetup() method in this classes to create this property definitions on demand using "new" to instantiate the objects.

Please look in this code and check if you think this method can be used in the other classes :

https://github.com/ivanfmartinez/homie-node-collection/commit/18b90f9b984bbd605f0a98f4e7afde7de0c0caaf#diff-f817ca7fed45d85f6e3067b00c9499a2L15

luebbe commented 4 years ago

Yes, that's nice. One suggestion concerning the "reverse" logic in your relaynode implementation. Why don't you make relayOnValue and relayOffValue uint8_t variables and set them in the constructor? You would get rid of the two functions.

ivanfmartinez commented 4 years ago

I have changed the logic to use the variables.

luebbe commented 4 years ago

Hi @ivanfmartinez , what is your intention behind the timeout implementation? How is it supposed to work? You didn't document anything and from looking at it and my current testing, it's buggy.

ivanfmartinez commented 4 years ago

The timeout is working in my devices.

The timeout define maximum time (in seconds) the relay will stay active.

The timeout attribute in the class will have the time when the relay should be deactivated.

The timeout property receives number of seconds to keep the relay active.

The loop method is responsible to check with uptime object to control this.

luebbe commented 4 years ago

Maybe it's working for you as you intended, but from the implementation it is not clear how it is supposed to work. ;)

Some points are not clear to me:

I'd like to to document the timeout feature and the settings in readme.md, but the description has to be correct.

ivanfmartinez commented 4 years ago

If you send an integer greather than zero for "timeout" property and the relay is on it will change the time for the relay be in on state for the number of seconds received (5 will keep the relay on for 5 seconds since the message is received) https://github.com/ivanfmartinez/homie-node-collection/blob/18b90f9b984bbd605f0a98f4e7afde7de0c0caaf/src/RelayNode.cpp#L65

If you send true for "on" property it will go on and use the relayOnLimit or globalOnLimit if defined with values greather than zero. If you send integer for "on" property it will go on and use the integer received as the time in seconds to keep relay on (since message was received). https://github.com/ivanfmartinez/homie-node-collection/blob/18b90f9b984bbd605f0a98f4e7afde7de0c0caaf/src/RelayNode.cpp#L51

The current implementation does no check for negative values, considering the current logic using negative numbers will make the timeout to expire and the relay will be turned off..

luebbe commented 4 years ago

Done using a slightly different approach in ac38d3b1f8e16385a7dd693f0ed4ed23ce05220c