`NetworkClient::downloadFile` sets an oddly short http timeout

[smoores-dev]

https://github.com/SolderedElectronics/Inkplate-Arduino-library/blob/master/src/include/NetworkClient.cpp#L540

Here a timeout is set for 1 second for all file downloads. This seems unexpectedly short; in my case, I have a low-resource server generating a PNG on-demand for the Inkplate, and apparently sometimes it takes slightly more than one second to respond!

I'm happy to open a PR for the change, but I'd like to gather context first:

• Is there a reason that I'm not thinking of that it's important to limit the timeout for file downloads specifically?
• Should we simply remove this line (it was added here without any explanation I see) and rely on the default timeout, which can be set in display.connectWiFi? Or should we plumb through a download-specific timeout, at least to the level of drawPngFromWeb et al?

[rsoric]

Hi @SMores

We understand what you mean, while we were writing these functions, downloading files to ESP32 turned out to be a bit tricky to figure out how to do reliably and over HTTPS.

Regarding the timeout, I don't see why it wouldn't be a good idea to make it into a modifiable variable (with a default value, so users who don't care don't have to set it).

You might be interested - we have ran into a problem recently with downloading a relatively large file to Inkplate - the file would just cut off at some point and the whole file wouldn't get saved, just a chunk of it. We have went around this by implementing a timeout in this way:

`if (httpCode == 200) { long n = 0;

    long now = millis();

    while (millis() - now < DOWNLOAD_TIMEOUT)
    {
        while (http.getStream().available())
        {
            data[n++] = http.getStream().read();
            now = millis();
        }
    }

    *dataLen = n;
    data[n++] = 0;
}`

Basically, check if stream is available for the length of the timeout after the last packet is sent. We are looking into adding this more globally into the Inkplate library which would solve these kinds of issues.

For now, I would suggest to just remove the line. I'll write to you here when we get around to implementing this fix.

-Rob

[smoores-dev]

Sounds great! Thanks so much @rsoric!

For now, I would suggest to just remove the line

Would you like me to open a PR to do this, or are you just suggesting I remove the line locally for now, and we can wait for this broader fix here?

[smoores-dev]

Just leaving some more notes here:

The calls to http.getStream().setNoDelay(true) and http.getStream().setTimeout(1) are potentially... not doing anything? In order to properly increase the timeout, I had to call http.setTimeout(), which takes an argument in milliseconds (I'm doing http.setTimeout(30 * 1000)). Otherwise, the initial http.GET() would use the default timeout of 5000ms, which would occasionally timeout.

[rsoric]

@SMores remove the line locally for now. We periodically make improvements on the Inkplate library's dev branch and then make a merge to main when enough fixes have been made and fully tested. It's possible that the setNoDelay and setTimeout calls were something left over that was an attempt to fix this issue.

[rsoric]

Closing this as there's no more activity, please reopen the issue if you think it needs more attention