UBLOX cellular api's for UDP and TCP #7619

mudassar-ublox · 2018-07-27T09:02:20Z

Description

This PR updates the cellular API's for the u-blox targets. Some u-blox targets must use an internal IP stack while others can use both internal and external IP stacks. This PR introduces support for the internal IP stack; a new cellular target type, called UBLOX_AT, is added in "CellularTargets.h" and AT classes are added in targets/UBLOX. The API to the internal IP stack supports both UDP and TCP socket operations.

Supported Targets: UBLOX_C030_U201
Tool Chains: ARM, GCC_ARM, IAR
ARM_Build.txt
GCC_ARM_Build.txt
IAR_Build.txt

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

AriParkkila

This looks quite good. Description could tell TCP is client only.

AriParkkila · 2018-07-27T10:14:26Z

features/cellular/framework/AT/AT_CellularStack.h

@@ -98,6 +98,7 @@ class AT_CellularStack : public NetworkStack, public AT_CellularBase
        bool started; // socket has been opened on modem stack
        bool tx_ready; // socket is ready for sending on modem stack
        bool rx_avail; // socket has data for reading on modem stack
+        volatile nsapi_size_t pending_bytes; // The number of received bytes pending


You could probably use rx_avail flag here just to indicate that some data is available because you always read how much data really is available when recveiving? If not then you could change type of rx_avail and use it instead of new pending_bytes variable.

There is no need to use volatile here.

If rx_avail is unused and nobody else is using it then we can change its type and use it as well.

AriParkkila · 2018-07-27T10:17:41Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT.cpp

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2017, Arm Limited and affiliates.


Years are passing so fast, it's already 2018 :) See also the other files.

AriParkkila · 2018-07-27T10:25:01Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT.h

