Fix for 32 bit processors.

[Timmmm]

The time-outs do not ever time-out on 32 bit processors if t0 happens to be close to 65535. This means that if you try to open an SD card that isn't present your app will sometimes freeze entirely in card.init() rather than returning an error like it is supposed to.

[ArduinoBot]

Can I build this pull request?

[Timmmm]

Yes? Explain yourself, robot.

On 22 August 2014 12:51, ArduinoBot [email protected] wrote:

Can I build this pull request?

—
Reply to this email directly or view it on GitHub
#1977 (comment).

[ffissore]

@Timmmm sorry for the comment from arduinobot. we are setting up automated build of all PRs, so that we can provide a downloadable version of the patched IDE

[agdl]

@Timmmm can u provide me an example sketch that shows the freezing please?

[matthijskooijman]

Just looked at the commit, it looks good to me. AFAIU, what happens is that the compiler applies integer promotion, which on a 32-bit system promotes to uint32_t, breaking the overflow.

In the original code:

(((uint16_t)millis() - t0) > SD_INIT_TIMEOUT)

Both sides of the subtraction are uint16_t, but are promoted to uint32_t before subtracting. Overflow in the subtraction now also happens in 32-bit, breaking the calculation.

As an example, consider that t0 is 65535. If (the lower 16 bits of) millis is 1, the expected result is 2 (2 ms passed since t0). However, when this calculation is done in 32-bits, the result is 1 - 65535 = 2^32 + 1 - 65535 = 4294901762 = 0xffff0002 which is clearly incorrect. However, the lower 16 bits of the result do contain the right answer, so casting to uint16_t after subtracting yields correct results.

Note that my analysis would only explain that the timeout would happen too soon around the overflow time, I can't explain the code locking up (but that might be a side effect of some other code?)

I actually think the cast to uint16_t for the result of millis could be dropped as well (unless the compiler doesn't know to optimize away the upper bits of the subtraction without it - would have to be tested).

[ArduinoBot]

Can one of the admins verify this patch?

[facchinm]

Very interesting bug!
AFAICS, the unexpected behaviour is in AVR side, not ARM.
If t0 approaches 2^16, the while loop will last forever because we are checking if a number that can be at most 2^16 will ever become bigger than the max value it can assume + a big constant (SD_INIT_TIMEOUT ).
So the strange thing is that this code works with 8bit AVR; I checked the generated ASM and no promotion is applied so when (uint16_t)millis() wraps the result of the check is TRUE (I believe this is a side effect and not the expected behaviour).
However, on ARM with the patch applied only the intermediate value checked against SD_INIT_TIMEOUT is promoted, while the mainline version promotes both the operands before subtracting (leading to the wrong result).
We should take a look in all shared libraries to check if this happens somewhere else.
Merging soon

[facchinm]

Merged as #2808