Major refactoring: changing Network inheritance from CellularNetwork to new class CellularContext #8579

jarvte · 2018-10-30T07:13:51Z

Description

Major refactoring: changing NetworkInterface inheritance from CellularNetwork to new class CellularContext

this was done to support multiple APN/PDP Contexts
big PR but big part of changes are moving methods from (AT_)CellularNetwork to (AT_)CellularContext
and also moving overridden methods in target folder
CellularDevice owns new CellularStateMachine class which is based on old CellularConnectionFSM which was deleted as it's now obsolete. This was done so that we don't have to expose state machine as an API as it's logic and states might change
state machine now runs device to attached state (previous was to connect), CellularContext class does context activation and connection
changing inheritance affects many classes so it was quite impossible to a smaller PR (also UNIT tests needed to be modified for PR to pass)
after this change main API's for user application is CellularDevice and CellularContext
tested with MTS_DRAGONFLY, ADV_WISE_1570, UBLOX_C027, GEMALTO_CINTERION and QUECTEL_BG96
@AriParkkila @mirelachirica please review

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[X] Breaking change

0xc0170 · 2018-10-30T09:15:25Z

after this change main API's for user application is CellularDevice and CellularContext

How to migrate from 5.10 to 5.11 (if it includes this one) ? Is there a way to make this backward compatible or only one way forward (migration guide needed) ?

jarvte · 2018-10-30T10:13:02Z

after this change main API's for user application is CellularDevice and CellularContext

How to migrate from 5.10 to 5.11 (if it includes this one) ? Is there a way to make this backward compatible or only one way forward (migration guide needed) ?

https://github.com/ARMmbed/mbed-os-example-cellular will still work. It uses CellularBase interface and new class CellularContext is derived from CellulalBase. We also have an internal ticket to update handbook documentation.

mirelachirica · 2018-10-30T10:14:19Z

features/cellular/framework/AT/AT_CellularContext.cpp

+using namespace mbed;
+
+AT_CellularContext::AT_CellularContext(ATHandler &at, CellularDevice *device, const char *apn, nsapi_ip_stack_t stack) :
+        AT_CellularBase(at), _ip_stack_type_requested(DEFAULT_STACK), _is_connected(false), _is_blocking(true),


_ip_stack_type_requested(DEFAULT_STACK) -> _ip_stack_type_requested(stack) ?

true, will fix

Can we really make API to prefer some IP version? Could just use a flag to prefer non-IP and/or control plane?

agreed with the team that we will remove nsapi_ip_stack_t stack from the constructor.

bulislaw · 2018-10-30T16:08:01Z

Are all the APIs mentioned here stay the same and all the example still work? https://os.mbed.com/docs/v5.10/apis/cellular-api.html

jarvte · 2018-11-02T12:45:54Z

@bulislaw

Are all the APIs mentioned here stay the same and all the example still work? https://os.mbed.com/docs/v5.10/apis/cellular-api.html

Yes, example still works as now new class CellularContext is derived from CellularBase.

AriParkkila · 2018-10-30T12:24:16Z

features/netsocket/mbed_lib.json

-        "default-cellular-apn": null,
-        "default-cellular-username": null,
-        "default-cellular-password": null,
+        "default-cellular-plmn": 0,


Remove from here, can be changed via Device API.

AriParkkila · 2018-10-30T12:30:19Z

UNITTESTS/stubs/AT_CellularContext_stub.cpp

