Skip to content

ESP8266 - fix send buffer exhaustion handling #9309

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 10, 2019
Merged

ESP8266 - fix send buffer exhaustion handling #9309

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions components/wifi/esp8266-driver/ESP8266/ESP8266.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,7 @@ bool ESP8266::dns_lookup(const char *name, char *ip)

nsapi_error_t ESP8266::send(int id, const void *data, uint32_t amount)
{
nsapi_error_t ret = NSAPI_ERROR_DEVICE_ERROR;
// +CIPSEND supports up to 2048 bytes at a time
// Data stream can be truncated
if (amount > 2048 && _sock_i[id].proto == NSAPI_TCP) {
Expand All @@ -576,31 +577,50 @@ nsapi_error_t ESP8266::send(int id, const void *data, uint32_t amount)
_smutex.lock();
set_timeout(ESP8266_SEND_TIMEOUT);
_busy = false;
if (_parser.send("AT+CIPSEND=%d,%lu", id, amount)
&& _parser.recv(">")
&& _parser.write((char *)data, (int)amount) >= 0
&& _parser.recv("SEND OK")) {
// No flow control, data overrun is possible
if (_serial_rts == NC) {
while (_parser.process_oob()); // Drain USART receive register
}
_smutex.unlock();
return NSAPI_ERROR_OK;
_error = false;
if (!_parser.send("AT+CIPSEND=%d,%lu", id, amount)) {
tr_debug("ESP8266::send(): AT+CIPSEND failed");
goto END;
}
if (_error) {
_error = false;

if(!_parser.recv(">")) {
tr_debug("ESP8266::send(): didn't get \">\"");
ret = NSAPI_ERROR_WOULD_BLOCK;
goto END;
}

if (_parser.write((char *)data, (int)amount) >= 0 && _parser.recv("SEND OK")) {
ret = NSAPI_ERROR_OK;
}

END:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using the goto statement, the ifs above could be transformed into an if - else if - else if structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duly noted, but I'll keep the original for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like the goto END type of handling where we have multiple cases which all should lead to shared error handling.
Its like try { ... } catch { ... } finally { ... }

Copy link
Contributor

Choose a reason for hiding this comment

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

In a previous life, I worked with do { ... } while (false). I've always had mixed feelings about it.

_process_oob(ESP8266_RECV_TIMEOUT, true); // Drain USART receive register to avoid data overrun

// error hierarchy, from low to high
if (_busy) {
set_timeout();
_smutex.unlock();
tr_debug("returning WOULD_BLOCK");
return NSAPI_ERROR_WOULD_BLOCK;
ret = NSAPI_ERROR_WOULD_BLOCK;
tr_debug("ESP8266::send(): modem busy");
}

if (ret == NSAPI_ERROR_DEVICE_ERROR) {
ret = NSAPI_ERROR_WOULD_BLOCK;
tr_debug("ESP8266::send(): send failed");
}

if (_error) {
ret = NSAPI_ERROR_CONNECTION_LOST;
tr_debug("ESP8266::send(): connection disrupted");
Copy link
Contributor

Choose a reason for hiding this comment

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

is this actually true?

Should we test that socket is actually closed? We should know that, because OOBs are already handled.

Copy link
Contributor Author

@VeijoPesonen VeijoPesonen Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should we test that socket is actually closed?

That is done on the next if-clause.

If the connection cannot be established or gets
disrupted during data transmission, the system
returns:
ERROR

From the documentation.

So the question is can there occur an ERROR without socket being closed...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. true.. Thanks.

}

if (!_sock_i[id].open && ret != NSAPI_ERROR_OK) {
ret = NSAPI_ERROR_CONNECTION_LOST;
tr_debug("ESP8266::send(): socket closed abruptly");
}

set_timeout();
_smutex.unlock();

return NSAPI_ERROR_DEVICE_ERROR;
return ret;
}

void ESP8266::_oob_packet_hdlr()
Expand Down