Tick Timer Wrap

[carlk3]

This is sort of a code review comment, not something I have actually encountered, let alone recreated, but I think there is a latent defect in SdioCard::stopTransmission. If the millis() counter wraps (every 49 days), this code might have a problem:

        uint32_t end = millis() + 5000;
        while (millis() < end && isBusy())

For example, say the first call to millis() returns 0xFFFFFFF0 and the second (or subsequent) call to millis() returns 0xFFFFFFFF:

        uint32_t end = millis() + 5000; // 0xFFFFFFF0 + 5000 = 0x00001378
        while (millis() < end && isBusy()) // while (0xFFFFFFFF < 0x00001378...

I would rewrite it like:

        uint32_t start = millis();
        while (millis() - start < 5000 && isBusy());

Now if millis() returns 0xFFFFFFF0 and 0xFFFFFFFF, as above:

        uint32_t start = millis();  // 0xFFFFFFF0
        while (millis() - start < 5000 && isBusy()); // while (0xFFFFFFFF - 0xFFFFFFF0 < 5000...

As another example, if millis() returns 0xFFFFFFFF and 0x0000000F:

        uint32_t start = millis();  // 0xFFFFFFFF
        while (millis() - start < 5000 && isBusy()); // while (0x0000000F - 0xFFFFFFFF < 5000...
// or, while (0x00000010 < 5000... (using modulo arithmetic, since we're dealing with uint32_t)

Note: I chose 0xFFFFFFF0 and 0xFFFFFFFF for purposes of illustration, but it could just as well be 0xFFFFFFFE and 0xFFFFFFFF, for example (which might be more likely in real life).

[carlk3]

Test case:

static void test_timeout() {
    static uint32_t millis = 0xffffffff - 5000 + 8;        // tick counter
    float uptime = (float)millis / (1000 * 60 * 60 * 24);  //  (Hz * sec * min * hours)
    printf("uptime: %f days\n", uptime);
    uint32_t start = millis;
    printf("start: 0x%08lx\n", start);
    uint32_t end = millis + 5000;
    printf("end: 0x%08lx\n", end);

    for (unsigned i = 0; i < 32; ++i) {
        printf("millis: 0x%08lx, millis - start: %lu, millis < end? %s, millis - start < 5000? %s\n",
               millis, millis - start, millis < end ? "true" : "false", millis - start < 5000 ? "true" : "false");
        ++millis;  // timer tick
        if (0xffffec8d == millis) {
            printf("...\n");
            millis = 0xffffffff - 7;
        }
    }
}

Output:

uptime: 49.710209 days
start: 0xffffec7f
end: 0x00000007
millis: 0xffffec7f, millis - start: 0, millis < end? false, millis - start < 5000? true
millis: 0xffffec80, millis - start: 1, millis < end? false, millis - start < 5000? true
millis: 0xffffec81, millis - start: 2, millis < end? false, millis - start < 5000? true
millis: 0xffffec82, millis - start: 3, millis < end? false, millis - start < 5000? true
millis: 0xffffec83, millis - start: 4, millis < end? false, millis - start < 5000? true
millis: 0xffffec84, millis - start: 5, millis < end? false, millis - start < 5000? true
millis: 0xffffec85, millis - start: 6, millis < end? false, millis - start < 5000? true
millis: 0xffffec86, millis - start: 7, millis < end? false, millis - start < 5000? true
millis: 0xffffec87, millis - start: 8, millis < end? false, millis - start < 5000? true
millis: 0xffffec88, millis - start: 9, millis < end? false, millis - start < 5000? true
millis: 0xffffec89, millis - start: 10, millis < end? false, millis - start < 5000? true
millis: 0xffffec8a, millis - start: 11, millis < end? false, millis - start < 5000? true
millis: 0xffffec8b, millis - start: 12, millis < end? false, millis - start < 5000? true
millis: 0xffffec8c, millis - start: 13, millis < end? false, millis - start < 5000? true
...
millis: 0xfffffff8, millis - start: 4985, millis < end? false, millis - start < 5000? true
millis: 0xfffffff9, millis - start: 4986, millis < end? false, millis - start < 5000? true
millis: 0xfffffffa, millis - start: 4987, millis < end? false, millis - start < 5000? true
millis: 0xfffffffb, millis - start: 4988, millis < end? false, millis - start < 5000? true
millis: 0xfffffffc, millis - start: 4989, millis < end? false, millis - start < 5000? true
millis: 0xfffffffd, millis - start: 4990, millis < end? false, millis - start < 5000? true
millis: 0xfffffffe, millis - start: 4991, millis < end? false, millis - start < 5000? true
millis: 0xffffffff, millis - start: 4992, millis < end? false, millis - start < 5000? true
millis: 0x00000000, millis - start: 4993, millis < end? true, millis - start < 5000? true
millis: 0x00000001, millis - start: 4994, millis < end? true, millis - start < 5000? true
millis: 0x00000002, millis - start: 4995, millis < end? true, millis - start < 5000? true
millis: 0x00000003, millis - start: 4996, millis < end? true, millis - start < 5000? true
millis: 0x00000004, millis - start: 4997, millis < end? true, millis - start < 5000? true
millis: 0x00000005, millis - start: 4998, millis < end? true, millis - start < 5000? true
millis: 0x00000006, millis - start: 4999, millis < end? true, millis - start < 5000? true
millis: 0x00000007, millis - start: 5000, millis < end? false, millis - start < 5000? false
millis: 0x00000008, millis - start: 5001, millis < end? false, millis - start < 5000? false
millis: 0x00000009, millis - start: 5002, millis < end? false, millis - start < 5000? false

[PetteriAimonen]

I agree, there is a latent bug here.

After 49 days of uptime, if SDIO access occurs during the wrapover, it will return an error which will be reported to SCSI host.

In a quick review, other timeouts in the codebase are using the correct form (uint32_t)(millis() - start) < 100.
Should be an easy code change.

[morio]

Fix has been merged into main