Cellular: UBLOX_C030_N211 Cellular API's #9637

mudassar-ublox · 2019-02-07T13:13:17Z

Description

This PR updates the cellular API's for UBLOX target UBLOX_C030_N211. It has target type UBLOX_N2XX. These API support only UDP operations.

Supported Targets: UBLOX_C030_N211
Tool Chains: ARM, GCC_ARM, IAR

ARM_Build.txt
GCC_Build.txt
IAR_Build.txt

Pull request type

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

Reviewers

ciarmcom · 2019-02-07T14:00:30Z

@mudassar-ublox, thank you for your changes.
@ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2019-02-07T14:51:39Z

Will this conflict with #9568 ?

0xc0170 · 2019-02-11T12:05:48Z

@ARMmbed/mbed-os-wan Can you please review and advice how to resolve conflicts here?

Conflicting files
features/cellular/TESTS/api/cellular_power/main.cpp
features/cellular/framework/API/CellularPower.h
features/cellular/framework/AT/AT_CellularBase.h
features/cellular/framework/AT/AT_CellularPower.cpp
features/cellular/framework/AT/AT_CellularPower.h
features/cellular/framework/AT/AT_CellularSMS.cpp

fahimalavi · 2019-02-11T12:17:14Z

Pull request is getting updated but any advice to resolve conflicts will be better. Thanks

cmonr · 2019-02-11T21:58:41Z

@fahim-ublox To resolve the conflicts, this PR needs to be rebased against master.
The rebase process will pause for each conflict to allow for manual resolution.

It's possible that @ARMmbed/mbed-os-wan might need to help.

AriParkkila

Unfortunately feature-cellular-refactor introduced quite many (breaking) changes that need to be resolved here, but the changes seem to be quite straight forward. Note that after SupportedFeature was changed to CellularProperty it requires that every modem driver must have a complete property map and thus need to be updated when adding new properties like AT_CellularSMSMmodeText.

AriParkkila · 2019-02-12T08:34:45Z

features/cellular/TESTS/api/cellular_power/main.cpp

@@ -81,7 +81,7 @@ static void test_power_interface()
    TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED);
    wait_for_power(pwr);

-    TEST_ASSERT(pwr->set_power_level(1, 0) == NSAPI_ERROR_OK);
+    TEST_ASSERT(pwr->set_power_level(1, 0, MBED_CONF_APP_CELLULAR_SIM_PIN) == NSAPI_ERROR_OK);


Test was removed.

AriParkkila · 2019-02-12T08:35:18Z

features/cellular/framework/API/CellularPower.h

@@ -93,7 +93,7 @@ class CellularPower {
     *  @return         NSAPI_ERROR_OK on success
     *                  NSAPI_ERROR_DEVICE_ERROR on failure
     */
-    virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0) = 0;
+    virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0, const char *sim_pin = NULL) = 0;


CellularPower API was removed.

AriParkkila · 2019-02-12T08:39:44Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp

+{
+}
+
+nsapi_error_t UBLOX_N2XX_CellularPower::on()


on() was split to soft/hard_power_on

hard_power_on to call ::onboard_modem_init()
hard_power_off to call ::onboard_modem_deinit()
soft_power_on to call ::onboard_modem_power_up()
soft_power_off to call ::onboard_modem_power_down()

AriParkkila · 2019-02-12T08:40:27Z

features/cellular/framework/AT/AT_CellularPower.cpp

@@ -56,7 +56,7 @@ nsapi_error_t AT_CellularPower::set_at_mode()
    return _at.unlock_return_error();
 }

-nsapi_error_t AT_CellularPower::set_power_level(int func_level, int do_reset)
+nsapi_error_t AT_CellularPower::set_power_level(int func_level, int do_reset, const char *sim_pin)


File was removed.

AriParkkila · 2019-02-12T08:40:38Z

features/cellular/framework/AT/AT_CellularPower.h

@@ -40,7 +40,7 @@ class AT_CellularPower : public CellularPower, public AT_CellularBase {

    virtual nsapi_error_t set_at_mode();

-    virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0);
+    virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0, const char *sim_pin = NULL);


File was removed.

