cellular: eps ciot optimization network support check #8709

ghost · 2018-11-12T06:57:22Z

Description

CCIOTOPTI URC handler added, tested with unittests, no HW support currently available to test with

Pull request type

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

ghost · 2018-11-12T06:58:06Z

@mirelachirica Please review

0xc0170 · 2018-11-12T09:05:31Z

travis-ci/astyle — Passed, 50 files (+1 files)

Can you fix please?

mirelachirica · 2018-11-13T07:24:42Z

features/cellular/framework/AT/AT_CellularNetwork.h

@@ -198,6 +200,7 @@ class AT_CellularNetwork : public CellularNetwork, public AT_CellularBase {
    nsapi_ip_stack_t _ip_stack_type;
    int _cid;
    Callback<void(nsapi_event_t, intptr_t)> _connection_status_cb;
+    Callback<void(Supported_Network_Opt)> _ciot_network_support_cb;


I would include "opt" in the name of the callback _ciotopt_network_support_cb or _ciot_opt_network_support_cb.

mirelachirica · 2018-11-13T07:31:26Z

features/cellular/framework/API/CellularNetwork.h

@@ -55,6 +55,7 @@ class CellularNetwork : public NetworkInterface {
                                            optimization. */
        SUPPORTED_UE_OPT_MAX
    };
+    typedef Supported_UE_Opt Supported_Network_Opt;


What about one type which doesnt have UE or network in the name of the values and it could be used bithe as UE and network values? Like:
enum CIoT_Opt {
CIOT_OPT_NO_SUPPORT
CIOT_OPT_CONTROL_PLANE
....

mirelachirica · 2018-11-13T07:33:31Z

features/cellular/framework/API/CellularNetwork.h

     *  @param preferred_opt Preferred CIoT EPS optimizations.
+     *  @param network_support_cb This callback will be called when network optimisation support is known


when network CIoT optimisation support

mirelachirica · 2018-11-13T07:40:40Z

features/cellular/framework/AT/AT_CellularNetwork.cpp

+        _ciot_network_support_cb(supported_network_opt);
+    }
+}
+
 nsapi_error_t AT_CellularNetwork::get_ciot_optimization_config(Supported_UE_Opt &supported_opt,
                                                               Preferred_UE_Opt &preferred_opt)
 {


This API name should be changed to indicate that this will return UE ciot optimizations.

I think a getter for the Network's ciot optimization is needed. Some flag/member to be set in urc_cciotopti()?

mirelachirica · 2018-11-14T07:18:43Z

features/cellular/framework/API/CellularNetwork.h

+
+    /** Get Network CIoT optimizations.
+     *
+     *  @param supported_opt Supported CIoT EPS optimizations. CIOT_OPT_MAX will be returned,


supported_opt -> supported_network_opt Network supported CIoT EPS optimizations.....

mirelachirica · 2018-11-14T07:24:28Z

Could you still have 2 commits only? One for astyle fixes and one for the "cellular: ciot eps optimization support". Now there is a third commit which has both kind changes.

mirelachirica · 2018-11-14T07:46:30Z

Now there is a third commit which has both kind changes.

That is not true. My mistake.

Still the CIoT optimization changes better be in one commit than split in two commits which are not functional/logical splits.

cmonr · 2018-11-15T18:10:37Z

@TeemuKultala Mind rebasing so that we can get CI started?

cmonr · 2018-11-17T03:04:44Z

/morph build

mbed-ci · 2018-11-17T06:26:41Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-11-17T08:37:59Z

Exporter Build : SUCCESS

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

NirSonnenschein · 2018-11-18T12:47:10Z

@ARMmbed/mbed-os-maintainers note: This one still needs a maintainer review

0xc0170 · 2018-11-19T14:31:57Z

travis-ci/astyle — Passed, 64 files (+4 files)

The best to avoid suprised, can you rebase on top of master to update travis config (this would error with the new config). This still needs to be addressed here, please update the code

0xc0170 · 2018-11-19T14:33:30Z

I fail to find how this is breaking and why it is breaking. Please explain in the commit (the first commit that is the most important should be expanded).

My/our expectation for even functionality change is to have at least one paragraph in the commit message to introduce the change (it's not few lines fix).

-added an API for checking network eps ciot optimization support -renamed the API for getting the UE parameters -the API for setting the UE parameters includes now a callback, which will be called once network support for eps ciot optimization is known

0xc0170 · 2018-11-20T12:10:21Z

Is this breaking, how?

mirelachirica · 2018-11-20T12:16:39Z

features/cellular/framework/API/CellularNetwork.h

-                                                       Preferred_UE_Opt preferred_opt) = 0;
+    virtual nsapi_error_t set_ciot_optimization_config(CIoT_Supported_Opt supported_opt,
+            CIoT_Preferred_UE_Opt preferred_opt,
+            Callback<void(CIoT_Supported_Opt)> network_support_cb) = 0;


@0xc0170: I assume is breaking because of this change and the one from line 278 bellow, renaming
get_ciot_optimization_config ->get_ciot_ue_optimization_config

bulislaw · 2018-11-22T16:55:16Z

@AnttiKauppila that's marked as a breaking change, we are committed to maintaining backward compatibility. Is this necessary, can we not massage it so it doesn't break the public APIs?

AnttiKauppila · 2018-11-23T09:45:25Z

Changing the enum values are also a breaking change, CIoT_ was added, was there a name clash of some sort @TeemuKultala? If we don't change the namings and modify that added callback to be a pointer (defaulting to NULL) then this would be a minor break and would be fine for 5.11.

ghost · 2018-11-23T10:19:55Z

@AnttiKauppila The support enum was previously referring to UE values, but is now used for both network and UE values, and therefore more common name taken into use. The preferred enum changed to be consistent with the supported enum. CCIOTOPTI is a Rel-14 feature, so most likely there are few, if any, users for this API

AnttiKauppila · 2018-11-26T09:02:42Z

@0xc0170 @bulislaw Let's move this to 5.12, it is not mandatory in 5.11

0xc0170 · 2018-11-26T10:16:50Z

@0xc0170 @bulislaw Let's move this to 5.12, it is not mandatory in 5.11

release version updated

AnttiKauppila

This is a new feature which is needed in 5.12. This is breaking a functionality which was not even working fully earlier and therefore the "breaking" thing is not critical here.

AnttiKauppila · 2018-12-03T14:22:32Z

@0xc0170 @cmonr Is it possible to trigger CI for this?

cmonr

LGTM

cmonr · 2018-12-03T16:27:30Z

CI started

mbed-ci · 2018-12-03T20:35:26Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 8
Build artifacts
Build logs

adbridge · 2018-12-04T17:44:29Z

This needs to go to a feature branch

ghost · 2018-12-05T14:14:23Z

This is the feature branch PR: #8979

cmonr · 2018-12-14T00:48:37Z

Closing since #8979 already came in.

0xc0170 added the needs: review label Nov 12, 2018

0xc0170 requested a review from mirelachirica November 12, 2018 09:05

0xc0170 added needs: work and removed needs: review labels Nov 12, 2018

mirelachirica reviewed Nov 13, 2018

View reviewed changes

mirelachirica reviewed Nov 14, 2018

View reviewed changes

mirelachirica approved these changes Nov 16, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.11.0-rc1 and removed needs: work labels Nov 16, 2018

NirSonnenschein requested a review from a team November 18, 2018 12:44

NirSonnenschein added risk: G BREAKING-CHANGE labels Nov 18, 2018

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

mirelachirica reviewed Nov 20, 2018

View reviewed changes

cellular: eps ciot optimization network support check astyle fix

c3c5c25

bulislaw requested a review from AnttiKauppila November 22, 2018 16:51

0xc0170 added release-version: 5.12.0-rc1 and removed release-version: 5.11.0-rc1 labels Nov 26, 2018

cmonr removed the risk: G label Nov 27, 2018

AnttiKauppila approved these changes Dec 3, 2018

View reviewed changes

cmonr approved these changes Dec 3, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels Dec 3, 2018

cmonr added ready for merge and removed needs: CI labels Dec 4, 2018

cmonr added the do not merge label Dec 4, 2018

ghost mentioned this pull request Dec 5, 2018

Cellular ciot eps on feature branch #8979

Merged

cmonr closed this Dec 14, 2018

cmonr removed do not merge ready for merge labels Dec 14, 2018

ghost deleted the cellular_ciot_eps branch March 7, 2019 08:26

		* @param preferred_opt Preferred CIoT EPS optimizations.
		* @param network_support_cb This callback will be called when network optimisation support is known

cellular: eps ciot optimization network support check #8709

cellular: eps ciot optimization network support check #8709

Uh oh!

Conversation

ghost commented Nov 12, 2018

Description

Pull request type

Uh oh!

ghost commented Nov 12, 2018

Uh oh!

0xc0170 commented Nov 12, 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!

mirelachirica commented Nov 14, 2018

Uh oh!

mirelachirica commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Nov 15, 2018

Uh oh!

cmonr commented Nov 17, 2018

Uh oh!

mbed-ci commented Nov 17, 2018

Triggering tests

Uh oh!

mbed-ci commented Nov 17, 2018

Uh oh!

NirSonnenschein commented Nov 18, 2018

Uh oh!

0xc0170 commented Nov 19, 2018

Uh oh!

0xc0170 commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 20, 2018

Uh oh!

mirelachirica Nov 20, 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!

bulislaw commented Nov 22, 2018

Uh oh!

AnttiKauppila commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 23, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnttiKauppila commented Nov 26, 2018

Uh oh!

0xc0170 commented Nov 26, 2018

Uh oh!

AnttiKauppila left a comment

Choose a reason for hiding this comment

Uh oh!

AnttiKauppila commented Dec 3, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Dec 3, 2018

Uh oh!

mbed-ci commented Dec 3, 2018

Uh oh!

adbridge commented Dec 4, 2018

Uh oh!

ghost commented Dec 5, 2018

Uh oh!

cmonr commented Dec 14, 2018

mirelachirica commented Nov 14, 2018 •

edited

Loading

0xc0170 commented Nov 19, 2018 •

edited

Loading

mirelachirica Nov 20, 2018 •

edited

Loading

AnttiKauppila commented Nov 23, 2018 •

edited

Loading

ghost commented Nov 23, 2018 •

edited by ghost

Loading