+            CellularContext::AuthenticationType type) {
+}
+
+void AT_CellularContext::set_apn_credentials(const char* apn, const char *uname, const char *pwd,


Remove this, two times almost the same function.

removed both set_apn_xxx methods as we changed to derive from CellularBase

AriParkkila · 2018-11-02T12:37:53Z

features/cellular/framework/AT/AT_CellularContext.cpp

+
+uint32_t AT_CellularContext::get_timeout_for_operation(ContextOperation op) const
+{
+    uint32_t timeout = 10 * 60 * 1000; // default timeout is 10 minutes as registration and attach may take time


This should be more than 10 minutes, probably something like 30 minutes. Maybe could be defined as NETWORK_TIMEOUT.

AriParkkila · 2018-11-02T12:44:29Z

features/cellular/framework/AT/AT_CellularContext.cpp

+    } else {
+        MBED_ASSERT(_cid >= 0 && _cid <= 99);
+        char cmd_buf[sizeof("ATD*99***xx#")];
+        std::sprintf(cmd_buf, "ATD*99***%d#", _cid);


This is missing one commit from master.

added the missing fix

AriParkkila · 2018-11-02T13:12:42Z

features/cellular/framework/targets/GEMALTO/CINTERION/GEMALTO_CINTERION_CellularContext.cpp

+
+bool GEMALTO_CINTERION_CellularContext::stack_type_supported(nsapi_ip_stack_t requested_stack)
+{
+#if NSAPI_PPP_AVAILABLE


Missing one commit, this function should be:

if (GEMALTO_CINTERION_Module::get_model() == GEMALTO_CINTERION_Module::ModelBGS2) { return (requested_stack == IPV4_STACK); } return (requested_stack == IPV4_STACK || requested_stack == IPV6_STACK);

AriParkkila · 2018-11-02T13:15:44Z

features/cellular/framework/targets/TELIT/HE910/TELIT_HE910_CellularContext.cpp

+
+bool TELIT_HE910_CellularContext::stack_type_supported(nsapi_ip_stack_t requested_stack)
+{
+    return requested_stack == IPV4_STACK || requested_stack == IPV6_STACK;


This is missing the same commit as in Gemalto drivers.

fixed as in master

mirelachirica · 2018-11-05T08:56:48Z

features/cellular/framework/API/CellularContext.h

+     *
+     * @return pointer to interface, if any.
+     */
+    static CellularBase *get_default_instance();


This is CellularBase too.

true, moved under CellularBase

mirelachirica · 2018-11-05T09:17:25Z

features/cellular/framework/AT/AT_CellularContext.h

+    nsapi_error_t check_operation(nsapi_error_t err, ContextOperation op);
+
+private:
+    nsapi_ip_stack_t _ip_stack_type_requested;


Since there is no more requested stack type(removed from constructor) it means this is not relevant anymore.

True, but we agreed that we are not remove this in this PR as it will change getting the context logic which will change anyway after this PR.

mirelachirica · 2018-11-05T11:16:39Z

features/cellular/framework/API/CellularDevice.h

+     *  CellularContext (might be many) and CellularStateMachine if available.
+     *
+     */
+    void cellular_callback(nsapi_event_t ev, intptr_t ptr);


Could this be rephrased more clear? It is not clear who is actually calling it and to which case PPP reffers to.

bulislaw · 2018-11-05T15:06:53Z

As long as the backward compatibility is not broken I'm ok with that. The code review will need to be performed by Cllular team.

CC: @kjbracey-arm

cmonr · 2018-11-12T13:47:55Z

@jarvte Thanks for the heads up. Will start when able.

0xc0170 · 2018-11-12T17:45:37Z

/morph build

mbed-ci · 2018-11-12T19:57:42Z

Build : FAILURE

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

0xc0170 · 2018-11-13T10:21:50Z

travis-ci/astyle — Passed, 61 files (+12 files)

Please clean up these 12 files

…jor refactor.

jarvte · 2018-11-13T19:19:36Z

travis-ci/astyle — Passed, 61 files (+12 files)

Please clean up these 12 files

@0xc0170 @cmonr Astyle and other fixes done

NirSonnenschein · 2018-11-14T08:52:43Z

/morph build

mbed-ci · 2018-11-14T09:39:59Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-11-14T13:15:28Z

Test : SUCCESS

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

mbed-ci · 2018-11-14T13:52:43Z

Exporter Build : SUCCESS

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

maclobdell · 2018-11-30T11:34:09Z

Has this been tested with MTS_DRAGONFLY_L471QG (the new Dragonfly Nano)? There is hardware in Oulu.

jarvte · 2018-11-30T11:42:17Z

Has this been tested with MTS_DRAGONFLY_L471QG (the new Dragonfly Nano)? There is hardware in Oulu.

No I don't think we tested with that. I didn't know that we have that here.

0xc0170 added needs: review BREAKING-CHANGE labels Oct 30, 2018

0xc0170 requested review from bulislaw and a team October 30, 2018 09:15

mirelachirica reviewed Oct 30, 2018

View reviewed changes

0xc0170 added needs: work release-version: 5.11.0-rc1 and removed needs: review labels Oct 30, 2018

jarvte force-pushed the cellular_context branch 4 times, most recently from 02fbf22 to b7b077e Compare November 2, 2018 11:42

AriParkkila suggested changes Nov 2, 2018

View reviewed changes

jarvte force-pushed the cellular_context branch 2 times, most recently from d902716 to b46a66e Compare November 5, 2018 08:08

mirelachirica reviewed Nov 5, 2018

View reviewed changes

jarvte force-pushed the cellular_context branch from b46a66e to b891f5b Compare November 5, 2018 10:40

mirelachirica reviewed Nov 5, 2018

View reviewed changes

jarvte force-pushed the cellular_context branch from b891f5b to 57126be Compare November 5, 2018 12:37

jarvte force-pushed the cellular_context branch 4 times, most recently from a093041 to c0858b9 Compare November 7, 2018 07:12

AriParkkila approved these changes Nov 7, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels Nov 12, 2018

Cellular: Added missing MDMRTS and MDMCTS for TARGET_MTB_ADV_WISE_1570.

1b3db96

jarvte force-pushed the cellular_context branch 2 times, most recently from 33dd456 to 132e94b Compare November 13, 2018 10:08

0xc0170 added needs: work and removed needs: CI labels Nov 13, 2018

jarvte force-pushed the cellular_context branch from 132e94b to 7a64520 Compare November 13, 2018 15:55

Cellular: fixed cellular greentea tests after major refactoring.

14f3740

jarvte force-pushed the cellular_context branch from 7a64520 to 14f3740 Compare November 13, 2018 16:34

Cellular: astyle fixes after introducing CellularContext class and ma…

88213d3

…jor refactor.

cmonr added needs: CI and removed needs: work labels Nov 13, 2018

cmonr added ready for merge and removed needs: CI labels Nov 14, 2018

adbridge merged commit 4d07bcb into ARMmbed:master Nov 14, 2018

studavekar removed the ready for merge label Nov 14, 2018

jarvte mentioned this pull request Nov 27, 2018

Cellular: update handbook to 5.11 ARMmbed/mbed-os-5-docs#843

Merged

jarvte deleted the cellular_context branch December 3, 2018 07:04

Major refactoring: changing Network inheritance from CellularNetwork to new class CellularContext #8579

Major refactoring: changing Network inheritance from CellularNetwork to new class CellularContext #8579

Uh oh!

Conversation

jarvte commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

0xc0170 commented Oct 30, 2018

Uh oh!

jarvte commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

bulislaw commented Oct 30, 2018

Uh oh!

jarvte commented Nov 2, 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!

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!

jarvte Nov 5, 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!

bulislaw commented Nov 5, 2018

Uh oh!

cmonr commented Nov 12, 2018

Uh oh!

0xc0170 commented Nov 12, 2018

Uh oh!

mbed-ci commented Nov 12, 2018

Uh oh!

0xc0170 commented Nov 13, 2018

Uh oh!

jarvte commented Nov 13, 2018

Uh oh!

NirSonnenschein commented Nov 14, 2018

Uh oh!

mbed-ci commented Nov 14, 2018

Triggering tests

jarvte commented Oct 30, 2018 •

edited

Loading

jarvte commented Oct 30, 2018 •

edited

Loading

jarvte Nov 5, 2018 •

edited

Loading

maclobdell commented Nov 30, 2018 •

edited

Loading