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

Photon/P1: TCPClient: non-blocking, blocking, blocking with timeout writes support #1485

Merged
merged 8 commits into from Feb 5, 2018

Conversation

@avtolstoy
Copy link
Member

commented Jan 31, 2018

Problem

Unlike TCPClient::read(), TCPClient::write() is a blocking call and whenever TCP send queue gets completely filled (see #1461 for a particular case on Photon/P1 with low wifi signal), the application thread may be blocked for an indefinite amount of time.

There needs to be a way to specify the maximum amount of time the write should take.

Solution

New TCPClient::write() overloads are added:

  • size_t write(uint8_t b, system_tick_t timeout);
  • size_t write(const uint8_t *buf, size_t size, system_tick_t timeout);

Additional notes

write() methods return size_t, which is an unsigned integer, however in case of an error, TCPClient::write() method may return a negative error code cast to size_t. We are still keeping this behavior, with a thought of deprecating it in the next release.

Applications should use TCPClient::getWriteError()/clearWriteError() methods to check for errors, while TCPClient::write() should only return the number of bytes written.

Steps to Test

  • wiring/api

Example App

#include "application.h"

SYSTEM_MODE(SEMI_AUTOMATIC);

Serial1LogHandler dbg(115200, LOG_LEVEL_ALL);

TCPServer tcpServerNonBlocking(5000);
TCPServer tcpServerBlockingTimeout(5001);
TCPServer tcpServerBlocking(5555);

void setup() {
    WiFi.on();
    WiFi.connect();
    WiFi.selectAntenna(ANT_EXTERNAL);
}

static void sendBunchOfData(TCPClient&& client, system_tick_t timeout) {
    while (client.connected()) {
        uint8_t buf[500];
        memset(buf, 'a', 500);
        int rc = client.write(buf, sizeof(buf), timeout);
        LOG(TRACE, "%d", rc);
    }
}

void loop() {
    sendBunchOfData(tcpServerNonBlocking.available(), 0);
    sendBunchOfData(tcpServerBlockingTimeout.available(), 1000);
    sendBunchOfData(tcpServerBlocking.available(), SOCKET_WAIT_FOREVER);
}

Connect to the appropriate port using telnet or something similar, while looking at the log place a finger onto the external antenna connector. Device should be unable to send data, causing TCP send queue to fill up. The behavior on each of the ports should be the following:

  • 5000: as soon as the tcp queue is full, the log should be filled with a constant flow of -16 error codes
  • 5001: as soon as the tcp queue is full, the log should show -16 error codes every second
  • 5555: as soon as the tcp queue is full, there should be no log entries, indicating that the application thread is blocked

References


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation NEEDS TO BE UPDATED
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@avtolstoy avtolstoy requested a review from m-mcgowan Jan 31, 2018

@avtolstoy avtolstoy added this to the 0.8.0-rc.2 milestone Feb 5, 2018

@@ -66,6 +67,9 @@ class Print
}
virtual size_t write(const uint8_t *buffer, size_t size);

virtual size_t write(uint8_t b, system_tick_t timeout) { return write(b); }

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Feb 5, 2018

Contributor

I would prefer to be careful here and add the new write methods to TCPClient only. The reason is that there is only one stream implementation that supports it, and I imagine users will be confused with the presence of the new methods that don't do anything useful for most streams.


size_t TCPClient::write(const uint8_t *buffer, size_t size, system_tick_t timeout)
{
int ret = status() ? socket_send_ex(d_->sock, buffer, size, 0, timeout, nullptr) : -1;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Feb 5, 2018

Contributor

should we call clearWriteError() here?

@m-mcgowan m-mcgowan merged commit cc880e5 into develop Feb 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@m-mcgowan m-mcgowan deleted the feature/socket-send-nonblocking-timeout branch Feb 5, 2018

avtolstoy added a commit to particle-iot/docs that referenced this pull request Feb 23, 2018
Photon/P1: TCPClient: non-blocking, blocking, blocking with timeout w…
…rites support (particle-iot/device-os#1485)

New TCPClient::write() and TCPServer::write() overloads documented:
- size_t write(uint8_t b, system_tick_t timeout);
- size_t write(const uint8_t *buf, size_t size, system_tick_t timeout);

TCPClient/TCPServer::getWriteError() and TCPClient/TCPServer::clearWriteError() documented.
@elcojacobs

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2018

Some overloads seem to be missing.

write("string") works, but write("string", 1000) results in a compile error, because the compiler doesn't want to convert const char[] to const uint8_t *.

I can hack my way around this, but for most users, I think this will be confusing. Should all supported write prototypes be added with timeout?

@elcojacobs

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

How would you feel about being able to set a timeout value on the TcpClient class instance, instead of passing it with every function call?
That avoids the function overloading and I think will be even easier to use.

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2018

Sounds like a good idea analogous to how it's done with regular BSD-socket interface: setsockopt() with SO_RCVTIMEO and SO_SNDTIMEO. Having both options available also doesn't sound too bad :)

I also agree that size_t write(const char *str, system_tick_t timeout) needs to be added. It was an oversight on my part.

@elcojacobs

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2018

Are you going to implement this? I can have a go at it too if you want.

technobly added a commit to particle-iot/docs that referenced this pull request Apr 9, 2018
Merge pull request #745 from particle-iot/feature/tcpclient-non-block…
…ing-timeout

Photon/P1: TCPClient: non-blocking, blocking, blocking with timeout writes support (particle-iot/device-os#1485)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.