Non-IP socket implementation for NIDD over CP #12065

AriParkkila · 2019-12-10T11:41:03Z

Summary of changes

Change cellular Non-IP socket implementation for NIDD over CP to be asynchronous and to conform better to 3GPP TS 27.007:

Add NIDD over CP socket tests
Add NIDD for U-blox/N2XX
Add NIDD for Quectel/BC95
Change CellularNonIPSocket to poll before timeout
Change ATHandler debug print levels
Change 3GPP TS 27.007 NIDD to async
Add count/dequeue methods in CellularList
Change CellularNonIPSocket to singleton
Fix Non-IP of Quectel/BG96 modem driver

Impact of changes

Migration actions required

Documentation

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

The provided test cases pass with mbed test -n tests-netsocket-nidd --app-config=tools/test_configs/NIDDInterface.json on:

MTB_ADV_WISE_1570-GCC_ARM
NRF52840_DK-GCC_ARM with "QUECTEL_BG96.provide-default": true
MTB_STM_L475-GCC_ARM with "UBLOX_N2XX.provide-default": true

For mbed-os-cellular-example define "sock-type": "NONIP" and "cellular.control-plane-opt": true, and also "cellular.clear-on-connect": true may be required to prevent the default IP context.

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

Reviewers

ciarmcom · 2019-12-10T12:00:35Z

@AriParkkila, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

kivaisan · 2019-12-11T10:36:32Z

features/cellular/framework/common/CellularUtil.cpp

