millis overflow bug

[fxkr]

Thank you for your library.

This bug is untested, this came up when I reviewed the code.

...
return lastRequest < millis() - MHZ14A_RESPONSE_TIME;
...
lastRequest = millis(); // runs only if above returns true

millis() overflows after ~50 days of runtime (2**32 / 1000/60/60/24 days) and wraps around to 0.

If the readCO2UART is called regularly enough, then in the transition period where millis() has just wrapped around, the term millis() - MHZ14A_RESPONSE_TIME will wrap around backwards and be larger than lastRequest, the condition will succeed, and lastRequest will be set to a wrapped around millis() value and all is fine.

However, if after the wraparound, readCO2UART is not called again until after such time as millis() > MHZ14A_RESPONSE_TIME, then millis() - MHZ14A_RESPONSE_TIME will be less than lastRequest for the next ~50 days, and readCO2UART will not query the sensor again in that time. This is a bug. For each subsequent wraparound we'll be in the same situation as before, where the library can either work or break for the next 50 days, depending only on the timing of calls to readCO2UART.

The easiest fix would be to restructure the condition like so:

return millis() - lastRequest >= MHZ14A_RESPONSE_TIME;

now - last >= interval is pretty much the standard pattern for wrap-around-safety. Obviously it assumes that millis() and lastRequest are both unsigned, and that lastRequest is only ever set to the current (or recent past) time, never to a future time. That is both true here.

[gerritzen]

I would like to suggest removing isReady() completely or at least make it an option one can disable. Is it possible that the line in the data sheet was misinterpreted?

T_{90} < 120s does not mean one can only read the sensor every 120 seconds. If you suddenly expose the sensor to a new atmosphere, it takes less than 120 seconds to cover 90% of the difference. So if you go from 400ppm to 800ppm, after 120 seconds the sensor should report at least 760ppm. That is not a reason to prevent the library from reading it out earlier anyway.

Since the response time is a physical property of the sensor, it should be handled regardless of readout channel. Currently, it is checked for UART and the check is commented out for PWM.

[MattMills]

Agreed, I was just looking at this code with confusion trying to see why UART only reported readings occasionally, I removed all the weird response time stuff from isReady() and it works fine now.

[wer-is-paul]

Check my Pull Request where i tried to tackle this

[SNMetamorph]

Fixed in 1.4.1 version, may be close it?

[tobiasschuerg]

Thanks for the reminder :-)