Skip to content

Fix rare, intermittent, endless loop in timeouts #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tarim8
Copy link

@Tarim8 Tarim8 commented Jan 20, 2019

Don't reduce millis() and timeout_start_ms to 16 bit as this causes
wrapping every 65 seconds which can lead to endless loops from
checkTimeoutExpired() if the sensor times out when the 16 bit millis() is
within io_timeout of wrapping.

Don't reduce millis() and timeout_start_ms to 16 bit as this causes
wrapping every 65 seconds which can lead to endless loops from
checkTimeoutExpired() if the sensor times out when the 16 bit millis() is
within io_timeout of wrapping.
@kevin-pololu
Copy link
Member

As long as we aren't checking for any timeouts longer than 65 seconds, it isn't necessary to use 32-bit arithmetic since, if we cast the result of the subtraction to uint16_t, it will underflow and wrap around to give the expected result. (For example, if the lower 16 bits of millis() is 200 and timeout_start_ms is 65400, 200 - 65400 produces 336 when the result is truncated to 16 bits.)

That being said, there is a bug with the code: it is casting one of the operands to 16 bits, not the result of the subtraction. This should still work fine on 8-bit processors, but it will probably give unexpected results on a 32-bit processor (where both operands will probably be promoted to 32 bits and the result will be 32 bits as well). In other words, it should be

(uint16_t)(millis() - timeout_start_ms)

instead of

((uint16_t)millis() - timeout_start_ms)

I think this affects a lot of our other libraries as well.

@swilson86
Copy link

A rare bug? I get this all the time. Easily reproduced if you put your hand in front of the sensor then point it at something like the ceiling, then your hand again. It will fail with the 65535 TIMEOUT stuck in that loop until you reboot. I hope you merge this fix soon. Thanks for the good work on the library so far!!

@kevin-pololu kevin-pololu mentioned this pull request Jun 18, 2019
@ghost
Copy link

ghost commented Oct 22, 2019

A rare bug? I get this all the time. Easily reproduced if you put your hand in front of the sensor then point it at something like the ceiling, then your hand again. It will fail with the 65535 TIMEOUT stuck in that loop until you reboot. I hope you merge this fix soon. Thanks for the good work on the library so far!!

I experience the same behavior consistently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants