gysmo38 / mitsubishi2MQTT

Mitsubishi to MQTT with ESP8266 module
GNU Lesser General Public License v2.1
371 stars 132 forks source link

some pieces of code in mitsubishi2mqtt.ino that implement delay by comparing to millis() can be improved to work better on a wrap #257

Open xalier1 opened 4 months ago

xalier1 commented 4 months ago

In mitsubishi2mqtt.ino the variable wifi_timeout is compared to millis() to implement a delay in a few places in a way that doesn't work properly when millis() nears the unsigned long wrap point.

There are lots of examples of properly implemented millis() delay code in mitsubishi2mqtt.ino. Delays using variables lastTempSend, lastRemoteTemp , lastMqttRetry, etc in mitsubishi2mqtt.ino are all implemented in a way that works at the unsigned long wrap point. The delays associated with the variable wifi_timeout should copy the way a delay is done using delay code associated with the variable lastTempSend as an example.

When looking for correct examples of code, I avoided the code associated with the variable lastHpSync copied to here as follows:

if (((millis() - lastHpSync > durationNextSync) or lastHpSync == 0)) { lastHpSync = millis();

It may seem to convenient to use lastHpSync == 0 as a "do immediately without delay" flag but remember that in the statement "lastHpSync = millis();", millis() can return a value of 0 which would cause an unintended "do immediately without delay" in a subsequent pass. The probability of a 0 being returned by millis() is low but if possible, it's best to implement code that works as intended all the time rather than most of the time. In an environment that is not memory constrained and is not performance constrained, it is better to replace "lastHpSync == 0" with an explicit "do immediately" flag that is initialized to true at startup and is set to false once the first pass has occurred. An example of the replacement code could be:

if (((millis() - lastHpSync > durationNextSync) or firstHpSync)) { lastHpSync = millis(); firstHpSync = false;

where firstHpSync is declared as a global bool initialized to "true"