Skip to content
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

Replace peek() with available() during connection status check to avoid loosing messages #50

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

giulcioffi
Copy link
Contributor

@giulcioffi giulcioffi commented Jul 9, 2020

When trying to perform an OTA update via the cloud servers using the nina as OTA storage, the connection randomly breaks down during download. A lower number of bytes are indeed received since some of them go lost during the disconnection-reconnection phases.
During an OTA update the binary is sent by chunks of 256 bytes. These chunks are received by the Nina and then forwarded via SPI and handled by the libraries. When the ArduinoIoTCloud library receives a chunk of data, it tries to decrypt it (this is done by recvrec_ack() function in ssl_engine.c ). If the decryption fails, an error is raised and the disconnection state is reached.
The reason why the decryption is sometimes failing is not related to data corruption, but happens because an entire chunk of data go lost. This same error systematically happens at each disconnection.
Each chunk is identified in its header by an increasing number. Most likely the decryption is failing because it is expecting a certain number but reading the next one. By dumping these headers from the nina-fw, it is possible to notice that the chuck of data does not go lost in the SPI connection, but is already skipped by the Nina itself. This happens because the message reception and the buffer handling on the Nina side is managed from both WiFiClient::read() and WiFiClient::peek() functions. WiFiClient::read() is the one used to actually read the messages. WiFiClient::peek() is, instead, improperly called from getClientStateTcp(). Then, if inside WiFiClient::connected() in the nina-fw we replace peek() with available() it is possible to perform a "safe" check without dropping chucks of messages.

@giulcioffi giulcioffi requested a review from aentinger July 9, 2020 08:42
Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this PR on a MKR WiFi 1010 and it resolved the situation with loosing connection during the download of a large file. Great work @giulcioffi and 👍 for your perseverance tracking this one down 🚀

@aentinger aentinger merged commit 759397b into arduino:master Jul 9, 2020
@Jauwaad
Copy link

Jauwaad commented Apr 24, 2021

This change causes the following issue #65

aentinger added a commit that referenced this pull request Apr 27, 2021
giulcioffi added a commit to giulcioffi/nina-fw that referenced this pull request Jul 29, 2021
This reverts commit 6f82133.
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