+{
+
+public:
+	UBLOX_AT(events::EventQueue &queue);


Please run astyle.exe -n --options=.astylerc <file> for coding guidelines.

Updated all files.

AriParkkila · 2018-07-27T10:29:22Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularNetwork.cpp

+
+bool UBLOX_AT_CellularNetwork::has_registration(RegistrationType reg_type)
+{
+    return (reg_type == C_REG || reg_type == C_GREG);


LTE / NB-IoT should be C_EREG instead of C_GREG.

It is not LTE or NB-Iot module so no need to add 'C_EREG '.

AriParkkila · 2018-07-27T11:01:45Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularNetwork.cpp

+
+nsapi_error_t UBLOX_AT_CellularNetwork::detach()
+{
+    _at.lock();


Could call parent implementation AT_CellularNetwork::detach();?

After detach we are receiving two urc's, '+CGREG' and '+UUPSDD', to process these urc's we have added 50 ms wait here otherwise "CellularNetwork test detach" fails.

Could you add wait just before when "test detach" is called?

Are you suggesting to add wait in CellularNetwork test detach test case?
if yes then I can remove my detach() function and will use parent implementation.

AriParkkila · 2018-07-27T12:26:21Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+    _at.resp_start();
+    _at.resp_stop();
+    _at.unlock();
+


get_last_error() before unlocking.

AriParkkila · 2018-07-27T12:27:40Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+        return NSAPI_ERROR_DEVICE_ERROR;
+    }
+
+    _at.lock();


lock not needed due to socket_sendto_impl has locks.

AriParkkila · 2018-07-27T12:28:56Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+    CellularSocket *socket = NULL;
+
+    for (unsigned int x = 0; (socket == NULL) && (x < UBLOX_MAX_SOCKET); x++) {
+        if (_socket) {


If _socket is NULL then we probably want to assert :)

AriParkkila · 2018-07-27T12:37:28Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+        _at.resp_start("+UDNSRN:");
+        if (_at.info_resp()) {
+            _at.read_string(ipAddress, sizeof(ipAddress));
+            _at.resp_stop();


It doesn't change anything here but logically resp_stop would be in the same scope with resp_start

AriParkkila · 2018-07-27T12:41:04Z

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F437xG/TARGET_UBLOX_C030/onboard_modem_api.c

 #endif
+    /* wait for modem to power off properly*/
+    wait(10);


Is this really needed here?

mudassar-ublox · 2018-08-01T09:08:12Z

@AriParkkila please review again. Do you have any other queries?

AriParkkila · 2018-08-02T09:38:25Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+    uint8_t ch = 0;
+    int port = 0;
+    Timer timer;
+


An application may call recvfrom even when there is no incoming data. In these cases function should return immediately with NSAPI_ERROR_WOULD_BLOCK (async) instead of waiting for timer.read_ms() < SOCKET_TIMEOUT).

AriParkkila · 2018-08-02T09:43:29Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+        _at.write_int(address.get_port());
+        _at.write_int(size);
+        _at.cmd_stop();
+        wait_ms(50);


mbed_poll is a better way to wait for an input

AriParkkila · 2018-08-02T09:53:57Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularNetwork.cpp

+
+nsapi_error_t UBLOX_AT_CellularNetwork::detach()
+{
+    _at.lock();


Could you add wait just before when "test detach" is called?

NeilMacMullen · 2018-08-02T14:29:19Z

See #7679 and #7680 for issues that affect SARA-R4

cmonr · 2018-08-09T01:37:07Z

@AriParkkila In reference to those linked issues, would it be possible for this PR to continue if those are left open, or are those issues blocking basic functionality and/or would they cause tests to fail?

fahimalavi · 2018-08-09T05:53:15Z

+UDNSRN will be supported in later modem firmware versions of R410M variant. This pull request was generated after all tests getting passed.

AriParkkila

I think you should still fix async socket reading. If recv waits for an second that causes unnecessary unresponsiveness and battery consumption.

AriParkkila · 2018-08-13T06:46:39Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

+    uint8_t ch = 0;
+    int port = 0;
+    Timer timer;
+


You can always return NSAPI_ERROR_WOULD_BLOCK:

In blocking mode Mbed OS socket layer waits for a message until socket-timeout and if one is received via URC then socket layer calls recvfrom again to actually read the message.

In non-blocking mode control returns immediately to a calling application. When message is received via URC it issues callback on an application, then the application needs to call recvfrom again to actually read the message.

AriParkkila · 2018-08-15T10:21:52Z

features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp

@@ -275,6 +273,13 @@ nsapi_size_or_error_t UBLOX_AT_CellularStack::socket_recvfrom_impl(CellularSocke
    int port = 0;
    Timer timer;

+    if (!socket->rx_avail) {


I think rx_avail should not be cleared in sendto, you need to set it in URC and reset after all is read in recvfrom.

Because you are now updating pending_bytes in URC, you could use that as well and remove rx_avail completely, and then check for if (socket->pending_bytes > 0).

Are you saying that remove rx_avail from my class completely and use pending_bytes only, for checking availability of data as well.

AriParkkila

Please fix rx_avail and it looks good to me.

mudassar-ublox · 2018-08-15T12:00:39Z

Updated and removed rx_avail, using pending_bytes for availability of data. @AriParkkila Please review now.

cmonr · 2018-08-17T16:17:16Z

/morph build

mbed-ci · 2018-08-17T18:02:04Z

Build : SUCCESS

Build number : 2823
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7619/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-17T19:17:34Z

Exporter Build : FAILURE

Build number : 2453
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7619/

cmonr · 2018-08-17T21:04:09Z

Export failure does not appear related to PR.
/morph export-build

cmonr · 2018-08-17T21:15:10Z

/morph mbed2-build

mbed-ci · 2018-08-18T02:10:16Z

Test : SUCCESS

Build number : 2573
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7619/2573

0xc0170 · 2018-08-21T11:19:07Z

/morph export-build
/morph uvisor-test

0xc0170 · 2018-08-21T11:31:06Z

Jenkins machine fail, restarting

/morph export-build

0xc0170 · 2018-08-21T11:50:35Z

@ARMmbed/mbed-os-test Can you review the above exporter failure (IllegalStateException)

OPpuolitaival · 2018-08-21T11:54:20Z

@0xc0170 Seems to be jenkins failure. Please rerun export job

mbed-ci · 2018-08-21T12:57:09Z

Exporter Build : SUCCESS

Build number : 2478
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7619/

mbed-ci · 2018-08-21T13:32:42Z

Exporter Build : SUCCESS

Build number : 2479
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7619/

UBLOX cellular api's for UDP and TCP

UBLOX cellular api's for UDP and TCP

a8abecc

0xc0170 requested a review from a team July 27, 2018 09:16

0xc0170 added the needs: review label Jul 27, 2018

AriParkkila suggested changes Jul 27, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jul 27, 2018

mudassar-ublox force-pushed the cellular_ublox_udp_tcp_imp branch from 0fa9e3a to cd7dc50 Compare July 30, 2018 12:42

Artistic style applied and updated power function

ffb4f92

mudassar-ublox force-pushed the cellular_ublox_udp_tcp_imp branch from cd7dc50 to ffb4f92 Compare July 30, 2018 14:42

socket_open function removed

122e771

AriParkkila reviewed Aug 2, 2018

View reviewed changes

NeilMacMullen mentioned this pull request Aug 2, 2018

UBLOX cellular api's for UDP and TCP do not work for Sara R4 (CATM1) #7679

Closed

AriParkkila suggested changes Aug 13, 2018

View reviewed changes

Support added for async sockets

f9aaee5

AriParkkila reviewed Aug 15, 2018

View reviewed changes

AriParkkila approved these changes Aug 15, 2018

View reviewed changes

Replaced rx_avail with pending_bytes

f512668

AriParkkila approved these changes Aug 15, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.10.0-rc1 and removed needs: work labels Aug 15, 2018

0xc0170 approved these changes Aug 21, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Aug 21, 2018

cmonr merged commit 17a525c into ARMmbed:master Aug 21, 2018

0xc0170 removed the ready for merge label Aug 21, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7619 from u-blox/cellular_ublox_udp_tcp_imp

49460cb

UBLOX cellular api's for UDP and TCP

mudassar-ublox mentioned this pull request Mar 1, 2019

Cellular: UBLOX_C030_N211 Cellular API's #9637

Merged

		@@ -0,0 +1,59 @@
		/*
		* Copyright (c) 2017, Arm Limited and affiliates.

UBLOX cellular api's for UDP and TCP #7619

UBLOX cellular api's for UDP and TCP #7619

Uh oh!

Conversation

mudassar-ublox commented Jul 27, 2018

Description

Pull request type

Uh oh!

AriParkkila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mudassar-ublox Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mudassar-ublox commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NeilMacMullen commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Aug 9, 2018

Uh oh!

fahimalavi commented Aug 9, 2018

Uh oh!

AriParkkila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AriParkkila left a comment

mudassar-ublox Jul 30, 2018 •

edited

Loading

NeilMacMullen commented Aug 2, 2018 •

edited

Loading