@@ -378,6 +378,8 @@ pdp_type_t string_to_pdp_type(const char *pdp_type_str)
        pdp_type = IPV4_PDP_TYPE;
    } else if (len == 6 && memcmp(pdp_type_str, "Non-IP", len) == 0) {
        pdp_type = NON_IP_PDP_TYPE;
+    } else if (len == 5 && memcmp(pdp_type_str, "NONIP", len) == 0) {


Is this now a bit problematic if modem has a non-standard string for Non-IP? I think this should be tied to get_nonip_context_type_str() method somehow.

So far, I have seen only NONIP and Non-IP strings and this is an easy way to add support without breaking existing APIs.

How about giving get_nonip_context_type_str() as a function pointer parameter to string_to_pdp_type() . Then as a last step it would check match from that?

Wouldn't that break string_to_pdp_type API?

That's ok for mbed-os 6.

string_to_pdp_type() is only used in cellular context classes so maybe it should be moved to CellularContext in mbed-os 6. Then it can directly call get_nonip_context_type_str().

kivaisan · 2019-12-11T10:40:12Z

features/cellular/framework/device/CellularContext.cpp

@@ -69,6 +69,10 @@ CellularContext::CellularContext() : _next(0), _stack(0), _pdp_type(DEFAULT_PDP_

 void CellularContext::cp_data_received()
 {
+    if (!_cp_netif) {
+        tr_warn("Cellular Non-IP callback missing");


Maybe tr_error is better here or maybe even an assert.

It's actually fine (but not desirable) to receive callback before _cp_netif is created. E.g. QUECTEL_BG96_CellularContext::urc_nidd is subscribed already at its constructor, but _cp_netif create is deferred until CellularNonIPSocket::open so the modem may be sending events to a previous NIDD context.

bulislaw · 2019-12-11T14:02:09Z

@AnttiKauppila please review

0xc0170 · 2019-12-11T14:17:34Z

@AriParkkila There are conflicts now with master

AriParkkila · 2019-12-12T08:10:57Z

@0xc0170 It's now rebased.

adbridge · 2019-12-19T17:17:57Z

@AriParkkila it unfortunately looks like a further rebase is required

AriParkkila · 2019-12-20T06:35:19Z

@adbridge It's rebased now.

adbridge · 2019-12-23T12:10:12Z

CI started

mbed-ci · 2019-12-23T12:34:43Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

adbridge

Some very minor comments, but not enough to block the approval

adbridge · 2019-12-23T12:14:42Z

TESTS/netsocket/nidd/niddsocket_echotest.cpp

+    signals.set(SIGNAL_SIGIO_RX | SIGNAL_SIGIO_TX);
+}
+
+void NIDDSOCKET_ECHOTEST_NONBLOCK()


Why do you have some function names all in capitals? This doesn't conform to the coding guidelines ...

adbridge · 2019-12-23T12:37:12Z

features/cellular/framework/AT/AT_CellularNetwork.cpp

@@ -682,7 +686,7 @@ nsapi_error_t AT_CellularNetwork::clear()
            }
            context = context->next;
        }
-#ifdef MBED_CONF_NSAPI_DEFAULT_CELLULAR_APN
+#if defined(MBED_CONF_NSAPI_DEFAULT_CELLULAR_APN) && !MBED_CONF_CELLULAR_CONTROL_PLANE_OPT
        char pdp_type_str[sizeof("IPV4V6")];
        if (_device.get_property(AT_CellularDevice::PROPERTY_IPV4V6_PDP_TYPE) ||
                (_device.get_property(AT_CellularDevice::PROPERTY_IPV4_PDP_TYPE) && _device.get_property(AT_CellularDevice::PROPERTY_IPV6_PDP_TYPE))) {


Has the indentation gone a little astray here ?

adbridge · 2019-12-23T12:39:58Z

features/cellular/framework/targets/QUECTEL/BC95/QUECTEL_BC95_CellularNetwork.cpp

@@ -38,3 +38,26 @@ nsapi_error_t QUECTEL_BC95_CellularNetwork::set_access_technology_impl(RadioAcce

    return NSAPI_ERROR_OK;
 }
+
+nsapi_error_t QUECTEL_BC95_CellularNetwork::clear()


Camelcase function name ? Strictly we shouldn't have those but it seems there are similar ones already so consistency is more important..

adbridge · 2019-12-23T12:50:22Z

@AriParkkila looks like there is a failure in the compilation of the Unittests...

Add a missing license header. Remove semaphores and add +CRTDCP to support async operation. Fix delete context and disconnect to execute just once. Add support for NONIP PPD type. Change CellularNetwork::clear() to virtual so it can be overridden.

AriParkkila · 2020-01-07T06:37:42Z

@0xc0170 It's now rebased, and should be ready for merge.

0xc0170 · 2020-01-07T12:00:56Z

@AriParkkila Thanks, can you address also one astyle failure (might be due to the rebase) ? we will restart CI

0xc0170 · 2020-01-07T12:10:42Z

CI started

mbed-ci · 2020-01-07T14:54:21Z

Test run: SUCCESS

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

adbridge · 2020-02-17T13:52:34Z

@bulislaw @0xc0170 This is changing an API so looks like it should be breaking change to me?

0xc0170 · 2020-02-17T16:08:23Z

@ARMmbed/mbed-os-wan We are reviewing 6.0.0 PRs and their release notes. Please review above ^^

My take here this is not breaking. Is this just a patch as set above ? In case not, please add Release notes.

AriParkkila · 2020-02-18T05:39:15Z

@adbridge I don't think this PR has API changes. Can you elaborate please?

ciarmcom requested review from a team December 10, 2019 12:00

ciarmcom added the needs: review label Dec 10, 2019

AriParkkila changed the title ~~Cell fea nidd~~ Non-IP socket implementation for NIDD over CP Dec 10, 2019

kivaisan reviewed Dec 11, 2019

View reviewed changes

AnttiKauppila added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Dec 11, 2019

0xc0170 requested a review from AnttiKauppila December 11, 2019 14:17

0xc0170 added needs: work and removed needs: review labels Dec 11, 2019

AriParkkila force-pushed the cell-fea-nidd branch from a7f68d2 to addbd8c Compare December 12, 2019 08:07

AnttiKauppila approved these changes Dec 16, 2019

View reviewed changes

kivaisan approved these changes Dec 18, 2019

View reviewed changes

AriParkkila force-pushed the cell-fea-nidd branch from addbd8c to 4300582 Compare December 20, 2019 06:33

adbridge added needs: CI and removed needs: work labels Dec 23, 2019

adbridge approved these changes Dec 23, 2019

View reviewed changes

adbridge removed the needs: CI label Dec 24, 2019

Ari Parkkila added 9 commits January 6, 2020 22:33

Cellular: Change CellularNonIPSocket to singleton

7374b22

Cellular: Add count/dequeue methods in CellularList

972d8a6

Cellular: Change ATHandler debug print levels

f4158ca

Cellular: Change CellularNonIPSocket to poll before timeout

033402d

Cellular: Add NIDD for Quectel/BC95

f695e27

Cellular: Add NIDD for U-blox/N2XX

85baad8

Cellular: Add NIDD over CP socket tests

27024b3

Cellular: Fix astyle

4b3ef73

AriParkkila force-pushed the cell-fea-nidd branch from 4212817 to 558b322 Compare January 7, 2020 06:36

0xc0170 added needs: CI needs: work and removed needs: work needs: CI labels Jan 7, 2020

Cellular: Refactor CellularBase from AT_ControlPlane_netif

7899fea

AriParkkila force-pushed the cell-fea-nidd branch from 558b322 to 7899fea Compare January 7, 2020 12:03

0xc0170 added needs: CI and removed needs: work labels Jan 7, 2020

0xc0170 removed the needs: CI label Jan 7, 2020

0xc0170 merged commit 3e790cc into ARMmbed:master Jan 7, 2020

adbridge added BREAKING-CHANGE Incomplete Release Notes and removed BREAKING-CHANGE labels Feb 17, 2020

Non-IP socket implementation for NIDD over CP #12065

Non-IP socket implementation for NIDD over CP #12065

Uh oh!

Conversation

AriParkkila commented Dec 10, 2019

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Dec 10, 2019

Uh oh!

kivaisan Dec 11, 2019 • 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!

kivaisan Dec 11, 2019 • 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!

bulislaw commented Dec 11, 2019

Uh oh!

0xc0170 commented Dec 11, 2019

Uh oh!

AriParkkila commented Dec 12, 2019

Uh oh!

adbridge commented Dec 19, 2019

Uh oh!

AriParkkila commented Dec 20, 2019

Uh oh!

adbridge commented Dec 23, 2019

Uh oh!

mbed-ci commented Dec 23, 2019

Uh oh!

adbridge 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!

adbridge commented Dec 23, 2019

Uh oh!

AriParkkila commented Jan 7, 2020

Uh oh!

0xc0170 commented Jan 7, 2020

Uh oh!

0xc0170 commented Jan 7, 2020

Uh oh!

mbed-ci commented Jan 7, 2020

Uh oh!

adbridge commented Feb 17, 2020

Uh oh!

0xc0170 commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AriParkkila commented Feb 18, 2020

Uh oh!

Uh oh!

kivaisan Dec 11, 2019 •

edited

Loading

kivaisan Dec 11, 2019 •

edited

Loading

0xc0170 commented Feb 17, 2020 •

edited

Loading