ESP8266: Avoid duplicate data sends (5.15) #13164

ccli8 · 2020-06-22T02:09:52Z

Summary of changes

Backport of #12157 to mbed-os-5.15.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@michalpasztamobica

We are now checking if ESP8266 has confirmed receiving data over serial port with an undocumented (but existing) "Recv x bytes" message. Next we are explicitly waiting for an official "SEND OK".

1. Fix 'spurious close' by adding close() in open(). 'spurious close' gets frequent and cannot ignore when send() changes to asynchronous. User can retry open() until 'spurious close' gets true. 2. Allow only one actively sending socket because: (1) ESP8266 AT packets 'SEND OK'/'SEND FAIL' are not associated with socket ID. No way to tell them. (2) In original implementation, ESP8266::send() is synchronous, which implies only one actively sending socket. 3. Register 'SEND OK'/'SEND FAIL' oobs, like others in ESP8266::ESP8266 constructor. Don't get involved in oob management with send status because ESP8266 modem possibly doesn't reply these packets on error case. 4. Now that ESP8266::send() changes to asynchronous, drop the code with _parser.recv("SEND OK")/_parser.recv("SEND FAIL"). _parser.recv("SEND OK")/_parser.recv("SEND FAIL") and 'SEND OK'/'SEND FAIL' oobs both consume 'SEND OK'/'SEND FAIL' packets and complicate flow control.

This is because the ESP8266 is now waiting for SEND OK and takes much more to complete the send_repeat, and echo_burst tests in RAAS. (but has higher pass ratio).

ciarmcom · 2020-06-22T03:00:28Z

@ccli8, thank you for your changes.
@michalpasztamobica @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica

If the fix is required on the 5.15 branch, then I don't mind. Technically this looks sane.

ccli8 · 2020-06-22T05:35:20Z

If the fix is required on the 5.15 branch, then I don't mind. Technically this looks sane.

@michalpasztamobica Some Nuvoton's targets use ESP8266 as default network interface and still need to maintain 5.15 support. So backport this PR.

0xc0170 · 2020-06-26T13:07:44Z

CI started

mbed-ci · 2020-06-26T14:51:44Z

Test run: FAILED

Summary: 2 of 10 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_dynamic-memory-usage-lts
jenkins-ci/mbed-os-ci_greentea-test-lts

michalpasztamobica · 2020-06-29T06:00:13Z

The greentea failures look relevant, they happenned on K66F+ESP8266, not on other targets:
These GT tests failed:

[2020-06-26T14:39:24.795Z] 	test case: 'TCPSOCKET_SEND_REPEAT' ........................................................... FAIL in 10.86 sec
[2020-06-26T14:39:24.795Z] 	test case: 'TCPSOCKET_SEND_TIMEOUT' .......................................................... FAIL in 0.10 sec
[2020-06-26T14:39:24.795Z] 	test case: 'TCPSOCKET_SETSOCKOPT_KEEPALIVE_VALID' ............................................ OK in 0.12 sec
[2020-06-26T14:39:24.795Z] 	test case: 'TCPSOCKET_THREAD_PER_SOCKET_SAFETY' .............................................. FAIL in 2.81 sec

@ccli8 , are you able to try it on your desk and see if you can reproduce it?

The dynamic memory check look rather suspicious, as it claims to have failed with K66F and ethernet:

[2020-06-26T13:56:30.748Z] | GCC_ARM-K66F | K66F          | ethernet   | ethernet   | 0      | 1      | ERROR  | 78.27              |

Pull request has been modified.

ccli8 · 2020-06-30T08:08:58Z

are you able to try it on your desk and see if you can reproduce it?

@michalpasztamobica Reproducible on my NUMAKER_IOT_M487. With send(...) changing to non-block, modem can be busy in sending previous data and fail AT+CIPCLOSE in close(...). netsocket-tcp tests require close(...) be OK. I add more retries in close(...) to make it succeed to some degree to pass GT tests.

michalpasztamobica · 2020-06-30T11:14:57Z

@ccli8 , I wonder - is the _busy flag set in the scenario you described? If so - perhaps it would be safer to only loop if the flag is indeed set? With the latest change, not only is the maximum loop count increased but also a 1s delay added to every loop, which may block close() for up to 10 seconds.
Also - if possible, it would be worth checking if the socket is in non-blocking mode and only looping in that case :).
Let me know your thoughts on this.

With send(...) changing to non-block, modem can be busy in sending previous data and 'busy' the current 'AT+CIPCLOSE' command. In blocking mode, add retries to avoid spurious close to some degree. This is required to pass GT netsocket-tcp/netsocket-tls tests which expect close(...) to be OK.

ccli8 · 2020-07-01T08:08:49Z

@michalpasztamobica Recover ESP8266::close(...) and move loop logic to ESP8266Interface::socket_close(...) on blocking mode.

0xc0170 · 2020-07-22T15:53:56Z

CI started

mbed-ci · 2020-07-22T17:39:58Z

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 3
Build artifacts

adbridge · 2020-07-28T15:32:39Z

Approved by @andypowers

Update to mbed-os 6.0 is suspended due to incompatibility. Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

Update to mbed-os 6.0 is suspended due to incompatibility. Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

michalpasztamobica and others added 6 commits June 19, 2020 17:45

ESP8266: add a retry mechanism to avoid duplicate data sends

5f4ba9d

We are now checking if ESP8266 has confirmed receiving data over serial port with an undocumented (but existing) "Recv x bytes" message. Next we are explicitly waiting for an official "SEND OK".

ESP8266: Improve error handling and partial sends

9669a4f

ESP8266: Introduce single send_status instead of two flags

8c5cc71

ESP8266: accept partial writes for TCP and clean up code

5a163b1

Increase UDP test timeout to 1200s

2d42667

This is because the ESP8266 is now waiting for SEND OK and takes much more to complete the send_repeat, and echo_burst tests in RAAS. (but has higher pass ratio).

mergify bot added the Release review required label Jun 22, 2020

ciarmcom requested review from michalpasztamobica and a team June 22, 2020 03:00

ciarmcom added the needs: review label Jun 22, 2020

michalpasztamobica approved these changes Jun 22, 2020

View reviewed changes

0xc0170 previously approved these changes Jun 22, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jun 22, 2020

mergify bot added needs: work and removed needs: CI labels Jun 26, 2020

ccli8 force-pushed the esp8266_send_retry_5.15 branch from 07ad4a8 to 935134e Compare July 1, 2020 08:04

0xc0170 added needs: CI and removed needs: work labels Jul 22, 2020

0xc0170 approved these changes Jul 24, 2020

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Jul 24, 2020

adbridge merged commit a85f2bb into ARMmbed:mbed-os-5.15 Jul 28, 2020

adbridge added release-version: 5.15.5 and removed Release review required ready for merge labels Jul 28, 2020

ccli8 added a commit to OpenNuvoton/NuMaker-mbed-Aliyun-IoT-CSDK-example that referenced this pull request Aug 28, 2020

Update to mbed-os 5.15.5

ec90b47

Update to mbed-os 6.0 is suspended due to incompatibility. Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

ccli8 added a commit to OpenNuvoton/Mbed-to-Azure-IoT-Hub that referenced this pull request Sep 30, 2020

Nuvoton: Update to mbed-os 5.15.5

6d68153

Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

cyliangtw deleted the esp8266_send_retry_5.15 branch March 9, 2023 03:59

ccli8 added a commit to ccli8/NuMaker-mbed-ce-AWS-IoT-example that referenced this pull request Nov 26, 2024

Update to mbed-os 5.15.5

32b755d

Update to mbed-os 6.0 is suspended due to incompatibility. Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

ccli8 added a commit to ccli8/NuMaker-mbed-ce-wifi-tcp-example that referenced this pull request Jan 7, 2025

Update to mbed-os 5.15.5

7101217

Update to mbed-os 6.0 is suspended due to incompatibility. Update to mbed-os 5.15.5 which fixes ESP8266 driver issue: ARMmbed/mbed-os#13164

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESP8266: Avoid duplicate data sends (5.15) #13164

ESP8266: Avoid duplicate data sends (5.15) #13164

ccli8 commented Jun 22, 2020

ciarmcom commented Jun 22, 2020

michalpasztamobica left a comment

ccli8 commented Jun 22, 2020

0xc0170 commented Jun 26, 2020

mbed-ci commented Jun 26, 2020

michalpasztamobica commented Jun 29, 2020

ccli8 commented Jun 30, 2020

michalpasztamobica commented Jun 30, 2020

ccli8 commented Jul 1, 2020

0xc0170 commented Jul 22, 2020

mbed-ci commented Jul 22, 2020

adbridge commented Jul 28, 2020

ESP8266: Avoid duplicate data sends (5.15) #13164

ESP8266: Avoid duplicate data sends (5.15) #13164

Conversation

ccli8 commented Jun 22, 2020

Summary of changes

Pull request type

Test results

Reviewers

ciarmcom commented Jun 22, 2020

michalpasztamobica left a comment

Choose a reason for hiding this comment

ccli8 commented Jun 22, 2020

0xc0170 commented Jun 26, 2020

mbed-ci commented Jun 26, 2020

Test run: FAILED

michalpasztamobica commented Jun 29, 2020

ccli8 commented Jun 30, 2020

michalpasztamobica commented Jun 30, 2020

ccli8 commented Jul 1, 2020

0xc0170 commented Jul 22, 2020

mbed-ci commented Jul 22, 2020

Test run: SUCCESS

adbridge commented Jul 28, 2020