AriParkkila · 2019-02-12T09:04:31Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.h

@@ -0,0 +1,45 @@
+/*


CellularPower api was removed, remove this file.

AriParkkila · 2019-02-12T09:04:47Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp

+}
+
+
+nsapi_error_t UBLOX_N2XX_CellularPower::set_power_level(int func_level, int do_reset, const char *sim_pin)


Function was removed.

AriParkkila · 2019-02-12T09:05:47Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp

+    return NSAPI_ERROR_OK;
+}
+
+nsapi_error_t UBLOX_N2XX_CellularPower::set_at_mode()


Function was changed to CellularDevice::init.

AriParkkila · 2019-02-12T09:06:00Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp

@@ -0,0 +1,92 @@
+/*


CellularPower api was removed, remove this file.

AriParkkila · 2019-02-12T09:06:29Z

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularNetwork.h

@@ -0,0 +1,43 @@
+/*


This file can likely be removed.

0xc0170 · 2019-02-18T13:47:30Z

@mudassar-ublox Do you need any assistance for updating this PR?

mudassar-ublox · 2019-02-19T05:59:03Z

@mudassar-ublox Do you need any assistance for updating this PR?

There are few other commands which are also not supported for this target. So I am adding them in CellularProperty in AT_CellularBase like this, commands CNMI, CSMP, CMGF, CSDH, is that fine

enum CellularProperty {
    PROPERTY_C_EREG,            // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_C_GREG,            // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_C_REG,             // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_AT_CGSN_WITH_TYPE, // 0 = not supported, 1 = supported. AT+CGSN without type is likely always supported similar to AT+GSN.
    PROPERTY_AT_CGDATA,         // 0 = not supported, 1 = supported. Alternative is to support only ATD*99***<cid>#
    PROPERTY_AT_CGAUTH,         // 0 = not supported, 1 = supported. APN authentication AT commands supported
    PROPERTY_AT_CNMI,           // 0 = not supported, 1 = supported. New message (SMS) indication AT command
    PROPERTY_AT_CSMP,           // 0 = not supported, 1 = supported. Set text mode AT command
    PROPERTY_AT_CMGF,           // 0 = not supported, 1 = supported. Set preferred message format AT command
    PROPERTY_AT_CSDH,           // 0 = not supported, 1 = supported. Show text mode AT command
    PROPERTY_IPV4_PDP_TYPE,     // 0 = not supported, 1 = supported. Does modem support IPV4?
    PROPERTY_IPV6_PDP_TYPE,     // 0 = not supported, 1 = supported. Does modem support IPV6?
    PROPERTY_IPV4V6_PDP_TYPE,   // 0 = not supported, 1 = supported. Does modem support dual stack IPV4V6?
    PROPERTY_NON_IP_PDP_TYPE,   // 0 = not supported, 1 = supported. Does modem support Non-IP?
    PROPERTY_MAX
}

AriParkkila · 2019-02-19T06:57:05Z

PROPERTY_AT_CGAUTH

This is probably not needed, but you should simply override APN authentication similar to UBLOX_AT_CellularContext::activate_profile.

mudassar-ublox · 2019-02-19T07:59:33Z

PROPERTY_AT_CGAUTH

This is probably not needed, but you should simply override APN authentication similar to UBLOX_AT_CellularContext::activate_profile.

you mean to override AT_CellularContext::do_user_authentication() function ?
PROPERTY_AT_CGAUTH already handled in it so why need to override it.

AriParkkila · 2019-02-19T08:06:21Z

@mudassar-ublox if the modem supports AT+CGAUTH then my comment was incorrect :)

And yes, it's fine to add new AT commands you mentioned (you just need to add those in all modem drivers).

mudassar-ublox · 2019-02-19T09:29:09Z

@mudassar-ublox if the modem supports AT+CGAUTH then my comment was incorrect :)

And yes, it's fine to add new AT commands you mentioned (you just need to add those in all modem drivers).

For other modem drivers, I have not idea whether these commands are supported for them or not. Can you please do that accordingly after my PR. For other UBLOX drivers I will update it.

AriParkkila · 2019-02-19T10:11:33Z

@mudassar-ublox unfortunately your PR will never pass without adding properties to each driver, but you can define them all simply supported due to that won't change any current behavior.

0xc0170 · 2019-02-20T14:33:48Z

Removing 5.12 release label as this needs further work and review

mudassar-ublox · 2019-02-21T09:54:11Z

I have updated PR, please review now.

AriParkkila · 2019-02-21T10:12:25Z

@mudassar-ublox cellular properties look good. but still need to refactor power and SMS classes.

mudassar-ublox · 2019-02-22T09:30:45Z

I have updated PR, power classes are removed and SMS classes are already updated. Please review and let me know if anything else is required.

mudassar-ublox · 2019-03-01T12:56:20Z

@mudassar-ublox please check the remaining comments and it looks good :)

I noticed that CellularSocket is initialized incorrectly, and because of that UBLOX_N2XX_CellularStack probably fails on multiple sockets simultaneously. As a workaround please apply the patch below.

diff --git i/features/cellular/framework/AT/AT_CellularStack.cpp w/features/cellular/framework/AT/AT_CellularStack.cpp
index 8c09f73849..f83259b16c 100644
--- i/features/cellular/framework/AT/AT_CellularStack.cpp
+++ w/features/cellular/framework/AT/AT_CellularStack.cpp
@@ -134,9 +134,7 @@ nsapi_error_t AT_CellularStack::socket_open(nsapi_socket_t *handle, nsapi_protoc
     _socket[index] = new CellularSocket;
     CellularSocket *psock;
     psock = _socket[index];
-    memset(psock, 0, sizeof(CellularSocket));
     SocketAddress addr(0, get_dynamic_ip_port());
-    psock->id = index;
     psock->localAddress = addr;
     psock->proto = proto;
     *handle = psock;

I think psock->id = index; is required, to identify sockets by its ID ?

AriParkkila · 2019-03-01T13:07:57Z

I think psock->id = index; is required, to identify sockets by its ID ?

I noticed that UBLOX_N2XX_CellularStack::create_socket_impl reads that from modem as socket->id = sock_id; and because for example AT_CellularStack::find_socket does not check created flag it will mess up 'socket index' and ' modem socket id'. You are right that fix is not good in general, but I you should be able to use it as workaround if you want to test UBLOX_N2XX.

mudassar-ublox · 2019-03-01T13:38:11Z

Yes, in Ublox api's socket->id is read from modem but AT_CellularStack is parent class and other targets are also using it, i have not mush info about that are they doing the same as we are doing. However initialization is incorrect, it need to be removed.
'AT_CellularStack::find_socket' is checking NULL socket pointer so I think find_socket will return correct index .

mudassar-ublox · 2019-03-04T07:59:27Z

@AriParkkila you shared Astyle.exe on my previous pull request. Can I use that on this PR for Astyling issuse?
#7619 (comment)

mudassar-ublox · 2019-03-04T11:49:46Z

Updated api and removed Astyle issue. Please review now.

cmonr · 2019-03-05T21:54:33Z

@mudassar-ublox There are still astyle issues. Please review.

Astyle Quickfix: Feel free to run the following script to correct this PR's astyle issues identified by Travis CI.

git apply -3 <<'PATCH' && git commit -am "Applied suggested astyle fixes" && git push
diff --git a/features/cellular/TESTS/api/cellular_sms/main.cpp b/features/cellular/TESTS/api/cellular_sms/main.cpp
index 7947696dd1..79390bc0d6 100644
--- a/features/cellular/TESTS/api/cellular_sms/main.cpp
+++ b/features/cellular/TESTS/api/cellular_sms/main.cpp
@@ -111,20 +111,20 @@ static void test_sms_initialize_pdu_mode()
 {
     nsapi_error_t err = sms->initialize(CellularSMS::CellularSMSMmodePDU);
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 static void test_set_cscs()
 {
     nsapi_error_t err = sms->set_cscs("IRA");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
     err = sms->set_cscs("UCS2");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
     err = sms->set_cscs("GSM");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 static void test_set_csca()
@@ -144,7 +144,7 @@ static void test_set_cpms_me()
 {
     nsapi_error_t err = sms->set_cpms("ME", "ME", "ME");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 #ifdef MBED_CONF_APP_CELLULAR_PHONE_NUMBER
PATCH

mudassar-ublox · 2019-03-06T06:03:20Z

@cmonr suggested patch is applied, please review now.

RobMeades

LGTM.

0xc0170 · 2019-03-08T13:00:34Z

@AriParkkila Happy with it now?

0xc0170 · 2019-03-12T07:53:06Z

CI started

mbed-ci · 2019-03-12T10:21:47Z

Test run: SUCCESS

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

mudassar-ublox changed the title ~~C030_N211 cellular api~~ Cellular: UBLOX_C030_N211 Cellular API's Feb 7, 2019

ciarmcom requested review from a team February 7, 2019 14:00

ciarmcom added the needs: review label Feb 7, 2019

0xc0170 added needs: work release-version: 5.12.0-rc1 and removed needs: review labels Feb 11, 2019

AriParkkila suggested changes Feb 12, 2019

View reviewed changes

0xc0170 removed the release-version: 5.12.0-rc1 label Feb 20, 2019

C030_N211 cellular api

da797ef

mudassar-ublox force-pushed the C030_N211_Cellular_Driver branch from a99ca29 to 389d503 Compare February 21, 2019 09:53

mudassar-ublox force-pushed the C030_N211_Cellular_Driver branch from 389d503 to c85b12f Compare February 22, 2019 09:28

cmonr added needs: review and removed needs: work labels Feb 22, 2019

updated api and removed Astyle issue

578fb7a

Applied suggested astyle fixes

f19a412

cmonr requested review from AriParkkila and RobMeades and removed request for AriParkkila March 6, 2019 19:26

cmonr added needs: review and removed needs: work labels Mar 6, 2019

RobMeades approved these changes Mar 7, 2019

View reviewed changes

cmonr added the release-version: 5.12.1 label Mar 7, 2019

cmonr approved these changes Mar 7, 2019

View reviewed changes

0xc0170 approved these changes Mar 8, 2019

View reviewed changes

0xc0170 requested a review from a team March 8, 2019 13:00

AriParkkila approved these changes Mar 11, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Mar 11, 2019

0xc0170 added ready for merge and removed needs: CI labels Mar 12, 2019

0xc0170 merged commit 76fe726 into ARMmbed:master Mar 14, 2019

0xc0170 removed the ready for merge label Mar 14, 2019

		}


		nsapi_error_t UBLOX_N2XX_CellularPower::set_power_level(int func_level, int do_reset, const char *sim_pin)

Cellular: UBLOX_C030_N211 Cellular API's #9637

Cellular: UBLOX_C030_N211 Cellular API's #9637

Uh oh!

Conversation

mudassar-ublox commented Feb 7, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Feb 7, 2019

Uh oh!

0xc0170 commented Feb 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

fahimalavi commented Feb 11, 2019

Uh oh!

cmonr commented Feb 11, 2019

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!

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!

0xc0170 commented Feb 18, 2019

Uh oh!

mudassar-ublox commented Feb 19, 2019

Uh oh!

AriParkkila commented Feb 19, 2019

Uh oh!

mudassar-ublox commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AriParkkila commented Feb 19, 2019

Uh oh!

mudassar-ublox commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AriParkkila commented Feb 19, 2019

Uh oh!

0xc0170 commented Feb 20, 2019

Uh oh!

mudassar-ublox commented Feb 21, 2019

Uh oh!

AriParkkila commented Feb 21, 2019

Uh oh!

mudassar-ublox commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mudassar-ublox commented Mar 1, 2019

Uh oh!

AriParkkila commented Mar 1, 2019

Uh oh!

mudassar-ublox commented Mar 1, 2019

Uh oh!

mudassar-ublox commented Mar 4, 2019

Uh oh!

mudassar-ublox commented Mar 4, 2019

Uh oh!

cmonr commented Mar 5, 2019

Uh oh!

0xc0170 commented Feb 11, 2019 •

edited

Loading

mudassar-ublox commented Feb 19, 2019 •

edited

Loading

mudassar-ublox commented Feb 19, 2019 •

edited

Loading

mudassar-ublox commented Feb 22, 2019 •

edited

Loading