Skip to content

Improvement to timeout in Updater.cpp #6113

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

Closed
adrian-dybwad opened this issue May 19, 2019 · 2 comments · Fixed by #6117
Closed

Improvement to timeout in Updater.cpp #6113

adrian-dybwad opened this issue May 19, 2019 · 2 comments · Fixed by #6117
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.

Comments

@adrian-dybwad
Copy link
Contributor

Recently, when doing a deep dive into behavior in slow network environments, I can across the code in Updater.cpp that handles a timeout while an update is being streamed. Effectively, it will wait for 100ms after a timeout and then try once more. If this second attempt fails, the update fails. I would like to propose a change to this mechanism that makes the download succeed in even the slowest networks.

This change uses a call to millis() to track the start time, reseting this start time every time data is received. After 60 seconds of not receiving more data, it will timeout.

The new function is below with the new variable called "timeout":

size_t UpdaterClass::writeStream(Stream &data) {
    size_t written = 0;
    size_t toRead = 0;
    if(hasError() || !isRunning())
        return 0;

    if(!_verifyHeader(data.peek())) {
#ifdef DEBUG_UPDATER
        printError(DEBUG_UPDATER);
#endif
        _reset();
        return 0;
    }
    unsigned long timeout = millis();
    if (_progress_callback) {
        _progress_callback(0, _size);
    }
    if(_ledPin != -1) {
        pinMode(_ledPin, OUTPUT);
    }

    while(remaining()) {
        if(_ledPin != -1) {
            digitalWrite(_ledPin, _ledOn); // Switch LED on
        }
        size_t bytesToRead = _bufferSize - _bufferLen;
        if(bytesToRead > remaining()) {
            bytesToRead = remaining();
        }
        toRead = data.readBytes(_buffer + _bufferLen,  bytesToRead);
        if(toRead == 0) { //Timeout
          if (millis() - timeout > 60000) {
            _currentAddress = (_startAddress + _size);
            _setError(UPDATE_ERROR_STREAM);
            _reset();
            return written;
          }
          delay(100);
        } else {
          timeout = millis();
        }
        if(_ledPin != -1) {
            digitalWrite(_ledPin, !_ledOn); // Switch LED off
        }
        _bufferLen += toRead;
        if((_bufferLen == remaining() || _bufferLen == _bufferSize) && !_writeBuffer())
            return written;
        written += toRead;
        if(_progress_callback) {
            _progress_callback(progress(), _size);
        }
        yield();
    }
    if(_progress_callback) {
        _progress_callback(progress(), _size);
    }
    return written;
}

Also, I read another thread recently with comments thanking you all for the incredible work you do on this project. One response was it is not common to get thanks, so here is another. THANK YOU!!

@devyte
Copy link
Collaborator

devyte commented May 19, 2019

@adrionics thanks are appreciated 😃
Please

  1. rewrite your timeout change in terms of polledTimeout. That is a class that encapsulates precisely what you are trying to do. There are examples for it included in the core.
  2. make the timeout value a constexpr, or at least a define to allow easy configuring. I think 60s is way too long, but we can discuss that.
  3. make a PR with your proposal so that we can review and discuss in detail.

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label May 19, 2019
@adrian-dybwad
Copy link
Contributor Author

adrian-dybwad commented May 19, 2019

I created a pull request and added the timeout value as an optional parameter.

The reason I chose 60 seconds was from looking at some downloads on a very slow network. It is likely a bit high, but I would not go lower than about 30 seconds. The original timeout mechanism would create about a 16 second timeout.

I did not use the polledTimeout task since I thought this was quite a simple solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants