ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive… #12320

mtomczykmobica · 2020-01-28T11:15:28Z

…r to support UDP connection in server mode (get).

Summary of changes

Now we can use boards with ESP8266 in UDP server mode by binding port for listening.
No connecting needed in this mode.
example of useage:

#include "mbed.h"
#include "UDPSocket.h"

int main() {
static const int BUFF_SIZE = 1500;
char buffer[BUFF_SIZE] = {0};
int ret = 0;
SocketAddress udp_addr;
UDPSocket sock;

NetworkInterface *net = NetworkInterface::get_default_instance();

// Bring up the interface
if(0 != net->connect()) {
    printf("Error connecting\r\n\r\n");
    return -1;
}

sock.open(net);
sock.bind(7);
for (int i = 0; i < 100; i++)
{
    int recvd = sock.recvfrom(&udp_addr, buffer, BUFF_SIZE);
    if(recvd > 0) {
        sock.sendto(udp_addr, buffer, recvd);
    }
}
sock.close();
net->disconnect();
return 0;

}

Impact of changes

Migration actions required

Documentation

Not needed.

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

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

Reviewers

SeppoTakalo · 2020-01-28T11:36:29Z

Looks OK.
Please check the Astyle failures and fix those.

Pull request has been modified.

michalpasztamobica

Functionally this looks good to me.
I just have a few small improvement suggestions.

michalpasztamobica · 2020-01-28T13:42:10Z

components/wifi/esp8266-driver/ESP8266/ESP8266.h

+    int id;
+    nsapi_protocol_t proto;
+    bool connected;
+    bool binded;


Should it rather be "bound"? See here for example.

michalpasztamobica · 2020-01-28T13:43:48Z

components/wifi/esp8266-driver/ESP8266/ESP8266.h

@@ -167,6 +177,14 @@ class ESP8266 {
    */
    bool disconnect(void);

+    /**
+    * Enable or disconnect Remote IP and Port with +IPD


I guess this only enabled or disatble remote IP and port printing ? We can still connect to remote IPs if we set this to false, but they address and port will not be printed?

I also think that the name of this function is misleading. Could we make it ip_info_print or something similar?

michalpasztamobica · 2020-01-28T13:44:06Z

components/wifi/esp8266-driver/ESP8266/ESP8266.h

@@ -236,9 +254,10 @@ class ESP8266 {
    * @param addr the IP address of the destination
    * @param port the port on the destination
    * @param local_port UDP socket's local port, zero means any
+    * @param udp_mode UDP socket's mode, zero means can't change remote, 1 can change ones, 2 can change


"ones" -> "once"
"2 can change" -> "2 can change multiple times"

michalpasztamobica · 2020-01-28T13:51:07Z

components/wifi/esp8266-driver/ESP8266/ESP8266.cpp

+            if (!_parser.scanf("%d:", &amount)) {
+                return;
+            }
+        }


Seems that _parser.scanf("%d,", &amount) is done whether cip_info is 1 or 0, so let's just do it before the check?
What comes next could be merged so it takes less space and matches the style of how we normally do it in this file. So something like this:

} else { if (!_parser.scanf("%d,", &amount)) { return; } if (cip_info == 1) { if (!(_parser.scanf("%15[^,],", _ip_buffer) && _parser.scanf("%d:", &port)) { return; } }

It takes less space, is equally readable and matches the way we normally do this in this file.

Unfortunately you're wrong.
Delimiter for "amount" is different in version with showing ip, port against to version without showing ip, port. In first case it is "," in second case is ":"

True, I missed that.
so let's just merge the scanfs together:

if (!(_parser.scanf("%d,", &amount) && _parser.scanf("%15[^,],", _ip_buffer) && _parser.scanf("%d:", &port)) { return; }

the current structure if (!sth && !sthelse && !sthelse2) will return if ALL conditions fail, so it will have to check all of them, which makes no sense if the first one failed.
I think it is better to do if (!(sth && sthelse && sthelse2)), because then if one condition fails (for example the %d, match fails), the other conditions will not be checked (short-circuit evaluation).

Done. Yes you're right. I've broken logic operation.

mbed-ci · 2020-01-29T14:46:31Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

kjbracey · 2020-01-30T11:33:38Z

components/wifi/esp8266-driver/ESP8266/ESP8266.cpp

@@ -572,6 +588,8 @@ nsapi_error_t ESP8266::open_tcp(int id, const char *addr, int port, int keepaliv
    static const char *type = "TCP";
    bool done = false;

+    ip_info_print(0);


Isn't this changing a global modem setting?

Doesn't this mean opening a TCP socket will turn it off, breaking UDP?

Isn't it easier to just leave the setting permanently on?

I think a bit about that and you are right. We cannot have difrent ESP main configuration for difference socket. Fix added.

mtomczykmobica · 2020-01-30T13:03:46Z

CI result: https://jenkins-internal.mbed.com/view/IP%20Core/job/greentea-socket-tests-unstable/880/

…r to support UDP connection in server mode (get)

0xc0170 · 2020-02-05T15:16:52Z

CI started

mbed-ci · 2020-02-05T17:15:27Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

mtomczykmobica · 2020-02-06T07:08:20Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

Failures looks not related to my changes.

Failed:
[1580918479.41][CONN][RXD] >>> Running case #7: 'Test get with timeout on empty mailbox'...
[1580918479.41][CONN][INF] found KV pair in stream: {{__testcase_start;Test get with timeout on empty mailbox}}, queued...
[1580918480.11][CONN][RXD] :285::FAIL: Values Not Within Delta 5000 Expected 50000 Was 606795
[1580918480.21][CONN][INF] found KV pair in stream: {{__testcase_finish;Test get with timeout on empty mailbox;0;1}}, queued...
[1580918480.31][CONN][RXD] >>> 'Test get with timeout on empty mailbox': 0 passed, 1 failed with reason 'Assertion Failed'
[1580918480.31][CONN][RXD]
[1580918480.31][CONN][RXD] >>> Test cases: 6 passed, 1 failed with reason 'Assertion Failed'
[1580918480.41][CONN][RXD] >>> TESTS FAILED!

@0xc0170 Please rerun CI job.

mbed-ci · 2020-02-06T07:55:25Z

Test run: SUCCESS

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

mtomczykmobica requested review from michalpasztamobica, ccli8, VeijoPesonen, SeppoTakalo and AnttiKauppila January 28, 2020 11:16

mergify bot added the needs: work label Jan 28, 2020

SeppoTakalo previously approved these changes Jan 28, 2020

View reviewed changes

mtomczykmobica force-pushed the ONME-3433 branch from 9cb9866 to 9692cfe Compare January 28, 2020 11:37

michalpasztamobica suggested changes Jan 28, 2020

View reviewed changes

mtomczykmobica force-pushed the ONME-3433 branch 4 times, most recently from af07b42 to 243f412 Compare January 29, 2020 06:53

michalpasztamobica approved these changes Jan 29, 2020

View reviewed changes

SeppoTakalo added needs: CI and removed needs: work labels Jan 29, 2020

kjbracey reviewed Jan 30, 2020

View reviewed changes

mtomczykmobica force-pushed the ONME-3433 branch 3 times, most recently from 2513863 to 19b41d9 Compare January 31, 2020 07:46

mergify bot added needs: work and removed needs: CI labels Jan 31, 2020

mtomczykmobica force-pushed the ONME-3433 branch 3 times, most recently from c1f5e90 to 715a28c Compare January 31, 2020 08:00

ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive…

46718d0

…r to support UDP connection in server mode (get)

michalpasztamobica approved these changes Jan 31, 2020

View reviewed changes

mtomczykmobica force-pushed the ONME-3433 branch from 715a28c to 46718d0 Compare January 31, 2020 08:04

kjbracey approved these changes Feb 5, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Feb 5, 2020

0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 5, 2020

mergify bot added needs: work needs: CI and removed needs: CI needs: work labels Feb 5, 2020

mergify bot added ready for merge and removed needs: CI labels Feb 6, 2020

0xc0170 merged commit acece11 into ARMmbed:master Feb 6, 2020

mergify bot removed the ready for merge label Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive… #12320

ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive… #12320

mtomczykmobica commented Jan 28, 2020

SeppoTakalo commented Jan 28, 2020

michalpasztamobica left a comment

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 28, 2020

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 28, 2020

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 28, 2020

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 28, 2020

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 28, 2020

michalpasztamobica Jan 28, 2020

mtomczykmobica Jan 29, 2020

mbed-ci commented Jan 29, 2020

kjbracey Jan 30, 2020

mtomczykmobica Jan 30, 2020

mtomczykmobica commented Jan 30, 2020 •

edited

Loading

0xc0170 commented Feb 5, 2020

mbed-ci commented Feb 5, 2020

mtomczykmobica commented Feb 6, 2020

Test run: FAILED

mbed-ci commented Feb 6, 2020

ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive… #12320

ONME-3433 ESP8266 driver support for UDP get - modified ESP8266 drive… #12320

Conversation

mtomczykmobica commented Jan 28, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

SeppoTakalo commented Jan 28, 2020

michalpasztamobica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbed-ci commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomczykmobica commented Jan 30, 2020 • edited Loading

0xc0170 commented Feb 5, 2020

mbed-ci commented Feb 5, 2020

mtomczykmobica commented Feb 6, 2020

mbed-ci commented Feb 6, 2020

mtomczykmobica commented Jan 30, 2020 •

edited

Loading